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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 20 Mar 2026 13:03:57 +0200
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

The first I assume?  Might be up to 4 of them.

> 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.

No oversampling configuration?  If it's a fixed length burst I'd still
expect to see an indication of what it is and if we can flip back to
no oversampling by changing mode, that should be oversampling == 1.
Seems there is a depth setting for the averaging filters, that superficially
at least appears to be the right control for this.

Seems like there is an SPI burst mode as well?  That feels like very
standard oversampling.

> 
> 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>
A few comments inline.  All the modes etc in this driver are complex
enough I think this one needs a driver specific document in Documentation/iio

Thanks,

Jonathan

> 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

> @@ -121,6 +140,7 @@ static const struct iio_chan_spec ad4691_channels[] = {
>  	AD4691_CHANNEL(13),
>  	AD4691_CHANNEL(14),
>  	AD4691_CHANNEL(15),
> +	IIO_CHAN_SOFT_TIMESTAMP(16),
>  };
>  
>  static const struct iio_chan_spec ad4693_channels[] = {
> @@ -132,6 +152,7 @@ static const struct iio_chan_spec ad4693_channels[] = {
>  	AD4691_CHANNEL(5),
>  	AD4691_CHANNEL(6),
>  	AD4691_CHANNEL(7),
> +	IIO_CHAN_SOFT_TIMESTAMP(16),

Any big advantage in pushing it all the way down to 16?  It's ABI compliant
as these only have to be monotonic, but I'd feel it was more natural as 8 unless
there is something I'm missing.

>  };
>  
>  /*
> @@ -189,16 +210,63 @@ static const struct ad4691_chip_info ad4694_chip_info = {
>  struct ad4691_state {
>  	const struct ad4691_chip_info	*info;
>  	struct regmap			*regmap;
> +
> +	struct pwm_device		*conv_trigger;
> +	struct iio_trigger		*trig;
> +	int				irq;
> +
> +	bool				manual_mode;
> +
>  	int				vref_uV;
>  	bool				refbuf_en;
>  	bool				ldo_en;
> +	u32				cnv_period_ns;
>  	/*
>  	 * Synchronize access to members of the driver state, and ensure
>  	 * atomicity of consecutive SPI operations.
>  	 */
>  	struct mutex			lock;
> +	/*
> +	 * Per-buffer-enabl ree lifetimesources:
> +	 * Manual Mode - a pre-built SPI message that clocks out N+1
> +	 *		 transfers in one go.
> +	 * CNV Burst Mode - a pre-built SPI message that clocks out 2*N
> +	 *		    transfers in one go.
> +	 */
> +	void				*scan_devm_group;
> +	struct spi_message		scan_msg;
> +	struct spi_transfer		*scan_xfers;
> +	__be16				*scan_tx;
> +	__be16				*scan_rx;
> +	/* Scan buffer: one slot per channel (u16) plus timestamp */
> +	struct {
> +		u16 vals[16];
> +		s64 ts __aligned(8);

aligned_s64

That was introduced last year IIRC.


> +	} scan __aligned(IIO_DMA_MINALIGN);
>  };

>  static int ad4691_reg_read(void *context, unsigned int reg, unsigned int *val)
>  {
>  	struct spi_device *spi = context;
> @@ -341,14 +409,16 @@ static int ad4691_get_sampling_freq(struct ad4691_state *st, int *val)
>  static int ad4691_set_sampling_freq(struct iio_dev *indio_dev, int freq)
>  {
>  	struct ad4691_state *st = iio_priv(indio_dev);
> -	unsigned int start = (st->info->max_rate == HZ_PER_MHZ) ? 0 : 1;
>  	unsigned int i;
>  
> +	if (freq > st->info->max_rate)
> +		return -EINVAL;
> +
>  	IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
>  	if (IIO_DEV_ACQUIRE_FAILED(claim))
>  		return -EBUSY;
>  
> -	for (i = start; i < ARRAY_SIZE(ad4691_osc_freqs); i++) {
> +	for (i = 0; i < ARRAY_SIZE(ad4691_osc_freqs); i++) {

Why is the start offset no longer relevant?

>  		if ((int)ad4691_osc_freqs[i] == freq)
>  			return regmap_update_bits(st->regmap, AD4691_OSC_FREQ_REG,
>  						  AD4691_OSC_FREQ_MASK, i);

> @@ -386,8 +459,7 @@ static int ad4691_single_shot_read(struct iio_dev *indio_dev,
>  	guard(mutex)(&st->lock);
>  
>  	/*
> -	 * Use AUTONOMOUS mode for single-shot reads. The chip always
> -	 * operates in AUTONOMOUS mode in this driver revision.
> +	 * Use AUTONOMOUS mode for single-shot reads.
This edit doesn't seem to belong in this patch. I'd just put the new
text in place in the earlier patch.

> +static int ad4691_manual_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 n_xfers = n_active + 1;
> +	unsigned int k, i;
> +	int ret;
> +
> +	st->scan_devm_group = devres_open_group(dev, NULL, GFP_KERNEL);
> +	if (!st->scan_devm_group)
> +		return -ENOMEM;
> +
> +	st->scan_xfers = devm_kcalloc(dev, n_xfers, sizeof(*st->scan_xfers),
> +				      GFP_KERNEL);
> +	st->scan_tx = devm_kcalloc(dev, n_xfers, sizeof(*st->scan_tx),
> +				   GFP_KERNEL);
> +	st->scan_rx = devm_kcalloc(dev, n_xfers, 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);
> +
> +	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];
> +		st->scan_xfers[k].rx_buf = &st->scan_rx[k];
> +		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);
> +		k++;
> +	}
> +
> +	/* Final NOOP transfer to retrieve last channel's result. */
> +	st->scan_tx[k] = cpu_to_be16(AD4691_NOOP);
> +	st->scan_xfers[k].tx_buf = &st->scan_tx[k];
> +	st->scan_xfers[k].rx_buf = &st->scan_rx[k];
> +	st->scan_xfers[k].len = sizeof(__be16);
> +	spi_message_add_tail(&st->scan_xfers[k], &st->scan_msg);
> +
> +	devres_close_group(dev, st->scan_devm_group);

Similar comment to below on devres groups not really being appropriate
here.

> +
> +	st->scan_msg.spi = spi;
> +
> +	ret = spi_optimize_message(spi, &st->scan_msg);
> +	if (ret) {
> +		devres_release_group(dev, st->scan_devm_group);
If you were keeping this, better to do an error handling block via
gotos like you do for the other similar code.

> +		return ret;
> +	}
> +
> +	ret = ad4691_enter_conversion_mode(st);
> +	if (ret) {
> +		spi_unoptimize_message(&st->scan_msg);
> +		devres_release_group(dev, st->scan_devm_group);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad4691_manual_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;
> +
> +	ret = ad4691_exit_conversion_mode(st);
> +	spi_unoptimize_message(&st->scan_msg);
> +	devres_release_group(dev, st->scan_devm_group);
> +	return ret;
> +}
;
> +
> +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;
> +
> +	st->scan_xfers = devm_kcalloc(dev, 2 * n_active, sizeof(*st->scan_xfers),
> +				      GFP_KERNEL);

See later.  I don't see a benefit in using a devres group here. It's nothing
to do with device lifetime.

> +	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);

If you were keeping this (against comment below) why close it here?  Close it once
devm calls are done above.

> +
> +	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);
> +
> +	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);

I'm not seeing an obvious reason for devres group handling here.
Normally we do that if there is a path to remove() in which the cleanup
still needs to be done.  Here there isn't as we always turn the buffer
off earlier in the remove flow.  So I'd do things the old fashioned way
and free them by hand.


> +	return ret;
> +}

> +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,

No comma.

> +};

> +
> +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);

General rule (see cleanup.h comments) is don't use guard() or __free()
stuff in a function that also does goto.  Just handle the mutex()
the old fashioned way.  Alternative is to use a helper function and
just leave the iio_trigger_notify_done() in this outer call.

> +
> +	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;
> +}
> +


> @@ -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.

This reword doesn't obviously fit in this patch. Move it to earlier one.

> +	 * 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);
>  	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)

No need to check this. Just request the individual GPIOs when needed by name.

> +		return dev_err_probe(dev, -EINVAL,
> +				     "Invalid interrupt name '%s'\n", irq_name);
> +
> +	return ad4691_gpio_setup(st, gp_num);
> +}
> +
> +static int ad4691_setup_triggered_buffer(struct iio_dev *indio_dev,
> +					 struct ad4691_state *st)
> +{
> +	struct device *dev = regmap_get_device(st->regmap);
> +	int irq, ret;
> +
> +	st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> +					  indio_dev->name,
> +					  iio_device_id(indio_dev));
> +	if (!st->trig)
> +		return -ENOMEM;
> +
> +	st->trig->ops = &ad4691_trigger_ops;
> +	iio_trigger_set_drvdata(st->trig, st);
> +
> +	ret = devm_iio_trigger_register(dev, st->trig);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "IIO trigger register failed\n");
> +
> +	indio_dev->trig = iio_trigger_get(st->trig);
> +
> +	if (!st->manual_mode) {
> +		/*
> +		 * GP0 asserts at end-of-conversion. The IRQ handler stops
> +		 * conversions and fires the IIO trigger so the trigger handler
> +		 * can read and push the sample to the buffer. The IRQ is kept
> +		 * disabled until the buffer is enabled.
> +		 */
> +		irq = fwnode_irq_get(dev_fwnode(dev), 0);

Given the presence of interrupt names, there is no guarantee that this is GP0.
You need to get it by name to know that.  They can come in any order, or indeed
any given one can be not wired.

> +		if (irq < 0)
> +			return dev_err_probe(dev, irq,
> +					     "failed to get GP interrupt\n");
> +
> +		st->irq = irq;
> +
> +		ret = devm_request_threaded_irq(dev, irq, NULL,
> +						&ad4691_irq,
> +						IRQF_ONESHOT | IRQF_NO_AUTOEN,

When we are not enabling at boot it is usually helpful to add a comment for why
not.

> +						indio_dev->name, indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		return devm_iio_triggered_buffer_setup_ext(dev, indio_dev,
> +							   &iio_pollfunc_store_time,
> +							   &ad4691_trigger_handler,
> +							   IIO_BUFFER_DIRECTION_IN,
> +							   &ad4691_cnv_burst_buffer_setup_ops,
> +							   ad4691_buffer_attrs);
> +	}
> +
> +	return devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					       &iio_pollfunc_store_time,
> +					       &ad4691_trigger_handler,
> +					       &ad4691_manual_buffer_setup_ops);
>  }