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

[Andy Shevchenko]

On Sat, Apr 04, 2026 at 10:12:04AM -0500, David Lechner wrote:
> On 4/3/26 6:03 AM, Radu Sabau via B4 Relay wrote:

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

Tried my best to avoid clashes with David's review.

...

> >  #include <linux/array_size.h>
> >  #include <linux/bitfield.h>

> > +#include <linux/bitmap.h>
> >  #include <linux/bitops.h>

When bitmap.h is present, it implies bitops.h, hence the latter can be simply
replaced.

> >  #include <linux/cleanup.h>
> >  #include <linux/delay.h>
> >  #include <linux/dev_printk.h>
> >  #include <linux/device/devres.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>

...

> >  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)		\
> > -				    | BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> > +				    | BIT(IIO_CHAN_INFO_SAMP_FREQ)	\
> > +				    | BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
> >  		.info_mask_separate_available =				\
> > -				      BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> > +				      BIT(IIO_CHAN_INFO_SAMP_FREQ)	\
> > +				    | BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\

You may reduce churn by squeezing a new ones in between existing ones.
Also consider use usual patter of placing the operator on the same line where
left operand is (currently it goes with the right operand).

...

> >  struct ad4691_state {

Just to double check, when add a new field or fields into the data structure
check with `pahole` that the new members placed at the best or at least good
enough locations.

> >  	const struct ad4691_chip_info *info;
> >  	struct regmap *regmap;
> > +
> > +	struct pwm_device *conv_trigger;
> > +	int irq;
> > +
> > +	bool manual_mode;
> > +
> >  	int vref_uV;
> > +	u8 osr[16];
> >  	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-enable lifetime resources:
> > +	 * 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.
> > +	 */
> > +	struct spi_message scan_msg;
> > +	struct spi_transfer *scan_xfers;
> > +	__be16 *scan_tx;
> > +	__be16 *scan_rx;
> 
> Why not embed these arrays here? Then we don't have to deal with
> alloc/free later.
> 
> > +	/* Scan buffer: one slot per channel plus timestamp */
> > +	struct {
> > +		u16 vals[16];
> > +		aligned_s64 ts;
> > +	} scan __aligned(IIO_DMA_MINALIGN);
> 
> Better would be IIO_DECLARE_BUFFER_WITH_TS() since we don't always
> use all vals.
> 
> Also, current usage doesn't need to be DMA-safe because scan_tx
> is being used for the actual SPI xfer.
> 
> >  };

...

> > +static int ad4691_gpio_setup(struct ad4691_state *st, unsigned int gp_num)
> > +{
> > +	unsigned int shift = 4 * (gp_num % 2);
> > +
> > +	return regmap_update_bits(st->regmap,
> > +				  AD4691_GPIO_MODE1_REG + gp_num / 2,
> > +				  AD4691_GP_MODE_MASK << shift,
> > +				  AD4691_GP_MODE_DATA_READY << shift);

Not sure if compiler will see % and / together, I would go with two more
temporary variables to make it clear to it:

	... _bit_off = % 2;
	... _reg_off = / 2;

The practical example is described, for example, here:
9b3cd5c7099f ("regmap: place foo / 8 and foo % 8 closer to each other").

> > +}

...

> > +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,
> > +					      iio_get_masklength(indio_dev));

In such cases please split definition and assignment. Will take two lines, but
readability will be better.

> > +	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);
> 
> Usually, we make st->scan_xfers a fixed array with the max number of possible
> xfers. Then we don't have to deal with alloc/free.

And please if it's still be required to allocate and possible use kzalloc_objs().

> > +	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;
> 
> This isn't how the SPI framework is intended to be used. We should
> have st->spi = spi in probe instead.
> 
> > +
> > +	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_cnv_burst_buffer_preenable(struct iio_dev *indio_dev)

As per above.

...

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

Why casting?

> > +}

...

> > +static IIO_DEVICE_ATTR(sampling_frequency, 0644,
> > +		       sampling_frequency_show,
> > +		       sampling_frequency_store, 0);

IIO_DEVICE_ATTR_RW().

...

> > +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++;
> > +		}
> 
> I suppose this is fine, but we usually try to avoid extra copiying and
> byte swapping of bufferes like this if we can. It seems completly doable
> in both modes. Manual mode will just one extra two-byte buffer for the
> throw-away conversion on the first read xfer (or just write to the same
> element twice).

And in case it's still needed, we may introduce a helper in
include/linux/byteorder/generic.h calling it memcpy_to/from_be16().

> > +		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 int ad4691_setup_triggered_buffer(struct iio_dev *indio_dev,
> > +					 struct ad4691_state *st)
> > +{
> > +	struct device *dev = regmap_get_device(st->regmap);
> > +	struct iio_trigger *trig;
> > +	unsigned int i;
> > +	int irq, ret;
> > +
> > +	trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> > +				      indio_dev->name,
> > +				      iio_device_id(indio_dev));
> > +	if (!trig)
> > +		return -ENOMEM;
> > +
> > +	trig->ops = &ad4691_trigger_ops;
> > +	iio_trigger_set_drvdata(trig, st);
> > +
> > +	ret = devm_iio_trigger_register(dev, trig);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "IIO trigger register failed\n");
> > +
> > +	indio_dev->trig = iio_trigger_get(trig);
> > +
> > +	if (!st->manual_mode) {
> 
> I would invert the if since the other case is shorter.

+1.

> > +		/*
> > +		 * The GP pin named in interrupt-names 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 = -ENODEV;
> > +		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;
> > +		}
> > +		if (irq <= 0)
> > +			return dev_err_probe(dev, irq < 0 ? irq : -ENODEV,
> > +					     "failed to get GP interrupt\n");
> 
> Usually we would usually just use spi->irq since it already
> has been looked up. But I guess it is OK to do it like this.

No, it's not. (Linux) IRQ shouldn't ever be 0, so this check is effectively a
dead code.

		irq = -ENXIO; // Note, this is the error code used by core for
			      // IRQ not found.
		...
		if (irq < 0)
			return dev_err_probe(dev, irq, "failed to get GP interrupt\n");

> > +		st->irq = irq;
> > +
> > +		ret = ad4691_gpio_setup(st, i);
> > +		if (ret)
> > +			return ret;
> > +
> > +		/*
> > +		 * IRQ is kept disabled until the buffer is enabled to prevent
> > +		 * spurious DATA_READY events before the SPI message is set up.
> > +		 */
> > +		ret = devm_request_threaded_irq(dev, irq, NULL,
> > +						&ad4691_irq,
> > +						IRQF_ONESHOT | IRQF_NO_AUTOEN,
> > +						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);
> > +}
> > +

-- 
With Best Regards,
Andy Shevchenko