| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZyFCDHsA4hoVnNy2@surfacebook.localdomain>
Date: Tue, 29 Oct 2024 22:14:04 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Julien Stephan <jstephan@...libre.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>,
Michael Hennerich <Michael.Hennerich@...log.com>,
Nuno Sá <nuno.sa@...log.com>,
David Lechner <dlechner@...libre.com>,
Jonathan Cameron <jic23@...nel.org>, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC] iio: adc: ad7380: add alert support
Tue, Oct 29, 2024 at 04:02:46PM +0100, Julien Stephan kirjoitti:
> The alert functionality is an out of range indicator and can be used as an
> early indicator of an out of bounds conversion result.
>
> ALERT_LOW_THRESHOLD and ALERT_HIGH_THRESHOLD registers are common to all
> channels.
>
> When using 1 SDO line (only mode supported by the driver right now), i.e
> data outputs only on SDOA, SDOB (or SDOD for 4 channels variants) is
> used as an alert pin. The alert pin is updated at the end of the
> conversion (set to low if an alert occurs) and is cleared on a falling
> edge of CS.
>
> The ALERT register contains information about the exact alert status:
> channel and direction. Unfortunately we can't read this register because
> in buffered read we cannot claim for direct mode.
>
> User can set high/low thresholds and enable event detection using the
> regular iio events:
>
> events/in_thresh_falling_value
> events/in_thresh_rising_value
> events/thresh_either_en
>
> Because we cannot read ALERT register, we can't determine the exact
> channel that triggers the alert, neither the direction (hight/low
> threshold violation), so we send and IIO_EV_DIR_EITHER event for all
> channels. This is ok, because the primary use case for this chip is to
> hardwire the alert line to shutdown the device.
>
> When reading a channel raw data, we have to trigger 2 spi transactions: a
> first transaction that will trigger a conversion and a second
> transaction to read the conversion. By design a new conversion is
> initiated on each falling edge of CS. This will trigger a second
> interrupt. To avoid that we disable irq in the hard irq handler and
> re-enable them in thread handler.
...
> #include <linux/iio/buffer.h>
> #include <linux/iio/iio.h>
> +#include <linux/iio/events.h>
Perhaps keep it ordered?
> #include <linux/iio/trigger_consumer.h>
> #include <linux/iio/triggered_buffer.h>
...
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> + struct ad7380_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + if (state == st->alert_en)
> + return 0;
> +
> + /*
> + * According to the datasheet, high threshold must always be
> + * greater than low threshold
Missed period at the end.
> + */
> + if (state && st->high_th < st->low_th)
> + return -EINVAL;
> +
> + ret = regmap_update_bits(st->regmap,
> + AD7380_REG_ADDR_CONFIG1,
> + AD7380_CONFIG1_ALERTEN,
> + FIELD_PREP(AD7380_CONFIG1_ALERTEN, state));
> +
> + if (ret)
> + return ret;
> +
> + st->alert_en = state;
> +
> + return 0;
> + }
> + unreachable();
> +}
...
> +static int ad7380_write_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int val, int val2)
> +{
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> + struct ad7380_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + /*
> + * According to the datasheet,
> + * AD7380_REG_ADDR_ALERT_HIGH_TH[11:0] are the MSB of the 16-bit
> + * internal alert high register. LSB are set to 0x0.
> + * AD7380_REG_ADDR_ALERT_LOW_TH[11:0] are the MSB of the 16 bit
> + * internal alert low register. LSB are set to 0xf.
> + *
> + * When alert is enabled the conversion from the adc is compared
> + * immediately to the alert high/low thresholds, before any
> + * oversampling. This means that the thresholds are the same for
> + * normal mode and oversampling mode.
> + * For 12 and 14 bits, the thresholds are still on 16 bits.
> + */
> + if (val < 0 || val > 2047)
What about having val >= BIT(11) here?
> + return -EINVAL;
> +
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + ret = regmap_write(st->regmap,
> + AD7380_REG_ADDR_ALERT_HIGH_TH,
> + val);
> + if (!ret)
> + st->high_th = val << 4 | 0xf;
GENMASK() ? Predefined constant?
> + return ret;
if (ret)
return ret;
...
return 0;
(yes, more verbose, but better for reading and maintenance)
> + case IIO_EV_DIR_FALLING:
> + ret = regmap_write(st->regmap,
> + AD7380_REG_ADDR_ALERT_LOW_TH,
> + val);
> + if (!ret)
> + st->low_th = val << 4;
> + return ret;
> + default:
> + return -EINVAL;
> + }
> + }
> + unreachable();
> +}
...
> + st->high_th = 0x7ff;
> + st->low_th = 0x800;
I would go with BIT(11) - 1 and BIT(11) as it seems related to the amount of
bits in the hardware.
...
> +static irqreturn_t ad7380_event_handler(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + s64 timestamp = iio_get_time_ns(indio_dev);
> + struct ad7380_state *st = iio_priv(indio_dev);
> + const struct iio_chan_spec *chan = &indio_dev->channels[0];
> + int i = 0, j = 0;
Why signed? And for 'i' the assignment is redundant.
> +
> + for (i = 0; i < st->chip_info->num_channels - 1; i++) {
> + iio_push_event(indio_dev,
> + chan->differential ?
> + IIO_DIFF_EVENT_CODE(IIO_VOLTAGE,
> + j,
> + j + 1,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_EITHER) :
> + IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
> + i,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_EITHER),
> + timestamp);
> + j += 2;
> + }
> +
> + enable_irq(irq);
> +
> + return IRQ_HANDLED;
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists