Re: [PATCH 02/10] iio: adc: at91-sama5d2_adc: use struct with aligned_s64 timestamp

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 18 Apr 2025 14:58:21 -0500
David Lechner <dlechner@baylibre.com> wrote:

> Use a struct with aligned s64 timestamp_instead of a padded array for
> the buffer used for iio_push_to_buffers_with_ts(). This makes it easier
> to see the correctness of the size and alignment of the buffer.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 414610afcb2c4128a63cf76767803c32cb01ac5e..07ced924f7a6ae36fe538021a45adbf7d76c2e69 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -21,6 +21,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/property.h>
>  #include <linux/sched.h>
> +#include <linux/types.h>
>  #include <linux/units.h>
>  #include <linux/wait.h>
>  #include <linux/iio/iio.h>
> @@ -586,15 +587,6 @@ struct at91_adc_temp {
>  	u16				saved_oversampling;
>  };
>  
> -/*
> - * Buffer size requirements:
> - * No channels * bytes_per_channel(2) + timestamp bytes (8)
> - * Divided by 2 because we need half words.
> - * We assume 32 channels for now, has to be increased if needed.
> - * Nobody minds a buffer being too big.
> - */
> -#define AT91_BUFFER_MAX_HWORDS ((32 * 2 + 8) / 2)
> -
>  struct at91_adc_state {
>  	void __iomem			*base;
>  	int				irq;
> @@ -617,7 +609,10 @@ struct at91_adc_state {
>  	struct iio_dev			*indio_dev;
>  	struct device			*dev;
>  	/* Ensure naturally aligned timestamp */
> -	u16				buffer[AT91_BUFFER_MAX_HWORDS] __aligned(8);
> +	struct {
> +		u16 data[32];

When you rework this into the large buffer scheme, can you add a define
or some other means to establish where that 32 comes from!
We've lost the comment as a result of this refactor so need to put that
info back somehow.


> +		aligned_s64 timestamp;
> +	} buffer;
>  	/*
>  	 * lock to prevent concurrent 'single conversion' requests through
>  	 * sysfs.
> @@ -1481,14 +1476,14 @@ static void at91_adc_trigger_handler_nodma(struct iio_dev *indio_dev,
>  		if (chan->type == IIO_VOLTAGE) {
>  			val = at91_adc_read_chan(st, chan->address);
>  			at91_adc_adjust_val_osr(st, &val);
> -			st->buffer[i] = val;
> +			st->buffer.data[i] = val;
>  		} else {
> -			st->buffer[i] = 0;
> +			st->buffer.data[i] = 0;
>  			WARN(true, "This trigger cannot handle this type of channel");
>  		}
>  		i++;
>  	}
> -	iio_push_to_buffers_with_timestamp(indio_dev, st->buffer,
> +	iio_push_to_buffers_with_timestamp(indio_dev, &st->buffer,
>  					   pf->timestamp);
>  }
>  
> @@ -1643,7 +1638,7 @@ static void at91_adc_touch_data_handler(struct iio_dev *indio_dev)
>  			at91_adc_read_pressure(st, chan->channel, &val);
>  		else
>  			continue;
> -		st->buffer[i] = val;
> +		st->buffer.data[i] = val;
>  		i++;
>  	}
>  	/*
> @@ -1691,7 +1686,7 @@ static void at91_adc_workq_handler(struct work_struct *workq)
>  					struct at91_adc_state, touch_st);
>  	struct iio_dev *indio_dev = st->indio_dev;
>  
> -	iio_push_to_buffers(indio_dev, st->buffer);
> +	iio_push_to_buffers(indio_dev, st->buffer.data);
>  }
>  
>  static irqreturn_t at91_adc_interrupt(int irq, void *private)
>