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

[David Lechner]

On 5/18/26 9:21 AM, Jonathan Cameron wrote:
> On Sun, 17 May 2026 14:21:30 -0500
> David Lechner <dlechner@xxxxxxxxxxxx> wrote:
> 
>> On 5/17/26 7:25 AM, Jonathan Cameron wrote:
>>> On Sat, 16 May 2026 12:32:51 -0500
>>> David Lechner <dlechner@xxxxxxxxxxxx> wrote:
>>>   
>>>> On 5/15/26 8:31 AM, Radu Sabau via B4 Relay wrote:  
>>>>> From: Radu Sabau <radu.sabau@xxxxxxxxxx>
>>>>>
>>>>> 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@xxxxxxxxxx>  
>>>   
>>>>> +
>>>>> +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.

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