[PATCH RFC v2 0/4] iio: adc: ad7380: add alert support

[PATCH RFC v2 0/4] iio: adc: ad7380: add alert support

[Julien Stephan]

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.

As a reminder, an RFC was sent [1] with several open questions.

Here is a summary of the changes made:

- I now have a better understanding of the alert high/low registers, and
  it is much simpler than I initially thought: users can use the same
scale as the raw value; we just need to extract the 12 MSBs.
- IRQs are now disabled by default and only enabled before read_raw and
  buffered_read operations.
- I implemented the reset timeout mechanism, as suggested in the RFC, to
  avoid generating too many events during buffered reads (this has no
effect on read_raw).
- Reading registers via debugfs no longer triggers events.
- The reset_timeout attribute is added only if an IRQ is present in the
  device tree. However, the high/low thresholds and enable attributes
  are always available. This allows configuration of the thresholds and
  alert enablement even when no interrupts are defined in the device tree.
  For example, a user can enable alerts and hardwire the interrupt line,
  without relying on user events.

- I added an alert section to the documentation.

- Two preliminary commits have been added to this series:
  - A cleanup patch to remove iio_device_claim_direct_scoped calls.
  - A patch to implement regcache.

[1]: https://lore.kernel.org/r/20241029-ad7380-add-aleyyrt-support-v1-1-d0359401b788@baylibre.com

Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---
Changes in v2:
- fix read/write high/low thresholds
- add reset_timeout mechanism for buffered reads
- implement regcache
- add cleanup patch to remove iio_device_claim_direct_scoped calls
- add alert section in the Documentation page
- Link to v1: https://lore.kernel.org/r/20241029-ad7380-add-aleyyrt-support-v1-1-d0359401b788@baylibre.com

---
Julien Stephan (4):
      iio: adc: ad7380: do not use iio_device_claim_direct_scoped anymore
      iio: adc: ad7380: enable regmap cache
      iio: adc: ad7380: add alert support
      docs: iio: ad7380: add alert support

 Documentation/iio/ad7380.rst |  56 ++++-
 drivers/iio/adc/ad7380.c     | 531 +++++++++++++++++++++++++++++++++++++++----
 2 files changed, 537 insertions(+), 50 deletions(-)
---
base-commit: 5ab39233382c621d3271cc274d1534e1b687f4d3
change-id: 20241029-ad7380-add-alert-support-4d0dd6cea8cd

Best regards,
-- 
Julien Stephan <jstephan@baylibre.com>

[PATCH RFC v2 1/4] iio: adc: ad7380: do not use iio_device_claim_direct_scoped anymore

[Julien Stephan]

Rollback and remove the scoped version of iio_dvice_claim_direct_mode.

Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---
 drivers/iio/adc/ad7380.c | 89 ++++++++++++++++++++++++++++--------------------
 1 file changed, 53 insertions(+), 36 deletions(-)

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index 4f32cb22f140442b831dc9a4f275e88e4ab2388e..4e26e0e7ac1d5a1c4c67118dbc34f2921fc171a4 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -675,15 +675,21 @@ static const struct regmap_config ad7380_regmap_config = {
 static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
 				     u32 writeval, u32 *readval)
 {
-	iio_device_claim_direct_scoped(return  -EBUSY, indio_dev) {
-		struct ad7380_state *st = iio_priv(indio_dev);
+	struct ad7380_state *st = iio_priv(indio_dev);
+	int ret;
 
-		if (readval)
-			return regmap_read(st->regmap, reg, readval);
-		else
-			return regmap_write(st->regmap, reg, writeval);
-	}
-	unreachable();
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
+
+	if (readval)
+		ret = regmap_read(st->regmap, reg, readval);
+	else
+		ret = regmap_write(st->regmap, reg, writeval);
+
+	iio_device_release_direct_mode(indio_dev);
+
+	return ret;
 }
 
 /*
@@ -920,6 +926,7 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
 {
 	struct ad7380_state *st = iio_priv(indio_dev);
 	const struct iio_scan_type *scan_type;
+	int ret;
 
 	scan_type = iio_get_current_scan_type(indio_dev, chan);
 
@@ -928,11 +935,16 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
 
 	switch (info) {
 	case IIO_CHAN_INFO_RAW:
-		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
-			return ad7380_read_direct(st, chan->scan_index,
-						  scan_type, val);
-		}
-		unreachable();
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		ret = ad7380_read_direct(st, chan->scan_index,
+					 scan_type, val);
+
+		iio_device_release_direct_mode(indio_dev);
+
+		return ret;
 	case IIO_CHAN_INFO_SCALE:
 		/*
 		 * According to the datasheet, the LSB size is:
@@ -1024,31 +1036,36 @@ static int ad7380_write_raw(struct iio_dev *indio_dev,
 		/* always enable resolution boost when oversampling is enabled */
 		boost = osr > 0 ? 1 : 0;
 
-		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
-			ret = regmap_update_bits(st->regmap,
-					AD7380_REG_ADDR_CONFIG1,
-					AD7380_CONFIG1_OSR | AD7380_CONFIG1_RES,
-					FIELD_PREP(AD7380_CONFIG1_OSR, osr) |
-					FIELD_PREP(AD7380_CONFIG1_RES, boost));
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
 
-			if (ret)
-				return ret;
+		ret = regmap_update_bits(st->regmap,
+					 AD7380_REG_ADDR_CONFIG1,
+					 AD7380_CONFIG1_OSR | AD7380_CONFIG1_RES,
+					 FIELD_PREP(AD7380_CONFIG1_OSR, osr) |
+					 FIELD_PREP(AD7380_CONFIG1_RES, boost));
 
-			st->oversampling_ratio = val;
-			st->resolution_boost_enabled = boost;
-
-			/*
-			 * Perform a soft reset. This will flush the oversampling
-			 * block and FIFO but will maintain the content of the
-			 * configurable registers.
-			 */
-			return regmap_update_bits(st->regmap,
-					AD7380_REG_ADDR_CONFIG2,
-					AD7380_CONFIG2_RESET,
-					FIELD_PREP(AD7380_CONFIG2_RESET,
-						   AD7380_CONFIG2_RESET_SOFT));
-		}
-		unreachable();
+		if (ret)
+			goto err;
+
+		st->oversampling_ratio = val;
+		st->resolution_boost_enabled = boost;
+
+		/*
+		 * Perform a soft reset. This will flush the oversampling
+		 * block and FIFO but will maintain the content of the
+		 * configurable registers.
+		 */
+		ret = regmap_update_bits(st->regmap,
+					 AD7380_REG_ADDR_CONFIG2,
+					 AD7380_CONFIG2_RESET,
+					 FIELD_PREP(AD7380_CONFIG2_RESET,
+						    AD7380_CONFIG2_RESET_SOFT));
+err:
+		iio_device_release_direct_mode(indio_dev);
+
+		return ret;
 	default:
 		return -EINVAL;
 	}

-- 
2.47.1

Re: [PATCH RFC v2 1/4] iio: adc: ad7380: do not use iio_device_claim_direct_scoped anymore

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 312 bytes --]

Hello Julien,

On Tue, Dec 24, 2024 at 10:34:30AM +0100, Julien Stephan wrote:
> Rollback and remove the scoped version of iio_dvice_claim_direct_mode.

Is this a preparation for one of the later patches in this series?
Mentioning the reasoning for this change in the commit log would be
good.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC v2 1/4] iio: adc: ad7380: do not use iio_device_claim_direct_scoped anymore

[Jonathan Cameron]

On Fri, 27 Dec 2024 09:43:31 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Hello Julien,
> 
> On Tue, Dec 24, 2024 at 10:34:30AM +0100, Julien Stephan wrote:
> > Rollback and remove the scoped version of iio_dvice_claim_direct_mode.  
> 
> Is this a preparation for one of the later patches in this series?
> Mentioning the reasoning for this change in the commit log would be
> good.
> 
> Best regards
> Uwe

I'd guess this is in response to a comment I made recently.
The conditional scoped handlers are turning out to be a real pain so
my plan is to rip them out entirely.

There are readability problems, compiler and linker handling issues
and a mess of other reasons these are much harder to use than they
originally seemed.

I've still very keen on the rest of the cleanup.h stuff, just not this
corner!

Jonathan

Re: [PATCH RFC v2 1/4] iio: adc: ad7380: do not use iio_device_claim_direct_scoped anymore

[Jonathan Cameron]

On Tue, 24 Dec 2024 10:34:30 +0100
Julien Stephan <jstephan@baylibre.com> wrote:

> Rollback and remove the scoped version of iio_dvice_claim_direct_mode.
> 
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
> ---
>  drivers/iio/adc/ad7380.c | 89 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 53 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index 4f32cb22f140442b831dc9a4f275e88e4ab2388e..4e26e0e7ac1d5a1c4c67118dbc34f2921fc171a4 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -675,15 +675,21 @@ static const struct regmap_config ad7380_regmap_config = {
>  static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
>  				     u32 writeval, u32 *readval)
>  {
> -	iio_device_claim_direct_scoped(return  -EBUSY, indio_dev) {
> -		struct ad7380_state *st = iio_priv(indio_dev);
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +	int ret;
>  
> -		if (readval)
> -			return regmap_read(st->regmap, reg, readval);
> -		else
> -			return regmap_write(st->regmap, reg, writeval);
> -	}
> -	unreachable();
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	if (readval)
> +		ret = regmap_read(st->regmap, reg, readval);
> +	else
> +		ret = regmap_write(st->regmap, reg, writeval);
> +
> +	iio_device_release_direct_mode(indio_dev);
> +
> +	return ret;
>  }
>  
>  /*
> @@ -920,6 +926,7 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
>  {
>  	struct ad7380_state *st = iio_priv(indio_dev);
>  	const struct iio_scan_type *scan_type;
> +	int ret;
>  
>  	scan_type = iio_get_current_scan_type(indio_dev, chan);
>  
> @@ -928,11 +935,16 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (info) {
>  	case IIO_CHAN_INFO_RAW:
> -		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> -			return ad7380_read_direct(st, chan->scan_index,
> -						  scan_type, val);
> -		}
> -		unreachable();
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		ret = ad7380_read_direct(st, chan->scan_index,
> +					 scan_type, val);
> +
> +		iio_device_release_direct_mode(indio_dev);
> +
> +		return ret;
>  	case IIO_CHAN_INFO_SCALE:
>  		/*
>  		 * According to the datasheet, the LSB size is:
> @@ -1024,31 +1036,36 @@ static int ad7380_write_raw(struct iio_dev *indio_dev,
>  		/* always enable resolution boost when oversampling is enabled */
>  		boost = osr > 0 ? 1 : 0;
>  
> -		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> -			ret = regmap_update_bits(st->regmap,
> -					AD7380_REG_ADDR_CONFIG1,
> -					AD7380_CONFIG1_OSR | AD7380_CONFIG1_RES,
> -					FIELD_PREP(AD7380_CONFIG1_OSR, osr) |
> -					FIELD_PREP(AD7380_CONFIG1_RES, boost));
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
>  
> -			if (ret)
> -				return ret;
> +		ret = regmap_update_bits(st->regmap,
> +					 AD7380_REG_ADDR_CONFIG1,
> +					 AD7380_CONFIG1_OSR | AD7380_CONFIG1_RES,
> +					 FIELD_PREP(AD7380_CONFIG1_OSR, osr) |
> +					 FIELD_PREP(AD7380_CONFIG1_RES, boost));
>  
> -			st->oversampling_ratio = val;
> -			st->resolution_boost_enabled = boost;
> -
> -			/*
> -			 * Perform a soft reset. This will flush the oversampling
> -			 * block and FIFO but will maintain the content of the
> -			 * configurable registers.
> -			 */
> -			return regmap_update_bits(st->regmap,
> -					AD7380_REG_ADDR_CONFIG2,
> -					AD7380_CONFIG2_RESET,
> -					FIELD_PREP(AD7380_CONFIG2_RESET,
> -						   AD7380_CONFIG2_RESET_SOFT));
> -		}
> -		unreachable();
> +		if (ret)
> +			goto err;
> +
> +		st->oversampling_ratio = val;
> +		st->resolution_boost_enabled = boost;
> +
> +		/*
> +		 * Perform a soft reset. This will flush the oversampling
> +		 * block and FIFO but will maintain the content of the
> +		 * configurable registers.
> +		 */
> +		ret = regmap_update_bits(st->regmap,
> +					 AD7380_REG_ADDR_CONFIG2,
> +					 AD7380_CONFIG2_RESET,
> +					 FIELD_PREP(AD7380_CONFIG2_RESET,
> +						    AD7380_CONFIG2_RESET_SOFT));
> +err:
Labels within switch statements can become hard to maintainer / read.

Id' suggest factoring out the bits between claim and release as a single
__ad7380_write_oversample() or something along those lines.

Otherwise looks good to me and thanks for doing this.

Jonathan


> +		iio_device_release_direct_mode(indio_dev);
> +
> +		return ret;
>  	default:
>  		return -EINVAL;
>  	}
> 

[PATCH RFC v2 2/4] iio: adc: ad7380: enable regmap cache

[Julien Stephan]

Enable regmap cache, to avoid useless access on spi bus.
Don't store anymore the oversampling ratio in private data structure.

Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---
 drivers/iio/adc/ad7380.c | 98 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 84 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index 4e26e0e7ac1d5a1c4c67118dbc34f2921fc171a4..b49067c36fdd1bb0e760faf71d7fa0c8c1f610e9 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -582,7 +582,6 @@ struct ad7380_state {
 	const struct ad7380_chip_info *chip_info;
 	struct spi_device *spi;
 	struct regmap *regmap;
-	unsigned int oversampling_ratio;
 	bool resolution_boost_enabled;
 	unsigned int ch;
 	bool seq;
@@ -663,6 +662,20 @@ static int ad7380_regmap_reg_read(void *context, unsigned int reg,
 	return 0;
 }
 
+static const struct reg_default ad7380_reg_defaults[] = {
+	{ AD7380_REG_ADDR_ALERT_LOW_TH, 0x800 },
+	{ AD7380_REG_ADDR_ALERT_HIGH_TH, 0x7FF },
+};
+
+static const struct regmap_range ad7380_volatile_reg_ranges[] = {
+	regmap_reg_range(AD7380_REG_ADDR_CONFIG2, AD7380_REG_ADDR_ALERT),
+};
+
+static const struct regmap_access_table ad7380_volatile_regs = {
+	.yes_ranges = ad7380_volatile_reg_ranges,
+	.n_yes_ranges = ARRAY_SIZE(ad7380_volatile_reg_ranges),
+};
+
 static const struct regmap_config ad7380_regmap_config = {
 	.reg_bits = 3,
 	.val_bits = 12,
@@ -670,6 +683,10 @@ static const struct regmap_config ad7380_regmap_config = {
 	.reg_write = ad7380_regmap_reg_write,
 	.max_register = AD7380_REG_ADDR_ALERT_HIGH_TH,
 	.can_sleep = true,
+	.reg_defaults = ad7380_reg_defaults,
+	.num_reg_defaults = ARRAY_SIZE(ad7380_reg_defaults),
+	.volatile_table = &ad7380_volatile_regs,
+	.cache_type = REGCACHE_MAPLE,
 };
 
 static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
@@ -692,6 +709,37 @@ static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
 	return ret;
 }
 
+/**
+ * ad7380_regval_to_osr - convert OSR register value to ratio
+ * @regval: register value to check
+ *
+ * Returns: the ratio corresponding to the OSR register. If regval is not in
+ * bound, return 1 (oversampling disabled)
+ *
+ */
+static int ad7380_regval_to_osr(int regval)
+{
+	if (regval < 0 || regval >= ARRAY_SIZE(ad7380_oversampling_ratios))
+		return 1;
+
+	return ad7380_oversampling_ratios[regval];
+}
+
+static int ad7380_get_osr(struct ad7380_state *st, int *val)
+{
+	int tmp;
+	int ret = 0;
+
+	ret = regmap_read(st->regmap, AD7380_REG_ADDR_CONFIG1, &tmp);
+	if (ret)
+		return ret;
+
+	*val = ad7380_regval_to_osr(FIELD_GET(AD7380_CONFIG1_OSR, tmp));
+
+	return 0;
+}
+
+
 /*
  * When switching channel, the ADC require an additional settling time.
  * According to the datasheet, data is value on the third CS low. We already
@@ -707,11 +755,15 @@ static int ad7380_set_ch(struct ad7380_state *st, unsigned int ch)
 			.unit = SPI_DELAY_UNIT_NSECS,
 		}
 	};
-	int ret;
+	int oversampling_ratio, ret;
 
 	if (st->ch == ch)
 		return 0;
 
+	ret = ad7380_get_osr(st, &oversampling_ratio);
+	if (ret)
+		return ret;
+
 	ret = regmap_update_bits(st->regmap,
 				 AD7380_REG_ADDR_CONFIG1,
 				 AD7380_CONFIG1_CH,
@@ -722,9 +774,9 @@ static int ad7380_set_ch(struct ad7380_state *st, unsigned int ch)
 
 	st->ch = ch;
 
-	if (st->oversampling_ratio > 1)
+	if (oversampling_ratio > 1)
 		xfer.delay.value = T_CONVERT_0_NS +
-			T_CONVERT_X_NS * (st->oversampling_ratio - 1) *
+			T_CONVERT_X_NS * (oversampling_ratio - 1) *
 			st->chip_info->num_simult_channels / AD7380_NUM_SDO_LINES;
 
 	return spi_sync_transfer(st->spi, &xfer, 1);
@@ -735,20 +787,25 @@ static int ad7380_set_ch(struct ad7380_state *st, unsigned int ch)
  * @st:		device instance specific state
  * @scan_type:	current scan type
  */
-static void ad7380_update_xfers(struct ad7380_state *st,
+static int ad7380_update_xfers(struct ad7380_state *st,
 				const struct iio_scan_type *scan_type)
 {
 	struct spi_transfer *xfer = st->seq ? st->seq_xfer : st->normal_xfer;
 	unsigned int t_convert = T_CONVERT_NS;
+	int oversampling_ratio, ret;
 
 	/*
 	 * In the case of oversampling, conversion time is higher than in normal
 	 * mode. Technically T_CONVERT_X_NS is lower for some chips, but we use
 	 * the maximum value for simplicity for now.
 	 */
-	if (st->oversampling_ratio > 1)
+	ret = ad7380_get_osr(st, &oversampling_ratio);
+	if (ret)
+		return ret;
+
+	if (oversampling_ratio > 1)
 		t_convert = T_CONVERT_0_NS + T_CONVERT_X_NS *
-			(st->oversampling_ratio - 1) *
+			(oversampling_ratio - 1) *
 			st->chip_info->num_simult_channels / AD7380_NUM_SDO_LINES;
 
 	if (st->seq) {
@@ -761,7 +818,7 @@ static void ad7380_update_xfers(struct ad7380_state *st,
 			st->chip_info->num_simult_channels;
 		xfer[3].rx_buf = xfer[2].rx_buf + xfer[2].len;
 		/* Additional delay required here when oversampling is enabled */
-		if (st->oversampling_ratio > 1)
+		if (oversampling_ratio > 1)
 			xfer[2].delay.value = t_convert;
 		else
 			xfer[2].delay.value = 0;
@@ -773,6 +830,8 @@ static void ad7380_update_xfers(struct ad7380_state *st,
 		xfer[1].len = BITS_TO_BYTES(scan_type->storagebits) *
 			st->chip_info->num_simult_channels;
 	}
+
+	return 0;
 }
 
 static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
@@ -780,6 +839,7 @@ static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
 	struct ad7380_state *st = iio_priv(indio_dev);
 	const struct iio_scan_type *scan_type;
 	struct spi_message *msg = &st->normal_msg;
+	int ret;
 
 	/*
 	 * Currently, we always read all channels at the same time. The scan_type
@@ -791,7 +851,6 @@ static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
 
 	if (st->chip_info->has_mux) {
 		unsigned int index;
-		int ret;
 
 		/*
 		 * Depending on the requested scan_mask and current state,
@@ -822,7 +881,9 @@ static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
 
 	}
 
-	ad7380_update_xfers(st, scan_type);
+	ret = ad7380_update_xfers(st, scan_type);
+	if (ret)
+		return ret;
 
 	return spi_optimize_message(st->spi, msg);
 }
@@ -895,7 +956,9 @@ static int ad7380_read_direct(struct ad7380_state *st, unsigned int scan_index,
 			return ret;
 	}
 
-	ad7380_update_xfers(st, scan_type);
+	ret = ad7380_update_xfers(st, scan_type);
+	if (ret)
+		return ret;
 
 	ret = spi_sync(st->spi, &st->normal_msg);
 	if (ret < 0)
@@ -973,7 +1036,16 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
 
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-		*val = st->oversampling_ratio;
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		ret = ad7380_get_osr(st, val);
+
+		iio_device_release_direct_mode(indio_dev);
+
+		if (ret)
+			return ret;
 
 		return IIO_VAL_INT;
 	default:
@@ -1049,7 +1121,6 @@ static int ad7380_write_raw(struct iio_dev *indio_dev,
 		if (ret)
 			goto err;
 
-		st->oversampling_ratio = val;
 		st->resolution_boost_enabled = boost;
 
 		/*
@@ -1109,7 +1180,6 @@ static int ad7380_init(struct ad7380_state *st, bool external_ref_en)
 	}
 
 	/* This is the default value after reset. */
-	st->oversampling_ratio = 1;
 	st->ch = 0;
 	st->seq = false;
 

-- 
2.47.1

Re: [PATCH RFC v2 2/4] iio: adc: ad7380: enable regmap cache

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 360 bytes --]

On Tue, Dec 24, 2024 at 10:34:31AM +0100, Julien Stephan wrote:
> Enable regmap cache, to avoid useless access on spi bus.
> Don't store anymore the oversampling ratio in private data structure.

I would split that. There are a few changes in this patch that I don't
understand, e.g. why does the return type of ad7380_update_xfers()
change?

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC v2 2/4] iio: adc: ad7380: enable regmap cache

[Jonathan Cameron]

On Fri, 27 Dec 2024 09:48:35 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> On Tue, Dec 24, 2024 at 10:34:31AM +0100, Julien Stephan wrote:
> > Enable regmap cache, to avoid useless access on spi bus.
> > Don't store anymore the oversampling ratio in private data structure.  
> 
> I would split that. 

Splitting it probably makes sense, though the regcache enabling patch will be small
(which is fine).

> There are a few changes in this patch that I don't
> understand, e.g. why does the return type of ad7380_update_xfers()
> change?

On first call of this, the register contents may not be in the cache
so you might get an error from the bus read. Even if we known it is in the cache
(and from a quick glance I'm not sure we do as we haven't forced a full
fill of the regcache or accessed this register) from a local code point of view
it is correct to handle the potential error and pass it to higher layers.

Jonathan


> 
> Best regards
> Uwe

Re: [PATCH RFC v2 2/4] iio: adc: ad7380: enable regmap cache

[Jonathan Cameron]

On Tue, 24 Dec 2024 10:34:31 +0100
Julien Stephan <jstephan@baylibre.com> wrote:

> Enable regmap cache, to avoid useless access on spi bus.
> Don't store anymore the oversampling ratio in private data structure.
> 
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>

A few really minor things inline,

Jonathan

>  static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
> @@ -692,6 +709,37 @@ static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
>  	return ret;
>  }
>  
> +/**
> + * ad7380_regval_to_osr - convert OSR register value to ratio
> + * @regval: register value to check
> + *
> + * Returns: the ratio corresponding to the OSR register. If regval is not in
> + * bound, return 1 (oversampling disabled)
> + *
> + */
> +static int ad7380_regval_to_osr(int regval)

Make regval an unsigned int and yout can drop one test.
The FIELD_GET is never going to give you a signed value anyway.

> +{
> +	if (regval < 0 || regval >= ARRAY_SIZE(ad7380_oversampling_ratios))
> +		return 1;
> +
> +	return ad7380_oversampling_ratios[regval];
> +}
> +
> +static int ad7380_get_osr(struct ad7380_state *st, int *val)
> +{
> +	int tmp;
> +	int ret = 0;

No point in initializing.

> +
> +	ret = regmap_read(st->regmap, AD7380_REG_ADDR_CONFIG1, &tmp);
> +	if (ret)
> +		return ret;
> +
> +	*val = ad7380_regval_to_osr(FIELD_GET(AD7380_CONFIG1_OSR, tmp));
> +
> +	return 0;
> +}
> +
> +

Trivial but one blank line almost always enough!

>  /*

[PATCH RFC v2 3/4] iio: adc: ad7380: add alert support

[Julien Stephan]

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

If the interrupt properties is present in the device tree, an IIO event
will be generated for each interrupt received.
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.

In buffered reads, if input stays out of thresholds limit, an interrupt
will be generated for each sample read, because the alert pin is cleared
on a falling edge of CS (i.e when starting a new conversion). To avoid
generating to much interrupt, we introduce a reset_timeout that can be
used to disable interrupt for a given time (in ms)

  events/thresh_either_reset_timeout

When an interrupt is received, interrupts are disabled and re-enabled
after thresh_either_reset_timeout ms. If the reset timeout is set to 0,
interrupt are re-enabled directly.
Note: interrupts are always disabled at least during the handling of the
previous interrupt, because each read triggers 2 transactions, that can
lead to 2 interrupts for a single user read. IRQF_ONESHOT is not enough,
because, it postpones the 2nd irq after the handling of the first one,
which can still trigger 2 interrupts for a single user read.

Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---
 drivers/iio/adc/ad7380.c | 354 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 350 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index b49067c36fdd1bb0e760faf71d7fa0c8c1f610e9..fb59c2c064f402ea797fc0e563bd535d0e0bf785 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -34,6 +34,7 @@
 #include <linux/util_macros.h>
 
 #include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
@@ -112,6 +113,43 @@ 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),
+	},
+};
+
+static const struct iio_event_spec ad7380_events_irq[] = {
+	{
+		.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) |
+			BIT(IIO_EV_INFO_RESET_TIMEOUT),
+	},
+};
+
 enum {
 	AD7380_SCAN_TYPE_NORMAL,
 	AD7380_SCAN_TYPE_RESOLUTION_BOOST,
@@ -214,6 +252,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 AD7380_CHANNEL(index, bits, diff, sign)		\
@@ -585,6 +625,10 @@ struct ad7380_state {
 	bool resolution_boost_enabled;
 	unsigned int ch;
 	bool seq;
+	struct timer_list alert_timer;
+	unsigned int alert_reset_timeout_ms;
+	atomic_t irq_enabled;
+	bool buffered_read_enabled;
 	unsigned int vref_mv;
 	unsigned int vcm_mv[MAX_NUM_CHANNELS];
 	unsigned int gain_milli[MAX_NUM_CHANNELS];
@@ -689,6 +733,26 @@ static const struct regmap_config ad7380_regmap_config = {
 	.cache_type = REGCACHE_MAPLE,
 };
 
+static void ad7380_enable_irq(struct ad7380_state *st)
+{
+	if (st->spi->irq && !atomic_cmpxchg(&st->irq_enabled, 0, 1))
+		enable_irq(st->spi->irq);
+}
+
+static void ad7380_disable_irq(struct ad7380_state *st)
+{
+	if (st->spi->irq && atomic_cmpxchg(&st->irq_enabled, 1, 0)) {
+		disable_irq(st->spi->irq);
+		synchronize_irq(st->spi->irq);
+	}
+}
+
+static void ad7380_disable_irq_nosync(struct ad7380_state *st)
+{
+	if (st->spi->irq && atomic_cmpxchg(&st->irq_enabled, 1, 0))
+		disable_irq_nosync(st->spi->irq);
+}
+
 static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
 				     u32 writeval, u32 *readval)
 {
@@ -727,8 +791,8 @@ static int ad7380_regval_to_osr(int regval)
 
 static int ad7380_get_osr(struct ad7380_state *st, int *val)
 {
-	int tmp;
-	int ret = 0;
+	u32 tmp;
+	int ret;
 
 	ret = regmap_read(st->regmap, AD7380_REG_ADDR_CONFIG1, &tmp);
 	if (ret)
@@ -834,6 +898,13 @@ static int ad7380_update_xfers(struct ad7380_state *st,
 	return 0;
 }
 
+static void ad7380_handle_event_reset_timeout(struct timer_list *t)
+{
+	struct ad7380_state *st = from_timer(st, t, alert_timer);
+
+	ad7380_enable_irq(st);
+}
+
 static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
 {
 	struct ad7380_state *st = iio_priv(indio_dev);
@@ -841,6 +912,12 @@ static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
 	struct spi_message *msg = &st->normal_msg;
 	int ret;
 
+	timer_setup(&st->alert_timer, ad7380_handle_event_reset_timeout, 0);
+
+	ad7380_enable_irq(st);
+
+	st->buffered_read_enabled = true;
+
 	/*
 	 * Currently, we always read all channels at the same time. The scan_type
 	 * is the same for all channels, so we just pass the first channel.
@@ -894,6 +971,8 @@ static int ad7380_triggered_buffer_postdisable(struct iio_dev *indio_dev)
 	struct spi_message *msg = &st->normal_msg;
 	int ret;
 
+	st->buffered_read_enabled = false;
+
 	if (st->seq) {
 		ret = regmap_update_bits(st->regmap,
 					 AD7380_REG_ADDR_CONFIG1,
@@ -908,6 +987,9 @@ static int ad7380_triggered_buffer_postdisable(struct iio_dev *indio_dev)
 
 	spi_unoptimize_message(msg);
 
+	ad7380_disable_irq(st);
+	timer_shutdown_sync(&st->alert_timer);
+
 	return 0;
 }
 
@@ -1002,8 +1084,11 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
 		if (ret)
 			return ret;
 
-		ret = ad7380_read_direct(st, chan->scan_index,
-					 scan_type, val);
+		ad7380_enable_irq(st);
+
+		ret = ad7380_read_direct(st, chan->scan_index, scan_type, val);
+
+		ad7380_disable_irq(st);
 
 		iio_device_release_direct_mode(indio_dev);
 
@@ -1151,12 +1236,190 @@ 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);
+	int alert_en, tmp, ret;
+
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(st->regmap, AD7380_REG_ADDR_CONFIG1, &tmp);
+
+	iio_device_release_direct_mode(indio_dev);
+
+	if (ret)
+		return ret;
+
+	alert_en = FIELD_GET(AD7380_CONFIG1_ALERTEN, tmp);
+
+	return alert_en;
+}
+
+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,
+				     bool state)
+{
+	struct ad7380_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(st->regmap,
+				 AD7380_REG_ADDR_CONFIG1,
+				 AD7380_CONFIG1_ALERTEN,
+				 FIELD_PREP(AD7380_CONFIG1_ALERTEN, state));
+
+	iio_device_release_direct_mode(indio_dev);
+
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+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);
+	int ret, tmp;
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		ret = iio_device_claim_direct_mode(indio_dev);
+
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = regmap_read(st->regmap,
+					  AD7380_REG_ADDR_ALERT_HIGH_TH,
+					  &tmp);
+			if (ret)
+				return ret;
+
+			*val = FIELD_GET(AD7380_ALERT_HIGH_TH, tmp);
+			ret = IIO_VAL_INT;
+			break;
+		case IIO_EV_DIR_FALLING:
+			ret = regmap_read(st->regmap,
+					  AD7380_REG_ADDR_ALERT_LOW_TH,
+					  &tmp);
+			if (ret)
+				return ret;
+
+			FIELD_GET(AD7380_ALERT_LOW_TH, tmp);
+			ret = IIO_VAL_INT;
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
+
+		iio_device_release_direct_mode(indio_dev);
+		return ret;
+	case IIO_EV_INFO_RESET_TIMEOUT:
+		*val = st->alert_reset_timeout_ms;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+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)
+{
+	struct ad7380_state *st = iio_priv(indio_dev);
+	const struct iio_scan_type *scan_type;
+	int ret;
+	u16 th;
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		/*
+		 * According to the datasheet,
+		 * AD7380_REG_ADDR_ALERT_HIGH_TH[11:0] are the 12 MSB of the
+		 * 16-bits internal alert high register. LSB are set to 0xf.
+		 * AD7380_REG_ADDR_ALERT_LOW_TH[11:0] are the 12 MSB of the
+		 * 16 bits internal alert low register. LSB are set to 0x0.
+		 *
+		 * 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.
+		 */
+
+		/* Extract the 12 MSB of val */
+		scan_type = iio_get_current_scan_type(indio_dev, chan);
+		if (IS_ERR(scan_type))
+			return PTR_ERR(scan_type);
+
+		th = val >> (scan_type->realbits - 12);
+
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = regmap_write(st->regmap,
+					   AD7380_REG_ADDR_ALERT_HIGH_TH,
+					   th);
+			if (ret)
+				return ret;
+
+			break;
+		case IIO_EV_DIR_FALLING:
+			ret = regmap_write(st->regmap,
+					   AD7380_REG_ADDR_ALERT_LOW_TH,
+					   th);
+			if (ret)
+				return ret;
+
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
+
+		iio_device_release_direct_mode(indio_dev);
+		return ret;
+	case IIO_EV_INFO_RESET_TIMEOUT:
+		if (val < 0)
+			return -EINVAL;
+		st->alert_reset_timeout_ms = val;
+		timer_reduce(&st->alert_timer, jiffies + msecs_to_jiffies(val));
+
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
 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, bool external_ref_en)
@@ -1182,6 +1445,12 @@ static int ad7380_init(struct ad7380_state *st, bool external_ref_en)
 	/* This is the default value after reset. */
 	st->ch = 0;
 	st->seq = false;
+	st->buffered_read_enabled = false;
+
+	/*
+	 * Set a minimum default 1s delay between each alert in buffered reads
+	 */
+	st->alert_reset_timeout_ms = 1000;
 
 	/* SPI 1-wire mode */
 	return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
@@ -1190,6 +1459,50 @@ static int ad7380_init(struct ad7380_state *st, bool external_ref_en)
 					     AD7380_NUM_SDO_LINES));
 }
 
+static irqreturn_t ad7380_primary_event_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct ad7380_state *st = iio_priv(indio_dev);
+
+	if (!atomic_read(&st->irq_enabled))
+		return IRQ_NONE;
+
+	ad7380_disable_irq_nosync(st);
+
+	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];
+	unsigned int i;
+
+	for (i = 0; i < st->chip_info->num_channels - 1; i++) {
+		iio_push_event(indio_dev,
+			       chan->differential ?
+			       IIO_DIFF_EVENT_CODE(IIO_VOLTAGE,
+						   2 * i,
+						   2 * i + 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);
+	}
+
+	if (st->spi->irq && st->buffered_read_enabled)
+		mod_timer(&st->alert_timer,
+			  jiffies +
+			  msecs_to_jiffies(st->alert_reset_timeout_ms));
+
+	return IRQ_HANDLED;
+}
+
 static int ad7380_probe(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
@@ -1361,6 +1674,39 @@ 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) {
+		struct iio_chan_spec *chans;
+		size_t size;
+		int ret;
+
+		ret = devm_request_threaded_irq(dev, spi->irq,
+						&ad7380_primary_event_handler,
+						&ad7380_event_handler,
+						IRQF_TRIGGER_FALLING | IRQF_ONESHOT
+						| IRQF_NO_AUTOEN,
+						indio_dev->name,
+						indio_dev);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to request irq\n");
+
+		atomic_set(&st->irq_enabled, 0);
+
+		/*
+		 * Copy channels to be able to update the event_spec, since some
+		 * attributes are visible only when irq are configured
+		 */
+		size = indio_dev->num_channels * sizeof(*indio_dev->channels);
+		chans = devm_kzalloc(dev, size, GFP_KERNEL);
+		if (!chans)
+			return -ENOMEM;
+
+		memcpy(chans, indio_dev->channels, size);
+		chans->event_spec = ad7380_events_irq;
+		chans->num_event_specs = ARRAY_SIZE(ad7380_events_irq);
+
+		indio_dev->channels = chans;
+	}
+
 	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
 					      iio_pollfunc_store_time,
 					      ad7380_trigger_handler,

-- 
2.47.1

Re: [PATCH RFC v2 3/4] iio: adc: ad7380: add alert support

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 8115 bytes --]

Hello Julien,

On Tue, Dec 24, 2024 at 10:34:32AM +0100, Julien Stephan wrote:
> +static void ad7380_enable_irq(struct ad7380_state *st)
> +{
> +	if (st->spi->irq && !atomic_cmpxchg(&st->irq_enabled, 0, 1))
> +		enable_irq(st->spi->irq);
> +}
> +
> +static void ad7380_disable_irq(struct ad7380_state *st)
> +{
> +	if (st->spi->irq && atomic_cmpxchg(&st->irq_enabled, 1, 0)) {
> +		disable_irq(st->spi->irq);
> +		synchronize_irq(st->spi->irq);

disable_irq() already cares for synchronize_irq().

> +	}
> +}
> +
> +static void ad7380_disable_irq_nosync(struct ad7380_state *st)
> +{
> +	if (st->spi->irq && atomic_cmpxchg(&st->irq_enabled, 1, 0))
> +		disable_irq_nosync(st->spi->irq);
> +}
> +
>  static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
>  				     u32 writeval, u32 *readval)
>  {
> @@ -727,8 +791,8 @@ static int ad7380_regval_to_osr(int regval)
>  
>  static int ad7380_get_osr(struct ad7380_state *st, int *val)
>  {
> -	int tmp;
> -	int ret = 0;
> +	u32 tmp;
> +	int ret;

unrelated change?

>  	ret = regmap_read(st->regmap, AD7380_REG_ADDR_CONFIG1, &tmp);
>  	if (ret)
> @@ -834,6 +898,13 @@ static int ad7380_update_xfers(struct ad7380_state *st,
>  	return 0;
>  }
>  
> +static void ad7380_handle_event_reset_timeout(struct timer_list *t)
> +{
> +	struct ad7380_state *st = from_timer(st, t, alert_timer);
> +
> +	ad7380_enable_irq(st);
> +}
> +
>  static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
>  {
>  	struct ad7380_state *st = iio_priv(indio_dev);
> @@ -841,6 +912,12 @@ static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
>  	struct spi_message *msg = &st->normal_msg;
>  	int ret;
>  
> +	timer_setup(&st->alert_timer, ad7380_handle_event_reset_timeout, 0);
> +
> +	ad7380_enable_irq(st);
> +
> +	st->buffered_read_enabled = true;
> +
>  	/*
>  	 * Currently, we always read all channels at the same time. The scan_type
>  	 * is the same for all channels, so we just pass the first channel.
> @@ -894,6 +971,8 @@ static int ad7380_triggered_buffer_postdisable(struct iio_dev *indio_dev)
>  	struct spi_message *msg = &st->normal_msg;
>  	int ret;
>  
> +	st->buffered_read_enabled = false;
> +
>  	if (st->seq) {
>  		ret = regmap_update_bits(st->regmap,
>  					 AD7380_REG_ADDR_CONFIG1,
> @@ -908,6 +987,9 @@ static int ad7380_triggered_buffer_postdisable(struct iio_dev *indio_dev)
>  
>  	spi_unoptimize_message(msg);
>  
> +	ad7380_disable_irq(st);
> +	timer_shutdown_sync(&st->alert_timer);

If the timer triggers between ad7380_disable_irq() and
timer_shutdown_sync() the irq ends up enabled. It's unclear to me why
the irq is enabled in a timer.

> +
>  	return 0;
>  }
>  
> @@ -1002,8 +1084,11 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
>  		if (ret)
>  			return ret;
>  
> -		ret = ad7380_read_direct(st, chan->scan_index,
> -					 scan_type, val);
> +		ad7380_enable_irq(st);
> +
> +		ret = ad7380_read_direct(st, chan->scan_index, scan_type, val);
> +
> +		ad7380_disable_irq(st);

Why do you enable the irq only during register reading? Wouldn't it be
natural to have it enabled constantly (assuming a valid limit is
configured)?

>  		iio_device_release_direct_mode(indio_dev);
>  
> @@ -1151,12 +1236,190 @@ 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);
> +	int alert_en, tmp, ret;
> +
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD7380_REG_ADDR_CONFIG1, &tmp);
> +
> +	iio_device_release_direct_mode(indio_dev);
> +
> +	if (ret)
> +		return ret;
> +
> +	alert_en = FIELD_GET(AD7380_CONFIG1_ALERTEN, tmp);
> +
> +	return alert_en;
> +}
> +
> +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,
> +				     bool state)
> +{
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(st->regmap,
> +				 AD7380_REG_ADDR_CONFIG1,
> +				 AD7380_CONFIG1_ALERTEN,
> +				 FIELD_PREP(AD7380_CONFIG1_ALERTEN, state));
> +
> +	iio_device_release_direct_mode(indio_dev);
> +
> +	if (ret)
> +		return ret;
> +
> +	return 0;

The last three code lines are equivalent to a plain

	return ret;

> +}
> +
> [...]
> @@ -1190,6 +1459,50 @@ static int ad7380_init(struct ad7380_state *st, bool external_ref_en)
>  					     AD7380_NUM_SDO_LINES));
>  }
>  
> +static irqreturn_t ad7380_primary_event_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +
> +	if (!atomic_read(&st->irq_enabled))
> +		return IRQ_NONE;
> +
> +	ad7380_disable_irq_nosync(st);

I wonder why this is needed. The irq is requested with IRQF_ONESHOT, so
the irq is already masked here? And if you drop the masking here, you
can (I think) simplify the irq state tracking and drop the _nosync
variant completely.

> +	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];
> +	unsigned int i;
> +
> +	for (i = 0; i < st->chip_info->num_channels - 1; i++) {
> +		iio_push_event(indio_dev,
> +			       chan->differential ?
> +			       IIO_DIFF_EVENT_CODE(IIO_VOLTAGE,
> +						   2 * i,
> +						   2 * i + 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);
> +	}
> +
> +	if (st->spi->irq && st->buffered_read_enabled)
> +		mod_timer(&st->alert_timer,
> +			  jiffies +
> +			  msecs_to_jiffies(st->alert_reset_timeout_ms));
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int ad7380_probe(struct spi_device *spi)
>  {
>  	struct device *dev = &spi->dev;
> @@ -1361,6 +1674,39 @@ 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) {
> +		struct iio_chan_spec *chans;
> +		size_t size;
> +		int ret;
> +
> +		ret = devm_request_threaded_irq(dev, spi->irq,
> +						&ad7380_primary_event_handler,
> +						&ad7380_event_handler,
> +						IRQF_TRIGGER_FALLING | IRQF_ONESHOT

I didn't double check, but I think with the explicit
IRQF_TRIGGER_FALLING you overwrite the trigger info from the device
tree.

> +						| IRQF_NO_AUTOEN,
> +						indio_dev->name,
> +						indio_dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to request irq\n");
> +
> +		atomic_set(&st->irq_enabled, 0);
> +
> +		/*
> +		 * Copy channels to be able to update the event_spec, since some
> +		 * attributes are visible only when irq are configured
> +		 */
> +		size = indio_dev->num_channels * sizeof(*indio_dev->channels);
> +		chans = devm_kzalloc(dev, size, GFP_KERNEL);
> +		if (!chans)
> +			return -ENOMEM;
> +
> +		memcpy(chans, indio_dev->channels, size);
> +		chans->event_spec = ad7380_events_irq;
> +		chans->num_event_specs = ARRAY_SIZE(ad7380_events_irq);
> +
> +		indio_dev->channels = chans;
> +	}
> +
>  	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
>  					      iio_pollfunc_store_time,
>  					      ad7380_trigger_handler,

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC v2 3/4] iio: adc: ad7380: add alert support

[Jonathan Cameron]

On Tue, 24 Dec 2024 10:34:32 +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
> 
> If the interrupt properties is present in the device tree, an IIO event
> will be generated for each interrupt received.
> 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.
> 
> In buffered reads, if input stays out of thresholds limit, an interrupt
> will be generated for each sample read, because the alert pin is cleared
> on a falling edge of CS (i.e when starting a new conversion). To avoid
> generating to much interrupt, we introduce a reset_timeout that can be
> used to disable interrupt for a given time (in ms)
> 
>   events/thresh_either_reset_timeout
> 
> When an interrupt is received, interrupts are disabled and re-enabled
> after thresh_either_reset_timeout ms. If the reset timeout is set to 0,
> interrupt are re-enabled directly.
> Note: interrupts are always disabled at least during the handling of the
> previous interrupt, because each read triggers 2 transactions, that can
> lead to 2 interrupts for a single user read. IRQF_ONESHOT is not enough,
> because, it postpones the 2nd irq after the handling of the first one,
> which can still trigger 2 interrupts for a single user read.

After some of our recent discussions around interrupt handling and
the guarantees (that aren't) made, even disabling the interrupt doesn't
prevent some irq chips queuing up future interrupts.

https://lore.kernel.org/all/io53lznz3qp3jd5rohqsjhosnmdzd6d44sdbwu5jcfrs3rz2a2@orquwgflrtyc/

I'm not sure this alert can actually work as a result :(
I am struggling to come up with a scheme that will work.


> 
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
> ---
>  drivers/iio/adc/ad7380.c | 354 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 350 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index b49067c36fdd1bb0e760faf71d7fa0c8c1f610e9..fb59c2c064f402ea797fc0e563bd535d0e0bf785 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -34,6 +34,7 @@

...

> +
>  enum {
>  	AD7380_SCAN_TYPE_NORMAL,
>  	AD7380_SCAN_TYPE_RESOLUTION_BOOST,
> @@ -214,6 +252,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),				\

These are set below. So why here as well?

>  }

>  static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
>  				     u32 writeval, u32 *readval)
>  {
> @@ -727,8 +791,8 @@ static int ad7380_regval_to_osr(int regval)
>  
>  static int ad7380_get_osr(struct ad7380_state *st, int *val)
>  {
> -	int tmp;
> -	int ret = 0;
> +	u32 tmp;
> +	int ret;

Why these changes?

>  
>  	ret = regmap_read(st->regmap, AD7380_REG_ADDR_CONFIG1, &tmp);
>  	if (ret)
> @@ -834,6 +898,13 @@ static int ad7380_update_xfers(struct ad7380_state *st,
>  	return 0;
>  }


> @@ -1002,8 +1084,11 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
>  		if (ret)
>  			return ret;
>  
> -		ret = ad7380_read_direct(st, chan->scan_index,
> -					 scan_type, val);
> +		ad7380_enable_irq(st);
> +
> +		ret = ad7380_read_direct(st, chan->scan_index, scan_type, val);

Avoid the reformat.  It's fine in principle but in practice it makes it
harder to see there is no change here, so not in a patching doing anything else.

> +
> +		ad7380_disable_irq(st);
>  
>  		iio_device_release_direct_mode(indio_dev);
>  
> @@ -1151,12 +1236,190 @@ static int ad7380_get_current_scan_type(const struct iio_dev *indio_dev,
>  					    : AD7380_SCAN_TYPE_NORMAL;

> +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,
> +				     bool state)
> +{
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(st->regmap,
> +				 AD7380_REG_ADDR_CONFIG1,
> +				 AD7380_CONFIG1_ALERTEN,
> +				 FIELD_PREP(AD7380_CONFIG1_ALERTEN, state));
> +
> +	iio_device_release_direct_mode(indio_dev);
> +
	return ret;

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +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);
> +	int ret, tmp;
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +
Check ret

> +		switch (dir) {
> +		case IIO_EV_DIR_RISING:
> +			ret = regmap_read(st->regmap,
> +					  AD7380_REG_ADDR_ALERT_HIGH_TH,
> +					  &tmp);
> +			if (ret)
> +				return ret;
Holding direct mode.  As below. Factor out everything with direct mode
held into a separate function. That can then return directly
where it makes sense with the release handled by the caller.

> +
> +			*val = FIELD_GET(AD7380_ALERT_HIGH_TH, tmp);
> +			ret = IIO_VAL_INT;
> +			break;
> +		case IIO_EV_DIR_FALLING:
> +			ret = regmap_read(st->regmap,
> +					  AD7380_REG_ADDR_ALERT_LOW_TH,
> +					  &tmp);
> +			if (ret)
> +				return ret;
> +
> +			FIELD_GET(AD7380_ALERT_LOW_TH, tmp);
> +			ret = IIO_VAL_INT;
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		iio_device_release_direct_mode(indio_dev);
> +		return ret;
> +	case IIO_EV_INFO_RESET_TIMEOUT:
> +		*val = st->alert_reset_timeout_ms;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +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)
> +{
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +	const struct iio_scan_type *scan_type;
> +	int ret;
> +	u16 th;
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
Similar to earlier suggestion. I'd factor the contents of this out
as a helper.... Thus avoiding...
> +
> +		/*
> +		 * According to the datasheet,
> +		 * AD7380_REG_ADDR_ALERT_HIGH_TH[11:0] are the 12 MSB of the
> +		 * 16-bits internal alert high register. LSB are set to 0xf.
> +		 * AD7380_REG_ADDR_ALERT_LOW_TH[11:0] are the 12 MSB of the
> +		 * 16 bits internal alert low register. LSB are set to 0x0.
> +		 *
> +		 * When alert is enabled the conversion from the adc is compared

ADC

> +		 * immediately to the alert high/low thresholds, before any
> +		 * oversampling. This means that the thresholds are the same for
> +		 * normal mode and oversampling mode.
> +		 */
> +
> +		/* Extract the 12 MSB of val */
> +		scan_type = iio_get_current_scan_type(indio_dev, chan);
> +		if (IS_ERR(scan_type))

...this.  You are still holding the device in direct mode which it can never
leave.  With a helper direct returns here and below work fine giving
cleaner code.

> +			return PTR_ERR(scan_type);
> +
> +		th = val >> (scan_type->realbits - 12);
> +
> +		switch (dir) {
> +		case IIO_EV_DIR_RISING:
> +			ret = regmap_write(st->regmap,
> +					   AD7380_REG_ADDR_ALERT_HIGH_TH,
> +					   th);
> +			if (ret)
> +				return ret;
> +
> +			break;
> +		case IIO_EV_DIR_FALLING:
> +			ret = regmap_write(st->regmap,
> +					   AD7380_REG_ADDR_ALERT_LOW_TH,
> +					   th);
> +			if (ret)
> +				return ret;
> +
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		iio_device_release_direct_mode(indio_dev);
> +		return ret;
> +	case IIO_EV_INFO_RESET_TIMEOUT:
> +		if (val < 0)
> +			return -EINVAL;
> +		st->alert_reset_timeout_ms = val;
> +		timer_reduce(&st->alert_timer, jiffies + msecs_to_jiffies(val));
> +
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}

>  static int ad7380_probe(struct spi_device *spi)
>  {
>  	struct device *dev = &spi->dev;
> @@ -1361,6 +1674,39 @@ 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) {
> +		struct iio_chan_spec *chans;
> +		size_t size;
> +		int ret;
> +
> +		ret = devm_request_threaded_irq(dev, spi->irq,
> +						&ad7380_primary_event_handler,
> +						&ad7380_event_handler,
> +						IRQF_TRIGGER_FALLING | IRQF_ONESHOT

As Uwe mentioned. Normally we'd not have the direction here. The interrupt
may be wired to a different pin with an inverter in the path for example.
Hence this is left as a problem for DT.


> +						| IRQF_NO_AUTOEN,
> +						indio_dev->name,
> +						indio_dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to request irq\n");
> +
> +		atomic_set(&st->irq_enabled, 0);
> +
> +		/*
> +		 * Copy channels to be able to update the event_spec, since some
> +		 * attributes are visible only when irq are configured
> +		 */
> +		size = indio_dev->num_channels * sizeof(*indio_dev->channels);
> +		chans = devm_kzalloc(dev, size, GFP_KERNEL);

devm_kmemdup()

Normally I'd argue for multiple static const arrays, but I guess there are a lot
of options here so that would become huge so this seems reasonable.


> +		if (!chans)
> +			return -ENOMEM;
> +
> +		memcpy(chans, indio_dev->channels, size);
> +		chans->event_spec = ad7380_events_irq;
> +		chans->num_event_specs = ARRAY_SIZE(ad7380_events_irq);
> +
> +		indio_dev->channels = chans;
> +	}
> +
>  	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
>  					      iio_pollfunc_store_time,
>  					      ad7380_trigger_handler,
> 

Re: [PATCH RFC v2 3/4] iio: adc: ad7380: add alert support

[David Lechner]

On 12/28/24 8:24 AM, Jonathan Cameron wrote:
> On Tue, 24 Dec 2024 10:34:32 +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
>>
>> If the interrupt properties is present in the device tree, an IIO event
>> will be generated for each interrupt received.
>> 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.
>>
>> In buffered reads, if input stays out of thresholds limit, an interrupt
>> will be generated for each sample read, because the alert pin is cleared
>> on a falling edge of CS (i.e when starting a new conversion). To avoid
>> generating to much interrupt, we introduce a reset_timeout that can be
>> used to disable interrupt for a given time (in ms)
>>
>>   events/thresh_either_reset_timeout
>>
>> When an interrupt is received, interrupts are disabled and re-enabled
>> after thresh_either_reset_timeout ms. If the reset timeout is set to 0,
>> interrupt are re-enabled directly.
>> Note: interrupts are always disabled at least during the handling of the
>> previous interrupt, because each read triggers 2 transactions, that can
>> lead to 2 interrupts for a single user read. IRQF_ONESHOT is not enough,
>> because, it postpones the 2nd irq after the handling of the first one,
>> which can still trigger 2 interrupts for a single user read.
> 
> After some of our recent discussions around interrupt handling and
> the guarantees (that aren't) made, even disabling the interrupt doesn't
> prevent some irq chips queuing up future interrupts.
> 
> https://lore.kernel.org/all/io53lznz3qp3jd5rohqsjhosnmdzd6d44sdbwu5jcfrs3rz2a2@orquwgflrtyc/
> 
> I'm not sure this alert can actually work as a result :(
> I am struggling to come up with a scheme that will work.
> 
Would it work if we change it to a level-triggered interrupt instead of edge
triggered?

Since the main purpose of this is to trigger a hardware shutdown, perhaps we
could just omit the interrupt/emitting the event and keep the threshold and
enable attributes if we can't come up with a reasonable way to handle the
interrupts?

Re: [PATCH RFC v2 3/4] iio: adc: ad7380: add alert support

[Julien Stephan]

Le lun. 6 janv. 2025 à 16:29, David Lechner <dlechner@baylibre.com> a écrit :
>
> On 12/28/24 8:24 AM, Jonathan Cameron wrote:
> > On Tue, 24 Dec 2024 10:34:32 +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
> >>
> >> If the interrupt properties is present in the device tree, an IIO event
> >> will be generated for each interrupt received.
> >> 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.
> >>
> >> In buffered reads, if input stays out of thresholds limit, an interrupt
> >> will be generated for each sample read, because the alert pin is cleared
> >> on a falling edge of CS (i.e when starting a new conversion). To avoid
> >> generating to much interrupt, we introduce a reset_timeout that can be
> >> used to disable interrupt for a given time (in ms)
> >>
> >>   events/thresh_either_reset_timeout
> >>
> >> When an interrupt is received, interrupts are disabled and re-enabled
> >> after thresh_either_reset_timeout ms. If the reset timeout is set to 0,
> >> interrupt are re-enabled directly.
> >> Note: interrupts are always disabled at least during the handling of the
> >> previous interrupt, because each read triggers 2 transactions, that can
> >> lead to 2 interrupts for a single user read. IRQF_ONESHOT is not enough,
> >> because, it postpones the 2nd irq after the handling of the first one,
> >> which can still trigger 2 interrupts for a single user read.
> >
> > After some of our recent discussions around interrupt handling and
> > the guarantees (that aren't) made, even disabling the interrupt doesn't
> > prevent some irq chips queuing up future interrupts.
> >
> > https://lore.kernel.org/all/io53lznz3qp3jd5rohqsjhosnmdzd6d44sdbwu5jcfrs3rz2a2@orquwgflrtyc/
> >
> > I'm not sure this alert can actually work as a result :(
> > I am struggling to come up with a scheme that will work.
> >
> Would it work if we change it to a level-triggered interrupt instead of edge
> triggered?
>
> Since the main purpose of this is to trigger a hardware shutdown, perhaps we
> could just omit the interrupt/emitting the event and keep the threshold and
> enable attributes if we can't come up with a reasonable way to handle the
> interrupts?
>

Hi Jonathan, and David,

I think this is getting very complicated for something not that useful
in practice.
If needed we can go back on this later to find an appropriate solution.
I sent a non RFC V3 version, removing the interrupt handling? Does
that work for you?

Cheers
Julein

Re: [PATCH RFC v2 3/4] iio: adc: ad7380: add alert support

[Jonathan Cameron]

On Tue, 7 Jan 2025 09:51:23 +0100
Julien Stephan <jstephan@baylibre.com> wrote:

> Le lun. 6 janv. 2025 à 16:29, David Lechner <dlechner@baylibre.com> a écrit :
> >
> > On 12/28/24 8:24 AM, Jonathan Cameron wrote:  
> > > On Tue, 24 Dec 2024 10:34:32 +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
> > >>
> > >> If the interrupt properties is present in the device tree, an IIO event
> > >> will be generated for each interrupt received.
> > >> 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.
> > >>
> > >> In buffered reads, if input stays out of thresholds limit, an interrupt
> > >> will be generated for each sample read, because the alert pin is cleared
> > >> on a falling edge of CS (i.e when starting a new conversion). To avoid
> > >> generating to much interrupt, we introduce a reset_timeout that can be
> > >> used to disable interrupt for a given time (in ms)
> > >>
> > >>   events/thresh_either_reset_timeout
> > >>
> > >> When an interrupt is received, interrupts are disabled and re-enabled
> > >> after thresh_either_reset_timeout ms. If the reset timeout is set to 0,
> > >> interrupt are re-enabled directly.
> > >> Note: interrupts are always disabled at least during the handling of the
> > >> previous interrupt, because each read triggers 2 transactions, that can
> > >> lead to 2 interrupts for a single user read. IRQF_ONESHOT is not enough,
> > >> because, it postpones the 2nd irq after the handling of the first one,
> > >> which can still trigger 2 interrupts for a single user read.  
> > >
> > > After some of our recent discussions around interrupt handling and
> > > the guarantees (that aren't) made, even disabling the interrupt doesn't
> > > prevent some irq chips queuing up future interrupts.
> > >
> > > https://lore.kernel.org/all/io53lznz3qp3jd5rohqsjhosnmdzd6d44sdbwu5jcfrs3rz2a2@orquwgflrtyc/
> > >
> > > I'm not sure this alert can actually work as a result :(
> > > I am struggling to come up with a scheme that will work.
> > >  
> > Would it work if we change it to a level-triggered interrupt instead of edge
> > triggered?

Whilst I'd hope it would I'm not 100% sure.

> >
> > Since the main purpose of this is to trigger a hardware shutdown, perhaps we
> > could just omit the interrupt/emitting the event and keep the threshold and
> > enable attributes if we can't come up with a reasonable way to handle the
> > interrupts?
> >  
> 
> Hi Jonathan, and David,
> 
> I think this is getting very complicated for something not that useful
> in practice.
> If needed we can go back on this later to find an appropriate solution.
> I sent a non RFC V3 version, removing the interrupt handling? Does
> that work for you?

Works for me.

> 
> Cheers
> Julein
> 

Re: [PATCH RFC v2 3/4] iio: adc: ad7380: add alert support

[Jonathan Cameron]

On Sat, 11 Jan 2025 12:51:45 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 7 Jan 2025 09:51:23 +0100
> Julien Stephan <jstephan@baylibre.com> wrote:
> 
> > Le lun. 6 janv. 2025 à 16:29, David Lechner <dlechner@baylibre.com> a écrit :  
> > >
> > > On 12/28/24 8:24 AM, Jonathan Cameron wrote:    
> > > > On Tue, 24 Dec 2024 10:34:32 +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
> > > >>
> > > >> If the interrupt properties is present in the device tree, an IIO event
> > > >> will be generated for each interrupt received.
> > > >> 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.
> > > >>
> > > >> In buffered reads, if input stays out of thresholds limit, an interrupt
> > > >> will be generated for each sample read, because the alert pin is cleared
> > > >> on a falling edge of CS (i.e when starting a new conversion). To avoid
> > > >> generating to much interrupt, we introduce a reset_timeout that can be
> > > >> used to disable interrupt for a given time (in ms)
> > > >>
> > > >>   events/thresh_either_reset_timeout
> > > >>
> > > >> When an interrupt is received, interrupts are disabled and re-enabled
> > > >> after thresh_either_reset_timeout ms. If the reset timeout is set to 0,
> > > >> interrupt are re-enabled directly.
> > > >> Note: interrupts are always disabled at least during the handling of the
> > > >> previous interrupt, because each read triggers 2 transactions, that can
> > > >> lead to 2 interrupts for a single user read. IRQF_ONESHOT is not enough,
> > > >> because, it postpones the 2nd irq after the handling of the first one,
> > > >> which can still trigger 2 interrupts for a single user read.    
> > > >
> > > > After some of our recent discussions around interrupt handling and
> > > > the guarantees (that aren't) made, even disabling the interrupt doesn't
> > > > prevent some irq chips queuing up future interrupts.
> > > >
> > > > https://lore.kernel.org/all/io53lznz3qp3jd5rohqsjhosnmdzd6d44sdbwu5jcfrs3rz2a2@orquwgflrtyc/
> > > >
> > > > I'm not sure this alert can actually work as a result :(
> > > > I am struggling to come up with a scheme that will work.
> > > >    
> > > Would it work if we change it to a level-triggered interrupt instead of edge
> > > triggered?  
> 
> Whilst I'd hope it would I'm not 100% sure.

How much do we care about a potential double report?  Maybe we just let it happen?
Does it create an infinite chain?

Jonathan

> 
> > >
> > > Since the main purpose of this is to trigger a hardware shutdown, perhaps we
> > > could just omit the interrupt/emitting the event and keep the threshold and
> > > enable attributes if we can't come up with a reasonable way to handle the
> > > interrupts?
> > >    
> > 
> > Hi Jonathan, and David,
> > 
> > I think this is getting very complicated for something not that useful
> > in practice.
> > If needed we can go back on this later to find an appropriate solution.
> > I sent a non RFC V3 version, removing the interrupt handling? Does
> > that work for you?  
> 
> Works for me.
> 
> > 
> > Cheers
> > Julein
> >   
> 
> 

[PATCH RFC v2 4/4] docs: iio: ad7380: add alert support

[Julien Stephan]

Add a section for alert support, explaining how user can use iio events
attributes to enable alert and set thresholds, but also what kind of
events will be generated.

Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---
 Documentation/iio/ad7380.rst | 56 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/Documentation/iio/ad7380.rst b/Documentation/iio/ad7380.rst
index c46127700e14ca9ec3cac0bd5776b6702f2659e2..7cca4a9ad42ae8b3fda2e063ffd864ffa9dac2f1 100644
--- a/Documentation/iio/ad7380.rst
+++ b/Documentation/iio/ad7380.rst
@@ -92,6 +92,61 @@ must restart iiod using the following command:
 
 	root:~# systemctl restart iiod
 
+Alert
+-----
+
+When configured in 1 SDO line mode (see `SPI wiring modes`_), the SDOB or the
+SDOD line (respectively for the 2 or 4 channels variants) can act as an alert
+pin.
+
+At the end of a conversion the low-active alert pin gets asserted if the
+conversion result exceeds the alert high limit or falls below the alert low
+limit. It is cleared, on a falling edge of CS. The alert pin is common to all
+channels.
+
+User can enable alert using the regular iio events attribute:
+
+.. code-block:: bash
+
+	events/thresh_either_en
+
+The high and low thresholds are common to all channels and can also be set using
+regular iio events attributes:
+
+.. code-block:: bash
+
+	events/in_thresh_falling_value
+	events/in_thresh_rising_value
+
+User space IIO events
+~~~~~~~~~~~~~~~~~~~~~
+
+If the ``interrupts`` property is set in the device tree, IIO events will be
+generated for alerts.  A register identifies the faulty channel, and direction,
+but during buffered reads, registers are inaccessible, making it impossible to
+know the exact channel triggering the alert. A generic event is sent for each
+channel, resulting in 2 or 4 events per alert, depending on the number of
+channels:
+
+.. code-block:: bash
+
+	Event: time: 1733501917162945723, type: voltage, channel: 0-1, evtype: thresh, direction: either
+	Event: time: 1733501917162945723, type: voltage, channel: 2-3, evtype: thresh, direction: either
+
+
+Alert Reset timeout (buffered read only)
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+In buffered reads, if input stays out of thresholds limit, an interrupt will be
+generated for each sample read, because the alert pin is cleared when CS get
+active (i.e when starting a new conversion). As a result, excessive event
+generation can occur. User can set a reset timeout in milliseconds, during
+which interrupt will be disabled:
+
+.. code-block:: bash
+
+	events/thresh_either_reset_timeout
+
 Channel selection and sequencer (single-end chips only)
 -------------------------------------------------------
 
@@ -144,7 +199,6 @@ Unimplemented features
 - Rolling average oversampling
 - Power down mode
 - CRC indication
-- Alert
 
 
 Device buffers

-- 
2.47.1