| Message ID | 20241029-ad7380-add-alert-support-v1-1-d0359401b788@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [RFC] iio: adc: ad7380: add alert support | expand |
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; > +}
On Tue, 29 Oct 2024 16:02:46 +0100 Julien Stephan <jstephan@baylibre.com> wrote: > 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. IRQF_ONESHOT not enough? > > Signed-off-by: Julien Stephan <jstephan@baylibre.com> > --- > Hello, > > The ad738x family includes a built-in alert mechanism for early > detection of out-of-bounds conversion results. This series introduces > this functionality to the ad7380 family, with several open questions. > > About High/Low Threshold sysfs entries: > The internal high/low threshold registers are 16 bits and thresholds must > be computed on a 16 bits scale, regardless of the actual ADC resolution, > meaning SCALE and OFFSET attributes or realbits can't be used to compute > the raw value. > > For example, with a 3.3V reference and a 16-bit scale, the calculation for > a 1.3V high threshold is 1300/0.100708 ≈ 13000 >> 4 = 812. User has to > write 812 in sysfs entry. This value is valid for all chips, regardless > of the actual ADC resolution. > Is it okay to have a raw threshold value that is not on the same scale > as the channel raw value? What about when reading back the threshold > values? That's pretty unintuitive and ABI has always assumed equivalent scale. Can we fiddle the scale so the driver is responsible for rescaling form something that matches _raw scaling to what is needed here? There are drivers that do this because there threshold registers are lower precision than the data ones. I'm not sure we have a case in a driver supporting multiple resolutions of ADC though. Principle is the same, just different maths to do. > > About event Handling: > There is an alert register containing two status > bits per ADC, one corresponding to the high limit, and the other to the > low limit. A logical OR of alert signals for all ADCs creates a common > alert value which drives out the alert pin. > Unfortunately we cannot read the alert register to determine the exact > channel and direction, because in buffered read, we cannot claim for > direct mode. So: > * we send an alert event on ALL channels That's ugly but I can't think of much else you can do :( > * we set the direction to IIO_EV_DIR_EITHER (xilinx-xadc is doing that) > Does that work? Yes. It's not unheard of to just have 'out of range' signals on ADCs. Hence we have the either direction. With hindsight maybe we should just have sent two events but that either thing goes back a long way so userspace has to understand it and it fits your usecase here. > > Moreover, the user can not reliably read alert register manually because > when reading a channel, we have to do 2 transactions: 1 to trigger > conversion and 1 to read data (this one will also trigger a new > conversion). When reading alert register, the register will not reflect the > alert of the first conversion. > Does it make sens to send generic event if user can't read register? Sure. Some devices don't have status registers at all so you just need to know how the line is configured. > > Interrupt Handling: > Should we consider enabling interrupt only once per buffered read? If > thresholds are continuously exceeded a lot of interrupts are generated. We do have a concept of *reset_timeout though so far that's always been for accelerometer tap events. You could set up something similar for this and reenable the interrupt only after X seconds. > > ADI engineers suggest the primary use of this alert pin is as a hardware > shutdown signal for the ADC. Therefore, is it better to skip event > handling and provide threshold configuration only? Would custom > attributes be more appropriate than events in this case? It it's shutdown signaling should we just push this to a DT binding? Seems likely to vary by board design, but not by usecase. Ignoring these questions here is a review of the code. Thanks, Jonathan > > #define DEFINE_AD7380_2_CHANNEL(name, bits, diff, sign) \ > @@ -481,6 +502,9 @@ struct ad7380_state { > bool resolution_boost_enabled; > unsigned int ch; > bool seq; > + int high_th; > + int low_th; u16? Though there is a confusing shift so I'm not quite sure what it's scale is! > + bool alert_en; > unsigned int vref_mv; > unsigned int vcm_mv[MAX_NUM_CHANNELS]; > /* xfers, message an buffer for reading sample data */ > @@ -948,12 +972,133 @@ static int ad7380_get_current_scan_type(const struct iio_dev *indio_dev, > : AD7380_SCAN_TYPE_NORMAL; > } > > +static int ad7380_read_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir) > +{ > + struct ad7380_state *st = iio_priv(indio_dev); > + > + return st->alert_en ? 1 : 0; > +} > + > +static int ad7380_write_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + int state) Obviously this will need updating to bool. > +{ > + 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 > + */ > + 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_read_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) > +{ > + struct ad7380_state *st = iio_priv(indio_dev); > + > + switch (dir) { > + case IIO_EV_DIR_RISING: > + *val = st->high_th; return IIO_VAL_INT here. > + break; > + case IIO_EV_DIR_FALLING: > + *val = st->low_th; > + break; > + default: > + return -EINVAL; > + } > + > + return IIO_VAL_INT; > +} > + > +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) { If David's if guard stuff is in place before this gets merged, definitely prefer that here (and probably everywhere else we have this call!) > + 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) > + return -EINVAL; > + > + switch (dir) { > + case IIO_EV_DIR_RISING: > + ret = regmap_write(st->regmap, > + AD7380_REG_ADDR_ALERT_HIGH_TH, > + val); > + if (!ret) if (ret) return ret; st-high_th = ... return 0; So keep the error path as the out of line one. > + st->high_th = val << 4 | 0xf; read and write should be the same value. Here it seem it will read back 16 * the written value which leaves me confused. > + return ret; > + 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(); > +} > + > +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; No need to init i. can also init j in the for loop preamble. however, i * 2 and i * 2 + 1 seem to have the same value so no need for j. > + > + 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); As I ask above, I'm confused on why oneshot doesn't effectively do this for you already. > + > + return IRQ_HANDLED; > +}
diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c index e8bddfb0d07dbcf3e2a43344a4e0516f4d617548..595542aeaa5768fc577298a58715156b6cc4e7fd 100644 --- a/drivers/iio/adc/ad7380.c +++ b/drivers/iio/adc/ad7380.c @@ -30,6 +30,7 @@ #include <linux/iio/buffer.h> #include <linux/iio/iio.h> +#include <linux/iio/events.h> #include <linux/iio/trigger_consumer.h> #include <linux/iio/triggered_buffer.h> @@ -92,6 +93,24 @@ struct ad7380_chip_info { const struct ad7380_timing_specs *timing_specs; }; +static const struct iio_event_spec ad7380_events[] = { + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_RISING, + .mask_shared_by_dir = BIT(IIO_EV_INFO_VALUE), + }, + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_FALLING, + .mask_shared_by_dir = BIT(IIO_EV_INFO_VALUE), + }, + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_EITHER, + .mask_shared_by_all = BIT(IIO_EV_INFO_ENABLE), + }, +}; + enum { AD7380_SCAN_TYPE_NORMAL, AD7380_SCAN_TYPE_RESOLUTION_BOOST, @@ -193,6 +212,8 @@ static const struct iio_scan_type ad7380_scan_type_16_u[] = { .has_ext_scan_type = 1, \ .ext_scan_type = ad7380_scan_type_##bits##_##sign, \ .num_ext_scan_type = ARRAY_SIZE(ad7380_scan_type_##bits##_##sign), \ + .event_spec = ad7380_events, \ + .num_event_specs = ARRAY_SIZE(ad7380_events), \ } #define DEFINE_AD7380_2_CHANNEL(name, bits, diff, sign) \ @@ -481,6 +502,9 @@ struct ad7380_state { bool resolution_boost_enabled; unsigned int ch; bool seq; + int high_th; + int low_th; + bool alert_en; unsigned int vref_mv; unsigned int vcm_mv[MAX_NUM_CHANNELS]; /* xfers, message an buffer for reading sample data */ @@ -948,12 +972,133 @@ static int ad7380_get_current_scan_type(const struct iio_dev *indio_dev, : AD7380_SCAN_TYPE_NORMAL; } +static int ad7380_read_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir) +{ + struct ad7380_state *st = iio_priv(indio_dev); + + return st->alert_en ? 1 : 0; +} + +static int ad7380_write_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + int state) +{ + 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 + */ + 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_read_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) +{ + struct ad7380_state *st = iio_priv(indio_dev); + + switch (dir) { + case IIO_EV_DIR_RISING: + *val = st->high_th; + break; + case IIO_EV_DIR_FALLING: + *val = st->low_th; + break; + default: + return -EINVAL; + } + + return IIO_VAL_INT; +} + +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) + 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; + return ret; + 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(); +} + static const struct iio_info ad7380_info = { .read_raw = &ad7380_read_raw, .read_avail = &ad7380_read_avail, .write_raw = &ad7380_write_raw, .get_current_scan_type = &ad7380_get_current_scan_type, .debugfs_reg_access = &ad7380_debugfs_reg_access, + .read_event_config = &ad7380_read_event_config, + .write_event_config = &ad7380_write_event_config, + .read_event_value = &ad7380_read_event_value, + .write_event_value = &ad7380_write_event_value, }; static int ad7380_init(struct ad7380_state *st, struct regulator *vref) @@ -980,6 +1125,8 @@ static int ad7380_init(struct ad7380_state *st, struct regulator *vref) st->oversampling_ratio = 1; st->ch = 0; st->seq = false; + st->high_th = 0x7ff; + st->low_th = 0x800; /* SPI 1-wire mode */ return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2, @@ -992,6 +1139,42 @@ static void ad7380_regulator_disable(void *p) regulator_disable(p); } +static irqreturn_t ad7380_primary_event_handler(int irq, void *private) +{ + disable_irq_nosync(irq); + + return IRQ_WAKE_THREAD; +} + +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; + + 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; +} + static int ad7380_probe(struct spi_device *spi) { struct iio_dev *indio_dev; @@ -1128,6 +1311,17 @@ static int ad7380_probe(struct spi_device *spi) indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->available_scan_masks = st->chip_info->available_scan_masks; + if (spi->irq) { + ret = devm_request_threaded_irq(&spi->dev, spi->irq, + &ad7380_primary_event_handler, + &ad7380_event_handler, + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, + indio_dev->name, + indio_dev); + if (ret) + return dev_err_probe(&spi->dev, ret, "Failed to allocate irq\n"); + } + ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, iio_pollfunc_store_time, ad7380_trigger_handler,
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. Signed-off-by: Julien Stephan <jstephan@baylibre.com> --- Hello, The ad738x family includes a built-in alert mechanism for early detection of out-of-bounds conversion results. This series introduces this functionality to the ad7380 family, with several open questions. About High/Low Threshold sysfs entries: The internal high/low threshold registers are 16 bits and thresholds must be computed on a 16 bits scale, regardless of the actual ADC resolution, meaning SCALE and OFFSET attributes or realbits can't be used to compute the raw value. For example, with a 3.3V reference and a 16-bit scale, the calculation for a 1.3V high threshold is 1300/0.100708 ≈ 13000 >> 4 = 812. User has to write 812 in sysfs entry. This value is valid for all chips, regardless of the actual ADC resolution. Is it okay to have a raw threshold value that is not on the same scale as the channel raw value? What about when reading back the threshold values? About event Handling: There is an alert register containing two status bits per ADC, one corresponding to the high limit, and the other to the low limit. A logical OR of alert signals for all ADCs creates a common alert value which drives out the alert pin. Unfortunately we cannot read the alert register to determine the exact channel and direction, because in buffered read, we cannot claim for direct mode. So: * we send an alert event on ALL channels * we set the direction to IIO_EV_DIR_EITHER (xilinx-xadc is doing that) Does that work? Moreover, the user can not reliably read alert register manually because when reading a channel, we have to do 2 transactions: 1 to trigger conversion and 1 to read data (this one will also trigger a new conversion). When reading alert register, the register will not reflect the alert of the first conversion. Does it make sens to send generic event if user can't read register? Interrupt Handling: Should we consider enabling interrupt only once per buffered read? If thresholds are continuously exceeded a lot of interrupts are generated. ADI engineers suggest the primary use of this alert pin is as a hardware shutdown signal for the ADC. Therefore, is it better to skip event handling and provide threshold configuration only? Would custom attributes be more appropriate than events in this case? --- drivers/iio/adc/ad7380.c | 194 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 194 insertions(+) --- base-commit: 9090ececac9ff1e22fb7e042f3c886990a8fb090 change-id: 20241029-ad7380-add-alert-support-4d0dd6cea8cd prerequisite-change-id: 20241024-iio-add-macro-for-even-identifier-for-differential-channels-1bd8afdacf42:v1 prerequisite-patch-id: 95f88e2bd4b05642726a4e2431e80d0986f52075 prerequisite-patch-id: 5c66618adbc3a6ffb0d3a186b2a9b2fc8f814364 Best regards,