RE: [RFC PATCH 4/4] iio: imu: adis16400: Fix buffer alignment requirements.

["Sa, Nuno" <Nuno.Sa@analog.com>]

> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, May 1, 2021 7:25 PM
> To: linux-iio@vger.kernel.org
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>; Sa, Nuno
> <Nuno.Sa@analog.com>
> Subject: [RFC PATCH 4/4] iio: imu: adis16400: Fix buffer alignment
> requirements.
> 
> 
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> iio_push_to_buffers_with_timestamp() requires that the buffer
> is 8 byte alignment to ensure an inserted timestamp is naturally
> aligned.
> 
> This requirement was not met here when burst mode is in use beause
> of a leading u16. Use the new iio_push_to_buffers_with_ts_na()
> function
> that has more relaxed requirements.
> 
> It is somewhat complex to access that actual data length, but a
> safe bound can be found by using scan_bytes - sizeof(timestamp) so
> that
> is used in this path.
> 
> More efficient approaches exist, but this ensure correctness at the
> cost of using a bounce buffer.
> 
> Fixes: 5075e0720d93 ("iio: imu: adis: generalize burst mode support")
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/imu/adis16400.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
> index 768aa493a1a6..c6d03a37373b 100644
> --- a/drivers/iio/imu/adis16400.c
> +++ b/drivers/iio/imu/adis16400.c
> @@ -663,13 +663,23 @@ static irqreturn_t
> adis16400_trigger_handler(int irq, void *p)
>  		spi_setup(st->adis.spi);
>  	}
> 
> -	if (st->variant->flags & ADIS16400_BURST_DIAG_STAT)
> +	if (st->variant->flags & ADIS16400_BURST_DIAG_STAT) {
>  		buffer = adis->buffer + sizeof(u16);
> -	else
> -		buffer = adis->buffer;
> +		/*
> +		 * The size here is always larger than, or equal to the
> true
> +		 * size of the channel data. This may result in a larger
> copy
> +		 * than necessary, but as the target buffer will be
> +		 * buffer->scan_bytes this will be safe.
> +		 */
> +		iio_push_to_buffers_with_ts_na(indio_dev, buffer,
> +					       indio_dev->scan_bytes -
> sizeof(pf->timestamp),
> +					       pf->timestamp);
> +	} else {
> +		iio_push_to_buffers_with_timestamp(indio_dev,
> +						   adis->buffer,
> +						   pf->timestamp);
> +	}
> 

Hi Jonathan,

This looks good to me although I think there's some stuff to care in
' iio_push_to_buffers_with_ts_na()'. However, for this patch alone:

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

I will also check if I find any HW to test this...

- Nuno Sá