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

-----Original Message-----
From: Andy Shevchenko <redacted>
Sent: Friday, March 13, 2026 11:13 AM
To: Sabau, Radu bogdan <Radu.Sabau@analog.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael
[off-list ref]; Jonathan Cameron [off-list ref];
David Lechner [off-list ref]; Sa, Nuno [off-list ref];
Andy Shevchenko [off-list ref]; Rob Herring [off-list ref];
Krzysztof Kozlowski [off-list ref]; Conor Dooley
[off-list ref]; Uwe Kleine-König [off-list ref]; Liam
Girdwood [off-list ref]; Mark Brown [off-list ref]; Linus
Walleij [off-list ref]; Bartosz Golaszewski [off-list ref]; Philipp
Zabel [off-list ref]; linux-iio@vger.kernel.org;
devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
pwm@vger.kernel.org; linux-gpio@vger.kernel.org
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:

quoted

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]).

...

quoted

 #include <linux/device.h>
 #include <linux/err.h>

quoted

 #include <linux/reset.h>

This is unordered.

quoted

+#include <linux/hrtimer.h>
 #include <linux/interrupt.h>
 #include <linux/math.h>
 #include <linux/module.h>

...

quoted

+#include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>

quoted

Unneeded blank line.

quoted

-#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.

quoted

 #define AD4691_GPIO_MODE1_REG			0x196
 #define AD4691_GPIO_MODE2_REG			0x197
 #define AD4691_GPIO_READ			0x1A0

...

quoted

 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.

...

quoted

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

quoted

+	unsigned char tx_data[ALIGN(3, sizeof(s64)) + sizeof(s64)];

Don't we have macro for this?

...

quoted

+static int ad4691_transfer(struct ad4691_state *st, int command,

Signed 'command'? Why?

quoted

+			   unsigned int *val)

...

quoted

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

quoted

+	u32 acc_mask[2] = { mask & 0xFF, mask >> 8 };

Same Q as per previous patch. This seems very wrong.

quoted

+	int n_active = hweight_long(*indio_dev->active_scan_mask);

Why signed?

quoted

+	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 *

quoted

+					  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",

quoted

+				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.

quoted

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

quoted

+				   AD4691_ACC_COUNT_VAL);
+		if (ret)
+			return ret;
+	}
+
+	return ad4691_sampling_enable(st, true);
+}

...

quoted

+{
+	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

quoted

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

quoted

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

quoted

+		st->scan.vals[prev_chan] = val;

How do you guarantee that prev_chan never ever be -1 here?

quoted

+	} else {
+		iio_for_each_active_channel(indio_dev, i) {
+			ret = regmap_read(st->regmap, AD4691_AVG_IN(i),

&val);

quoted

+			if (ret)
+				goto done;
+
+			st->scan.vals[i] = val;
+		}
+
+		regmap_write(st->regmap, AD4691_STATE_RESET_REG,

AD4691_STATE_RESET_ALL);

quoted

+
+		/* 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;
+}

...

quoted

 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;

quoted

 		st->adc_mode = AD4691_MANUAL_MODE;
 		st->sampling_period =

ns_to_ktime(DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC,

quoted

 			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.

quoted

 	return regmap_write(st->regmap, AD4691_GPIO_MODE1_REG,

AD4691_ADC_BUSY);

quoted

 }

...

quoted

+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

quoted

+	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

quoted

+		 * conversions start with each SPI transfer. Use an hrtimer to
+		 * schedule periodic reads.
+		 */
+		hrtimer_setup(&st->sampling_timer,

ad4691_sampling_timer_handler,

quoted

+			      CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+		st->sampling_period =

ns_to_ktime(DIV_ROUND_CLOSEST_ULL(

quoted

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

quoted

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