[PATCH] iio: adc: ad7766: Update to use iio_push_to_buffers_with_ts()

[PATCH] iio: adc: ad7766: Update to use iio_push_to_buffers_with_ts()

[Ethan Tidmore]

The old ABI function iio_push_to_buffers_with_timestamp() is no longer
preferred due to it being inherently unsafe.

Update to the current standard iio_push_to_buffers_with_ts().

Signed-off-by: Ethan Tidmore <ethantidmore06@gmail.com>
---
I asked a while back regarding possible things that would suffice for a
GSoC proposal, that has came and went, but I remember Jonathan
mentioning wanting this to be done. If this is correct, would a series
be preferred next time?

 drivers/iio/adc/ad7766.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad7766.c b/drivers/iio/adc/ad7766.c
index 9e4a66477d2d..4f256ce522c2 100644
--- a/drivers/iio/adc/ad7766.c
+++ b/drivers/iio/adc/ad7766.c
@@ -74,8 +74,8 @@ static irqreturn_t ad7766_trigger_handler(int irq, void *p)
 	if (ret < 0)
 		goto done;
 
-	iio_push_to_buffers_with_timestamp(indio_dev, ad7766->data,
-		pf->timestamp);
+	iio_push_to_buffers_with_ts(indio_dev, ad7766->data, sizeof(ad7766->data),
+				    pf->timestamp);
 done:
 	iio_trigger_notify_done(indio_dev->trig);
 
-- 
2.54.0

Re: [PATCH] iio: adc: ad7766: Update to use iio_push_to_buffers_with_ts()

[Andy Shevchenko]

On Sun, May 10, 2026 at 05:14:04PM -0500, Ethan Tidmore wrote:
> The old ABI function iio_push_to_buffers_with_timestamp() is no longer
> preferred due to it being inherently unsafe.
> 
> Update to the current standard iio_push_to_buffers_with_ts().

Nice, but there is nothing about correctness of the change. This is quite
sensitive area which is ABI. Any breakage is a big deal. Have you studied
the case deeper? What are the contents of the buffers with the old one and
new one versus channels enabled? Is timestamp located in the same offset
in both cases?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: ad7766: Update to use iio_push_to_buffers_with_ts()

[Jonathan Cameron]

On Mon, 11 May 2026 13:40:26 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Sun, May 10, 2026 at 05:14:04PM -0500, Ethan Tidmore wrote:
> > The old ABI function iio_push_to_buffers_with_timestamp() is no longer
> > preferred due to it being inherently unsafe.
> > 
> > Update to the current standard iio_push_to_buffers_with_ts().  
> 
> Nice, but there is nothing about correctness of the change. This is quite
> sensitive area which is ABI. Any breakage is a big deal. Have you studied
> the case deeper? What are the contents of the buffers with the old one and
> new one versus channels enabled? Is timestamp located in the same offset
> in both cases?
> 
Lazy hint on this - check the implementations. One's implemented using the
other making these questions straight forward.

More interesting to me would be a statement of why that size is correct one
to pass. That is the most common source of bugs around these changes rather
than data placement issues (good to understand though!)  Note that we have
broken data placement by 'cleaning' up the functions themselves in the past
but not just switching from one to the other.

So patch is right but a brief statement that struct ad7766.data is an
embedded array and so we can just take it's size would be nice to have in
the patch description.

Thanks!

Jonathan

Re: [PATCH] iio: adc: ad7766: Update to use iio_push_to_buffers_with_ts()

[David Lechner]

On 5/10/26 5:14 PM, Ethan Tidmore wrote:
> The old ABI function iio_push_to_buffers_with_timestamp() is no longer
> preferred due to it being inherently unsafe.
> 
> Update to the current standard iio_push_to_buffers_with_ts().
> 
> Signed-off-by: Ethan Tidmore <ethantidmore06@gmail.com>
> ---
> I asked a while back regarding possible things that would suffice for a
> GSoC proposal, that has came and went, but I remember Jonathan
> mentioning wanting this to be done. If this is correct, would a series
> be preferred next time?

When I look at these, I always look at how the buffer is being defined
and used elsewhere to see if there are any bugs or room for improvement.

In this case, we could consider changing:

	unsigned char data[ALIGN(3, sizeof(s64)) + sizeof(s64)]	__aligned(IIO_DMA_MINALIGN);

to something a bit easier to understand.

The most direct replacement would be:

	IIO_DECLARE_DMA_BUFFER_WITH_TS(u8, data, 4);

In addition to improving readability, there are two more improvements
in this change that need to be called out. I used u8 because that is
shorter than unsigned char, but it is the same type.

And the 3 in the original implementation is actually wrong. The SPI
transfer is doing:

	/* First byte always 0 */
	ad7766->xfer.rx_buf = &ad7766->data[1];
	ad7766->xfer.len = 3;

So it skipping the first element of the array and writing to the next
3 elements, so we actually need to declare 4 elements in the array.

Other than those 2 changes, the macro expands to the original code.

In cases like this where there is only one channel (or cases where there
are multiple channels, but we always read all channels), we prefer a struct
instead to make it easier to see the layout.

This case is a bit odd since there isn't a 24-bit data type but it works
to use a struct anyway. So we could also write the buffer declaration as:

	struct {
		/* 24-bit big-endian value stored in 32-bits */
		u8 data[4]:
		aligned_s64 ts;
	} __aligned(IIO_DMA_MINALIGN);


> 
>  drivers/iio/adc/ad7766.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7766.c b/drivers/iio/adc/ad7766.c
> index 9e4a66477d2d..4f256ce522c2 100644
> --- a/drivers/iio/adc/ad7766.c
> +++ b/drivers/iio/adc/ad7766.c
> @@ -74,8 +74,8 @@ static irqreturn_t ad7766_trigger_handler(int irq, void *p)
>  	if (ret < 0)
>  		goto done;
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, ad7766->data,
> -		pf->timestamp);
> +	iio_push_to_buffers_with_ts(indio_dev, ad7766->data, sizeof(ad7766->data),
> +				    pf->timestamp);
>  done:
>  	iio_trigger_notify_done(indio_dev->trig);
>