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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 18 May 2026 09:36:18 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 5/18/26 9:21 AM, Jonathan Cameron wrote:
> > 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.  
> 
> I guess you didn't see the other series cleaning up IIO_TIMESTAMP I already
> sent yet.
> 
:( That's what I get for not reading all my email before starting to reply!


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