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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 05 Mar 2026 14:23:29 +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.
> 
> For CNV Clock, CNV Burst, Autonomous and SPI Burst modes the GP0 pin
> is configured as DATA_READY output and an interrupt is registered on
> its falling edge. Each interrupt fires the trigger, which reads
> accumulated results from the internal accumulator registers via regmap.
> 
> For Manual Mode (pipelined SPI protocol) there is no DATA_READY
> signal, so an hrtimer-based IIO trigger is used instead. The timer
> period is derived from the requested sampling frequency and validated
> against the minimum SPI transfer time for the active channel count.

This needs an explanation of why you can't just use a normal hrtimer trigger.
Those always run the risk of being set too fast, but we normally don't care
about that corner case. We don't want to have the equivalent code in every
driver.

> The trigger handler walks the active scan mask issuing one 3-byte SPI
> transfer per channel (selecting the next channel while reading the
> previous result) and pushes samples to the IIO buffer.
> 
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>

A few other comments inline.

> diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> index dee8bc312d44..ab48f336e46c 100644
> --- a/drivers/iio/adc/ad4691.c
> +++ b/drivers/iio/adc/ad4691.c

>  
> @@ -287,6 +294,8 @@ struct ad4691_state {
>  
>  	struct regulator_bulk_data	regulators[AD4691_NUM_REGULATORS];
>  
> +	struct iio_trigger		*trig;
> +
>  	enum ad4691_adc_mode		adc_mode;
>  
>  	int				vref;
> @@ -297,6 +306,8 @@ struct ad4691_state {
>  	 */
>  	struct mutex			lock;
>  
> +	/* hrtimer for MANUAL_MODE triggered buffer (non-offload) */
> +	struct hrtimer			sampling_timer;
>  	ktime_t				sampling_period;
>  
>  	/* DMA (thus cache coherency maintenance) may require the
> @@ -306,6 +317,11 @@ struct ad4691_state {
>  	 */
>  	unsigned char rx_data[ALIGN(3, sizeof(s64)) + sizeof(s64)]	__aligned(IIO_DMA_MINALIGN);
>  	unsigned char tx_data[ALIGN(3, sizeof(s64)) + sizeof(s64)]	__aligned(IIO_DMA_MINALIGN);
> +	/* Scan buffer for triggered buffer push (one sample + timestamp) */
> +	struct {
> +		u32 val;
> +		s64 ts __aligned(8);
> +	} scan __aligned(IIO_DMA_MINALIGN); 

Same question as earlier on whether we need more padding from aligning yet again.
Maybe we do for this one, I haven't checked.


>  };


> +
> +static irqreturn_t ad4691_irq(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +
> +	/*
> +	 * DATA_READY 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.
> +	 *
> +	 * In direct (read_raw) mode the buffer is not enabled; read_raw uses
> +	 * a timed delay and stops conversions itself, so skip the trigger poll.
> +	 */
> +	ad4691_sampling_enable(st, false);
> +
> +	if (iio_buffer_enabled(indio_dev))

How did we get here otherwise?

> +		iio_trigger_poll(st->trig);
> +
> +	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);
> +	unsigned int val;
> +	int ret, i;
> +
> +	mutex_lock(&st->lock);
> +
> +	if (st->adc_mode == AD4691_MANUAL_MODE) {
> +		unsigned int prev_val;
> +		int prev_chan = -1;
> +
> +		/*
> +		 * MANUAL_MODE with CNV tied to CS: each transfer triggers a
> +		 * conversion AND returns the previous conversion's result.
> +		 * First transfer returns garbage, so we do N+1 transfers for
> +		 * N channels.
> +		 */
> +		iio_for_each_active_channel(indio_dev, i) {
> +			ret = ad4691_transfer(st, AD4691_ADC_CHAN(i), &val);
> +			if (ret)
> +				goto done;
> +
> +			/* Push previous channel's data (skip first - garbage) */
> +			if (prev_chan >= 0) {
> +				st->scan.val = prev_val;
> +				iio_push_to_buffers_with_ts(indio_dev,
> +					&st->scan, sizeof(st->scan),
> +					iio_get_time_ns(indio_dev));

We don't push one channel at a time... You need to build up a scan locally
and push it in one go.

> +			}
> +			prev_val = val;
> +			prev_chan = i;
> +		}
> +
> +		/* Final NOOP transfer to get last channel's data */
> +		ret = ad4691_transfer(st, AD4691_NOOP, &val);
> +		if (ret)
> +			goto done;
> +
> +		st->scan.val = val;
> +		iio_push_to_buffers_with_ts(indio_dev, &st->scan, sizeof(st->scan),
> +					    iio_get_time_ns(indio_dev));
> +		goto done;
> +	}
> +
> +	for (i = 0; i < st->chip->num_channels; i++) {
> +		if (BIT(i) & *indio_dev->active_scan_mask) {
> +			ret = regmap_read(st->regmap, AD4691_AVG_IN(i), &val);
> +			if (ret)
> +				goto done;
> +
> +			st->scan.val = val;
> +			iio_push_to_buffers_with_ts(indio_dev, &st->scan, sizeof(st->scan),
> +						    iio_get_time_ns(indio_dev));
As above.

> +		}
> +	}
> +
> +	regmap_write(st->regmap, AD4691_STATE_RESET_REG, AD4691_STATE_RESET_ALL);
> +
> +	/* START next conversion. */
> +	switch (st->adc_mode) {
> +	case AD4691_CNV_CLOCK_MODE:
> +	case AD4691_CNV_BURST_MODE:
> +	case AD4691_AUTONOMOUS_MODE:
> +	case AD4691_SPI_BURST_MODE:
> +		ad4691_sampling_enable(st, true);
> +		break;
> +	case AD4691_MANUAL_MODE:
> +	default:
> +		break;
> +	}
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +	mutex_unlock(&st->lock);

Why hold the lock over the notify done?  Having moved it up you can share the done label
code block and this one.

> +	return IRQ_HANDLED;
> +done:
> +	mutex_unlock(&st->lock);
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
> +
>  static const struct iio_info ad4691_info = {
>  	.read_raw = &ad4691_read_raw,
>  	.read_avail = &ad4691_read_avail,
> @@ -1121,6 +1364,75 @@ static void ad4691_setup_channels(struct iio_dev *indio_dev,
>  	indio_dev->num_channels = st->chip->num_channels;
>  }
>  
> +static int ad4691_setup_triggered_buffer(struct iio_dev *indio_dev,
> +					 struct ad4691_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	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 dev_err_probe(dev, -ENOMEM,
> +				     "Failed to allocate IIO trigger\n");
> +
> +	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);
> +
> +	switch (st->adc_mode) {
> +	case AD4691_CNV_CLOCK_MODE:
> +	case AD4691_CNV_BURST_MODE:
> +	case AD4691_AUTONOMOUS_MODE:
> +	case AD4691_SPI_BURST_MODE:
> +		/*
> +		 * DATA_READY asserts at end-of-conversion (or when the
> +		 * accumulator fills in AUTONOMOUS_MODE). The IRQ handler stops
> +		 * conversions and fires the IIO trigger so the trigger handler
> +		 * can read and push the sample to the buffer.
> +		 */
> +		irq = fwnode_irq_get_byname(dev_fwnode(dev), "DRDY");
> +		if (irq <= 0)
> +			return dev_err_probe(dev, irq ? irq : -ENOENT,
> +					     "failed to get DRDY interrupt\n");
> +
> +		ret = devm_request_threaded_irq(dev, irq, NULL,
> +						&ad4691_irq,
> +						IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
Interrupt direction is a question for firmware. A driver should
not be setting it. We have some historical bugs in this area that we can't
fix because board maybe relying on the driver overriding but we don't want to
introduce more of them!
> +						indio_dev->name, indio_dev);