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

[Sabau, Radu bogdan]

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
> Sent: Friday, March 13, 2026 11:13 AM
> To: Sabau, Radu bogdan <Radu.Sabau@xxxxxxxxxx>
> Cc: Lars-Peter Clausen <lars@xxxxxxxxxx>; Hennerich, Michael
> <Michael.Hennerich@xxxxxxxxxx>; Jonathan Cameron <jic23@xxxxxxxxxx>;
> David Lechner <dlechner@xxxxxxxxxxxx>; Sa, Nuno <Nuno.Sa@xxxxxxxxxx>;
> Andy Shevchenko <andy@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>;
> Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; Uwe Kleine-König <ukleinek@xxxxxxxxxx>; Liam
> Girdwood <lgirdwood@xxxxxxxxx>; Mark Brown <broonie@xxxxxxxxxx>; Linus
> Walleij <linusw@xxxxxxxxxx>; Bartosz Golaszewski <brgl@xxxxxxxxxx>; Philipp
> Zabel <p.zabel@xxxxxxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> pwm@xxxxxxxxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 3/4] iio: adc: ad4691: add triggered buffer support
> 
> [External]
> 
> On Fri, Mar 13, 2026 at 12:07:27PM +0200, Radu Sabau via B4 Relay wrote:
> 
> > Add buffered capture support using the IIO triggered buffer framework.
> >
> > Both operating modes share a single IIO trigger and trigger handler.
> > The handler builds a complete scan — one u32 slot per channel at its
> > scan_index position, followed by a timestamp — and pushes it to the
> > IIO buffer in a single iio_push_to_buffers_with_ts() call.
> >
> > For CNV Clock Mode the GP0 pin is configured as DATA_READY output. The
> > IRQ handler stops conversions and fires the IIO trigger; the trigger
> > handler reads accumulated results from the AVG_IN registers via regmap
> > and restarts conversions for the next cycle.
> >
> > For Manual Mode there is no DATA_READY signal; CNV is tied to SPI CS
> > so conversions are triggered by CS assertion rather than by a dedicated
> > pin. The standard iio-trig-hrtimer module is not used because the timer
> > period must be derived from the SPI clock rate and the number of active
> > channels: the pipelined protocol requires N+1 SPI transfers per scan
> > (the first result is garbage and is discarded), so the minimum period
> > depends on both the SPI speed and the live channel count at buffer
> > enable time. A driver-private hrtimer whose period is recomputed by
> > buffer_postenable is simpler and avoids requiring the user to configure
> > an external trigger with the correct hardware-derived period.
> >
> > Manual mode channels use storagebits=32 (shift=8, realbits=16) so all
> > channel slots in the scan buffer are uniformly sized regardless of the
> > SPI wire format (24-bit transfer, 16-bit ADC data in bits[23:8]).
> 
> ...
> 
> >  #include <linux/device.h>
> >  #include <linux/err.h>
> 
> >  #include <linux/reset.h>
> 
> This is unordered.
> 
> > +#include <linux/hrtimer.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/math.h>
> >  #include <linux/module.h>
> 
> ...
> 
> > +#include <linux/iio/buffer.h>
> >  #include <linux/iio/iio.h>
> 
> >
> 
> Unneeded blank line.
> 
> > -#define AD4691_ACC_COUNT_VAL			0x3F
> > +#define AD4691_ACC_COUNT_VAL			0x01
> 
> No ping-pong, and actually this was not used at all. So, make sure you add
> constants when they are really started being used.

Hi Andy,

First of all, thank you again for you thorough review, it does help a lot
in improving this driver.

This value is being used in the buffer_postenable in order to make
sure we don't encounter oversampling, since Manual Mode doesn't
oversample, and per Jonathan's review, there is no reason to support
both oversampled and raw readings at the same time.

> 
> >  #define AD4691_GPIO_MODE1_REG			0x196
> >  #define AD4691_GPIO_MODE2_REG			0x197
> >  #define AD4691_GPIO_READ			0x1A0
> 
> ...
> 
> >  static const struct iio_chan_spec ad4691_manual_channels[] = {
> > -	AD4691_CHANNEL(0, 0, 16, 24, 8),
> > -	AD4691_CHANNEL(1, 1, 16, 24, 8),
> > -	AD4691_CHANNEL(2, 2, 16, 24, 8),
> > -	AD4691_CHANNEL(3, 3, 16, 24, 8),
> > -	AD4691_CHANNEL(4, 4, 16, 24, 8),
> > -	AD4691_CHANNEL(5, 5, 16, 24, 8),
> > -	AD4691_CHANNEL(6, 6, 16, 24, 8),
> > -	AD4691_CHANNEL(7, 7, 16, 24, 8),
> > -	AD4691_CHANNEL(8, 8, 16, 24, 8),
> > -	AD4691_CHANNEL(9, 9, 16, 24, 8),
> > -	AD4691_CHANNEL(10, 10, 16, 24, 8),
> > -	AD4691_CHANNEL(11, 11, 16, 24, 8),
> > -	AD4691_CHANNEL(12, 12, 16, 24, 8),
> > -	AD4691_CHANNEL(13, 13, 16, 24, 8),
> > -	AD4691_CHANNEL(14, 14, 16, 24, 8),
> > -	AD4691_CHANNEL(15, 15, 16, 24, 8)
> > +	AD4691_CHANNEL(0, 0, 16, 32, 8),
> > +	AD4691_CHANNEL(1, 1, 16, 32, 8),
> > +	AD4691_CHANNEL(2, 2, 16, 32, 8),
> > +	AD4691_CHANNEL(3, 3, 16, 32, 8),
> > +	AD4691_CHANNEL(4, 4, 16, 32, 8),
> > +	AD4691_CHANNEL(5, 5, 16, 32, 8),
> > +	AD4691_CHANNEL(6, 6, 16, 32, 8),
> > +	AD4691_CHANNEL(7, 7, 16, 32, 8),
> > +	AD4691_CHANNEL(8, 8, 16, 32, 8),
> > +	AD4691_CHANNEL(9, 9, 16, 32, 8),
> > +	AD4691_CHANNEL(10, 10, 16, 32, 8),
> > +	AD4691_CHANNEL(11, 11, 16, 32, 8),
> > +	AD4691_CHANNEL(12, 12, 16, 32, 8),
> > +	AD4691_CHANNEL(13, 13, 16, 32, 8),
> > +	AD4691_CHANNEL(14, 14, 16, 32, 8),
> > +	AD4691_CHANNEL(15, 15, 16, 32, 8)
> >  };
> >
> >  static const struct iio_chan_spec ad4693_manual_channels[] = {
> > -	AD4691_CHANNEL(0, 0, 16, 24, 8),
> > -	AD4691_CHANNEL(1, 1, 16, 24, 8),
> > -	AD4691_CHANNEL(2, 2, 16, 24, 8),
> > -	AD4691_CHANNEL(3, 3, 16, 24, 8),
> > -	AD4691_CHANNEL(4, 4, 16, 24, 8),
> > -	AD4691_CHANNEL(5, 5, 16, 24, 8),
> > -	AD4691_CHANNEL(6, 6, 16, 24, 8),
> > -	AD4691_CHANNEL(7, 7, 16, 24, 8)
> > +	AD4691_CHANNEL(0, 0, 16, 32, 8),
> > +	AD4691_CHANNEL(1, 1, 16, 32, 8),
> > +	AD4691_CHANNEL(2, 2, 16, 32, 8),
> > +	AD4691_CHANNEL(3, 3, 16, 32, 8),
> > +	AD4691_CHANNEL(4, 4, 16, 32, 8),
> > +	AD4691_CHANNEL(5, 5, 16, 32, 8),
> > +	AD4691_CHANNEL(6, 6, 16, 32, 8),
> > +	AD4691_CHANNEL(7, 7, 16, 32, 8)
> >  };
> 
> Hold on, you just introduced them in the first patch. Are those wrong?!
> Please, fix this mess and make sure you have no - (minus) lines from
> the previous patch(es).
> 

The short answer is yes, the first patch's manual channel's structure is wrong
since the buffer scan slot expects 32-bits, but I guess this change should have
happened in the previous commit since the channels are first configured there.
I am sorry for this, I'll make sure to fix this in the next version alongside the
other comments.

> ...
> 
> > +	/*
> > +	 * DMA (thus cache coherency maintenance) may require the
> > +	 * transfer buffers to live in their own cache lines.
> > +	 */
> > +	unsigned char rx_data[ALIGN(3, sizeof(s64)) + sizeof(s64)]
> 	__aligned(IIO_DMA_MINALIGN);
> > +	unsigned char tx_data[ALIGN(3, sizeof(s64)) + sizeof(s64)];
> 
> Don't we have macro for this?
> 
> ...
> 
> > +static int ad4691_transfer(struct ad4691_state *st, int command,
> 
> Signed 'command'? Why?
> 
> > +			   unsigned int *val)
> 
> ...
> 
> > +static int ad4691_buffer_postenable(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);
> > +	u16 mask = ~(*indio_dev->active_scan_mask);
> 
> > +	u32 acc_mask[2] = { mask & 0xFF, mask >> 8 };
> 
> Same Q as per previous patch. This seems very wrong.
> 
> > +	int n_active = hweight_long(*indio_dev->active_scan_mask);
> 
> Why signed?
> 
> > +	unsigned int bit;
> > +	int ret;
> > +
> > +	ret = ad4691_enter_conversion_mode(st);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (st->adc_mode == AD4691_MANUAL_MODE) {
> > +		u64 min_period_ns;
> > +
> > +		/* N+1 transfers needed for N channels, with 50% overhead */
> > +		min_period_ns = div64_u64((u64)(n_active + 1) *
> AD4691_BITS_PER_XFER *
> > +					  NSEC_PER_SEC * 3,
> > +					  spi->max_speed_hz * 2);
> > +
> > +		if (ktime_to_ns(st->sampling_period) < min_period_ns) {
> > +			dev_err(dev,
> > +				"Sampling period %lld ns too short for %d
> channels. Min: %llu ns\n",
> > +				ktime_to_ns(st->sampling_period), n_active,
> > +				min_period_ns);
> > +			return -EINVAL;
> > +		}
> > +
> > +		hrtimer_start(&st->sampling_timer, st->sampling_period,
> > +			      HRTIMER_MODE_REL);
> > +		return 0;
> > +	}
> > +
> > +	/* CNV_CLOCK_MODE: configure sequencer and start PWM */
> > +	ret = regmap_write(st->regmap, AD4691_STATE_RESET_REG,
> > +			   AD4691_STATE_RESET_ALL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_bulk_write(st->regmap, AD4691_ACC_MASK1_REG,
> acc_mask, 2);
> 
> sizeof()? ARRAY_SIZE()? See comments per previous patch.
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
> > +			   *indio_dev->active_scan_mask);
> > +	if (ret)
> > +		return ret;
> > +
> > +	iio_for_each_active_channel(indio_dev, bit) {
> > +		ret = regmap_write(st->regmap,
> AD4691_ACC_COUNT_LIMIT(bit),
> > +				   AD4691_ACC_COUNT_VAL);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return ad4691_sampling_enable(st, true);
> > +}
> 
> ...
> 
> > +{
> > +	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, i;
> > +	int ret;
> > +
> > +	guard(mutex)(&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. Collect all results into scan.vals[], then push
> > +		 * the complete scan once.
> > +		 */
> > +		iio_for_each_active_channel(indio_dev, i) {
> > +			ret = ad4691_transfer(st, AD4691_ADC_CHAN(i),
> &val);
> > +			if (ret)
> > +				goto done;
> > +
> > +			if (prev_chan >= 0)
> > +				st->scan.vals[prev_chan] = prev_val;
> > +			prev_val = val;
> > +			prev_chan = i;
> > +		}
> > +
> > +		/* Final NOOP transfer to retrieve last channel's result */
> > +		ret = ad4691_transfer(st, AD4691_NOOP, &val);
> > +		if (ret)
> > +			goto done;
> 
> > +		st->scan.vals[prev_chan] = val;
> 
> How do you guarantee that prev_chan never ever be -1 here?
> 
> > +	} else {
> > +		iio_for_each_active_channel(indio_dev, i) {
> > +			ret = regmap_read(st->regmap, AD4691_AVG_IN(i),
> &val);
> > +			if (ret)
> > +				goto done;
> > +
> > +			st->scan.vals[i] = val;
> > +		}
> > +
> > +		regmap_write(st->regmap, AD4691_STATE_RESET_REG,
> AD4691_STATE_RESET_ALL);
> > +
> > +		/* Restart conversions for the next trigger cycle. */
> > +		ad4691_sampling_enable(st, true);
> > +	}
> > +
> > +	iio_push_to_buffers_with_ts(indio_dev, &st->scan, sizeof(st->scan),
> > +				    pf->timestamp);
> > +
> > +done:
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +	return IRQ_HANDLED;
> > +}
> 
> ...
> 
> >  static int ad4691_config(struct ad4691_state *st)
> >  {
> >  	struct device *dev = regmap_get_device(st->regmap);
> > +	struct spi_device *spi = to_spi_device(dev);
> >  	enum ad4691_ref_ctrl ref_val;
> >  	unsigned int reg_val;
> >  	int ret;
> 
> >  		st->adc_mode = AD4691_MANUAL_MODE;
> >  		st->sampling_period =
> ns_to_ktime(DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC,
> >  			AD4691_MANUAL_MODE_STD_FREQ(st->chip-
> >num_channels,
> > -				to_spi_device(dev)->max_speed_hz)));
> > +						    spi->max_speed_hz)));
> >  	}
> 
> Okay, this should be part of the previous patch.
> 
> >  	return regmap_write(st->regmap, AD4691_GPIO_MODE1_REG,
> AD4691_ADC_BUSY);
> >  }
> 
> ...
> 
> > +static int ad4691_setup_triggered_buffer(struct iio_dev *indio_dev,
> > +					 struct ad4691_state *st)
> > +{
> > +	struct device *dev = regmap_get_device(st->regmap);
> > +	struct spi_device *spi = to_spi_device(dev);
> > +	int irq, ret;
> > +
> > +	st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> > +					  indio_dev->name,
> > +					  iio_device_id(indio_dev));
> 
> It seems you ignored some of my comments. Please go back and read carefully
> what I commented on previous version of the series.

I am very sorry for this. I may have misunderstood the comments, and thus
seem like I ignored them, but this wasn't my intention at all.
If you refer to the trigger_alloc comment, I did talk to my senior colleagues
and they referred to the fact -ENOMEM return should have been enough,
but perhaps this was wrong too. Could you please clarify? 

Best Regards,
Radu

> 
> > +	if (!st->trig)
> > +		return -ENOMEM;
> > +
> > +	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);
> > +
> > +	if (st->adc_mode == AD4691_MANUAL_MODE) {
> > +		/*
> > +		 * No DATA_READY signal in MANUAL_MODE; CNV is tied to CS
> so
> > +		 * conversions start with each SPI transfer. Use an hrtimer to
> > +		 * schedule periodic reads.
> > +		 */
> > +		hrtimer_setup(&st->sampling_timer,
> ad4691_sampling_timer_handler,
> > +			      CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > +		st->sampling_period =
> ns_to_ktime(DIV_ROUND_CLOSEST_ULL(
> > +			NSEC_PER_SEC,
> > +			AD4691_MANUAL_MODE_STD_FREQ(st->chip-
> >num_channels,
> > +						    spi->max_speed_hz)));
> > +	} else {
> > +		/*
> > +		 * DATA_READY 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.
> > +		 */
> > +		irq = fwnode_irq_get(dev_fwnode(dev), 0);
> > +		if (irq < 0)
> > +			return dev_err_probe(dev, irq,
> > +					     "failed to get DATA_READY
> interrupt\n");
> > +
> > +		ret = devm_request_threaded_irq(dev, irq, NULL,
> > +						&ad4691_irq,
> > +						IRQF_ONESHOT,
> > +						indio_dev->name, indio_dev);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return devm_iio_triggered_buffer_setup(dev, indio_dev,
> > +					       &iio_pollfunc_store_time,
> > +					       &ad4691_trigger_handler,
> > +					       &ad4691_buffer_setup_ops);
> > +}
> 
> --
> With Best Regards,
> Andy Shevchenko
>