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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 11 May 2026 14:54:15 +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 active channel, densely paccked 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>

Sashiko pointed out you have a buffer that is big endian but
chan_spec doesn't reflect that.  That should have generated obvious
garbage output (unless you are actually testing on a be machine!)

Various other things came up, some of which I thought were in previous
reviews - but maybe I'm confusing drivers.

Thanks

Jonathan

> @@ -204,7 +230,14 @@ static const struct ad4691_chip_info ad4694_chip_info = {
>  struct ad4691_state {
>  	const struct ad4691_chip_info *info;
>  	struct regmap *regmap;
> +	struct spi_device *spi;
> +
> +	struct pwm_device *conv_trigger;
> +	int irq;
>  	int vref_uV;
> +	u32 cnv_period_ns;
> +
> +	bool manual_mode;
>  	bool refbuf_en;
>  	bool ldo_en;
>  	/*
> @@ -212,8 +245,56 @@ struct ad4691_state {
>  	 * 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;
> +	/*
> +	 * max 16 + 1 NOOP (manual) or 2*16 + 1 state-reset (CNV burst).
> +	 */
> +	struct spi_transfer scan_xfers[34];
> +	/*
> +	 * CNV burst: 16 AVG_IN addresses = 16.  Manual: 16 channel cmds +
> +	 * 1 NOOP = 17.  Stored as native u16; put_unaligned_be16() fills each
> +	 * slot so the SPI controller (bits_per_word=8) sends bytes MSB-first.
> +	 */
> +	u16 scan_tx[17] __aligned(IIO_DMA_MINALIGN);
> +	/*
> +	 * CNV burst state-reset: 4-byte write [addr_hi, addr_lo,
> +	 * STATE_RESET_ALL, OSC_EN=1]. CS is asserted throughout, so
> +	 * ADDR_DESCENDING writes byte[3]=1 to OSC_EN_REG (0x180) as a
> +	 * deliberate side-write, keeping the oscillator enabled. Shared
> +	 * with the offload path (mutually exclusive at probe).
> +	 */
> +	u8 scan_tx_reset[4] __aligned(IIO_DMA_MINALIGN);
> +	/*
> +	 * Scan buffer: one BE16 slot per active channel, plus timestamp.
> +	 * DMA-aligned because scan_xfers point rx_buf directly into vals[].
> +	 */
> +	IIO_DECLARE_DMA_BUFFER_WITH_TS(__be16, vals, 16);
As per comments elsewhere: If it is big endian why is the channel not marked
as such in the iio_chan_spec?

>  };

> +
> +static int ad4691_cnv_burst_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	unsigned int acc_mask;
> +	unsigned int k, i;
> +	int ret;
> +
> +	memset(st->scan_xfers, 0, sizeof(st->scan_xfers));
> +	memset(st->scan_tx, 0, sizeof(st->scan_tx));
> +
> +	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) {
> +		put_unaligned_be16(0x8000 | AD4691_AVG_IN(i), &st->scan_tx[k]);
> +		st->scan_xfers[2 * k].tx_buf = &st->scan_tx[k];
> +		st->scan_xfers[2 * k].len = sizeof(st->scan_tx[k]);
> +		spi_message_add_tail(&st->scan_xfers[2 * k], &st->scan_msg);
> +		st->scan_xfers[2 * k + 1].rx_buf = &st->vals[k];
> +		st->scan_xfers[2 * k + 1].len = sizeof(st->scan_tx[k]);
> +		st->scan_xfers[2 * k + 1].cs_change = 1;
> +		spi_message_add_tail(&st->scan_xfers[2 * k + 1], &st->scan_msg);
> +		k++;
> +	}
> +
> +	/*
> +	 * Append a 4-byte state-reset transfer [addr_hi, addr_lo,
> +	 * STATE_RESET_ALL, OSC_EN=1]. CS is asserted throughout, so
> +	 * ADDR_DESCENDING writes byte[3]=1 to OSC_EN_REG (0x180) as a
> +	 * deliberate side-write, keeping the oscillator enabled.
> +	 */
> +	put_unaligned_be16(AD4691_STATE_RESET_REG, st->scan_tx_reset);
> +	st->scan_tx_reset[2] = AD4691_STATE_RESET_ALL;
> +	st->scan_tx_reset[3] = 1;
> +	st->scan_xfers[2 * k].tx_buf = st->scan_tx_reset;
> +	st->scan_xfers[2 * k].len = sizeof(st->scan_tx_reset);
> +	st->scan_xfers[2 * k].cs_change = 1;

Our old friend - cs_change = 1 is very rarely the right thing to do on a
final message.  I thought this came up in an earlier version.

> +	spi_message_add_tail(&st->scan_xfers[2 * k], &st->scan_msg);
> +
> +	ret = spi_optimize_message(st->spi, &st->scan_msg);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
> +			   bitmap_read(indio_dev->active_scan_mask, 0,
> +				       iio_get_masklength(indio_dev)));
> +	if (ret)
> +		goto err_unoptimize;
> +
> +	acc_mask = ~bitmap_read(indio_dev->active_scan_mask, 0,
> +				iio_get_masklength(indio_dev)) & GENMASK(15, 0);
> +	ret = regmap_write(st->regmap, AD4691_ACC_MASK_REG, acc_mask);
> +	if (ret)
> +		goto err_unoptimize;
> +
> +	ret = ad4691_enter_conversion_mode(st);
> +	if (ret)
> +		goto err_unoptimize;
> +
> +	return 0;
> +
> +err_unoptimize:
> +	spi_unoptimize_message(&st->scan_msg);
> +	return ret;
> +}


> +
> +static const struct iio_trigger_ops ad4691_trigger_ops = {
> +	.validate_device = iio_trigger_validate_own_device,
Same thing about manual mode as mentioned below.

> +};

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

Maybe it was a different driver but I thought I commented on this before.
There are a bunch of races if you reenable this here - needs to be
in the trigger reenable callback.
(Sashiko is pointing this out as well with more detail on what those
races are)  The short story is that you can race and have a trigger between
the enable and the notify_done which will be dropped on the floor meaning
we never get in here again - IIRC there is (rather convoluted) code to handle that
corner case in via the reenable callback and a work item.

> +	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,
>  	.read_avail = &ad4691_read_avail,
>  	.debugfs_reg_access = &ad4691_reg_access,
> +	.validate_trigger = iio_validate_own_trigger,

Sashiko noted that this seems odd. If we only support our own
trigger how does manual mode work given it isn't registering one?

You will need to do something more clever to handle that.
Either pick between info structures or use custom validation callbacks.

>  };

...

> @@ -651,6 +1119,75 @@ static int ad4691_config(struct ad4691_state *st)
>  	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);

Sashiko correctly points out the general bug we have right now about leaking
trigger context on any error.
Need to solve that for all drivers though so don't worry about it here.
It did give more complex reasoning than normal however!

> +
> +	if (st->manual_mode)
> +		return devm_iio_triggered_buffer_setup(dev, indio_dev,
> +						       &iio_pollfunc_store_time,
> +						       &ad4691_trigger_handler,
> +						       &ad4691_manual_buffer_setup_ops);
> +
> +	/*
> +	 * 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 = -ENXIO;
> +	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 || irq == -EPROBE_DEFER)
> +			break;
> +	}
> +	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);
> +}
> +
>  static int ad4691_probe(struct spi_device *spi)
>  {
>  	struct device *dev = &spi->dev;
> @@ -663,6 +1200,7 @@ static int ad4691_probe(struct spi_device *spi)
>  		return -ENOMEM;
>  
>  	st = iio_priv(indio_dev);
> +	st->spi = spi;
>  	st->info = spi_get_device_match_data(spi);
>  	if (!st->info)
>  		return -ENODEV;
> @@ -692,8 +1230,9 @@ static int ad4691_probe(struct spi_device *spi)
>  	indio_dev->info = &ad4691_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> -	indio_dev->channels = st->info->sw_info->channels;
> -	indio_dev->num_channels = st->info->sw_info->num_channels;

You've lost me here. Where are these now set?

> +	ret = ad4691_setup_triggered_buffer(indio_dev, st);
> +	if (ret)
> +		return ret;
>  
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>