drivers/iio/adc/ad7380.c | 194 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 194 insertions(+)
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(+)
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,
---
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,
--
Julien Stephan <jstephan@baylibre.com>
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;
> +}
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
© 2016 - 2024 Red Hat, Inc.