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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 19 May 2026 15:20:24 +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 big-endian 16-bit (__be16) slot per active channel, densely packed
> in scan_index order, followed by a timestamp.
> 
> 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>
I didn't spot anything in my read through and most of what Sashiko
is commenting on is wrong or not a driver specific problem
(the trigger leak is a core problem)

One small thing Sashiko did pick up on though that I think could be a little nicer.
You may well already have this in hand as I know you are checking there
as well!

> +static void ad4691_read_scan(struct iio_dev *indio_dev, s64 ts)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +
> +	guard(mutex)(&st->lock);
> +
> +	spi_sync(st->spi, &st->scan_msg);

One thing from sashiko. If this fails we shouldn't push data and we should
make it clear somewhere (rate limited print or similar).

> +
> +	/*
> +	 * rx_buf pointers in scan_xfers point directly into scan.vals, so no
> +	 * copy is needed. The scan_msg already includes a STATE_RESET at the
> +	 * end (appended in preenable), so no explicit reset is needed here.
> +	 */
> +	iio_push_to_buffers_with_ts(indio_dev, st->vals, sizeof(st->vals), ts);
> +}