Re: [PATCH v9 2/6] iio: adc: ad4691: add initial driver for AD4691 family

Re: [PATCH v9 2/6] iio: adc: ad4691: add initial driver for AD4691 family

[Jonathan Cameron]

On Thu, 30 Apr 2026 13:16:44 +0300
Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org> wrote:

> From: Radu Sabau <radu.sabau@analog.com>
> 
> Add support for the Analog Devices AD4691 family of high-speed,
> low-power multichannel SAR ADCs: AD4691 (16-ch, 500 kSPS),
> AD4692 (16-ch, 1 MSPS), AD4693 (8-ch, 500 kSPS) and
> AD4694 (8-ch, 1 MSPS).
> 
> The driver implements a custom regmap layer over raw SPI to handle the
> device's mixed 1/2/3/4-byte register widths and uses the standard IIO
> read_raw/write_raw interface for single-channel reads.
> 
> The chip idles in Autonomous Mode so that single-shot read_raw can use
> the internal oscillator without disturbing the hardware configuration.
> 
> Three voltage supply domains are managed: avdd (required), vio, and a
> reference supply on either the REF pin (ref-supply, external buffer)
> or the REFIN pin (refin-supply, uses the on-chip reference buffer;
> REFBUF_EN is set accordingly). Hardware reset is performed via
> the reset controller framework; a software reset through SPI_CONFIG_A
> is used as fallback when no hardware reset is available.
> 
> Accumulator channel masking for single-shot reads uses ACC_MASK_REG via
> an ADDR_DESCENDING SPI write, which covers both mask bytes in a single
> 16-bit transfer.
> 
> Reviewed-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
Hi Radu

Just one query that Sashiko raised that made me look 
closer at how you are handling different register sizes.
https://sashiko.dev/#/patchset/20260430-ad4692-multichannel-sar-adc-driver-v9-0-33e439e4fb87%40analog.com

There was also a question about whether the sampling frequency control would
be better described as shared by all.

> diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> new file mode 100644
> index 000000000000..05826b762c7f
> --- /dev/null
> +++ b/drivers/iio/adc/ad4691.c


> +static int ad4691_reg_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +	struct spi_device *spi = context;
> +	u8 tx[2], rx[4];
> +	int ret;
> +
> +	/* Set bit 15 to mark the operation as READ. */
> +	put_unaligned_be16(0x8000 | reg, tx);
> +
> +	switch (reg) {
> +	case 0 ... AD4691_OSC_FREQ_REG:
> +	case AD4691_SPARE_CONTROL ... AD4691_ACC_SAT_OVR_REG(15):

Sashiko raised a query here.
"Will this result in a truncated 1-byte read for AD4691_ACC_MASK_REG (0x185)?
AD4691_ACC_MASK_REG falls into the range between AD4691_SPARE_CONTROL and
AD4691_ACC_SAT_OVR_REG(15). In ad4691_reg_write(), AD4691_ACC_MASK_REG is
handled explicitly alongside AD4691_STD_SEQ_CONFIG to perform a 16-bit
write, but it seems missing from the 2-byte read block here."

Just to check - the reasoning behind not just treating these as
fixed sized registers and using bulk reads and writes is the statement
about them being invalid if partially written?

The ACK_MASK_REG is documented as two separate 8 bit registers so why
attempt to treat it as a larger one?


> +		ret = spi_write_then_read(spi, tx, sizeof(tx), rx, 1);
> +		if (ret)
> +			return ret;
> +		*val = rx[0];
> +		return 0;
> +	case AD4691_STD_SEQ_CONFIG:
> +	case AD4691_AVG_IN(0) ... AD4691_AVG_IN(15):
> +		ret = spi_write_then_read(spi, tx, sizeof(tx), rx, 2);
> +		if (ret)
> +			return ret;
> +		*val = get_unaligned_be16(rx);
> +		return 0;
> +	case AD4691_AVG_STS_IN(0) ... AD4691_AVG_STS_IN(15):
> +	case AD4691_ACC_IN(0) ... AD4691_ACC_IN(15):
> +		ret = spi_write_then_read(spi, tx, sizeof(tx), rx, 3);
> +		if (ret)
> +			return ret;
> +		*val = get_unaligned_be24(rx);
> +		return 0;
> +	case AD4691_ACC_STS_DATA(0) ... AD4691_ACC_STS_DATA(15):
> +		ret = spi_write_then_read(spi, tx, sizeof(tx), rx, 4);
> +		if (ret)
> +			return ret;
> +		*val = get_unaligned_be32(rx);
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4691_reg_write(void *context, unsigned int reg, unsigned int val)
> +{
> +	struct spi_device *spi = context;
> +	u8 tx[4];
> +
> +	put_unaligned_be16(reg, tx);
> +
> +	switch (reg) {
> +	case 0 ... AD4691_OSC_FREQ_REG:
> +	case AD4691_SPARE_CONTROL ... AD4691_ACC_MASK_REG - 1:
> +	case AD4691_ACC_MASK_REG + 1 ... AD4691_GPIO_MODE2_REG:
> +		if (val > U8_MAX)
> +			return -EINVAL;
> +		tx[2] = val;
> +		return spi_write_then_read(spi, tx, 3, NULL, 0);
> +	case AD4691_ACC_MASK_REG:
> +	case AD4691_STD_SEQ_CONFIG:
> +		if (val > U16_MAX)
> +			return -EINVAL;
> +		put_unaligned_be16(val, &tx[2]);
> +		return spi_write_then_read(spi, tx, 4, NULL, 0);
> +	default:
> +		return -EINVAL;
> +	}
> +}

RE: [PATCH v9 2/6] iio: adc: ad4691: add initial driver for AD4691 family

[Sabau, Radu bogdan]

Addressing Sashiko's review for initial driver's patch.

> -----Original Message-----
> From: Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org>
> Sent: Thursday, April 30, 2026 1:17 PM

...

> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 60038ae8dfc4..3685a03aa8dc 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -139,6 +139,17 @@ config AD4170_4
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ad4170-4.
> 
> +config AD4691
> +	tristate "Analog Devices AD4691 Family ADC Driver"
> +	depends on SPI

"Should this driver also depend on REGULATOR? In ad4691_regulator_setup(),
it relies on devm_regulator_get_enable_read_voltage() to obtain its reference
voltage. If the kernel is compiled without CONFIG_REGULATOR, this function
is stubbed to return -ENODEV, causing probe to fail unconditionally."

Will add "depends on REGULATOR". Without it,
devm_regulator_get_enable_read_voltage() can be stubbed and
return -ENODEV, making the driver unusable regardless of hardware.

> +	select REGMAP
> +	help
> +	  Say yes here to build support for Analog Devices AD4691 Family

...

> +};
> +
> +#define AD4691_CHANNEL(ch)
> 	\
> +	{								\
> +		.type = IIO_VOLTAGE,					\
> +		.indexed = 1,						\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> 	\
> +				    | BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> +		.info_mask_separate_available =
> 	\
> +				      BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),
> 	\

"Since ad4691_set_sampling_freq() modifies the AD4691_OSC_FREQ_REG register
which controls the global internal oscillator frequency for the entire ADC,
should IIO_CHAN_INFO_SAMP_FREQ be exposed as info_mask_shared_by_all
instead of info_mask_separate? Changing the frequency for one channel
currently seems to silently alter it for all channels."

Agreed for this commit. With no per-channel OSR in the base driver
all channels share a single oscillator register, so info_mask_shared_by_all
is the correct annotation. Changing accordingly. The OSR commit switches it
back to info_mask_separate because at that point the effective rate is
genuinely per-channel (osc_freq / osr[N]).

> +		.channel = ch,						\
> +		.scan_index = ch,					\
> +		.scan_type = {						\
> +			.sign = 'u',					\
> +			.realbits = 16,					\
> +			.storagebits = 16,				\
> +		},							\
> +	}
> +

...

> +};
> +
> +static int ad4691_reg_read(void *context, unsigned int reg, unsigned int
> *val)
> +{
> +	struct spi_device *spi = context;
> +	u8 tx[2], rx[4];
> +	int ret;
> +
> +	/* Set bit 15 to mark the operation as READ. */
> +	put_unaligned_be16(0x8000 | reg, tx);
> +
> +	switch (reg) {
> +	case 0 ... AD4691_OSC_FREQ_REG:
> +	case AD4691_SPARE_CONTROL ... AD4691_ACC_SAT_OVR_REG(15):
> +		ret = spi_write_then_read(spi, tx, sizeof(tx), rx, 1);

"Will this result in a truncated 1-byte read for AD4691_ACC_MASK_REG (0x185)?
AD4691_ACC_MASK_REG falls into the range between AD4691_SPARE_CONTROL and
AD4691_ACC_SAT_OVR_REG(15). In ad4691_reg_write(), AD4691_ACC_MASK_REG is
handled explicitly alongside AD4691_STD_SEQ_CONFIG to perform a 16-bit
write, but it seems missing from the 2-byte read block here."

Valid bug. The write path already splits the range around ACC_MASK_REG; 
the read path was missing the same split. Fixed: ACC_MASK_REG now gets
a 2-byte read alongside STD_SEQ_CONFIG.

> +		if (ret)
> +			return ret;

...

> +
> +static int ad4691_reset(struct ad4691_state *st)
> +{
> +	struct device *dev = regmap_get_device(st->regmap);
> +	struct reset_control *rst;
> +
> +	rst = devm_reset_control_get_optional_exclusive(dev, NULL);
> +	if (IS_ERR(rst))
> +		return dev_err_probe(dev, PTR_ERR(rst), "Failed to get
> reset\n");
> +
> +	if (rst) {
> +		/*
> +		 * reset_gpio_probe() already drives the pin asserted, so the
> +		 * device is held in reset before we get here.
> +		 * devm_reset_control_get_optional_exclusive_deasserted()
> cannot
> +		 * be used because it deasserts immediately without delay; the
> +		 * datasheet (Table 5) requires a ≥300 µs reset pulse width
> +		 * before deassertion.
> +		 */
> +		fsleep(300);
> +		return reset_control_deassert(rst);

"Could this sequence break on driver reload?
When the driver is unloaded, the reset line remains in its deasserted state
because there is no cleanup action to re-assert it. Upon reloading, the 300
microsecond sleep will occur while the pin is already deasserted, meaning
the hardware might not receive the required reset pulse.
Should reset_control_assert() be called explicitly prior to the delay?"

Valid. reset_control_put() does not re-assert the line at unload, so
on reload the pin is already deasserted when we enter the 300 µs sleep.
Adding reset_control_assert(rst) before fsleep(300) to guarantee a proper
reset pulse on every probe including reloads.

> +	}
> +

...

> +
> +	st = iio_priv(indio_dev);
> +	st->info = spi_get_device_match_data(spi);

"Is it possible for st->info to be NULL here?
If the driver is manually bound to a device with a non-matching name using
the sysfs bind interface in combination with driver_override,
spi_get_device_match_data() could return NULL, which would cause a NULL
pointer dereference later in the probe sequence."

Agreed, will add the NULL check with -ENODEV immediately after
spi_get_device_match_data().

> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;

Re: [PATCH v9 2/6] iio: adc: ad4691: add initial driver for AD4691 family

[Jonathan Cameron]

On Thu, 7 May 2026 09:26:00 +0000
"Sabau, Radu bogdan" <Radu.Sabau@analog.com> wrote:

> Addressing Sashiko's review for initial driver's patch.
> 
> > -----Original Message-----
> > From: Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org>
> > Sent: Thursday, April 30, 2026 1:17 PM  
> 
> ...
> 
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 60038ae8dfc4..3685a03aa8dc 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -139,6 +139,17 @@ config AD4170_4
> >  	  To compile this driver as a module, choose M here: the module will be
> >  	  called ad4170-4.
> > 
> > +config AD4691
> > +	tristate "Analog Devices AD4691 Family ADC Driver"
> > +	depends on SPI  
> 
> "Should this driver also depend on REGULATOR? In ad4691_regulator_setup(),
> it relies on devm_regulator_get_enable_read_voltage() to obtain its reference
> voltage. If the kernel is compiled without CONFIG_REGULATOR, this function
> is stubbed to return -ENODEV, causing probe to fail unconditionally."
> 
> Will add "depends on REGULATOR". Without it,
> devm_regulator_get_enable_read_voltage() can be stubbed and
> return -ENODEV, making the driver unusable regardless of hardware.

If you really want to also add || COMPILE_TEST because
I want maximum build coverage and for that I don't mind if the driver
can actually probe or not.

> 
> > +	select REGMAP
> > +	help
> > +	  Say yes here to build support for Analog Devices AD4691 Family  
> 
> ...
> 
> > +};
> > +
> > +#define AD4691_CHANNEL(ch)
> > 	\
> > +	{								\
> > +		.type = IIO_VOLTAGE,					\
> > +		.indexed = 1,						\
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> > 	\
> > +				    | BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> > +		.info_mask_separate_available =
> > 	\
> > +				      BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> > +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),
> > 	\  
> 
> "Since ad4691_set_sampling_freq() modifies the AD4691_OSC_FREQ_REG register
> which controls the global internal oscillator frequency for the entire ADC,
> should IIO_CHAN_INFO_SAMP_FREQ be exposed as info_mask_shared_by_all
> instead of info_mask_separate? Changing the frequency for one channel
> currently seems to silently alter it for all channels."
> 
> Agreed for this commit. With no per-channel OSR in the base driver
> all channels share a single oscillator register, so info_mask_shared_by_all
> is the correct annotation. Changing accordingly. The OSR commit switches it
> back to info_mask_separate because at that point the effective rate is
> genuinely per-channel (osc_freq / osr[N]).

Don't.  That would mean an ABI change mid way through the series which we
don't want. Just add a note to the patch description.

...


> 
> > +
> > +static int ad4691_reset(struct ad4691_state *st)
> > +{
> > +	struct device *dev = regmap_get_device(st->regmap);
> > +	struct reset_control *rst;
> > +
> > +	rst = devm_reset_control_get_optional_exclusive(dev, NULL);
> > +	if (IS_ERR(rst))
> > +		return dev_err_probe(dev, PTR_ERR(rst), "Failed to get
> > reset\n");
> > +
> > +	if (rst) {
> > +		/*
> > +		 * reset_gpio_probe() already drives the pin asserted, so the
> > +		 * device is held in reset before we get here.
> > +		 * devm_reset_control_get_optional_exclusive_deasserted()
> > cannot
> > +		 * be used because it deasserts immediately without delay; the
> > +		 * datasheet (Table 5) requires a ≥300 µs reset pulse width
> > +		 * before deassertion.
> > +		 */
> > +		fsleep(300);
> > +		return reset_control_deassert(rst);  
> 
> "Could this sequence break on driver reload?
> When the driver is unloaded, the reset line remains in its deasserted state
> because there is no cleanup action to re-assert it. Upon reloading, the 300
> microsecond sleep will occur while the pin is already deasserted, meaning
> the hardware might not receive the required reset pulse.
> Should reset_control_assert() be called explicitly prior to the delay?"
> 
> Valid. reset_control_put() does not re-assert the line at unload, so
> on reload the pin is already deasserted when we enter the 300 µs sleep.
> Adding reset_control_assert(rst) before fsleep(300) to guarantee a proper
> reset pulse on every probe including reloads.

I'm not that fussed about driver reload bugs like this one but why
not fix it I guess.  Lots of cases of this in upstream code though.

> 
> > +	}
> > +  
> 
> ...
> 
> > +
> > +	st = iio_priv(indio_dev);
> > +	st->info = spi_get_device_match_data(spi);  
> 
> "Is it possible for st->info to be NULL here?
> If the driver is manually bound to a device with a non-matching name using
> the sysfs bind interface in combination with driver_override,
> spi_get_device_match_data() could return NULL, which would cause a NULL
> pointer dereference later in the probe sequence."
> 
> Agreed, will add the NULL check with -ENODEV immediately after
> spi_get_device_match_data().

Andy, you seeing this one?  Looks like we are putting these checks back in again.
Whilst anyone forcing a bind like this is onto a looser anyway we shouldn't
crash due to a null dereference.

> 
> > +
> > +	ret = devm_mutex_init(dev, &st->lock);
> > +	if (ret)
> > +		return ret;  
> 

Re: [PATCH v9 2/6] iio: adc: ad4691: add initial driver for AD4691 family

[Andy Shevchenko]

On Thu, May 07, 2026 at 03:15:49PM +0100, Jonathan Cameron wrote:
> On Thu, 7 May 2026 09:26:00 +0000
> "Sabau, Radu bogdan" <Radu.Sabau@analog.com> wrote:

...

> > > +	st->info = spi_get_device_match_data(spi);  
> > 
> > "Is it possible for st->info to be NULL here?
> > If the driver is manually bound to a device with a non-matching name using
> > the sysfs bind interface in combination with driver_override,
> > spi_get_device_match_data() could return NULL, which would cause a NULL
> > pointer dereference later in the probe sequence."
> > 
> > Agreed, will add the NULL check with -ENODEV immediately after
> > spi_get_device_match_data().
> 
> Andy, you seeing this one?  Looks like we are putting these checks back in again.
> Whilst anyone forcing a bind like this is onto a looser anyway we shouldn't
> crash due to a null dereference.

We should find a way how to disable that combination from the start. The driver
makes no sense to be instantiated from user space. Actually most of the drivers
nowadays should not be bound to the devices without driver data.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 2/6] iio: adc: ad4691: add initial driver for AD4691 family

[Andy Shevchenko]

On Fri, May 08, 2026 at 07:44:39AM +0300, Andy Shevchenko wrote:
> On Thu, May 07, 2026 at 03:15:49PM +0100, Jonathan Cameron wrote:
> > On Thu, 7 May 2026 09:26:00 +0000
> > "Sabau, Radu bogdan" <Radu.Sabau@analog.com> wrote:

...

> > > > +	st->info = spi_get_device_match_data(spi);  
> > > 
> > > "Is it possible for st->info to be NULL here?
> > > If the driver is manually bound to a device with a non-matching name using
> > > the sysfs bind interface in combination with driver_override,
> > > spi_get_device_match_data() could return NULL, which would cause a NULL
> > > pointer dereference later in the probe sequence."
> > > 
> > > Agreed, will add the NULL check with -ENODEV immediately after
> > > spi_get_device_match_data().
> > 
> > Andy, you seeing this one?  Looks like we are putting these checks back in again.
> > Whilst anyone forcing a bind like this is onto a looser anyway we shouldn't
> > crash due to a null dereference.
> 
> We should find a way how to disable that combination from the start. The driver
> makes no sense to be instantiated from user space. Actually most of the drivers
> nowadays should not be bound to the devices without driver data.

The 20260508095224.1275645-1-andriy.shevchenko@linux.intel.com has been just sent.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 3/6] iio: adc: ad4691: add triggered buffer support

[Jonathan Cameron]

> 
> > > +	for (i = 0; i < ARRAY_SIZE(ad4691_gp_names); i++) {
> > > +		irq = fwnode_irq_get_byname(dev_fwnode(dev),
> > > +					    ad4691_gp_names[i]);
> > > +		if (irq > 0)
> > > +			break;  
> > 
> > This is problematic in case the above returns EPROBE_DEFER. Can you confirm
> > it
> > may not ever happen? (Note, I don't know the answer.)
> >   
> 
> You are right, thanks for this!
I'm missing something. Why is that a problem?  Driver will return
the error and a dev_err_probe() is used so it won't print anything.
So probe will fail which is exactly what we want.

Jonathan

> 

Re: [PATCH v9 3/6] iio: adc: ad4691: add triggered buffer support

[Andy Shevchenko]

On Tue, May 05, 2026 at 02:26:40PM +0100, Jonathan Cameron wrote:

...

> > > > +	for (i = 0; i < ARRAY_SIZE(ad4691_gp_names); i++) {
> > > > +		irq = fwnode_irq_get_byname(dev_fwnode(dev),
> > > > +					    ad4691_gp_names[i]);
> > > > +		if (irq > 0)
> > > > +			break;  
> > > 
> > > This is problematic in case the above returns EPROBE_DEFER. Can you confirm
> > > it
> > > may not ever happen? (Note, I don't know the answer.)
> > 
> > You are right, thanks for this!
> I'm missing something. Why is that a problem?  Driver will return
> the error and a dev_err_probe() is used so it won't print anything.
> So probe will fail which is exactly what we want.

If there are two IRQs and the first one is probe deferred and second returns
an error, we return that error instead of the deferral probe.

May be I missed something, but I have no idea how in this case it may return
the first error code in such a case.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 3/6] iio: adc: ad4691: add triggered buffer support

[Jonathan Cameron]

On Tue, 5 May 2026 17:58:21 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, May 05, 2026 at 02:26:40PM +0100, Jonathan Cameron wrote:
> 
> ...
> 
> > > > > +	for (i = 0; i < ARRAY_SIZE(ad4691_gp_names); i++) {
> > > > > +		irq = fwnode_irq_get_byname(dev_fwnode(dev),
> > > > > +					    ad4691_gp_names[i]);
> > > > > +		if (irq > 0)
> > > > > +			break;    
> > > > 
> > > > This is problematic in case the above returns EPROBE_DEFER. Can you confirm
> > > > it
> > > > may not ever happen? (Note, I don't know the answer.)  
> > > 
> > > You are right, thanks for this!  
> > I'm missing something. Why is that a problem?  Driver will return
> > the error and a dev_err_probe() is used so it won't print anything.
> > So probe will fail which is exactly what we want.  
> 
> If there are two IRQs and the first one is probe deferred and second returns
> an error, we return that error instead of the deferral probe.
> 
> May be I missed something, but I have no idea how in this case it may return
> the first error code in such a case.
Ah. Indeed. I completely misread the code.  if (irq) would do the job to fix this.
J
> 

Re: [PATCH v9 3/6] iio: adc: ad4691: add triggered buffer support

[Andy Shevchenko]

On Tue, May 05, 2026 at 05:17:23PM +0100, Jonathan Cameron wrote:
> On Tue, 5 May 2026 17:58:21 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Tue, May 05, 2026 at 02:26:40PM +0100, Jonathan Cameron wrote:

...

> > > > > > +	for (i = 0; i < ARRAY_SIZE(ad4691_gp_names); i++) {
> > > > > > +		irq = fwnode_irq_get_byname(dev_fwnode(dev),
> > > > > > +					    ad4691_gp_names[i]);
> > > > > > +		if (irq > 0)
> > > > > > +			break;    
> > > > > 
> > > > > This is problematic in case the above returns EPROBE_DEFER. Can you confirm
> > > > > it
> > > > > may not ever happen? (Note, I don't know the answer.)  
> > > > 
> > > > You are right, thanks for this!  
> > > I'm missing something. Why is that a problem?  Driver will return
> > > the error and a dev_err_probe() is used so it won't print anything.
> > > So probe will fail which is exactly what we want.  
> > 
> > If there are two IRQs and the first one is probe deferred and second returns
> > an error, we return that error instead of the deferral probe.
> > 
> > May be I missed something, but I have no idea how in this case it may return
> > the first error code in such a case.
> Ah. Indeed. I completely misread the code.  if (irq) would do the job to fix this.

Not really, as we are only concerned about deferred probe.

	if (irq > 0 || irq == -EPROBE_DEFER)
		break;

will do the job. But again, please double check that the
fwnode_irq_get_byname() is even capable of returning deferral probe
(probably yes as my weak memory tells me).

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v9 3/6] iio: adc: ad4691: add triggered buffer support

[Sabau, Radu bogdan]

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@intel.com>
> Sent: Wednesday, May 6, 2026 10:26 AM

...

> 
> > > > > > > +	for (i = 0; i < ARRAY_SIZE(ad4691_gp_names); i++) {
> > > > > > > +		irq = fwnode_irq_get_byname(dev_fwnode(dev),
> > > > > > > +					    ad4691_gp_names[i]);
> > > > > > > +		if (irq > 0)
> > > > > > > +			break;
> > > > > >
> > > > > > This is problematic in case the above returns EPROBE_DEFER. Can you
> confirm
> > > > > > it
> > > > > > may not ever happen? (Note, I don't know the answer.)
> > > > >
> > > > > You are right, thanks for this!
> > > > I'm missing something. Why is that a problem?  Driver will return
> > > > the error and a dev_err_probe() is used so it won't print anything.
> > > > So probe will fail which is exactly what we want.
> > >
> > > If there are two IRQs and the first one is probe deferred and second returns
> > > an error, we return that error instead of the deferral probe.
> > >
> > > May be I missed something, but I have no idea how in this case it may
> return
> > > the first error code in such a case.
> > Ah. Indeed. I completely misread the code.  if (irq) would do the job to fix
> this.
> 
> Not really, as we are only concerned about deferred probe.
> 
> 	if (irq > 0 || irq == -EPROBE_DEFER)
> 		break;
> 
> will do the job. But again, please double check that the
> fwnode_irq_get_byname() is even capable of returning deferral probe
> (probably yes as my weak memory tells me).
> 
> --
Hi everyone,

I checked and fwnode_irq_get_byname() can indeed return deferral probe
and the fix I had in mind is like the above.

Thank you all for looking into this,
Radu

Re: [PATCH v9 3/6] iio: adc: ad4691: add triggered buffer support

[Jonathan Cameron]

On Thu, 30 Apr 2026 13:16:45 +0300
Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org> wrote:

> From: Radu Sabau <radu.sabau@analog.com>
> 
> Add buffered capture support using the IIO triggered buffer framework.
> 
> CNV Burst Mode: the GP pin identified by interrupt-names in the device
> tree is configured as DATA_READY output. The IRQ handler stops
> conversions and fires the IIO trigger; the trigger handler executes a
> pre-built SPI message that reads all active channels from the AVG_IN
> accumulator registers and then resets accumulator state and restarts
> conversions for the next cycle.
> 
> Manual Mode: CNV is tied to SPI CS so each transfer simultaneously
> reads the previous result and starts the next conversion (pipelined
> N+1 scheme). At preenable time a pre-built, optimised SPI message of
> N+1 transfers is constructed (N channel reads plus one NOOP to drain
> the pipeline). The trigger handler executes the message in a single
> spi_sync() call and collects the results. An external trigger (e.g.
> iio-trig-hrtimer) is required to drive the trigger at the desired
> sample rate.
> 
> Both modes share the same trigger handler and push a complete scan —
> one u16 slot per channel at its scan_index position, followed by a
> timestamp — to the IIO buffer via iio_push_to_buffers_with_ts().
> 
> The CNV Burst Mode sampling frequency (PWM period) is exposed as a
> buffer-level attribute via IIO_DEVICE_ATTR.
> 
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
Hi Radu

I haven't chased it through but Sashiko is raising concerns around the
irq enabling / disable tricks this pulling and they may well be correct.
Please take a close look at what happens in the remove path. We may
need some local driver logic to avoid a double disable.

https://sashiko.dev/#/patchset/20260430-ad4692-multichannel-sar-adc-driver-v9-0-33e439e4fb87%40analog.com

> diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> index 05826b762c7f..c1e3406fef18 100644
> --- a/drivers/iio/adc/ad4691.c
> +++ b/drivers/iio/adc/ad4691.c


> +
> +static irqreturn_t ad4691_irq(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +
> +	iio_trigger_poll(indio_dev->trig);
> +	/*
> +	 * Keep the DATA_READY IRQ disabled until the trigger handler has
> +	 * finished reading the scan, to prevent a new assertion mid-transfer.
> +	 * The PWM continues running uninterrupted; the IRQ is re-enabled in
> +	 * ad4691_trigger_handler once spi_sync completes.
> +	 *
> +	 * IRQF_ONESHOT already masks the hardware line during this threaded
> +	 * handler, so disable_irq_nosync here ensures the IRQ stays disabled
> +	 * even after IRQF_ONESHOT unmasks on return.
> +	 */
> +	disable_irq_nosync(st->irq);
> +
> +	return IRQ_HANDLED;
> +}

> +
> +static irqreturn_t ad4691_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +
> +	ad4691_read_scan(indio_dev, pf->timestamp);
> +	if (!st->manual_mode)
> +		enable_irq(st->irq);

I think this should be in the reenable_trigger callback rather than here.
That's ultimately fired by iio_trigger_notify_done.

Sashiko had quite a bit to say about the problem paths around the buffer
being disabled between the interrupt and the trigger handler. I don't think
using the reenable trigger solves that though :(  We might be able
to fix that centrally by always reenabling the trigger before
disconnecting it but that's rather ugly. There is a note for the async
trigger reenabling about a driver possibly needing to be careful
to not reenable if the whole trigger was disabled in the meantime but
this particular race isn't covered.


Terrible though it is we may need to have some extra infrastructure
to avoid the double disable (assuming it is real).


> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}

Re: [PATCH v9 3/6] iio: adc: ad4691: add triggered buffer support

[Jonathan Cameron]

On Thu, 30 Apr 2026 13:16:45 +0300
Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org> wrote:

> From: Radu Sabau <radu.sabau@analog.com>
> 
> Add buffered capture support using the IIO triggered buffer framework.
> 
> CNV Burst Mode: the GP pin identified by interrupt-names in the device
> tree is configured as DATA_READY output. The IRQ handler stops
> conversions and fires the IIO trigger; the trigger handler executes a
> pre-built SPI message that reads all active channels from the AVG_IN
> accumulator registers and then resets accumulator state and restarts
> conversions for the next cycle.
> 
> Manual Mode: CNV is tied to SPI CS so each transfer simultaneously
> reads the previous result and starts the next conversion (pipelined
> N+1 scheme). At preenable time a pre-built, optimised SPI message of
> N+1 transfers is constructed (N channel reads plus one NOOP to drain
> the pipeline). The trigger handler executes the message in a single
> spi_sync() call and collects the results. An external trigger (e.g.
> iio-trig-hrtimer) is required to drive the trigger at the desired
> sample rate.
> 
> Both modes share the same trigger handler and push a complete scan —
> one u16 slot per channel at its scan_index position, followed by a
> timestamp — to the IIO buffer via iio_push_to_buffers_with_ts().
> 
> The CNV Burst Mode sampling frequency (PWM period) is exposed as a
> buffer-level attribute via IIO_DEVICE_ATTR.
> 
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
Another Sashiko found issue inline.

Also it calls out that you have no validate_trigger which might mean
another trigger is used.  Moving the irq enable to the trigger_reenable
should solve that.

> +static ssize_t sampling_frequency_show(struct device *dev,
> +				       struct device_attribute *attr,
> +				       char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +
> +	return sysfs_emit(buf, "%lu\n", NSEC_PER_SEC / st->cnv_period_ns);
> +}
> +
> +static ssize_t sampling_frequency_store(struct device *dev,
> +					struct device_attribute *attr,
> +					const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	int freq, ret;
> +
> +	ret = kstrtoint(buf, 10, &freq);
I missed this but as Sashiko points out this could read in a negative
frequency. Given that's clearly silly kstrtouint()

> +	if (ret)
> +		return ret;
> +
> +	IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +	if (IIO_DEV_ACQUIRE_FAILED(claim))
> +		return -EBUSY;
> +
> +
> +	ret = ad4691_set_pwm_freq(st, freq);
> +	if (ret)
> +		return ret;
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR_RW(sampling_frequency, 0);
> +
> +static const struct iio_dev_attr *ad4691_buffer_attrs[] = {
> +	&iio_dev_attr_sampling_frequency,
> +	NULL
> +};

RE: [PATCH v9 3/6] iio: adc: ad4691: add triggered buffer support

[Sabau, Radu bogdan]

Addressing Sashiko's review for triggered buffer patch.

> -----Original Message-----
> From: Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org>
> Sent: Thursday, April 30, 2026 1:17 PM

...

> +static int ad4691_manual_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	unsigned int prev_i, k, i;
> +	bool first;
> +	int ret;
> +
> +	memset(st->scan_xfers, 0, sizeof(st->scan_xfers));
> +	memset(st->scan_tx, 0, sizeof(st->scan_tx));
> +
> +	spi_message_init(&st->scan_msg);
> +
> +	first = true;
> +	prev_i = 0;
> +	k = 0;
> +	iio_for_each_active_channel(indio_dev, i) {
> +		st->scan_tx[k] = cpu_to_be16(AD4691_ADC_CHAN(i));
> +		st->scan_xfers[k].tx_buf = &st->scan_tx[k];
> +		/*
> +		 * The pipeline means xfer[0] receives the residual from the
> +		 * previous sequence, not a valid sample for channel i. Point
> +		 * it at vals[i] anyway; xfer[1] (or the NOOP when only one
> +		 * channel is active) will overwrite that slot with the real
> +		 * result, so no separate dummy buffer is needed.
> +		 */
> +		if (first) {
> +			st->scan_xfers[k].rx_buf = &st->vals[i];
> +			first = false;
> +		} else {
> +			st->scan_xfers[k].rx_buf = &st->vals[prev_i];
> +		}


"The IIO subsystem expects data pushed to the buffer to be densely packed
according to the active channels in the scan mask.
If only a subset of channels are enabled, does assigning the rx_buf pointer
directly to absolute array indices at &st->vals[i] leave holes in the buffer?
When iio_push_to_buffers_with_ts() is called, this might cause it to read
uninitialized memory instead of the expected samples."

I would say there is no change needed. Writing to &st->vals[scan_index] and
passing the full array to iio_push_to_buffers_with_ts() is the standard IIO kfifo
pattern: the core demultiplexes by reading data[scan_index * storagebits/8]
for each active channel; holes at inactive indices are silently ignored.
The same pattern is used in ad4695, ad_sigma_delta, and others. The
pipeline residual in the first manual-mode transfer is overwritten by the
subsequent transfer before the scan is pushed, as the comment explains.

> +		st->scan_xfers[k].len = sizeof(__be16);
> +		st->scan_xfers[k].cs_change = 1;
> +		spi_message_add_tail(&st->scan_xfers[k], &st->scan_msg);
> +		prev_i = i;
> +		k++;
> +	}
> +

...

> +static int ad4691_cnv_burst_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	unsigned int k, i;
> +	int ret;
> +
> +	memset(st->scan_xfers, 0, sizeof(st->scan_xfers));
> +	memset(st->scan_tx, 0, sizeof(st->scan_tx));
> +
> +	spi_message_init(&st->scan_msg);
> +
> +	/*
> +	 * Each AVG_IN read needs two transfers: a 2-byte address write phase
> +	 * followed by a 2-byte data read phase. CS toggles between channels
> +	 * (cs_change=1 on the read phase of all but the last channel).
> +	 */
> +	k = 0;
> +	iio_for_each_active_channel(indio_dev, i) {
> +		st->scan_tx[k] = cpu_to_be16(0x8000 | AD4691_AVG_IN(i));
> +		st->scan_xfers[2 * k].tx_buf = &st->scan_tx[k];
> +		st->scan_xfers[2 * k].len = sizeof(__be16);
> +		spi_message_add_tail(&st->scan_xfers[2 * k], &st-
> >scan_msg);
> +		st->scan_xfers[2 * k + 1].rx_buf = &st->vals[i];
> +		st->scan_xfers[2 * k + 1].len = sizeof(__be16);
> +		st->scan_xfers[2 * k + 1].cs_change = 1;
> +		spi_message_add_tail(&st->scan_xfers[2 * k + 1], &st-
> >scan_msg);
> +		k++;
> +	}
> +
> +	st->scan_tx[k] = cpu_to_be16(AD4691_STATE_RESET_REG);
> +	st->scan_xfers[2 * k].tx_buf = &st->scan_tx[k];
> +	st->scan_xfers[2 * k].len = sizeof(__be16);
> +	spi_message_add_tail(&st->scan_xfers[2 * k], &st->scan_msg);
> +	st->scan_tx[k + 1] = cpu_to_be16(AD4691_STATE_RESET_ALL << 8);


"Does the iio_for_each_active_channel() loop iterate over the soft timestamp
channel if it is enabled?
If the soft timestamp channel is enabled, k would reach 17. Because
st->scan_tx is sized for 18 elements, does writing to st->scan_tx[k + 1] and
st->scan_xfers[2 * k + 1] overflow the bounds of these arrays?"

Valid defensive concern. With all data channels + timestamp enabled,
the loop iterates scan_index=16 (timestamp), pushing k to 17, and the
state-reset then accesses scan_xfers[34] which is out-of-bounds (array is
sized 34). Fixed by adding a guard to break out of the loop when
i >= num_channels - 1 (the timestamp scan_index), consistent with how
both ad4691_channels and ad4693_channels are laid out.

"Also, the AD4691 auto-decrements register addresses on multi-byte writes.
Could writing a 2-byte payload to the 1-byte AD4691_STATE_RESET_REG (0x181)
inadvertently write 0x00 to the adjacent lower register 0x180
(AD4691_OSC_EN_REG), disabling the internal oscillator?"

Real bug. The two back-to-back 2-byte transfers ([0x01,0x81] then
[0x01,0x00]) are sent with CS continuously asserted, so the chip sees a
4-byte write: 0x01 to STATE_RESET_REG and 0x00 to OSC_EN_REG, disabling
the oscillator mid-scan. Fixed by using a single 4-byte transfer
[addr_hi, addr_lo, STATE_RESET_ALL, OSC_EN=1] in a dedicated
u8 scan_tx_reset[4] field. The fourth byte deliberately re-enables
OSC_EN_REG (0x180) via ADDR_DESCENDING as a side-write, keeping the
oscillator enabled. This same buffer is reused by the offload path in
the following commit (the two paths are mutually exclusive at probe).

> +	st->scan_xfers[2 * k + 1].tx_buf = &st->scan_tx[k + 1];
> +	st->scan_xfers[2 * k + 1].len = sizeof(__be16);
> +	st->scan_xfers[2 * k + 1].cs_change = 1;
> +	spi_message_add_tail(&st->scan_xfers[2 * k + 1], &st->scan_msg);
> +
> +	ret = spi_optimize_message(st->spi, &st->scan_msg);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
> +			   bitmap_read(indio_dev->active_scan_mask, 0,
> +				       iio_get_masklength(indio_dev)));
> +	if (ret)
> +		goto err_unoptimize;
> +
> +	ret = regmap_write(st->regmap, AD4691_ACC_MASK_REG,
> +			   ~bitmap_read(indio_dev->active_scan_mask, 0,
> +				iio_get_masklength(indio_dev)) &
> GENMASK(15, 0));
> +	if (ret)
> +		goto err_unoptimize;
> +
> +	ret = ad4691_enter_conversion_mode(st);
> +	if (ret)
> +		goto err_unoptimize;
> +
> +	ret = ad4691_sampling_enable(st, true);
> +	if (ret)
> +		goto err_exit_conv;
> +
> +	enable_irq(st->irq);
> +	return 0;

"Is there a race condition introduced by enabling the PWM and unmasking the
IRQ here?
If a hardware interrupt fires before the IIO core attaches the trigger's poll
function, iio_trigger_poll() drops the event. Will the IRQ handler then call
disable_irq_nosync() without ever running the consumer thread to re-enable it?"

Valid. preenable is called before the IIO core attaches the trigger
poll function; if a DATA_READY IRQ fires in that window, iio_trigger_poll()
is dropped, disable_irq_nosync() disables the IRQ, and enable_irq() is
never called, leaving the IRQ stuck. Although the delay would need to be 
very great for this to happen, I moved sampling_enable(true) and
enable_irq() to a new postenable callback which the IIO core calls only
after the trigger poll function is attached.

> +
> +err_exit_conv:
> +	ad4691_exit_conversion_mode(st);
> +err_unoptimize:
> +	spi_unoptimize_message(&st->scan_msg);
> +	return ret;
> +}
> +
> +static int ad4691_cnv_burst_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	disable_irq(st->irq);

"During buffer teardown, the IIO core detaches the trigger consumer before
invoking the postdisable callback.
If an interrupt fires while the PWM is still running before the call to
ad4691_sampling_enable(), could ad4691_irq() disable the IRQ without a
consumer to re-enable it? Does the subsequent disable_irq() call then increase
the disable depth to 2, permanently disabling the hardware IRQ?"

Valid, fixed as part of the previous concern rework. IRQ disabling and
sampling_enable(false) are now in a new predisable callback (called before
the consumer is detached); postdisable only handles SPI cleanup.

> +
> +	ret = ad4691_sampling_enable(st, false);
> +	if (ret)
> +		return ret;

"If ad4691_sampling_enable() fails, does returning early bypass
spi_unoptimize_message() and leak the DMA mappings allocated by the SPI core
during preenable?"

Valid. If pwm_apply_might_sleep() inside sampling_enable(false) fails,
spi_unoptimize_message() was skipped. Always called now before returning
from postdisable regardless of sampling_enable() result.

> +
> +	ret = ad4691_exit_conversion_mode(st);

...

> +static ssize_t sampling_frequency_store(struct device *dev,
> +					struct device_attribute *attr,
> +					const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	int freq, ret;
> +
> +	ret = kstrtoint(buf, 10, &freq);
> +	if (ret)
> +		return ret;
> +
> +	IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +	if (IIO_DEV_ACQUIRE_FAILED(claim))
> +		return -EBUSY;
> +
> +
> +	ret = ad4691_set_pwm_freq(st, freq);


"If a negative value is passed to kstrtoint(), does ad4691_set_pwm_freq()
calculate a cnv_period_ns of 0 due to signed integer arithmetic rules?
Could this lead to a division by zero regression in sampling_frequency_show()
when it calculates NSEC_PER_SEC / st->cnv_period_ns?"

Valid. Switching sampling_frequency_store to kstrtouint + unsigned int freq so
Negative strings are rejected at parse time before reaching set_pwm_freq.

> +	if (ret)
> +		return ret;
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR_RW(sampling_frequency, 0);
> +
> +static const struct iio_dev_attr *ad4691_buffer_attrs[] = {
> +	&iio_dev_attr_sampling_frequency,
> +	NULL
> +};
> +
> +static irqreturn_t ad4691_irq(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +
> +	iio_trigger_poll(indio_dev->trig);
> +	/*
> +	 * Keep the DATA_READY IRQ disabled until the trigger handler has
> +	 * finished reading the scan, to prevent a new assertion mid-transfer.
> +	 * The PWM continues running uninterrupted; the IRQ is re-enabled in
> +	 * ad4691_trigger_handler once spi_sync completes.
> +	 *
> +	 * IRQF_ONESHOT already masks the hardware line during this
> threaded
> +	 * handler, so disable_irq_nosync here ensures the IRQ stays disabled
> +	 * even after IRQF_ONESHOT unmasks on return.
> +	 */
> +	disable_irq_nosync(st->irq);
> +
> +	return IRQ_HANDLED;
> +}

"Can a concurrency issue occur here between this threaded IRQ handler and the
trigger consumer thread?
Since iio_trigger_poll() wakes up the consumer thread, could the consumer
thread complete the SPI transfer and call enable_irq() before the provider
thread reaches disable_irq_nosync()? This might trigger an unbalanced enable
for IRQ warning and leave the hardware IRQ permanently disabled."

Valid. Moved disable_irq_nosync() before iio_trigger_poll() so the
IRQ is always disabled before the consumer thread gets a chance to call
enable_irq().

> +
> +static const struct iio_trigger_ops ad4691_trigger_ops = {
> +	.validate_device = iio_trigger_validate_own_device,

...

> +
>  static const struct iio_info ad4691_info = {
>  	.read_raw = &ad4691_read_raw,
>  	.write_raw = &ad4691_write_raw,
> @@ -514,6 +910,18 @@ static const struct iio_info ad4691_info = {
>  	.debugfs_reg_access = &ad4691_reg_access,
>  };

"Without a .validate_trigger callback here, can external triggers be attached?
If an external trigger fires, the consumer thread might execute
enable_irq(st->irq) without a corresponding hardware interrupt calling
disable_irq_nosync() first, leading to an unbalanced IRQ enable warning."

Valid. Added .validate_trigger = iio_validate_own_trigger to
ad4691_info to prevent external triggers from causing unbalanced
enable_irq() calls.

Re: [PATCH v9 3/6] iio: adc: ad4691: add triggered buffer support

[Jonathan Cameron]

On Thu, 7 May 2026 11:37:25 +0000
"Sabau, Radu bogdan" <Radu.Sabau@analog.com> wrote:

> Addressing Sashiko's review for triggered buffer patch.
> 
> > -----Original Message-----
> > From: Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org>
> > Sent: Thursday, April 30, 2026 1:17 PM  
> 
> ...
> 
> > +static int ad4691_manual_buffer_preenable(struct iio_dev *indio_dev)
> > +{
> > +	struct ad4691_state *st = iio_priv(indio_dev);
> > +	unsigned int prev_i, k, i;
> > +	bool first;
> > +	int ret;
> > +
> > +	memset(st->scan_xfers, 0, sizeof(st->scan_xfers));
> > +	memset(st->scan_tx, 0, sizeof(st->scan_tx));
> > +
> > +	spi_message_init(&st->scan_msg);
> > +
> > +	first = true;
> > +	prev_i = 0;
> > +	k = 0;
> > +	iio_for_each_active_channel(indio_dev, i) {
> > +		st->scan_tx[k] = cpu_to_be16(AD4691_ADC_CHAN(i));
> > +		st->scan_xfers[k].tx_buf = &st->scan_tx[k];
> > +		/*
> > +		 * The pipeline means xfer[0] receives the residual from the
> > +		 * previous sequence, not a valid sample for channel i. Point
> > +		 * it at vals[i] anyway; xfer[1] (or the NOOP when only one
> > +		 * channel is active) will overwrite that slot with the real
> > +		 * result, so no separate dummy buffer is needed.
> > +		 */
> > +		if (first) {
> > +			st->scan_xfers[k].rx_buf = &st->vals[i];
> > +			first = false;
> > +		} else {
> > +			st->scan_xfers[k].rx_buf = &st->vals[prev_i];
> > +		}  
> 
> 
> "The IIO subsystem expects data pushed to the buffer to be densely packed
> according to the active channels in the scan mask.
> If only a subset of channels are enabled, does assigning the rx_buf pointer
> directly to absolute array indices at &st->vals[i] leave holes in the buffer?
> When iio_push_to_buffers_with_ts() is called, this might cause it to read
> uninitialized memory instead of the expected samples."
> 
> I would say there is no change needed. Writing to &st->vals[scan_index] and
> passing the full array to iio_push_to_buffers_with_ts() is the standard IIO kfifo
> pattern: the core demultiplexes by reading data[scan_index * storagebits/8]
> for each active channel; holes at inactive indices are silently ignored.
> The same pattern is used in ad4695, ad_sigma_delta, and others. The
> pipeline residual in the first manual-mode transfer is overwritten by the
> subsequent transfer before the scan is pushed, as the comment explains.

This looks wrong to me.

What holes?  If available_scan_masks is set we will do a bunch of
demux work - but then this code would see the mask picked from that
list. If it's not then typically we won't (subject to multiple consumers
forcing it - but that still won't close up holes here).

If the active_scan_mask == the one requested, there is no demux at all
and I think that's the case here - the code pushes the data passed in
directly to the kfifo.

Perhaps given an illustration of what the layout of resulting data
is if only even numbered channels are enabled.



> 
> > +		st->scan_xfers[k].len = sizeof(__be16);
> > +		st->scan_xfers[k].cs_change = 1;
> > +		spi_message_add_tail(&st->scan_xfers[k], &st->scan_msg);
> > +		prev_i = i;
> > +		k++;
> > +	}
> > +  
> 
>
> > +	st->scan_xfers[2 * k + 1].len = sizeof(__be16);
> > +	st->scan_xfers[2 * k + 1].cs_change = 1;
> > +	spi_message_add_tail(&st->scan_xfers[2 * k + 1], &st->scan_msg);
> > +
> > +	ret = spi_optimize_message(st->spi, &st->scan_msg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
> > +			   bitmap_read(indio_dev->active_scan_mask, 0,
> > +				       iio_get_masklength(indio_dev)));
> > +	if (ret)
> > +		goto err_unoptimize;
> > +
> > +	ret = regmap_write(st->regmap, AD4691_ACC_MASK_REG,
> > +			   ~bitmap_read(indio_dev->active_scan_mask, 0,
> > +				iio_get_masklength(indio_dev)) &
> > GENMASK(15, 0));
> > +	if (ret)
> > +		goto err_unoptimize;
> > +
> > +	ret = ad4691_enter_conversion_mode(st);
> > +	if (ret)
> > +		goto err_unoptimize;
> > +
> > +	ret = ad4691_sampling_enable(st, true);
> > +	if (ret)
> > +		goto err_exit_conv;
> > +
> > +	enable_irq(st->irq);
> > +	return 0;  
> 
> "Is there a race condition introduced by enabling the PWM and unmasking the
> IRQ here?
> If a hardware interrupt fires before the IIO core attaches the trigger's poll
> function, iio_trigger_poll() drops the event. Will the IRQ handler then call
> disable_irq_nosync() without ever running the consumer thread to re-enable it?"
> 
> Valid. preenable is called before the IIO core attaches the trigger
> poll function; if a DATA_READY IRQ fires in that window, iio_trigger_poll()
> is dropped, disable_irq_nosync() disables the IRQ, and enable_irq() is
> never called, leaving the IRQ stuck. Although the delay would need to be 
> very great for this to happen, I moved sampling_enable(true) and
> enable_irq() to a new postenable callback which the IIO core calls only
> after the trigger poll function is attached.

Make sure to add a comment on why that is there.
Otherwise makes sense.

> 

Rest look fine to me.

RE: [PATCH v9 3/6] iio: adc: ad4691: add triggered buffer support

[Sabau, Radu bogdan]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Thursday, May 7, 2026 5:26 PM

...

> > > +static int ad4691_manual_buffer_preenable(struct iio_dev *indio_dev)
> > > +{
> > > +	struct ad4691_state *st = iio_priv(indio_dev);
> > > +	unsigned int prev_i, k, i;
> > > +	bool first;
> > > +	int ret;
> > > +
> > > +	memset(st->scan_xfers, 0, sizeof(st->scan_xfers));
> > > +	memset(st->scan_tx, 0, sizeof(st->scan_tx));
> > > +
> > > +	spi_message_init(&st->scan_msg);
> > > +
> > > +	first = true;
> > > +	prev_i = 0;
> > > +	k = 0;
> > > +	iio_for_each_active_channel(indio_dev, i) {
> > > +		st->scan_tx[k] = cpu_to_be16(AD4691_ADC_CHAN(i));
> > > +		st->scan_xfers[k].tx_buf = &st->scan_tx[k];
> > > +		/*
> > > +		 * The pipeline means xfer[0] receives the residual from the
> > > +		 * previous sequence, not a valid sample for channel i. Point
> > > +		 * it at vals[i] anyway; xfer[1] (or the NOOP when only one
> > > +		 * channel is active) will overwrite that slot with the real
> > > +		 * result, so no separate dummy buffer is needed.
> > > +		 */
> > > +		if (first) {
> > > +			st->scan_xfers[k].rx_buf = &st->vals[i];
> > > +			first = false;
> > > +		} else {
> > > +			st->scan_xfers[k].rx_buf = &st->vals[prev_i];
> > > +		}
> >
> >
> > "The IIO subsystem expects data pushed to the buffer to be densely packed
> > according to the active channels in the scan mask.
> > If only a subset of channels are enabled, does assigning the rx_buf pointer
> > directly to absolute array indices at &st->vals[i] leave holes in the buffer?
> > When iio_push_to_buffers_with_ts() is called, this might cause it to read
> > uninitialized memory instead of the expected samples."
> >
> > I would say there is no change needed. Writing to &st->vals[scan_index] and
> > passing the full array to iio_push_to_buffers_with_ts() is the standard IIO
> kfifo
> > pattern: the core demultiplexes by reading data[scan_index * storagebits/8]
> > for each active channel; holes at inactive indices are silently ignored.
> > The same pattern is used in ad4695, ad_sigma_delta, and others. The
> > pipeline residual in the first manual-mode transfer is overwritten by the
> > subsequent transfer before the scan is pushed, as the comment explains.
> 
> This looks wrong to me.
> 
> What holes?  If available_scan_masks is set we will do a bunch of
> demux work - but then this code would see the mask picked from that
> list. If it's not then typically we won't (subject to multiple consumers
> forcing it - but that still won't close up holes here).
> 
> If the active_scan_mask == the one requested, there is no demux at all
> and I think that's the case here - the code pushes the data passed in
> directly to the kfifo.
> 
> Perhaps given an illustration of what the layout of resulting data
> is if only even numbered channels are enabled.
> 

Correct. scan_bytes is a dense count (channels 0, 2, 4 active → 3 × 2 = 6
bytes). With the old sparse layout (rx_buf = &vals[scan_index]):

  vals[0] = ch0 result  (correct)
  vals[1] = hole (incorrect)  <- userspace reads as ch2
  vals[2] = ch2 result  (correct)  <-userspace reads as ch4

Fixed by using the slot counter k as the rx_buf index rather than the channel
index i, giving a densely packed vals[]. In manual mode xfer[k] delivers the
previous channel's result (pipelined), so subsequent transfers write into
vals[k-1]; vals[0] serves as a throwaway slot for the first transfer's garbage.

Re: [PATCH v9 6/6] docs: iio: adc: ad4691: add driver documentation

[Jonathan Cameron]

On Thu, 30 Apr 2026 13:16:48 +0300
Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org> wrote:

> From: Radu Sabau <radu.sabau@analog.com>
> 
> Add RST documentation for the AD4691 family ADC driver covering
> supported devices, IIO channels, operating modes, oversampling,
> reference voltage, LDO supply, reset, GP pins, SPI offload support,
> and buffer data format.
> 
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>

> diff --git a/Documentation/iio/ad4691.rst b/Documentation/iio/ad4691.rst
> new file mode 100644
> index 000000000000..38e2ad28a713
> --- /dev/null
> +++ b/Documentation/iio/ad4691.rst


> diff --git a/MAINTAINERS b/MAINTAINERS
> index 24e4502b8292..819d8b6eb6bb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1491,6 +1491,7 @@ S:	Supported
>  W:	https://ez.analog.com/linux-software-drivers
>  F:	Documentation/devicetree/bindings/iio/adc/adi,ad4691.yaml
>  F:	drivers/iio/adc/ad4691.c
> +F:	drivers/iio/adc/ad4691.rst
Not there. (Sashiko got this one)

>  
>  ANALOG DEVICES INC AD4695 DRIVER
>  M:	Michael Hennerich <michael.hennerich@analog.com>
> 

Re: [PATCH v9 4/6] iio: adc: ad4691: add SPI offload support

[Jonathan Cameron]

On Thu, 30 Apr 2026 13:16:46 +0300
Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org> wrote:

> From: Radu Sabau <radu.sabau@analog.com>
> 
> Add SPI offload support to enable DMA-based, CPU-independent data
> acquisition using the SPI Engine offload framework.
> 
> When an SPI offload is available (devm_spi_offload_get() succeeds),
> the driver registers a DMA engine IIO buffer and uses dedicated buffer
> setup operations. If no offload is available the existing software
> triggered buffer path is used unchanged.
> 
> Both CNV Burst Mode and Manual Mode support offload, but use different
> trigger mechanisms:
> 
> CNV Burst Mode: the SPI Engine is triggered by the ADC's DATA_READY
> signal on the GP pin specified by the trigger-source consumer reference
> in the device tree (one cell = GP pin number 0-3). For this mode the
> driver acts as both an SPI offload consumer (DMA RX stream, message
> optimization) and a trigger source provider: it registers the
> GP/DATA_READY output via devm_spi_offload_trigger_register() so the
> offload framework can match the '#trigger-source-cells' phandle and
> automatically fire the SPI Engine DMA transfer at end-of-conversion.
> 
> Manual Mode: the SPI Engine is triggered by a periodic trigger at
> the configured sampling frequency. The pre-built SPI message uses
> the pipelined CNV-on-CS protocol: N+1 16-bit transfers are issued
> for N active channels (the first result is discarded as garbage from
> the pipeline flush) and the remaining N results are captured by DMA.
> 
> All offload transfers use 16-bit frames (bits_per_word=16, len=2).
> The channel scan_type (storagebits=16, shift=0, IIO_BE) is shared
> between the software triggered-buffer and offload paths; no separate
> scan_type or channel array is needed for the offload case. The
> ad4691_manual_channels[] array introduced in the triggered-buffer
> commit is reused here: it hides the IIO_CHAN_INFO_OVERSAMPLING_RATIO
> attribute, which is not applicable in Manual Mode.

Probably good to call out that oversampling hasn't been introduced
to the driver yet. This confused Sashiko ;)

> 
> Kconfig gains a dependency on IIO_BUFFER_DMAENGINE.
> 
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
One minor thing inline.

>  static int ad4691_reg_read(void *context, unsigned int reg, unsigned int *val)
>  {
>  	struct spi_device *spi = context;
> @@ -712,6 +791,7 @@ static const struct iio_buffer_setup_ops ad4691_manual_buffer_setup_ops = {
>  static int ad4691_cnv_burst_buffer_preenable(struct iio_dev *indio_dev)
>  {
>  	struct ad4691_state *st = iio_priv(indio_dev);
> +	unsigned int acc_mask;
>  	unsigned int k, i;
>  	int ret;
>  
> @@ -758,9 +838,9 @@ static int ad4691_cnv_burst_buffer_preenable(struct iio_dev *indio_dev)
>  	if (ret)
>  		goto err_unoptimize;
>  
> -	ret = regmap_write(st->regmap, AD4691_ACC_MASK_REG,
> -			   ~bitmap_read(indio_dev->active_scan_mask, 0,
> -				iio_get_masklength(indio_dev)) & GENMASK(15, 0));
> +	acc_mask = ~bitmap_read(indio_dev->active_scan_mask, 0,
> +				iio_get_masklength(indio_dev)) & GENMASK(15, 0);

Not obvious to me why this change is here.  If you want it, push back to the
original patch that introduced this code.

> +	ret = regmap_write(st->regmap, AD4691_ACC_MASK_REG, acc_mask);
>  	if (ret)
>  		goto err_unoptimize;
>  
> @@ -803,6 +883,209 @@ static const struct iio_buffer_setup_ops ad4691_cnv_burst_buffer_setup_ops = {
>  	.postdisable = &ad4691_cnv_burst_buffer_postdisable,
>  };

Re: [PATCH v9 4/6] iio: adc: ad4691: add SPI offload support

[Jonathan Cameron]

On Thu, 30 Apr 2026 13:16:46 +0300
Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org> wrote:

> From: Radu Sabau <radu.sabau@analog.com>
> 
> Add SPI offload support to enable DMA-based, CPU-independent data
> acquisition using the SPI Engine offload framework.
> 
> When an SPI offload is available (devm_spi_offload_get() succeeds),
> the driver registers a DMA engine IIO buffer and uses dedicated buffer
> setup operations. If no offload is available the existing software
> triggered buffer path is used unchanged.
> 
> Both CNV Burst Mode and Manual Mode support offload, but use different
> trigger mechanisms:
> 
> CNV Burst Mode: the SPI Engine is triggered by the ADC's DATA_READY
> signal on the GP pin specified by the trigger-source consumer reference
> in the device tree (one cell = GP pin number 0-3). For this mode the
> driver acts as both an SPI offload consumer (DMA RX stream, message
> optimization) and a trigger source provider: it registers the
> GP/DATA_READY output via devm_spi_offload_trigger_register() so the
> offload framework can match the '#trigger-source-cells' phandle and
> automatically fire the SPI Engine DMA transfer at end-of-conversion.
> 
> Manual Mode: the SPI Engine is triggered by a periodic trigger at
> the configured sampling frequency. The pre-built SPI message uses
> the pipelined CNV-on-CS protocol: N+1 16-bit transfers are issued
> for N active channels (the first result is discarded as garbage from
> the pipeline flush) and the remaining N results are captured by DMA.
> 
> All offload transfers use 16-bit frames (bits_per_word=16, len=2).
> The channel scan_type (storagebits=16, shift=0, IIO_BE) is shared
> between the software triggered-buffer and offload paths; no separate
> scan_type or channel array is needed for the offload case. The
> ad4691_manual_channels[] array introduced in the triggered-buffer
> commit is reused here: it hides the IIO_CHAN_INFO_OVERSAMPLING_RATIO
> attribute, which is not applicable in Manual Mode.
> 
> Kconfig gains a dependency on IIO_BUFFER_DMAENGINE.
> 
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
In general have a read through Sashiko reviews once they come in and
if you agree with them reply to your own patches to say what you are
changing.
https://sashiko.dev/#/patchset/20260430-ad4692-multichannel-sar-adc-driver-v9-0-33e439e4fb87%40analog.com
No perfect but another one in here is something I missed completely.

A few of them called out here but please make sure you've addressed them
all or established them to be false (which happens!)


> +
> +struct ad4691_offload_state {
> +	struct spi_offload *offload;
> +	struct spi_offload_trigger *trigger;
> +	u64 trigger_hz;
> +	u8 tx_cmd[17][2];
> +	u8 tx_reset[4];
These two buffers share cachelines with trigger_hz. That can
be written via sysfs in the middle of a non coherent DMA transfer.
If it is you may loose the written value when a post transfer
flush copies back a stale cacheline (with the new data in tx_cmd
/ tx_reset).  These need marking to force them to have their own cache lines.


>  };
>  

> +
> +static int ad4691_manual_offload_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	struct ad4691_offload_state *offload = st->offload;
> +
> +	spi_offload_trigger_disable(offload->offload, offload->trigger);
> +	spi_unoptimize_message(&st->scan_msg);
> +
> +	return ad4691_exit_conversion_mode(st);
> +}
> +
> +static const struct iio_buffer_setup_ops ad4691_manual_offload_buffer_setup_ops = {
> +	.postenable = &ad4691_manual_offload_buffer_postenable,
> +	.predisable = &ad4691_manual_offload_buffer_predisable,
> +};
> +
> +static int ad4691_cnv_burst_offload_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	struct ad4691_offload_state *offload = st->offload;
> +	struct device *dev = regmap_get_device(st->regmap);
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct spi_offload_trigger_config config = {
> +		.type = SPI_OFFLOAD_TRIGGER_DATA_READY,
> +	};
> +	unsigned int bpw = indio_dev->channels[0].scan_type.realbits;
> +	unsigned int acc_mask;
> +	unsigned int bit, k;
> +	int ret;
> +
> +	ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
> +			   bitmap_read(indio_dev->active_scan_mask, 0,
> +				       iio_get_masklength(indio_dev)));
> +	if (ret)
> +		return ret;
> +
> +	acc_mask = ~bitmap_read(indio_dev->active_scan_mask, 0,
> +				iio_get_masklength(indio_dev)) & GENMASK(15, 0);
> +	ret = regmap_write(st->regmap, AD4691_ACC_MASK_REG, acc_mask);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4691_enter_conversion_mode(st);
> +	if (ret)
> +		return ret;
> +
> +	memset(st->scan_xfers, 0, sizeof(st->scan_xfers));
> +
> +	/*
> +	 * Each AVG_IN register read uses two 16-bit transfers:
> +	 *   TX: [reg_hi | 0x80, reg_lo]  (address, CS stays asserted)
> +	 *   RX: [data_hi, data_lo]       (data, storagebits=16, shift=0)
> +	 * The state reset is also split into two 16-bit transfers
> +	 * (address then value) to keep bits_per_word uniform throughout.
> +	 */
> +	k = 0;
> +	iio_for_each_active_channel(indio_dev, bit) {
> +		put_unaligned_be16(0x8000 | AD4691_AVG_IN(bit), offload->tx_cmd[k]);

Sashiko makes the interesting point that you are setting bpw to 16 at least sometimes
and that means the SPI bus will be dealing in u16s. As such the endian swap seems
odd.  Which also makes a mess if the word length isn't 16 bits. I'm assuming it
always is. If that's the case hard code it rather than giving impression this
will work otherwise.  The endian swap probably wants to be unconditional rather
than only if little endian host.

There is a related question whether the larger words interact with the channel
endianness marking. I think that should be fine but please check.


> +
> +		/* TX: address phase, CS stays asserted into data phase */
> +		st->scan_xfers[2 * k].tx_buf = offload->tx_cmd[k];
> +		st->scan_xfers[2 * k].len = sizeof(offload->tx_cmd[k]);
> +		st->scan_xfers[2 * k].bits_per_word = bpw;
> +
> +		/* RX: data phase, CS toggles after to delimit the next register op */
> +		st->scan_xfers[2 * k + 1].len = sizeof(offload->tx_cmd[k]);
> +		st->scan_xfers[2 * k + 1].bits_per_word = bpw;
> +		st->scan_xfers[2 * k + 1].offload_flags = SPI_OFFLOAD_XFER_RX_STREAM;
> +		st->scan_xfers[2 * k + 1].cs_change = 1;
> +		k++;
> +	}
> +
> +	/* State reset to re-arm DATA_READY for the next scan. */
> +	put_unaligned_be16(AD4691_STATE_RESET_REG, offload->tx_reset);
> +	offload->tx_reset[2] = AD4691_STATE_RESET_ALL;
> +
> +	st->scan_xfers[2 * k].tx_buf = offload->tx_reset;
> +	st->scan_xfers[2 * k].len = sizeof(offload->tx_cmd[k]);
> +	st->scan_xfers[2 * k].bits_per_word = bpw;
> +
> +	st->scan_xfers[2 * k + 1].tx_buf = &offload->tx_reset[2];
> +	st->scan_xfers[2 * k + 1].len = sizeof(offload->tx_cmd[k]);
> +	st->scan_xfers[2 * k + 1].bits_per_word = bpw;
> +	st->scan_xfers[2 * k + 1].cs_change = 1;
This is odd. Add a comment on why if you do want to leave CS active
after this sequence.

> +
> +	spi_message_init_with_transfers(&st->scan_msg, st->scan_xfers, 2 * k + 2);
> +	st->scan_msg.offload = offload->offload;
> +
> +	ret = spi_optimize_message(spi, &st->scan_msg);
> +	if (ret)
> +		goto err_exit_conversion;
> +
> +	ret = ad4691_sampling_enable(st, true);
There are some questions from Sashiko around whether the offload trigger should be enabled
first to avoid getting the device into an odd state. I haven't read the datasheet
in enough depth to check - so over to you to argue one way or the other!

> +	if (ret)
> +		goto err_unoptimize;
> +
> +	ret = spi_offload_trigger_enable(offload->offload, offload->trigger, &config);
> +	if (ret)
> +		goto err_sampling_disable;
> +
> +	return 0;
> +
> +err_sampling_disable:
> +	ad4691_sampling_enable(st, false);
> +err_unoptimize:
> +	spi_unoptimize_message(&st->scan_msg);
> +err_exit_conversion:
> +	ad4691_exit_conversion_mode(st);
> +	return ret;
> +}

RE: [PATCH v9 4/6] iio: adc: ad4691: add SPI offload support

[Sabau, Radu bogdan]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Tuesday, May 5, 2026 5:28 PM
> To: Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org>
> Cc: Sabau, Radu bogdan <Radu.Sabau@analog.com>; Lars-Peter Clausen
> <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> David Lechner <dlechner@baylibre.com>; Sa, Nuno <Nuno.Sa@analog.com>;
> Andy Shevchenko <andy@kernel.org>; Rob Herring <robh@kernel.org>;
> Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley
> <conor+dt@kernel.org>; Uwe Kleine-König <ukleinek@kernel.org>; Liam
> Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; Linus
> Walleij <linusw@kernel.org>; Bartosz Golaszewski <brgl@kernel.org>; Philipp
> Zabel <p.zabel@pengutronix.de>; Jonathan Corbet <corbet@lwn.net>; Shuah
> Khan <skhan@linuxfoundation.org>; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> pwm@vger.kernel.org; linux-gpio@vger.kernel.org; linux-doc@vger.kernel.org
> Subject: Re: [PATCH v9 4/6] iio: adc: ad4691: add SPI offload support
> 
> [External]
> 
> On Thu, 30 Apr 2026 13:16:46 +0300
> Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org>
> wrote:
> 
> > From: Radu Sabau <radu.sabau@analog.com>
> >
> > Add SPI offload support to enable DMA-based, CPU-independent data
> > acquisition using the SPI Engine offload framework.
> >
> > When an SPI offload is available (devm_spi_offload_get() succeeds),
> > the driver registers a DMA engine IIO buffer and uses dedicated buffer
> > setup operations. If no offload is available the existing software
> > triggered buffer path is used unchanged.
> >
> > Both CNV Burst Mode and Manual Mode support offload, but use different
> > trigger mechanisms:
> >
> > CNV Burst Mode: the SPI Engine is triggered by the ADC's DATA_READY
> > signal on the GP pin specified by the trigger-source consumer reference
> > in the device tree (one cell = GP pin number 0-3). For this mode the
> > driver acts as both an SPI offload consumer (DMA RX stream, message
> > optimization) and a trigger source provider: it registers the
> > GP/DATA_READY output via devm_spi_offload_trigger_register() so the
> > offload framework can match the '#trigger-source-cells' phandle and
> > automatically fire the SPI Engine DMA transfer at end-of-conversion.
> >
> > Manual Mode: the SPI Engine is triggered by a periodic trigger at
> > the configured sampling frequency. The pre-built SPI message uses
> > the pipelined CNV-on-CS protocol: N+1 16-bit transfers are issued
> > for N active channels (the first result is discarded as garbage from
> > the pipeline flush) and the remaining N results are captured by DMA.
> >
> > All offload transfers use 16-bit frames (bits_per_word=16, len=2).
> > The channel scan_type (storagebits=16, shift=0, IIO_BE) is shared
> > between the software triggered-buffer and offload paths; no separate
> > scan_type or channel array is needed for the offload case. The
> > ad4691_manual_channels[] array introduced in the triggered-buffer
> > commit is reused here: it hides the IIO_CHAN_INFO_OVERSAMPLING_RATIO
> > attribute, which is not applicable in Manual Mode.
> >
> > Kconfig gains a dependency on IIO_BUFFER_DMAENGINE.
> >
> > Signed-off-by: Radu Sabau <radu.sabau@analog.com>
> In general have a read through Sashiko reviews once they come in and
> if you agree with them reply to your own patches to say what you are
> changing.
> https://urldefense.com/v3/__https://sashiko.dev/*/patchset/20260430-
> ad4692-multichannel-sar-adc-driver-v9-0-
> 33e439e4fb87*40analog.com__;IyU!!A3Ni8CS0y2Y!-
> G5xU6m5PVXdQJCRVp_OOtLEM0gRiLsoZigLcBOrGi00oHLlyYgtxQAtx7azQuyZ
> Bkv8I3Cgy2WoEg$
> No perfect but another one in here is something I missed completely.
> 
> A few of them called out here but please make sure you've addressed them
> all or established them to be false (which happens!)
> 

Will have a look at Sashiko review in order to address everything in the next
version.
I will also reply to each patch and mention the change, argue to why I think the
raised concern could be false and perhaps have a follow-up discussion here.

RE: [PATCH v9 4/6] iio: adc: ad4691: add SPI offload support

[Sabau, Radu bogdan]

Addressing Sashiko's review for the offload support patch.

> -----Original Message-----
> From: Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org>
> Sent: Thursday, April 30, 2026 1:17 PM

...

> All offload transfers use 16-bit frames (bits_per_word=16, len=2).
> The channel scan_type (storagebits=16, shift=0, IIO_BE) is shared
> between the software triggered-buffer and offload paths; no separate
> scan_type or channel array is needed for the offload case. The
> ad4691_manual_channels[] array introduced in the triggered-buffer
> commit is reused here: it hides the IIO_CHAN_INFO_OVERSAMPLING_RATIO
> attribute, which is not applicable in Manual Mode.

"This isn't a bug, but ad4691_manual_channels and the oversampling attribute
don't appear to exist in this driver. Was this copied from another driver?"

Agreed on the commit message as per Jonathan's input as well.
Removed the two sentences referencing ad4691_manual_channels[] and
IIO_CHAN_INFO_OVERSAMPLING_RATIO — both were carried over from a
draft that included OSR support, which was split into the following commit.
The updated message now calls out explicitly that per-mode channel array
distinctions and oversampling support are introduced in the next commit.

> 
> Kconfig gains a dependency on IIO_BUFFER_DMAENGINE.
> 
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
> ---
>  drivers/iio/adc/Kconfig  |   2 +
>  drivers/iio/adc/ad4691.c | 392
> ++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 389 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index d498f16c0816..fdc6565933c5 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -143,8 +143,10 @@ config AD4691
>  	tristate "Analog Devices AD4691 Family ADC Driver"
>  	depends on SPI
>  	select IIO_BUFFER
> +	select IIO_BUFFER_DMAENGINE
>  	select IIO_TRIGGERED_BUFFER
>  	select REGMAP
> +	select SPI_OFFLOAD
>  	help
>  	  Say yes here to build support for Analog Devices AD4691 Family
> MuxSAR
>  	  SPI analog to digital converters (ADC).
> diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> index c1e3406fef18..50d81d87d4a0 100644
> --- a/drivers/iio/adc/ad4691.c
> +++ b/drivers/iio/adc/ad4691.c
> @@ -23,10 +23,14 @@

...

> +struct ad4691_offload_state {
> +	struct spi_offload *offload;
> +	struct spi_offload_trigger *trigger;
> +	u64 trigger_hz;
> +	u8 tx_cmd[17][2];
> +	u8 tx_reset[4];
>  };

"Could using these buffers for DMA transfers cause memory corruption on
architectures with non-coherent caches? 
They are not marked with __aligned(IIO_DMA_MINALIGN), so they share cachelines
with each other and adjacent struct members like trigger_hz."

Valid. struct ad4691_offload_state is allocated with devm_kzalloc
which does not guarantee cache-line alignment, so tx_cmd and tx_reset
shared cache lines with trigger_hz and the pointer fields. Removed both
from ad4691_offload_state entirely. The existing scan_tx[17] field in
struct ad4691_state is large enough for all 17 AVG_IN addresses and is
already __aligned(IIO_DMA_MINALIGN); reused it for the offload TX
commands (the two paths are mutually exclusive at probe).

> 
>  struct ad4691_state {
> @@ -252,7 +286,10 @@ struct ad4691_state {
>  	 *		    transfers in one go.
>  	 */
>  	struct spi_message scan_msg;

...

> +
> +static int ad4691_cnv_burst_offload_buffer_postenable(struct iio_dev
> *indio_dev)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	struct ad4691_offload_state *offload = st->offload;
> +	struct device *dev = regmap_get_device(st->regmap);
> +	struct spi_device *spi = to_spi_device(dev);

...

> +		/* TX: address phase, CS stays asserted into data phase */
> +		st->scan_xfers[2 * k].tx_buf = offload->tx_cmd[k];
> +		st->scan_xfers[2 * k].len = sizeof(offload->tx_cmd[k]);
> +		st->scan_xfers[2 * k].bits_per_word = bpw;

"When bits_per_word is greater than 8 (like bpw = 16 here), the SPI framework
treats tx_buf as an array of native 16-bit words.
On little-endian architectures, the controller will byte-swap the data before
transmitting it. Will using a u8 array and put_unaligned_be16() result in the
command bytes being reversed on the wire?"

Switched to cpu_to_be16() assigned directly into __be16 scan_tx[],
matching the non-offload path. This makes the intended wire format
self-evident and sidesteps the byte-ordering question entirely.

> +
> +		/* RX: data phase, CS toggles after to delimit the next register
> op */
> +		st->scan_xfers[2 * k + 1].len = sizeof(offload->tx_cmd[k]);
> +		st->scan_xfers[2 * k + 1].bits_per_word = bpw;
> +		st->scan_xfers[2 * k + 1].offload_flags =
> SPI_OFFLOAD_XFER_RX_STREAM;
> +		st->scan_xfers[2 * k + 1].cs_change = 1;
> +		k++;
> +	}
> +
> +	/* State reset to re-arm DATA_READY for the next scan. */
> +	put_unaligned_be16(AD4691_STATE_RESET_REG, offload->tx_reset);
> +	offload->tx_reset[2] = AD4691_STATE_RESET_ALL;
> +
> +	st->scan_xfers[2 * k].tx_buf = offload->tx_reset;
> +	st->scan_xfers[2 * k].len = sizeof(offload->tx_cmd[k]);
> +	st->scan_xfers[2 * k].bits_per_word = bpw;
> +
> +	st->scan_xfers[2 * k + 1].tx_buf = &offload->tx_reset[2];
> +	st->scan_xfers[2 * k + 1].len = sizeof(offload->tx_cmd[k]);

"Will passing &offload->tx_reset[2] directly as tx_buf cause DMA mapping
issues, since it is only 2-byte aligned?"

Addressed by the second concern's fix: no sub-aligned pointer into the middle
of a DMA buffer — the full 4-byte scan_tx_reset[] is passed as tx_buf
from its own cache-line-aligned start address.

> +	st->scan_xfers[2 * k + 1].bits_per_word = bpw;
> +	st->scan_xfers[2 * k + 1].cs_change = 1;

The ADI SPI Engine always deasserts CS at the end of its programmed
sequence regardless, though I will drop this in order to avoid confusion.

> +
> +	spi_message_init_with_transfers(&st->scan_msg, st->scan_xfers, 2 * k
> + 2);
> +	st->scan_msg.offload = offload->offload;
> +
> +	ret = spi_optimize_message(spi, &st->scan_msg);
> +	if (ret)
> +		goto err_exit_conversion;
> +
> +	ret = ad4691_sampling_enable(st, true);
> +	if (ret)
> +		goto err_unoptimize;
> +
> +	ret = spi_offload_trigger_enable(offload->offload, offload->trigger,
> &config);

"If the PWM starts generating DATA_READY trigger edges in ad4691_sampling_enable
before the SPI offload trigger is enabled, could the offload engine miss the
first trigger edge?
If that first edge is missed, wouldn't the DATA_READY line remain permanently
asserted (because the STATE_RESET command is never sent), deadlocking the
entire capture?"

Valid. Swapped the order: spi_offload_trigger_enable() first, then
ad4691_sampling_enable(), so the offload engine is armed before the first
CNV pulse fires. Without this, if DATA_READY asserts in the window between
sampling_enable() and trigger_enable(), the SPI Engine misses the rising
edge and the capture deadlocks.

> +	if (ret)
> +		goto err_sampling_disable;
> +
> +	return 0;
> +
> +err_sampling_disable:
> +	ad4691_sampling_enable(st, false);
> +err_unoptimize:
> +	spi_unoptimize_message(&st->scan_msg);
> +err_exit_conversion:
> +	ad4691_exit_conversion_mode(st);
> +	return ret;
> +}
> +
> +static int ad4691_cnv_burst_offload_buffer_predisable(struct iio_dev
> *indio_dev)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	struct ad4691_offload_state *offload = st->offload;
> +	int ret;
> +
> +	spi_offload_trigger_disable(offload->offload, offload->trigger);
> +
> +	ret = ad4691_sampling_enable(st, false);
> +	if (ret)
> +		return ret;
> +
> +	spi_unoptimize_message(&st->scan_msg);

"If ad4691_sampling_enable() fails and returns early, won't this bypass
spi_unoptimize_message() and permanently leak the DMA mappings and memory
allocated during SPI message optimization?"

Valid. spi_unoptimize_message() is now always called before returning
from predisable, regardless of sampling_enable() result.

> +
> +	return ad4691_exit_conversion_mode(st);
> +}
> +
> +static const struct iio_buffer_setup_ops
> ad4691_cnv_burst_offload_buffer_setup_ops = {
> +	.postenable = &ad4691_cnv_burst_offload_buffer_postenable,
> +	.predisable = &ad4691_cnv_burst_offload_buffer_predisable,
> +};
> +
>  static ssize_t sampling_frequency_show(struct device *dev,
>  				       struct device_attribute *attr,
>  				       char *buf)
> @@ -810,6 +1093,9 @@ static ssize_t sampling_frequency_show(struct
> device *dev,
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct ad4691_state *st = iio_priv(indio_dev);
> 
> +	if (st->manual_mode && st->offload)
> +		return sysfs_emit(buf, "%llu\n", st->offload->trigger_hz);

Valid. Using READ_ONCE in show and WRITE_ONCE in store for trigger_hz;
on 32-bit architectures a bare u64 read/write is not atomic.

> +
>  	return sysfs_emit(buf, "%lu\n", NSEC_PER_SEC / st->cnv_period_ns);
>  }
> 
> @@ -829,6 +1115,19 @@ static ssize_t sampling_frequency_store(struct
> device *dev,
>  	if (IIO_DEV_ACQUIRE_FAILED(claim))
>  		return -EBUSY;
> 
> +	if (st->manual_mode && st->offload) {
> +		struct spi_offload_trigger_config config = {
> +			.type = SPI_OFFLOAD_TRIGGER_PERIODIC,
> +			.periodic = { .frequency_hz = freq },
> +		};

"Does this code safely handle zero or negative inputs?
kstrtoint parses freq as a signed int, and the existing validation
(if (!freq) return -EINVAL) is bypassed for the offload path.
A negative value would sign-extend to a massive u64 value when assigned to
frequency_hz."

Addressed per previous patch.

> +
> +		ret = spi_offload_trigger_validate(st->offload->trigger,
> &config);

...

> -	ret = ad4691_setup_triggered_buffer(indio_dev, st);
> +	if (spi_offload) {
> +		indio_dev->channels = st->info->offload_info->channels;
> +		indio_dev->num_channels = st->info->offload_info-
> >num_channels;
> +		ret = ad4691_setup_offload(indio_dev, st, spi_offload);

"The offload path reuses the existing channels array which defines
scan_type.endianness = IIO_BE.
Since the SPI offload DMA RX stream delivers data as native 16-bit words,
which are little-endian on LE platforms, will the IIO subsystem incorrectly
byte-swap the native data before passing it to userspace?"

Agreed. AD4691_CHANNEL no longer sets .endianness, matching the
ad4695 approach and defaulting to IIO_CPU.

Re: [PATCH v9 4/6] iio: adc: ad4691: add SPI offload support

[Jonathan Cameron]

> > +		/* TX: address phase, CS stays asserted into data phase */
> > +		st->scan_xfers[2 * k].tx_buf = offload->tx_cmd[k];
> > +		st->scan_xfers[2 * k].len = sizeof(offload->tx_cmd[k]);
> > +		st->scan_xfers[2 * k].bits_per_word = bpw;  
> 
> "When bits_per_word is greater than 8 (like bpw = 16 here), the SPI framework
> treats tx_buf as an array of native 16-bit words.
> On little-endian architectures, the controller will byte-swap the data before
> transmitting it. Will using a u8 array and put_unaligned_be16() result in the
> command bytes being reversed on the wire?"
> 
> Switched to cpu_to_be16() assigned directly into __be16 scan_tx[],
> matching the non-offload path. This makes the intended wire format
> self-evident and sidesteps the byte-ordering question entirely.

This confuses me a bit because the SPI controller should work with
native endian and from that generate the expected big endian on the wire.

So on a little endian host byte order in address space is LH but it will
write top bit of H first thus the ADC channel address needs to be in the
second byte.
On a big endian host despite the ordering in memory being HL, the top
bit of H is still written first thus in needs to be in the first byte.


If you using cpu_to_be16() to assign a 16 bit value swapping only on little endian
and start with the cmd in L on little endian you'll end up with LH swapped to
HL and on big endian HL but the little endian SPI controller should then swap
it again sending what it thinks is the high byte first (L) whereas the big endian
system will send H.

Upshot. I think the field should be native endian. If a byte swap is needed
it should be unconditional and not rely on endianness of the host.

> 
> > +
> > +		/* RX: data phase, CS toggles after to delimit the next register
> > op */
> > +		st->scan_xfers[2 * k + 1].len = sizeof(offload->tx_cmd[k]);
> > +		st->scan_xfers[2 * k + 1].bits_per_word = bpw;
> > +		st->scan_xfers[2 * k + 1].offload_flags =
> > SPI_OFFLOAD_XFER_RX_STREAM;
> > +		st->scan_xfers[2 * k + 1].cs_change = 1;
> > +		k++;
> > +	}
> > +
> > +	/* State reset to re-arm DATA_READY for the next scan. */
> > +	put_unaligned_be16(AD4691_STATE_RESET_REG, offload->tx_reset);
> > +	offload->tx_reset[2] = AD4691_STATE_RESET_ALL;
> > +
> > +	st->scan_xfers[2 * k].tx_buf = offload->tx_reset;
> > +	st->scan_xfers[2 * k].len = sizeof(offload->tx_cmd[k]);
> > +	st->scan_xfers[2 * k].bits_per_word = bpw;
> > +
> > +	st->scan_xfers[2 * k + 1].tx_buf = &offload->tx_reset[2];
> > +	st->scan_xfers[2 * k + 1].len = sizeof(offload->tx_cmd[k]);  
> 
> "Will passing &offload->tx_reset[2] directly as tx_buf cause DMA mapping
> issues, since it is only 2-byte aligned?"
> 
> Addressed by the second concern's fix: no sub-aligned pointer into the middle
> of a DMA buffer — the full 4-byte scan_tx_reset[] is passed as tx_buf
> from its own cache-line-aligned start address.
> 

RE: [PATCH v9 4/6] iio: adc: ad4691: add SPI offload support

[Sabau, Radu bogdan]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Thursday, May 7, 2026 6:11 PM
> To: Sabau, Radu bogdan <Radu.Sabau@analog.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; David Lechner <dlechner@baylibre.com>;
> Sa, Nuno <Nuno.Sa@analog.com>; Andy Shevchenko <andy@kernel.org>;
> Rob Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>;
> Conor Dooley <conor+dt@kernel.org>; Uwe Kleine-König
> <ukleinek@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
> <broonie@kernel.org>; Linus Walleij <linusw@kernel.org>; Bartosz
> Golaszewski <brgl@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>;
> Jonathan Corbet <corbet@lwn.net>; Shuah Khan
> <skhan@linuxfoundation.org>; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> pwm@vger.kernel.org; linux-gpio@vger.kernel.org; linux-doc@vger.kernel.org
> Subject: Re: [PATCH v9 4/6] iio: adc: ad4691: add SPI offload support
> 
> 
> > > +		/* TX: address phase, CS stays asserted into data phase */
> > > +		st->scan_xfers[2 * k].tx_buf = offload->tx_cmd[k];
> > > +		st->scan_xfers[2 * k].len = sizeof(offload->tx_cmd[k]);
> > > +		st->scan_xfers[2 * k].bits_per_word = bpw;
> >
> > "When bits_per_word is greater than 8 (like bpw = 16 here), the SPI
> framework
> > treats tx_buf as an array of native 16-bit words.
> > On little-endian architectures, the controller will byte-swap the data before
> > transmitting it. Will using a u8 array and put_unaligned_be16() result in the
> > command bytes being reversed on the wire?"
> >
> > Switched to cpu_to_be16() assigned directly into __be16 scan_tx[],
> > matching the non-offload path. This makes the intended wire format
> > self-evident and sidesteps the byte-ordering question entirely.
> 
> This confuses me a bit because the SPI controller should work with
> native endian and from that generate the expected big endian on the wire.
> 
> So on a little endian host byte order in address space is LH but it will
> write top bit of H first thus the ADC channel address needs to be in the
> second byte.
> On a big endian host despite the ordering in memory being HL, the top
> bit of H is still written first thus in needs to be in the first byte.
> 
> 
> If you using cpu_to_be16() to assign a 16 bit value swapping only on little
> endian
> and start with the cmd in L on little endian you'll end up with LH swapped to
> HL and on big endian HL but the little endian SPI controller should then swap
> it again sending what it thinks is the high byte first (L) whereas the big endian
> system will send H.
> 
> Upshot. I think the field should be native endian. If a byte swap is needed
> it should be unconditional and not rely on endianness of the host.
> 

Correct. With bits_per_word=16 the SPI controller reads tx_buf as a native
16-bit word. cpu_to_be16() stores BE bytes, but on an LE host those bytes are
read back as a different native integer, sending the wrong byte first.

scan_tx is declared as u16. Both offload paths store the exact wire value as a
plain native u16 — no endianness macro. Native storage is self-consistent:
store X, read back X, SPI shifts X out MSB-first → correct wire bytes on any
host. Trace for AVG_IN(0) = 0x8201, expected wire [0x82, 0x01]:

  cpu_to_be16(0x8201), LE host → native 0x0182 → wire [0x01, 0x82]  (incorrect)
  (u16)0x8201,         LE host → native 0x8201 → wire [0x82, 0x01]  (correct)
  (u16)0x8201,         BE host → native 0x8201 → wire [0x82, 0x01]  (correct)

Manual offload: TX and RX are full-duplex (same clock cycles, shared xfer):

    st->scan_tx[k] = (u16)(AD4691_ADC_CHAN(bit) << 8);
    /* e.g. ch=0: 0x8000 → wire [0x80, 0x00] */

CNV burst offload: TX and RX are separate xfers, both use bits_per_word=bpw:

    st->scan_tx[k] = 0x8000 | AD4691_AVG_IN(bit);
    /* e.g. AVG_IN(0): 0x8201 → wire [0x82, 0x01] */

The state-reset transfer uses a u8 buffer with bits_per_word=8 (default);
put_unaligned_be16() writes bytes in memory order, which SPI sends as-is.

Re: [PATCH v9 5/6] iio: adc: ad4691: add oversampling support

[Jonathan Cameron]

On Thu, 30 Apr 2026 13:16:47 +0300
Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org> wrote:

> From: Radu Sabau <radu.sabau@analog.com>
> 
> Add per-channel oversampling ratio (OSR) support for CNV burst mode.
> The accumulator depth register (ACC_DEPTH_IN) is programmed with the
> selected OSR at buffer enable time and before each single-shot read.
> 
> Supported OSR values: 1, 2, 4, 8, 16, 32.
> 
> Introduce AD4691_MANUAL_CHANNEL() for manual mode channels, which do
> not expose the oversampling ratio attribute since OSR is not applicable
> in that mode. A separate manual_channels array is added to
> struct ad4691_channel_info and selected at probe time; offload paths
> reuse the same arrays with num_channels capping access before the soft
> timestamp entry.
> 
> in_voltageN_sampling_frequency represents the effective output rate for
> channel N, defined as osc_freq / osr[N]. The chip has one internal
> oscillator shared by all channels; each channel independently
> accumulates osr[N] oscillator cycles before producing a result.
> 
> Writing sampling_frequency computes needed_osc = freq * osr[N] and
> snaps down to the largest oscillator table entry that satisfies both
> osc <= needed_osc and osc % osr[N] == 0, guaranteeing an exact integer
> read-back. The result is stored in target_osc_freq_Hz and written to
> OSC_FREQ_REG at buffer enable and single-shot time, so sampling_frequency
> and oversampling_ratio can be set in any order.
> 
> in_voltageN_sampling_frequency_available is computed dynamically from
> the channel's current OSR, listing only oscillator table entries that
> divide evenly by osr[N], expressed as effective rates. The list becomes
> sparser as OSR increases, capping at max_rate / osr[N].
> 
> Writing oversampling_ratio stores the new OSR for that channel;
> target_osc_freq_Hz is left unchanged. The effective rate read back via
> in_voltageN_sampling_frequency becomes target_osc_freq_Hz / new_osr
> automatically. The two attributes are orthogonal: sampling_frequency
> controls the oscillator, oversampling_ratio controls the averaging depth.
> 
> OSR defaults to 1 (no accumulation) for all channels.
> 
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
Just one thing - from Sashiko again.

J
>  
>  static int ad4691_read_avail(struct iio_dev *indio_dev,
> @@ -540,10 +655,30 @@ static int ad4691_read_avail(struct iio_dev *indio_dev,
>  	unsigned int start = ad4691_samp_freq_start(st->info);
>  
>  	switch (mask) {
> -	case IIO_CHAN_INFO_SAMP_FREQ:
> -		*vals = &ad4691_osc_freqs_Hz[start];
> +	case IIO_CHAN_INFO_SAMP_FREQ: {
> +		unsigned int osr = st->osr[chan->channel];
> +		int n = 0;
> +
Sashiko shouts about possibly getting a torn set in here if osr were to be changed
whilst you were computing the array.  That's probably worth using locks to protect against.
> +		/*
> +		 * Only oscillator frequencies evenly divisible by the channel's
> +		 * OSR yield an integer effective rate; expose those as effective
> +		 * rates (osc / osr) so the user works entirely in output-sample
> +		 * space.
> +		 */
> +		for (unsigned int i = start; i < ARRAY_SIZE(ad4691_osc_freqs_Hz); i++) {
> +			if (ad4691_osc_freqs_Hz[i] % osr != 0)
> +				continue;
> +			st->samp_freq_avail[n++] = ad4691_osc_freqs_Hz[i] / osr;
> +		}
> +		*vals = st->samp_freq_avail;
>  		*type = IIO_VAL_INT;
> -		*length = ARRAY_SIZE(ad4691_osc_freqs_Hz) - start;
> +		*length = n;
> +		return IIO_AVAIL_LIST;
> +	}
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*vals = ad4691_oversampling_ratios;
> +		*type = IIO_VAL_INT;
> +		*length = ARRAY_SIZE(ad4691_oversampling_ratios);
>  		return IIO_AVAIL_LIST;
>  	default:
>  		return -EINVAL;

RE: [PATCH v9 5/6] iio: adc: ad4691: add oversampling support

[Sabau, Radu bogdan]

Addressing Sashiko's review for the oversampling support patch.

> -----Original Message-----
> From: Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org>
> Sent: Thursday, April 30, 2026 1:17 PM

...

> +/* CNV burst mode channel — exposes oversampling ratio. */
>  #define AD4691_CHANNEL(ch)
> 	\
> +	{								\
> +		.type = IIO_VOLTAGE,					\
> +		.indexed = 1,						\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> 	\
> +
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) | \
> +				      BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> +		.info_mask_separate_available =
> 	\
> +
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) | \
> +				      BIT(IIO_CHAN_INFO_SAMP_FREQ),	\

"The IIO_CHAN_INFO_SAMP_FREQ attribute is exported with info_mask_separate,
implying per-channel independence. However, writing to sampling_frequency
updates st->target_osc_freq_Hz, a global variable shared by all channels.
Does modifying this global variable on one channel silently alter the
effective sampling rate of all other channels? Should this use
info_mask_shared_by_all to comply with the IIO ABI?"

info_mask_separate is intentional here. The oscillator is shared, but
the effective output rate is genuinely per-channel: osc_freq / osr[N].
Writing in_voltageN_sampling_frequency sets the oscillator (which changes
the read-back for all channels), but reading it back for any channel always
returns osc_freq / osr[N] — a value specific to that channel's OSR. The
oscillator snaps to the largest table entry <= freq * osr[N] that is evenly
divisible by osr[N], so the read-back is always an integer for the channel
that drove the write. The same trade-off is present in ad4695.
info_mask_separate is the correct annotation for this per-channel view.

> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),
> 	\
> +		.channel = ch,						\
> +		.scan_index = ch,					\
> +		.scan_type = {						\
> +			.sign = 'u',					\
> +			.realbits = 16,					\
> +			.storagebits = 16,				\
> +			.endianness = IIO_BE,				\
> +		},							\
> +	}
> +

...

>  static int ad4691_read_avail(struct iio_dev *indio_dev,
> @@ -540,10 +655,30 @@ static int ad4691_read_avail(struct iio_dev
> *indio_dev,
>  	unsigned int start = ad4691_samp_freq_start(st->info);
> 
>  	switch (mask) {
> -	case IIO_CHAN_INFO_SAMP_FREQ:
> -		*vals = &ad4691_osc_freqs_Hz[start];
> +	case IIO_CHAN_INFO_SAMP_FREQ: {
> +		unsigned int osr = st->osr[chan->channel];
> +		int n = 0;
> +
> +		/*
> +		 * Only oscillator frequencies evenly divisible by the channel's
> +		 * OSR yield an integer effective rate; expose those as effective
> +		 * rates (osc / osr) so the user works entirely in output-sample
> +		 * space.
> +		 */
> +		for (unsigned int i = start; i <
> ARRAY_SIZE(ad4691_osc_freqs_Hz); i++) {
> +			if (ad4691_osc_freqs_Hz[i] % osr != 0)
> +				continue;
> +			st->samp_freq_avail[n++] = ad4691_osc_freqs_Hz[i] /
> osr;
> +		}
> +		*vals = st->samp_freq_avail;

"The IIO core calls read_avail() without holding driver-level locks that
serialize sysfs reads. Can concurrent reads of
in_voltageN_sampling_frequency_available for channels with different OSR
values race and overwrite st->samp_freq_avail simultaneously?
This could result in interleaved or corrupted output being formatted by the
IIO core and sent to userspace. Is a per-channel array or synchronization
required here?"

Valid. Sysfs serializes reads to the same sysfs file but not across
different channel _available files. Concurrent reads of
in_voltage0_sampling_frequency_available and
in_voltage1_sampling_frequency_available (with different OSRs) raced on
the shared samp_freq_avail[] scratch array. Made it a 2D array
[16][N_OSC_ENTRIES] so each channel has its own sub-array; concurrent
reads for different channels no longer interfere.

>  		*type = IIO_VAL_INT;
> -		*length = ARRAY_SIZE(ad4691_osc_freqs_Hz) - start;
> +		*length = n;
> +		return IIO_AVAIL_LIST;

...

> @@ -634,9 +780,29 @@ static int ad4691_write_raw(struct iio_dev
> *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int val, int val2, long mask)
>  {
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		return ad4691_set_sampling_freq(indio_dev, val);
> +		return ad4691_set_sampling_freq(indio_dev, chan, val);
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO: {
> +		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +		if (IIO_DEV_ACQUIRE_FAILED(claim))
> +			return -EBUSY;
> +
> +		for (unsigned int i = 0; i <
> ARRAY_SIZE(ad4691_oversampling_ratios); i++) {
> +			if (ad4691_oversampling_ratios[i] != val)
> +				continue;
> +			/*
> +			 * Store the new OSR; target_osc_freq_Hz is
> unchanged.
> +			 * The effective rate read back via
> in_voltageN_sampling_frequency
> +			 * becomes target_osc_freq_Hz / new_osr
> automatically.
> +			 */
> +			st->osr[chan->channel] = val;

"Writing oversampling_ratio updates osr but leaves target_osc_freq_Hz
unchanged. Does this break the target_osc_freq_Hz % osr == 0 guarantee
enforced in ad4691_set_sampling_freq()?
Reading sampling_frequency after this could yield a truncated integer. If
that exact integer is written back, it might compute a smaller oscillator
requirement and snap the device to a different, lower oscillator frequency.
Does modifying oversampling_ratio need to recalculate target_osc_freq_Hz
to maintain state consistency?"

Valid. Storing a new OSR without updating target_osc_freq_Hz can
break the target_osc_freq_Hz % osr == 0 invariant. For example with
target_osc_freq_Hz = 250 kHz and new OSR = 32: read-back is
250000 / 32 = 7812 (truncated); writing that value back computes
needed_osc = 249984, snapping the oscillator to the next lower table
entry — a silent frequency change on what the user expected to be a
no-op write-back.

Fixed: the OVERSAMPLING_RATIO write path now calls ad4691_find_osc_freq()
with old_effective * new_osr as the ceiling, where
old_effective = target_osc_freq_Hz / old_osr. This preserves the
divisibility invariant and keeps the oscillator as close as possible to
the previous effective rate.

> +			return 0;
> +		}
> +		return -EINVAL;
> +	}
>  	default:
>  		return -EINVAL;
>  	}
> @@ -691,6 +857,10 @@ static int ad4691_enter_conversion_mode(struct
> ad4691_state *st)
>  		return regmap_update_bits(st->regmap,
> AD4691_DEVICE_SETUP,
>  					  AD4691_MANUAL_MODE,
> AD4691_MANUAL_MODE);
> 
> +	ret = ad4691_write_osc_freq(st);
> +	if (ret)
> +		return ret;
> +
>  	ret = regmap_update_bits(st->regmap, AD4691_ADC_SETUP,
>  				 AD4691_ADC_MODE_MASK,
> AD4691_CNV_BURST_MODE);
>  	if (ret)
> @@ -844,6 +1014,12 @@ static int
> ad4691_cnv_burst_buffer_preenable(struct iio_dev *indio_dev)
>  	if (ret)
>  		goto err_unoptimize;
> 
> +	iio_for_each_active_channel(indio_dev, i) {
> +		ret = regmap_write(st->regmap, AD4691_ACC_DEPTH_IN(i),
> st->osr[i]);
> +		if (ret)
> +			goto err_unoptimize;
> +	}
> +

"If the IIO soft timestamp channel is enabled, its scan_index is 16.
iio_for_each_active_channel() iterates over all active channels including
the timestamp channel.
Will this loop process i = 16 and access st->osr[16] out of bounds, since
st->osr is an array of size 16?
Additionally, since AD4691_ACC_DEPTH_IN(16) resolves to 0x196
(AD4691_GPIO_MODE1_REG), will this silently corrupt the GPIO configuration
and potentially break the DATA_READY interrupt?
Furthermore, does the presence of the timestamp channel cause the loop
iteration to advance k to 17, making the state-reset SPI transfers appended
after the loop write to st->scan_xfers[34] and [35]? This would overflow
the scan_xfers array which is sized 34 and could corrupt the surrounding
struct ad4691_state."

Valid for the osr[] out-of-bounds and GPIO corruption concerns. With
the soft timestamp enabled (scan_index = 16), the loop would access
st->osr[16] out-of-bounds and write to AD4691_ACC_DEPTH_IN(16) = 0x196,
which is AD4691_GPIO_MODE1_REG, silently corrupting the GPIO configuration
and potentially breaking the DATA_READY interrupt.

Added the same guard used by the scan_xfers loops in the triggered-buffer
commit: if (i >= indio_dev->num_channels - 1) break.

The scan_xfers k-overflow concern is already handled by that existing guard
in a separate loop — it is not affected by the ACC_DEPTH_IN loop added
here.

Re: [PATCH v9 5/6] iio: adc: ad4691: add oversampling support

[Jonathan Cameron]

On Thu, 7 May 2026 11:56:53 +0000
"Sabau, Radu bogdan" <Radu.Sabau@analog.com> wrote:

> Addressing Sashiko's review for the oversampling support patch.
> 
One thing inline. I think Sashiko got it wrong..

> > @@ -691,6 +857,10 @@ static int ad4691_enter_conversion_mode(struct
> > ad4691_state *st)
> >  		return regmap_update_bits(st->regmap,
> > AD4691_DEVICE_SETUP,
> >  					  AD4691_MANUAL_MODE,
> > AD4691_MANUAL_MODE);
> > 
> > +	ret = ad4691_write_osc_freq(st);
> > +	if (ret)
> > +		return ret;
> > +
> >  	ret = regmap_update_bits(st->regmap, AD4691_ADC_SETUP,
> >  				 AD4691_ADC_MODE_MASK,
> > AD4691_CNV_BURST_MODE);
> >  	if (ret)
> > @@ -844,6 +1014,12 @@ static int
> > ad4691_cnv_burst_buffer_preenable(struct iio_dev *indio_dev)
> >  	if (ret)
> >  		goto err_unoptimize;
> > 
> > +	iio_for_each_active_channel(indio_dev, i) {
> > +		ret = regmap_write(st->regmap, AD4691_ACC_DEPTH_IN(i),
> > st->osr[i]);
> > +		if (ret)
> > +			goto err_unoptimize;
> > +	}
> > +  
> 
> "If the IIO soft timestamp channel is enabled, its scan_index is 16.
> iio_for_each_active_channel() iterates over all active channels including
> the timestamp channel.

I saw this one and think it's wrong.

It is sort of true and sort of not.  active_scan_mask never has the timestamp
channel set.  There is special handling for that channel.

https://elixir.bootlin.com/linux/v7.0.1/source/drivers/iio/industrialio-buffer.c#L613

IIRC the bitmap is technically one bit too large given that quirk
(I've not checked that today so I'm relying on memory of when this last came up).

Jonathan



> Will this loop process i = 16 and access st->osr[16] out of bounds, since
> st->osr is an array of size 16?
> Additionally, since AD4691_ACC_DEPTH_IN(16) resolves to 0x196
> (AD4691_GPIO_MODE1_REG), will this silently corrupt the GPIO configuration
> and potentially break the DATA_READY interrupt?
> Furthermore, does the presence of the timestamp channel cause the loop
> iteration to advance k to 17, making the state-reset SPI transfers appended
> after the loop write to st->scan_xfers[34] and [35]? This would overflow
> the scan_xfers array which is sized 34 and could corrupt the surrounding
> struct ad4691_state."
> 
> Valid for the osr[] out-of-bounds and GPIO corruption concerns. With
> the soft timestamp enabled (scan_index = 16), the loop would access
> st->osr[16] out-of-bounds and write to AD4691_ACC_DEPTH_IN(16) = 0x196,
> which is AD4691_GPIO_MODE1_REG, silently corrupting the GPIO configuration
> and potentially breaking the DATA_READY interrupt.
> 
> Added the same guard used by the scan_xfers loops in the triggered-buffer
> commit: if (i >= indio_dev->num_channels - 1) break.
> 
> The scan_xfers k-overflow concern is already handled by that existing guard
> in a separate loop — it is not affected by the ACC_DEPTH_IN loop added
> here.
>