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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 30 Apr 2026 13:16:45 +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 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>
Hi Radu

I haven't chased it through but Sashiko is raising concerns around the
irq enabling / disable tricks this pulling and they may well be correct.
Please take a close look at what happens in the remove path. We may
need some local driver logic to avoid a double disable.

https://sashiko.dev/#/patchset/20260430-ad4692-multichannel-sar-adc-driver-v9-0-33e439e4fb87%40analog.com

> diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> index 05826b762c7f..c1e3406fef18 100644
> --- a/drivers/iio/adc/ad4691.c
> +++ b/drivers/iio/adc/ad4691.c


> +
> +static irqreturn_t ad4691_irq(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +
> +	iio_trigger_poll(indio_dev->trig);
> +	/*
> +	 * Keep the DATA_READY IRQ disabled until the trigger handler has
> +	 * finished reading the scan, to prevent a new assertion mid-transfer.
> +	 * The PWM continues running uninterrupted; the IRQ is re-enabled in
> +	 * ad4691_trigger_handler once spi_sync completes.
> +	 *
> +	 * IRQF_ONESHOT already masks the hardware line during this threaded
> +	 * handler, so disable_irq_nosync here ensures the IRQ stays disabled
> +	 * even after IRQF_ONESHOT unmasks on return.
> +	 */
> +	disable_irq_nosync(st->irq);
> +
> +	return IRQ_HANDLED;
> +}

> +
> +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);
> +
> +	ad4691_read_scan(indio_dev, pf->timestamp);
> +	if (!st->manual_mode)
> +		enable_irq(st->irq);

I think this should be in the reenable_trigger callback rather than here.
That's ultimately fired by iio_trigger_notify_done.

Sashiko had quite a bit to say about the problem paths around the buffer
being disabled between the interrupt and the trigger handler. I don't think
using the reenable trigger solves that though :(  We might be able
to fix that centrally by always reenabling the trigger before
disconnecting it but that's rather ugly. There is a note for the async
trigger reenabling about a driver possibly needing to be careful
to not reenable if the whole trigger was disabled in the meantime but
this particular race isn't covered.


Terrible though it is we may need to have some extra infrastructure
to avoid the double disable (assuming it is real).


> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}