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

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 17 May 2026 14:21:30 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 5/17/26 7:25 AM, Jonathan Cameron wrote:
> > On Sat, 16 May 2026 12:32:51 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> >> On 5/15/26 8:31 AM, Radu Sabau via B4 Relay 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 big-endian 16-bit (__be16) slot per active channel, densely packed
> >>> 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>  
> >   
> >>> +
> >>> +static int ad4691_manual_buffer_preenable(struct iio_dev *indio_dev)
> >>> +{
> >>> +	struct ad4691_state *st = iio_priv(indio_dev);
> >>> +	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);
> >>> +
> >>> +	k = 0;
> >>> +	iio_for_each_active_channel(indio_dev, i) {
> >>> +		if (i >= indio_dev->num_channels - 1)
> >>> +			break; /* skip soft timestamp */    
> >>
> >> I don't think timestamp gets set in the scan mask. It is handled separately.  
> > 
> > FWIW that is a sashiko false postive (I believe anyway!)
> > If we do hit this please shout as we have a core bug.
> > 
> > If anyone has time to look at how hard it would be to tweak
> > iio_for_each_active_channel to skip a last element timestamp that
> > would be great.
> > 
> > I think that iterates one too far which is what sashiko is tripping over.
> > 
> > I'm only keen to fix that if we can make it low cost and hid it entirely
> > from drivers.
> > 
> > Jonathan
> >   
> This is what I came up with (totally untested).
> 
> Since timestamp can never be set in scan_mask/active_scan_mask, it should
> be safe to exclude it from masklength without breaking existing code.
Probably... 
> 
> I didn't check all callers of masklength/iio_get_masklength() though.

That was the bit that made me nervous. Particularly if there is an off
by one that is working by luck today - or someone who understood this
oddity and did it deliberately.

At one point we also had a few other timestamps - the ones come from hardware.
I can't remember how we handled those wrt to the scan mask.  I took a quick
look and thing they are all fine. 
FWIW a nice precursor would be to make sure all timestamp channels are assigned
using the macro. There are a few that are hand crafted.  I tested a few, but obviously
needs turning in to a proper set and cleaning up.

diff --git a/drivers/iio/adc/ad4170-4.c b/drivers/iio/adc/ad4170-4.c
index 627cbf5a37b0..890e25294baa 100644
--- a/drivers/iio/adc/ad4170-4.c
+++ b/drivers/iio/adc/ad4170-4.c
@@ -2385,9 +2385,7 @@ static int ad4170_parse_channels(struct iio_dev *indio_dev)
 	}
 
 	/* Add timestamp channel */
-	struct iio_chan_spec ts_chan = IIO_CHAN_SOFT_TIMESTAMP(chan_num);
-
-	st->chans[chan_num] = ts_chan;
+	st->chans[chan_num] = IIO_CHAN_SOFT_TIMESTAMP(chan_num);
 	num_channels = num_channels + 1;
 
 	indio_dev->num_channels = num_channels;
diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index 6e1930f7c65d..56baca1f5026 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -521,13 +521,7 @@ static int at91_adc_channel_init(struct iio_dev *idev)
 	}
 	timestamp = chan_array + idx;
 
-	timestamp->type = IIO_TIMESTAMP;
-	timestamp->channel = -1;
-	timestamp->scan_index = idx;
-	timestamp->scan_type.sign = 's';
-	timestamp->scan_type.realbits = 64;
-	timestamp->scan_type.storagebits = 64;
-
+	*timestamp = IIO_CHAN_SOFT_TIMESTAMP(idx);
 	idev->channels = chan_array;
 	return idev->num_channels;
 }
diff --git a/drivers/iio/adc/cc10001_adc.c b/drivers/iio/adc/cc10001_adc.c
index 2c51b90b7101..d42b747325aa 100644
--- a/drivers/iio/adc/cc10001_adc.c
+++ b/drivers/iio/adc/cc10001_adc.c
@@ -262,7 +262,7 @@ static const struct iio_info cc10001_adc_info = {
 static int cc10001_adc_channel_init(struct iio_dev *indio_dev,
 				    unsigned long channel_map)
 {
-	struct iio_chan_spec *chan_array, *timestamp;
+	struct iio_chan_spec *chan_array;
 	unsigned int bit, idx = 0;
 
 	indio_dev->num_channels = bitmap_weight(&channel_map,
@@ -289,13 +289,7 @@ static int cc10001_adc_channel_init(struct iio_dev *indio_dev,
 		idx++;
 	}
 
-	timestamp = &chan_array[idx];
-	timestamp->type = IIO_TIMESTAMP;
-	timestamp->channel = -1;
-	timestamp->scan_index = idx;
-	timestamp->scan_type.sign = 's';
-	timestamp->scan_type.realbits = 64;
-	timestamp->scan_type.storagebits = 64;
+	chan_array[idx] = IIO_CHAN_SOFT_TIMESTAMP(idx);
 
 	indio_dev->channels = chan_array;
 
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 96b05c86c325..702b2fc66326 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -353,7 +353,7 @@ static inline bool iio_channel_has_available(const struct iio_chan_spec *chan,
 		(chan->info_mask_shared_by_all_available & BIT(type));
 }
 
-#define IIO_CHAN_SOFT_TIMESTAMP(_si) {					\
+#define IIO_CHAN_SOFT_TIMESTAMP(_si) (struct iio_chan_spec) {		\
 	.type = IIO_TIMESTAMP,						\
 	.channel = -1,							\
 	.scan_index = _si,						\

Doing that will mean we can spot any unusual use of IIO_TIMESTAMP much more
easily.

Anyhow, basic approach looks good to me.

Jonathan



> 
> ---
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 9d66510a1d49..17f539fc23e2 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -2300,8 +2300,10 @@ int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  	if (channels) {
>  		int ml = 0;
>  
> -		for (i = 0; i < indio_dev->num_channels; i++)
> -			ml = max(ml, channels[i].scan_index + 1);
> +		for (i = 0; i < indio_dev->num_channels; i++) {
> +			if (channels[i].type != IIO_TIMESTAMP)
> +				ml = max(ml, channels[i].scan_index + 1);
> +		}
>  		ACCESS_PRIVATE(indio_dev, masklength) = ml;
>  	}
>  
> 
> 
>