Re: [PATCH v4 3/4] iio: adc: ad4691: add triggered buffer support

[Nuno Sá]

Hi Radu,

Some comments from me.

On Fri, 2026-03-20 at 13:03 +0200, Radu Sabau via B4 Relay wrote:
> From: Radu Sabau <radu.sabau@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> ---
>  drivers/iio/adc/Kconfig  |   2 +
>  drivers/iio/adc/ad4691.c | 584 +++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 571 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 3685a03aa8dc..d498f16c0816 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -142,6 +142,8 @@ config AD4170_4
>  config AD4691
>  	tristate "Analog Devices AD4691 Family ADC Driver"
>  	depends on SPI
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
>  	select REGMAP
>  	help
>  	  Say yes here to build support for Analog Devices AD4691 Family MuxSAR
> diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> index 5e02eb44ca44..db776de32846 100644
> --- a/drivers/iio/adc/ad4691.c
> +++ b/drivers/iio/adc/ad4691.c
> @@ -9,9 +9,12 @@
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> +#include <linux/interrupt.h>
>  #include <linux/math.h>
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> +#include <linux/pwm.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
> @@ -19,7 +22,12 @@
>  #include <linux/units.h>
>  #include <linux/unaligned.h>
>  
> +#include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
>  
>  #define AD4691_VREF_uV_MIN			2400000
>  #define AD4691_VREF_uV_MAX			5250000
> @@ -28,6 +36,8 @@
>  #define AD4691_VREF_3P3_uV_MAX			3750000
>  #define AD4691_VREF_4P096_uV_MAX		4500000
>  
> +#define AD4691_CNV_DUTY_CYCLE_NS		380
> +
>  #define AD4691_SPI_CONFIG_A_REG			0x000
>  #define AD4691_SW_RESET				(BIT(7) | BIT(0))
>  

...

> 
> +static int ad4691_cnv_burst_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(st->regmap);
> +	struct spi_device *spi = to_spi_device(dev);
> +	unsigned int n_active = hweight_long(*indio_dev->active_scan_mask);
> +	unsigned int bit, k, i;
> +	int ret;
> +
> +	st->scan_devm_group = devres_open_group(dev, NULL, GFP_KERNEL);
> +	if (!st->scan_devm_group)
> +		return -ENOMEM;

Agree with Jonathan. Not seeing a valid reason for the above.

> +
> +	st->scan_xfers = devm_kcalloc(dev, 2 * n_active, sizeof(*st->scan_xfers),
> +				      GFP_KERNEL);
> +	st->scan_tx = devm_kcalloc(dev, n_active, sizeof(*st->scan_tx),
> +				   GFP_KERNEL);
> +	st->scan_rx = devm_kcalloc(dev, n_active, sizeof(*st->scan_rx),
> +				   GFP_KERNEL);
> +	if (!st->scan_xfers || !st->scan_tx || !st->scan_rx) {
> +		devres_release_group(dev, st->scan_devm_group);
> +		return -ENOMEM;
> +	}
> +
> +	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->scan_rx[k];
> +		st->scan_xfers[2 * k + 1].len = sizeof(__be16);
> +		if (k < n_active - 1)
> +			st->scan_xfers[2 * k + 1].cs_change = 1;
> +		spi_message_add_tail(&st->scan_xfers[2 * k + 1], &st->scan_msg);
> +		k++;
> +	}
> +
> +	devres_close_group(dev, st->scan_devm_group);
> +
> +	st->scan_msg.spi = spi;
> +
> +	ret = spi_optimize_message(spi, &st->scan_msg);
> +	if (ret) {
> +		devres_release_group(dev, st->scan_devm_group);
> +		return ret;
> +	}
> +
> +	ret = regmap_write(st->regmap, AD4691_ACC_MASK_REG,
> +			   (u16)~(*indio_dev->active_scan_mask));
> +	if (ret)
> +		goto err;
> +
> +	ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
> +			   *indio_dev->active_scan_mask);
> +	if (ret)
> +		goto err;
> +
> +	iio_for_each_active_channel(indio_dev, bit) {
> +		ret = regmap_write(st->regmap, AD4691_ACC_COUNT_LIMIT(bit),
> +				   AD4691_ACC_COUNT_VAL);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	ret = ad4691_enter_conversion_mode(st);
> +	if (ret)
> +		goto err;
> +
> +	ret = ad4691_sampling_enable(st, true);
> +	if (ret)
> +		goto err;
> +
> +	enable_irq(st->irq);
> +	return 0;
> +err:
> +	spi_unoptimize_message(&st->scan_msg);
> +	devres_release_group(dev, st->scan_devm_group);
> +	return ret;
> +}
> +
> +static int ad4691_cnv_burst_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(st->regmap);
> +	int ret;
> +
> +	disable_irq(st->irq);

Should we use disable_irq_sync()?
> +
> +	ret = ad4691_sampling_enable(st, false);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
> +			   AD4691_SEQ_ALL_CHANNELS_OFF);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4691_exit_conversion_mode(st);
> +	spi_unoptimize_message(&st->scan_msg);
> +	devres_release_group(dev, st->scan_devm_group);
> +	return ret;
> +}
> +
> +static const struct iio_buffer_setup_ops ad4691_cnv_burst_buffer_setup_ops = {
> +	.preenable = &ad4691_cnv_burst_buffer_preenable,
> +	.postdisable = &ad4691_cnv_burst_buffer_postdisable,
> +};
> +
> +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);
> +
> +	if (st->manual_mode)
> +		return -ENODEV;

Can the above happen at all? I think you're making sure (at probe) this interface
never get's exposed in manual mode.

> +
> +	return sysfs_emit(buf, "%u\n", (u32)(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;
> +
> +	if (st->manual_mode)
> +		return -ENODEV;
> +
> +	ret = kstrtoint(buf, 10, &freq);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = ad4691_set_pwm_freq(st, freq);
> +	if (ret)
> +		return ret;
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR(sampling_frequency, 0644,
> +		       sampling_frequency_show,
> +		       sampling_frequency_store, 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);
> +
> +	/*
> +	 * GPx has asserted: stop conversions before reading so the
> +	 * accumulator does not continue sampling while the trigger handler
> +	 * processes the data. Then fire the IIO trigger to push the sample
> +	 * to the buffer.
> +	 */
> +	ad4691_sampling_enable(st, false);
> +	iio_trigger_poll(st->trig);

Not sure you need to save trig in your struct. We already have it in indio_dev->trig. Sure,
it is a private member but still fairly common to see (this patch included):

indio_dev->trig = iio_trigger_get(trig);

So I would say we either assume it's public or start to not allow the above
pattern.

Alternatively, I don't think you're using indio_dev driverdata right? Could save it
in there.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct iio_trigger_ops ad4691_trigger_ops = {
> +	.validate_device = iio_trigger_validate_own_device,
> +};
> +
> +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);
> +	unsigned int i, k = 0;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = spi_sync(st->scan_msg.spi, &st->scan_msg);
> +	if (ret)
> +		goto done;
> +
> +	if (st->manual_mode) {
> +		iio_for_each_active_channel(indio_dev, i) {
> +			st->scan.vals[i] = be16_to_cpu(st->scan_rx[k + 1]);
> +			k++;
> +		}
> +	} else {
> +		iio_for_each_active_channel(indio_dev, i) {
> +			st->scan.vals[i] = be16_to_cpu(st->scan_rx[k]);
> +			k++;
> +		}
> +
> +		ret = regmap_write(st->regmap, AD4691_STATE_RESET_REG,
> +				   AD4691_STATE_RESET_ALL);
> +		if (ret)
> +			goto done;
> +
> +		ret = ad4691_sampling_enable(st, true);
> +		if (ret)
> +			goto done;
> +	}
> +
> +	iio_push_to_buffers_with_ts(indio_dev, &st->scan, sizeof(st->scan),
> +				    pf->timestamp);
> +
> +done:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
> +
>  static const struct iio_info ad4691_info = {
>  	.read_raw = &ad4691_read_raw,
>  	.write_raw = &ad4691_write_raw,
> @@ -495,6 +934,25 @@ static const struct iio_info ad4691_info = {
>  	.debugfs_reg_access = &ad4691_reg_access,
>  };
>  
> +static int ad4691_pwm_setup(struct ad4691_state *st)
> +{
> +	struct device *dev = regmap_get_device(st->regmap);
> +	int ret;
> +
> +	st->conv_trigger = devm_pwm_get(dev, "cnv");
> +	if (IS_ERR(st->conv_trigger))
> +		return dev_err_probe(dev, PTR_ERR(st->conv_trigger),
> +				     "Failed to get cnv pwm\n");
> +
> +	ret = devm_add_action_or_reset(dev, ad4691_disable_pwm,
> +				       st->conv_trigger);

This is a suspicious pattern. But I do see it's used like this in more places and it's a no-op
if PWM is already disabled. Still, not sure if agree with this kind "unbalanced" handling.

> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to register PWM disable action\n");
> +
> +	return ad4691_set_pwm_freq(st, st->info->max_rate);
> +}
> +
>  static int ad4691_regulator_setup(struct ad4691_state *st)
>  {
>  	struct device *dev = regmap_get_device(st->regmap);
> @@ -555,12 +1013,30 @@ static int ad4691_reset(struct ad4691_state *st)
>  	return 0;
>  }
>  
> -static int ad4691_config(struct ad4691_state *st)
> +static int ad4691_config(struct ad4691_state *st, u32 max_speed_hz)

My eyes might be failing me but where is 'max_speed_hz' used?


>  {
>  	struct device *dev = regmap_get_device(st->regmap);
>  	enum ad4691_ref_ctrl ref_val;
> +	const char *irq_name;
> +	unsigned int gp_num;
>  	int ret;
>  
> +	/*
> +	 * Determine buffer conversion mode from DT: if a PWM is provided it
> +	 * drives the CNV pin (CNV_BURST_MODE); otherwise CNV is tied to CS
> +	 * and each SPI transfer triggers a conversion (MANUAL_MODE).
> +	 * Both modes idle in AUTONOMOUS mode so that read_raw can use the
> +	 * internal oscillator without disturbing the hardware configuration.
> +	 */
> +	if (device_property_present(dev, "pwms")) {
> +		st->manual_mode = false;
> +		ret = ad4691_pwm_setup(st);
> +		if (ret)
> +			return ret;
> +	} else {
> +		st->manual_mode = true;
> +	}
> +
>  	switch (st->vref_uV) {
>  	case AD4691_VREF_uV_MIN ... AD4691_VREF_2P5_uV_MAX:
>  		ref_val = AD4691_VREF_2P5;
> @@ -595,17 +1071,91 @@ static int ad4691_config(struct ad4691_state *st)
>  		return dev_err_probe(dev, ret, "Failed to write DEVICE_SETUP\n");
>  
>  	/*
> -	 * Set the internal oscillator to the highest valid rate for this chip.
> -	 * Index 0 (1 MHz) is valid only for AD4692/AD4694; AD4691/AD4693 start
> -	 * at index 1 (500 kHz).
> +	 * Set the internal oscillator to the highest rate this chip supports.
> +	 * Index 0 (1 MHz) exceeds the 500 kHz max of AD4691/AD4693, so those
> +	 * chips start at index 1 (500 kHz).
>  	 */
>  	ret = regmap_write(st->regmap, AD4691_OSC_FREQ_REG,
> -			   (st->info->max_rate == HZ_PER_MHZ) ? 0 : 1);
> +			   (ad4691_osc_freqs[0] > st->info->max_rate) ? 1 : 0);

Does this belong to this commit?

>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to write OSC_FREQ\n");
>  
>  	/* Device always operates in AUTONOMOUS mode. */
> -	return regmap_write(st->regmap, AD4691_ADC_SETUP, AD4691_AUTONOMOUS_MODE_VAL);
> +	ret = regmap_write(st->regmap, AD4691_ADC_SETUP, AD4691_AUTONOMOUS_MODE);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to write ADC_SETUp\n");
> +
> +	if (st->manual_mode)
> +		return 0;
> +
> +	ret = device_property_read_string_array(dev, "interrupt-names",
> +						&irq_name, 1);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to read interrupt-names\n");
> +
> +	if (strncmp(irq_name, "gp", 2) != 0 ||
> +	    kstrtouint(irq_name + 2, 10, &gp_num) || gp_num > 3)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Invalid interrupt name '%s'\n", irq_name);
> +

I would likely prefer something like [1] rather than the string parsing.

[1]: https://elixir.bootlin.com/linux/v7.0-rc5/source/drivers/iio/imu/adis16480.c#L1582

- Nuno Sá