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

["Nuno Sá" <noname.nuno@gmail.com>]

Hi Radu,

Looking good. Just some minor stuff.

On Fri, 2026-03-27 at 13:07 +0200, Radu Sabau via B4 Relay 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>
> ---
>  drivers/iio/adc/Kconfig  |   2 +
>  drivers/iio/adc/ad4691.c | 616 ++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 612 insertions(+), 6 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 f930efdb9d8c..b5a7646b46ca 100644
> --- a/drivers/iio/adc/ad4691.c
> +++ b/drivers/iio/adc/ad4691.c
> @@ -4,14 +4,18 @@
>   * Author: Radu Sabau <radu.sabau@analog.com>
>   */
>  #include <linux/bitfield.h>
> +#include <linux/bitmap.h>
>  #include <linux/bitops.h>
>  #include <linux/cleanup.h>
>  #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 +23,12 @@
>  #include <linux/units.h>
>  #include <linux/unaligned.h>
>  

...

> 
> +static void ad4691_free_scan_bufs(struct ad4691_state *st)
> +{
> +	kfree(st->scan_xfers);
> +	kfree(st->scan_tx);
> +	kfree(st->scan_rx);
> +	st->scan_xfers = NULL;
> +	st->scan_tx = NULL;
> +	st->scan_rx = NULL;

Do we need NULL. Are you actually checking for that anywhere?

> +}
> +
> +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 = bitmap_weight(indio_dev->active_scan_mask,
> +					      indio_dev->masklength);

ditto

> +	unsigned int n_xfers = n_active + 1;
> +	unsigned int k, i;
> +	int ret;
> +
> +	st->scan_xfers = kcalloc(n_xfers, sizeof(*st->scan_xfers), GFP_KERNEL);
> +	if (!st->scan_xfers)
> +		return -ENOMEM;
> +
> +	st->scan_tx = kcalloc(n_xfers, sizeof(*st->scan_tx), GFP_KERNEL);
> +	if (!st->scan_tx) {
> +		kfree(st->scan_xfers);
> +		return -ENOMEM;
> +	}
> +
> +	st->scan_rx = kcalloc(n_xfers, sizeof(*st->scan_rx), GFP_KERNEL);
> +	if (!st->scan_rx) {
> +		kfree(st->scan_tx);
> +		kfree(st->scan_xfers);
> +		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);
> +
> +	st->scan_msg.spi = spi;
> +
> +	ret = spi_optimize_message(spi, &st->scan_msg);
> +	if (ret) {
> +		ad4691_free_scan_bufs(st);
> +		return ret;
> +	}
> +
> +	ret = ad4691_enter_conversion_mode(st);
> +	if (ret) {
> +		spi_unoptimize_message(&st->scan_msg);
> +		ad4691_free_scan_bufs(st);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad4691_manual_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad4691_exit_conversion_mode(st);
> +	spi_unoptimize_message(&st->scan_msg);
> +	ad4691_free_scan_bufs(st);
> +	return ret;
> +}
> +
> +static const struct iio_buffer_setup_ops ad4691_manual_buffer_setup_ops = {
> +	.preenable = &ad4691_manual_buffer_preenable,
> +	.postdisable = &ad4691_manual_buffer_postdisable,
> +};
> +
> +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 = bitmap_weight(indio_dev->active_scan_mask,
> +					      indio_dev->masklength);

masklength is __private. Use iio_get_masklength(). I would expect sparse to
complain about the above.

> +	unsigned int bit, k, i;
> +	int ret;
> +
> +	st->scan_xfers = kcalloc(2 * n_active, sizeof(*st->scan_xfers),
> GFP_KERNEL);
> +	if (!st->scan_xfers)
> +		return -ENOMEM;
> +
> +	st->scan_tx = kcalloc(n_active, sizeof(*st->scan_tx), GFP_KERNEL);
> +	if (!st->scan_tx) {
> +		kfree(st->scan_xfers);
> +		return -ENOMEM;
> +	}
> +
> +	st->scan_rx = kcalloc(n_active, sizeof(*st->scan_rx), GFP_KERNEL);
> +	if (!st->scan_rx) {
> +		kfree(st->scan_tx);
> +		kfree(st->scan_xfers);
> +		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++;
> +	}
> +
> +	st->scan_msg.spi = spi;
> +
> +	ret = spi_optimize_message(spi, &st->scan_msg);
> +	if (ret) {
> +		ad4691_free_scan_bufs(st);
> +
Why not goto here?

Alternatively, to simplify error handling with buffers you can use some helper
variables with __free(kfree) and then in the end assign to st with no_free_ptr().

> 		return ret;
> +	}
> +
> +	ret = regmap_write(st->regmap, AD4691_ACC_MASK_REG,
> +			   (u16)~bitmap_read(indio_dev->active_scan_mask, 0,
> +					     indio_dev->masklength));
> +	if (ret)
> +		goto err;
> +
> +	ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
> +			   bitmap_read(indio_dev->active_scan_mask, 0,
> +				       indio_dev->masklength));
> +	if (ret)
> +		goto err;
> +
> +	iio_for_each_active_channel(indio_dev, bit) {
> +		ret = regmap_write(st->regmap, AD4691_ACC_DEPTH_IN(bit),
> +				   st->osr[bit]);
> +		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);
> +	ad4691_free_scan_bufs(st);
> +	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);
> +
> +	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);
> +	ad4691_free_scan_bufs(st);
> +	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);
> +
> +	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;
> +
> +	ret = kstrtoint(buf, 10, &freq);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = ad4691_set_pwm_freq(st, freq);
> +	if (ret)
> +		return ret;
> +

Question. Is it safe or does it make sense to change the above while buffering?
Same question might be valid for other stuff like oversampling.

> +	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(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct iio_trigger_ops ad4691_trigger_ops = {
> +	.validate_device = iio_trigger_validate_own_device,
> +};
> +
> +static int ad4691_read_scan(struct iio_dev *indio_dev, s64 timestamp)
> +{
> +	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)
> +		return ret;
> +
> +	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)
> +			return ret;
> +
> +		ret = ad4691_sampling_enable(st, true);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	iio_push_to_buffers_with_ts(indio_dev, &st->scan, sizeof(st->scan),
> +				    timestamp);
> +	return 0;
> +}
> +
> +static irqreturn_t ad4691_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +
> +	ad4691_read_scan(indio_dev, pf->timestamp);
> +	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,
> @@ -493,6 +982,18 @@ 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);
> +
> +	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");
> +
> +	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);
> @@ -557,8 +1058,25 @@ static int ad4691_config(struct ad4691_state *st)
>  {
>  	struct device *dev = regmap_get_device(st->regmap);
>  	enum ad4691_ref_ctrl ref_val;
> +	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;
> @@ -609,7 +1127,87 @@ static int ad4691_config(struct ad4691_state *st)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to write ADC_SETUP\n");
>  
> -	return 0;
> +	if (st->manual_mode)
> +		return 0;
> +
> +	for (gp_num = 0; gp_num < ARRAY_SIZE(ad4691_gp_names); gp_num++) {
> +		if (fwnode_irq_get_byname(dev_fwnode(dev),
> +					  ad4691_gp_names[gp_num]) > 0)

Don't love this line break. I'm also a bit puzzled. How does the above differs from
the trigger code? I guess this should be the same GP pin?


- Nuno Sá