[PATCH 0/4] iio: introduce IIO_DECLARE_BUFFER_WITH_TS

[PATCH 0/4] iio: introduce IIO_DECLARE_BUFFER_WITH_TS

[David Lechner]

Creating a buffer of the proper size and correct alignment for use with
iio_push_to_buffers_with_ts() is commonly used and not easy to get
right (as seen by a number of recent fixes on the mailing list).

In general, we prefer to use this pattern for creating such buffers:

struct {
    u16 data[2];
    aligned_s64 timestamp;
} buffer;

However, there are many cases where a driver may have a large number of
channels that can be optionally enabled or disabled in a scan or the
driver might support a range of chips that have different numbers of
channels or different storage sizes for the data.  In these cases, the
timestamp may not always be at the same place relative to the data. We
just allocate a buffer large enough for the largest possible case and
don't care exactly where the timestamp ends up in the buffer.

For these cases, we propose to introduce a new macro to make it easier
it easier for both the authors to get it right and for readers of the
code to not have to do all of the math to verify that it is correct.

I have just included a few examples of drivers that can make use of this
new macro, but there are dozens more.

---
David Lechner (4):
      iio: introduce IIO_DECLARE_BUFFER_WITH_TS
      iio: adc: ad4695: use IIO_DECLARE_BUFFER_WITH_TS
      iio: adc: ad7380: use IIO_DECLARE_BUFFER_WITH_TS
      iio: pressure: bmp280: use IIO_DECLARE_BUFFER_WITH_TS

 drivers/iio/adc/ad4695.c           |  6 ++----
 drivers/iio/adc/ad7380.c           |  4 ++--
 drivers/iio/pressure/bmp280-core.c |  8 ++++----
 drivers/iio/pressure/bmp280.h      |  3 +--
 include/linux/iio/iio.h            | 16 ++++++++++++++++
 5 files changed, 25 insertions(+), 12 deletions(-)
---
base-commit: aff301f37e220970c2f301b5c65a8bfedf52058e
change-id: 20250418-iio-introduce-iio_declare_buffer_with_ts-2f8773f7dad6

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

[PATCH 1/4] iio: introduce IIO_DECLARE_BUFFER_WITH_TS

[David Lechner]

Add a new macro to help with the common case of declaring a buffer that
is safe to use with iio_push_to_buffers_with_ts(). This is not trivial
to do correctly because of the alignment requirements of the timestamp.
This will make it easier for both authors and reviewers.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 include/linux/iio/iio.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 638cf2420fbd85cf2924d09d061df601d1d4bb2a..f523b046c627037c449170b7490ce2a351c6b9c0 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -7,6 +7,7 @@
 #ifndef _INDUSTRIAL_IO_H_
 #define _INDUSTRIAL_IO_H_
 
+#include <linux/align.h>
 #include <linux/device.h>
 #include <linux/cdev.h>
 #include <linux/compiler_types.h>
@@ -770,6 +771,21 @@ static inline void *iio_device_get_drvdata(const struct iio_dev *indio_dev)
 	return dev_get_drvdata(&indio_dev->dev);
 }
 
+/**
+ * IIO_DECLARE_BUFFER_WITH_TS() - Declare a buffer with timestamp
+ * @type: element type of the buffer
+ * @name: identifier name of the buffer
+ * @count: number of elements in the buffer
+ *
+ * Declares a buffer that is safe to use with iio_push_to_buffer_with_ts(). In
+ * addition to allocating enough space for @count elements of @type, it also
+ * allocates space for a s64 timestamp at the end of the buffer and ensures
+ * proper alignment of the timestamp.
+ */
+#define IIO_DECLARE_BUFFER_WITH_TS(type, name, count) \
+	type name[ALIGN((count), sizeof(s64) / sizeof(type)) \
+		  + sizeof(s64)/ sizeof(type)] __aligned(sizeof(s64))
+
 /*
  * Used to ensure the iio_priv() structure is aligned to allow that structure
  * to in turn include IIO_DMA_MINALIGN'd elements such as buffers which

-- 
2.43.0

[PATCH 2/4] iio: adc: ad4695: use IIO_DECLARE_BUFFER_WITH_TS

[David Lechner]

Use IIO_DECLARE_BUFFER_WITH_TS to declare the buffer that gets used with
iio_push_to_buffers_with_ts(). This makes the code a bit easier to read
and understand.

AD4695_MAX_CHANNEL_SIZE macro is dropped since it was making the line
too long and didn't add that much value.

AD4695_MAX_CHANNELS + 2 is changed to AD4695_MAX_CHANNELS + 1 because
previously we were overallocating. AD4695_MAX_CHANNELS is the number of
of voltage channels and + 1 is for the temperature channel.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad4695.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index 68c6625db0d75f4cade7cb029e94191118dbcaa0..9862ff99642b61db42b16b797ae046efd99c2811 100644
--- a/drivers/iio/adc/ad4695.c
+++ b/drivers/iio/adc/ad4695.c
@@ -106,8 +106,6 @@
 
 /* Max number of voltage input channels. */
 #define AD4695_MAX_CHANNELS		16
-/* Max size of 1 raw sample in bytes. */
-#define AD4695_MAX_CHANNEL_SIZE		2
 
 enum ad4695_in_pair {
 	AD4695_IN_PAIR_REFGND,
@@ -162,8 +160,8 @@ struct ad4695_state {
 	struct spi_transfer buf_read_xfer[AD4695_MAX_CHANNELS * 2 + 3];
 	struct spi_message buf_read_msg;
 	/* Raw conversion data received. */
-	u8 buf[ALIGN((AD4695_MAX_CHANNELS + 2) * AD4695_MAX_CHANNEL_SIZE,
-		     sizeof(s64)) + sizeof(s64)] __aligned(IIO_DMA_MINALIGN);
+	IIO_DECLARE_BUFFER_WITH_TS(u8, buf, (AD4695_MAX_CHANNELS + 1) * 2)
+		__aligned(IIO_DMA_MINALIGN);
 	u16 raw_data;
 	/* Commands to send for single conversion. */
 	u16 cnv_cmd;

-- 
2.43.0

[PATCH 3/4] iio: adc: ad7380: use IIO_DECLARE_BUFFER_WITH_TS

[David Lechner]

Use IIO_DECLARE_BUFFER_WITH_TS to declare the buffer that gets used with
iio_push_to_buffers_with_ts(). This makes the code a bit easier to read
and understand.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7380.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index f93e6c67766aa89b18c1a7dec02ae8912f65261c..f89b195c644024151c14977fd43e279a67439fb1 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -909,8 +909,8 @@ struct ad7380_state {
 	 * Make the buffer large enough for MAX_NUM_CHANNELS 32-bit samples and
 	 * one 64-bit aligned 64-bit timestamp.
 	 */
-	u8 scan_data[ALIGN(MAX_NUM_CHANNELS * sizeof(u32), sizeof(s64))
-			   + sizeof(s64)] __aligned(IIO_DMA_MINALIGN);
+	IIO_DECLARE_BUFFER_WITH_TS(u8, scan_data, MAX_NUM_CHANNELS * sizeof(u32))
+		__aligned(IIO_DMA_MINALIGN);
 	/* buffers for reading/writing registers */
 	u16 tx;
 	u16 rx;

-- 
2.43.0

[PATCH 4/4] iio: pressure: bmp280: use IIO_DECLARE_BUFFER_WITH_TS

[David Lechner]

Use IIO_DECLARE_BUFFER_WITH_TS to declare the buffer that gets used with
iio_push_to_buffers_with_ts(). This makes the code a bit easier to read
and understand.

The data type is changed so that we can drop the casts when the buffer
is used.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/pressure/bmp280-core.c | 8 ++++----
 drivers/iio/pressure/bmp280.h      | 3 +--
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index c20cc4a98c9c494a9c8843518ba2f17b41be18a9..847340e38fd069643c3d38037dab1bab727dcc8f 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -1107,7 +1107,7 @@ static irqreturn_t bmp280_trigger_handler(int irq, void *p)
 	struct bmp280_data *data = iio_priv(indio_dev);
 	u32 adc_temp, adc_press, comp_press;
 	s32 t_fine, comp_temp;
-	s32 *chans = (s32 *)data->sensor_data;
+	s32 *chans = data->sensor_data;
 	int ret;
 
 	guard(mutex)(&data->lock);
@@ -1228,7 +1228,7 @@ static irqreturn_t bme280_trigger_handler(int irq, void *p)
 	struct bmp280_data *data = iio_priv(indio_dev);
 	u32 adc_temp, adc_press, adc_humidity, comp_press, comp_humidity;
 	s32 t_fine, comp_temp;
-	s32 *chans = (s32 *)data->sensor_data;
+	s32 *chans = data->sensor_data;
 	int ret;
 
 	guard(mutex)(&data->lock);
@@ -1903,7 +1903,7 @@ static irqreturn_t bmp380_trigger_handler(int irq, void *p)
 	struct bmp280_data *data = iio_priv(indio_dev);
 	u32 adc_temp, adc_press, comp_press;
 	s32 t_fine, comp_temp;
-	s32 *chans = (s32 *)data->sensor_data;
+	s32 *chans = data->sensor_data;
 	int ret;
 
 	guard(mutex)(&data->lock);
@@ -2957,7 +2957,7 @@ static irqreturn_t bmp180_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct bmp280_data *data = iio_priv(indio_dev);
 	int ret, comp_temp, comp_press;
-	s32 *chans = (s32 *)data->sensor_data;
+	s32 *chans = data->sensor_data;
 
 	guard(mutex)(&data->lock);
 
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index 5b2ee1d0ee464797d1d9993a014d8f84c37d5596..86ec525ae40d92cc562e998dbe992f091343d88a 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -456,8 +456,7 @@ struct bmp280_data {
 	 * Data to push to userspace triggered buffer. Up to 3 channels and
 	 * s64 timestamp, aligned.
 	 */
-	u8 sensor_data[ALIGN(sizeof(s32) * BME280_NUM_MAX_CHANNELS, sizeof(s64))
-		       + sizeof(s64)] __aligned(sizeof(s64));
+	IIO_DECLARE_BUFFER_WITH_TS(s32, sensor_data, BME280_NUM_MAX_CHANNELS);
 
 	/* Value to hold the current operation mode of the device */
 	enum bmp280_op_mode op_mode;

-- 
2.43.0

Re: [PATCH 1/4] iio: introduce IIO_DECLARE_BUFFER_WITH_TS

[Andy Shevchenko]

On Fri, Apr 18, 2025 at 05:58:32PM -0500, David Lechner wrote:
> Add a new macro to help with the common case of declaring a buffer that
> is safe to use with iio_push_to_buffers_with_ts(). This is not trivial
> to do correctly because of the alignment requirements of the timestamp.
> This will make it easier for both authors and reviewers.

...

> +/**
> + * IIO_DECLARE_BUFFER_WITH_TS() - Declare a buffer with timestamp
> + * @type: element type of the buffer
> + * @name: identifier name of the buffer
> + * @count: number of elements in the buffer
> + *
> + * Declares a buffer that is safe to use with iio_push_to_buffer_with_ts(). In
> + * addition to allocating enough space for @count elements of @type, it also
> + * allocates space for a s64 timestamp at the end of the buffer and ensures
> + * proper alignment of the timestamp.
> + */
> +#define IIO_DECLARE_BUFFER_WITH_TS(type, name, count) \
> +	type name[ALIGN((count), sizeof(s64) / sizeof(type)) \
> +		  + sizeof(s64)/ sizeof(type)] __aligned(sizeof(s64))

Missing space and I would rather to see [...] on the same line independently on
the size as it will give better impression on what's going on here.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] iio: introduce IIO_DECLARE_BUFFER_WITH_TS

[Andy Shevchenko]

On Fri, Apr 18, 2025 at 05:58:32PM -0500, David Lechner wrote:
> Add a new macro to help with the common case of declaring a buffer that
> is safe to use with iio_push_to_buffers_with_ts(). This is not trivial
> to do correctly because of the alignment requirements of the timestamp.
> This will make it easier for both authors and reviewers.

...

> +#define IIO_DECLARE_BUFFER_WITH_TS(type, name, count) \
> +	type name[ALIGN((count), sizeof(s64) / sizeof(type)) \
> +		  + sizeof(s64)/ sizeof(type)] __aligned(sizeof(s64))

We have two alignments in a row in most of the cases, I would think that the
proper one is for DMA and this should not be used at all, it actually might be
a bug in bmp280.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] iio: adc: ad4695: use IIO_DECLARE_BUFFER_WITH_TS

[Andy Shevchenko]

On Fri, Apr 18, 2025 at 05:58:33PM -0500, David Lechner wrote:
> Use IIO_DECLARE_BUFFER_WITH_TS to declare the buffer that gets used with
> iio_push_to_buffers_with_ts(). This makes the code a bit easier to read
> and understand.
> 
> AD4695_MAX_CHANNEL_SIZE macro is dropped since it was making the line
> too long and didn't add that much value.
> 
> AD4695_MAX_CHANNELS + 2 is changed to AD4695_MAX_CHANNELS + 1 because
> previously we were overallocating. AD4695_MAX_CHANNELS is the number of
> of voltage channels and + 1 is for the temperature channel.

...

> -/* Max size of 1 raw sample in bytes. */
> -#define AD4695_MAX_CHANNEL_SIZE		2

>  	/* Raw conversion data received. */
> -	u8 buf[ALIGN((AD4695_MAX_CHANNELS + 2) * AD4695_MAX_CHANNEL_SIZE,
> -		     sizeof(s64)) + sizeof(s64)] __aligned(IIO_DMA_MINALIGN);
> +	IIO_DECLARE_BUFFER_WITH_TS(u8, buf, (AD4695_MAX_CHANNELS + 1) * 2)
> +		__aligned(IIO_DMA_MINALIGN);

I would rather expect this to be properly written as u16 / __le16 / __be16
instead of playing tricks with u8.

With all comments given so far I would expect here something like:

	IIO_DECLARE_BUFFER_WITH_TS(u16, buf, AD4695_MAX_CHANNELS + 1);


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] iio: adc: ad7380: use IIO_DECLARE_BUFFER_WITH_TS

[Andy Shevchenko]

On Fri, Apr 18, 2025 at 05:58:34PM -0500, David Lechner wrote:
> Use IIO_DECLARE_BUFFER_WITH_TS to declare the buffer that gets used with
> iio_push_to_buffers_with_ts(). This makes the code a bit easier to read
> and understand.

...

> -	u8 scan_data[ALIGN(MAX_NUM_CHANNELS * sizeof(u32), sizeof(s64))
> -			   + sizeof(s64)] __aligned(IIO_DMA_MINALIGN);
> +	IIO_DECLARE_BUFFER_WITH_TS(u8, scan_data, MAX_NUM_CHANNELS * sizeof(u32))
> +		__aligned(IIO_DMA_MINALIGN);

Why u8 and not u32?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] iio: pressure: bmp280: use IIO_DECLARE_BUFFER_WITH_TS

[Andy Shevchenko]

On Fri, Apr 18, 2025 at 05:58:35PM -0500, David Lechner wrote:
> Use IIO_DECLARE_BUFFER_WITH_TS to declare the buffer that gets used with
> iio_push_to_buffers_with_ts(). This makes the code a bit easier to read
> and understand.
> 
> The data type is changed so that we can drop the casts when the buffer
> is used.

This one is good, with the comment to have it DMA aligned.

Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] iio: adc: ad4695: use IIO_DECLARE_BUFFER_WITH_TS

[David Lechner]

On 4/19/25 11:38 AM, Andy Shevchenko wrote:
> On Fri, Apr 18, 2025 at 05:58:33PM -0500, David Lechner wrote:
>> Use IIO_DECLARE_BUFFER_WITH_TS to declare the buffer that gets used with
>> iio_push_to_buffers_with_ts(). This makes the code a bit easier to read
>> and understand.
>>
>> AD4695_MAX_CHANNEL_SIZE macro is dropped since it was making the line
>> too long and didn't add that much value.
>>
>> AD4695_MAX_CHANNELS + 2 is changed to AD4695_MAX_CHANNELS + 1 because
>> previously we were overallocating. AD4695_MAX_CHANNELS is the number of
>> of voltage channels and + 1 is for the temperature channel.
> 
> ...
> 
>> -/* Max size of 1 raw sample in bytes. */
>> -#define AD4695_MAX_CHANNEL_SIZE		2
> 
>>  	/* Raw conversion data received. */
>> -	u8 buf[ALIGN((AD4695_MAX_CHANNELS + 2) * AD4695_MAX_CHANNEL_SIZE,
>> -		     sizeof(s64)) + sizeof(s64)] __aligned(IIO_DMA_MINALIGN);
>> +	IIO_DECLARE_BUFFER_WITH_TS(u8, buf, (AD4695_MAX_CHANNELS + 1) * 2)
>> +		__aligned(IIO_DMA_MINALIGN);
> 
> I would rather expect this to be properly written as u16 / __le16 / __be16
> instead of playing tricks with u8.
> 
> With all comments given so far I would expect here something like:
> 
> 	IIO_DECLARE_BUFFER_WITH_TS(u16, buf, AD4695_MAX_CHANNELS + 1);
> 
> 

We would have to make significant changes to the driver to allow u16 instead
of u8. I don't remember why I did it that way in the first place, but I consider
changing it out of scope for this patch.

Re: [PATCH 3/4] iio: adc: ad7380: use IIO_DECLARE_BUFFER_WITH_TS

[David Lechner]

On 4/19/25 11:38 AM, Andy Shevchenko wrote:
> On Fri, Apr 18, 2025 at 05:58:34PM -0500, David Lechner wrote:
>> Use IIO_DECLARE_BUFFER_WITH_TS to declare the buffer that gets used with
>> iio_push_to_buffers_with_ts(). This makes the code a bit easier to read
>> and understand.
> 
> ...
> 
>> -	u8 scan_data[ALIGN(MAX_NUM_CHANNELS * sizeof(u32), sizeof(s64))
>> -			   + sizeof(s64)] __aligned(IIO_DMA_MINALIGN);
>> +	IIO_DECLARE_BUFFER_WITH_TS(u8, scan_data, MAX_NUM_CHANNELS * sizeof(u32))
>> +		__aligned(IIO_DMA_MINALIGN);
> 
> Why u8 and not u32?
> 

Because this driver supports many chips, some need u16 and some u32.

Re: [PATCH 4/4] iio: pressure: bmp280: use IIO_DECLARE_BUFFER_WITH_TS

[David Lechner]

On 4/19/25 11:39 AM, Andy Shevchenko wrote:
> On Fri, Apr 18, 2025 at 05:58:35PM -0500, David Lechner wrote:
>> Use IIO_DECLARE_BUFFER_WITH_TS to declare the buffer that gets used with
>> iio_push_to_buffers_with_ts(). This makes the code a bit easier to read
>> and understand.
>>
>> The data type is changed so that we can drop the casts when the buffer
>> is used.
> 
> This one is good, with the comment to have it DMA aligned.
> 
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> 

Strictly speaking, this one doesn't need to be DMA-safe. This buffer isn't
passed to SPI or any other bus. It is just used for iio_push_to_buffers_with_ts()
and has data copied to it from elsewhere just before that.

Re: [PATCH 0/4] iio: introduce IIO_DECLARE_BUFFER_WITH_TS

[Andy Shevchenko]

On Sat, Apr 19, 2025 at 1:59 AM David Lechner <dlechner@baylibre.com> wrote:
>
> Creating a buffer of the proper size and correct alignment for use with
> iio_push_to_buffers_with_ts() is commonly used and not easy to get
> right (as seen by a number of recent fixes on the mailing list).
>
> In general, we prefer to use this pattern for creating such buffers:
>
> struct {
>     u16 data[2];
>     aligned_s64 timestamp;
> } buffer;
>
> However, there are many cases where a driver may have a large number of
> channels that can be optionally enabled or disabled in a scan or the
> driver might support a range of chips that have different numbers of
> channels or different storage sizes for the data.  In these cases, the
> timestamp may not always be at the same place relative to the data. We
> just allocate a buffer large enough for the largest possible case and
> don't care exactly where the timestamp ends up in the buffer.
>
> For these cases, we propose to introduce a new macro to make it easier
> it easier for both the authors to get it right and for readers of the
> code to not have to do all of the math to verify that it is correct.
>
> I have just included a few examples of drivers that can make use of this
> new macro, but there are dozens more.

I'm going to answer here as the summary of my view to this series and
macro after your replies.

So, first of all, the macro embeds alignment which is used only once
in practice and the alignment used in most of the cases is DMA one.
Having two alignments in a row seems a bit weird to me. Second one, if
we drop alignment, it means each of the users will need it. That
significantly increases the line size and with high probability will
require two LoCs to occupy. And third, based on the examples, the
macro doesn't help much if we don't convert drivers to properly handle
what they are using instead of plain u8 in all of the cases. Yes, it
might require quite an invasive change to a driver, but this is how I
see it should go.

That said, it feels like this series took a half road.

I leave it to Jonothan, but I don't like it to be merged in this form.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] iio: adc: ad4695: use IIO_DECLARE_BUFFER_WITH_TS

[Jonathan Cameron]

On Sat, 19 Apr 2025 12:57:11 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 4/19/25 11:38 AM, Andy Shevchenko wrote:
> > On Fri, Apr 18, 2025 at 05:58:33PM -0500, David Lechner wrote:  
> >> Use IIO_DECLARE_BUFFER_WITH_TS to declare the buffer that gets used with
> >> iio_push_to_buffers_with_ts(). This makes the code a bit easier to read
> >> and understand.
> >>
> >> AD4695_MAX_CHANNEL_SIZE macro is dropped since it was making the line
> >> too long and didn't add that much value.
> >>
> >> AD4695_MAX_CHANNELS + 2 is changed to AD4695_MAX_CHANNELS + 1 because
> >> previously we were overallocating. AD4695_MAX_CHANNELS is the number of
> >> of voltage channels and + 1 is for the temperature channel.  
> > 
> > ...
> >   
> >> -/* Max size of 1 raw sample in bytes. */
> >> -#define AD4695_MAX_CHANNEL_SIZE		2  
> >   
> >>  	/* Raw conversion data received. */
> >> -	u8 buf[ALIGN((AD4695_MAX_CHANNELS + 2) * AD4695_MAX_CHANNEL_SIZE,
> >> -		     sizeof(s64)) + sizeof(s64)] __aligned(IIO_DMA_MINALIGN);
> >> +	IIO_DECLARE_BUFFER_WITH_TS(u8, buf, (AD4695_MAX_CHANNELS + 1) * 2)
> >> +		__aligned(IIO_DMA_MINALIGN);  
> > 
> > I would rather expect this to be properly written as u16 / __le16 / __be16
> > instead of playing tricks with u8.
> > 
> > With all comments given so far I would expect here something like:
> > 
> > 	IIO_DECLARE_BUFFER_WITH_TS(u16, buf, AD4695_MAX_CHANNELS + 1);
> > 
> >   
> 
> We would have to make significant changes to the driver to allow u16 instead
> of u8. I don't remember why I did it that way in the first place, but I consider
> changing it out of scope for this patch.

There are drivers where the size varies depending on the exact part.
Maybe this is a cut and paste from one of those or you thought this might get
bigger to support > 16bit channels in the future.  Either way, right now
it is always 16 bits so an appropriately sized type would be good as you
say.   I think such a change should be in a precursor patch probably
rather than left for another day.  Looks trivial to me given st->buf
is only accessed directly in 2 places.

Jonathan

Re: [PATCH 4/4] iio: pressure: bmp280: use IIO_DECLARE_BUFFER_WITH_TS

[Jonathan Cameron]

On Sat, 19 Apr 2025 13:04:08 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 4/19/25 11:39 AM, Andy Shevchenko wrote:
> > On Fri, Apr 18, 2025 at 05:58:35PM -0500, David Lechner wrote:  
> >> Use IIO_DECLARE_BUFFER_WITH_TS to declare the buffer that gets used with
> >> iio_push_to_buffers_with_ts(). This makes the code a bit easier to read
> >> and understand.
> >>
> >> The data type is changed so that we can drop the casts when the buffer
> >> is used.  
> > 
> > This one is good, with the comment to have it DMA aligned.
> > 
> > Reviewed-by: Andy Shevchenko <andy@kernel.org>
> >   
> 
> Strictly speaking, this one doesn't need to be DMA-safe. This buffer isn't
> passed to SPI or any other bus. It is just used for iio_push_to_buffers_with_ts()
> and has data copied to it from elsewhere just before that.


Silly question.  Why is it not just locally on the stack?  It's only 16 or 24 bytes...
I think that other than the bme280 we can use structures. (That one has 3 32bit channels)

So we can have the more readable form in all but one place in the driver.

Jonathan

Re: [PATCH 0/4] iio: introduce IIO_DECLARE_BUFFER_WITH_TS

[Jonathan Cameron]

On Sun, 20 Apr 2025 07:36:18 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sat, Apr 19, 2025 at 1:59 AM David Lechner <dlechner@baylibre.com> wrote:
> >
> > Creating a buffer of the proper size and correct alignment for use with
> > iio_push_to_buffers_with_ts() is commonly used and not easy to get
> > right (as seen by a number of recent fixes on the mailing list).
> >
> > In general, we prefer to use this pattern for creating such buffers:
> >
> > struct {
> >     u16 data[2];
> >     aligned_s64 timestamp;
> > } buffer;
> >
> > However, there are many cases where a driver may have a large number of
> > channels that can be optionally enabled or disabled in a scan or the
> > driver might support a range of chips that have different numbers of
> > channels or different storage sizes for the data.  In these cases, the
> > timestamp may not always be at the same place relative to the data. We
> > just allocate a buffer large enough for the largest possible case and
> > don't care exactly where the timestamp ends up in the buffer.
> >
> > For these cases, we propose to introduce a new macro to make it easier
> > it easier for both the authors to get it right and for readers of the
> > code to not have to do all of the math to verify that it is correct.
> >
> > I have just included a few examples of drivers that can make use of this
> > new macro, but there are dozens more.  
> 
> I'm going to answer here as the summary of my view to this series and
> macro after your replies.
> 
> So, first of all, the macro embeds alignment which is used only once
> in practice and the alignment used in most of the cases is DMA one.

It think that's because this is only converting a few examples.  There
are more of these to come (9 of the ones that David first converted
to structures then realized this was a better fit for starters!).

A lot might still be aligned for DMA but I'd expect to see more of
the cases that don't need that either because they are i2c only or
because data shuffling means the DMA hits a different buffer and we
unscramble it into this one.

> Having two alignments in a row seems a bit weird to me. Second one, if
> we drop alignment, it means each of the users will need it. That
> significantly increases the line size and with high probability will
> require two LoCs to occupy. And third, based on the examples, the
> macro doesn't help much if we don't convert drivers to properly handle
> what they are using instead of plain u8 in all of the cases. Yes, it
> might require quite an invasive change to a driver, but this is how I
> see it should go.

Agreed for almost all cases that a conversion to the right type
is good to have - preferably as a precursor.  There is that one case in
here where it depends on the specific part though which will remain
in a messier form.

> 
> That said, it feels like this series took a half road.
> 
> I leave it to Jonothan, but I don't like it to be merged in this form.
> 

Whilst there are no current platforms where IIO_DMA_MINALIGN is < 8
in theory it might be in future. 

The other way around, IIO_DMA_MINALIGN is large on some architectures
so could result in considerable additional padding and should only
be used where it is needed.  Also that alignment is useless on the
stack as it does nothing about data after the buffer. Hence there
isn't really a general solution with one or the other :(

So I like the idea in general as a way to make things a little better
but agree it is still somewhat ugly.

Maybe we could add...
IIO_DECLARE_BUFFER_WITH_TS_FOR_DMA()
Might be worth it?

Jonathan 

Re: [PATCH 1/4] iio: introduce IIO_DECLARE_BUFFER_WITH_TS

[Nuno Sá]

On Fri, 2025-04-18 at 17:58 -0500, David Lechner wrote:
> Add a new macro to help with the common case of declaring a buffer that
> is safe to use with iio_push_to_buffers_with_ts(). This is not trivial
> to do correctly because of the alignment requirements of the timestamp.
> This will make it easier for both authors and reviewers.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  include/linux/iio/iio.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index
> 638cf2420fbd85cf2924d09d061df601d1d4bb2a..f523b046c627037c449170b7490ce2a351c6
> b9c0 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -7,6 +7,7 @@
>  #ifndef _INDUSTRIAL_IO_H_
>  #define _INDUSTRIAL_IO_H_
>  
> +#include <linux/align.h>
>  #include <linux/device.h>
>  #include <linux/cdev.h>
>  #include <linux/compiler_types.h>
> @@ -770,6 +771,21 @@ static inline void *iio_device_get_drvdata(const struct
> iio_dev *indio_dev)
>  	return dev_get_drvdata(&indio_dev->dev);
>  }
>  
> +/**
> + * IIO_DECLARE_BUFFER_WITH_TS() - Declare a buffer with timestamp
> + * @type: element type of the buffer
> + * @name: identifier name of the buffer
> + * @count: number of elements in the buffer
> + *
> + * Declares a buffer that is safe to use with iio_push_to_buffer_with_ts().
> In
> + * addition to allocating enough space for @count elements of @type, it also
> + * allocates space for a s64 timestamp at the end of the buffer and ensures
> + * proper alignment of the timestamp.
> + */
> +#define IIO_DECLARE_BUFFER_WITH_TS(type, name, count) \
> +	type name[ALIGN((count), sizeof(s64) / sizeof(type)) \
> +		  + sizeof(s64)/ sizeof(type)] __aligned(sizeof(s64))
> +

I tend to agree with Andy on the two alignments thing. Independent of that I
guess this is also not intended for stack allocations? If so, we should maybe be
clear about that in the docs... If we do intend using stack allocations we could
improve it by making sure the build fails if "count" is not a constant
expression (likely it will already but I think we should make the intent clear).

- Nuno Sá

>  /*
>   * Used to ensure the iio_priv() structure is aligned to allow that structure
>   * to in turn include IIO_DMA_MINALIGN'd elements such as buffers which

Re: [PATCH 0/4] iio: introduce IIO_DECLARE_BUFFER_WITH_TS

[David Lechner]

On 4/21/25 8:03 AM, Jonathan Cameron wrote:
> On Sun, 20 Apr 2025 07:36:18 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
>> On Sat, Apr 19, 2025 at 1:59 AM David Lechner <dlechner@baylibre.com> wrote:
>>>

...

> 
> Maybe we could add...
> IIO_DECLARE_BUFFER_WITH_TS_FOR_DMA()
> Might be worth it?
> 
After reading everyone's comments, that was my first instinct. We should have
2 forms of the macro, one for DMA-safe and one for not.

Re: [PATCH 1/4] iio: introduce IIO_DECLARE_BUFFER_WITH_TS

[David Lechner]

On 4/19/25 11:33 AM, Andy Shevchenko wrote:
> On Fri, Apr 18, 2025 at 05:58:32PM -0500, David Lechner wrote:
>> Add a new macro to help with the common case of declaring a buffer that
>> is safe to use with iio_push_to_buffers_with_ts(). This is not trivial
>> to do correctly because of the alignment requirements of the timestamp.
>> This will make it easier for both authors and reviewers.
> 
> ...
> 
>> +/**
>> + * IIO_DECLARE_BUFFER_WITH_TS() - Declare a buffer with timestamp
>> + * @type: element type of the buffer
>> + * @name: identifier name of the buffer
>> + * @count: number of elements in the buffer
>> + *
>> + * Declares a buffer that is safe to use with iio_push_to_buffer_with_ts(). In
>> + * addition to allocating enough space for @count elements of @type, it also
>> + * allocates space for a s64 timestamp at the end of the buffer and ensures
>> + * proper alignment of the timestamp.
>> + */
>> +#define IIO_DECLARE_BUFFER_WITH_TS(type, name, count) \
>> +	type name[ALIGN((count), sizeof(s64) / sizeof(type)) \
>> +		  + sizeof(s64)/ sizeof(type)] __aligned(sizeof(s64))
> 
> Missing space

Sorry, but my eyes can't find any missing space. Can you be more specific?

> and I would rather to see [...] on the same line independently on> the size as it will give better impression on what's going on here.
> 

As long as Jonathan doesn't mind the long line. :-)

Re: [PATCH 1/4] iio: introduce IIO_DECLARE_BUFFER_WITH_TS

[Andy Shevchenko]

Mon, Apr 21, 2025 at 05:40:41PM -0500, David Lechner kirjoitti:
> On 4/19/25 11:33 AM, Andy Shevchenko wrote:
> > On Fri, Apr 18, 2025 at 05:58:32PM -0500, David Lechner wrote:

...

> >> +/**
> >> + * IIO_DECLARE_BUFFER_WITH_TS() - Declare a buffer with timestamp
> >> + * @type: element type of the buffer
> >> + * @name: identifier name of the buffer
> >> + * @count: number of elements in the buffer
> >> + *
> >> + * Declares a buffer that is safe to use with iio_push_to_buffer_with_ts(). In
> >> + * addition to allocating enough space for @count elements of @type, it also
> >> + * allocates space for a s64 timestamp at the end of the buffer and ensures
> >> + * proper alignment of the timestamp.
> >> + */
> >> +#define IIO_DECLARE_BUFFER_WITH_TS(type, name, count) \
> >> +	type name[ALIGN((count), sizeof(s64) / sizeof(type)) \
> >> +		  + sizeof(s64)/ sizeof(type)] __aligned(sizeof(s64))
> > 
> > Missing space
> 
> Sorry, but my eyes can't find any missing space. Can you be more specific?

As far as I can see it's missed after sizeof(s64)
Also I don't like to see the leading operators (like +, -, *, &).

> > and I would rather to see [...] on the same line independently on> the size as it will give better impression on what's going on here.
> 
> As long as Jonathan doesn't mind the long line. :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] iio: introduce IIO_DECLARE_BUFFER_WITH_TS

[David Lechner]

On 4/22/25 1:36 AM, Andy Shevchenko wrote:
> Mon, Apr 21, 2025 at 05:40:41PM -0500, David Lechner kirjoitti:
>> On 4/19/25 11:33 AM, Andy Shevchenko wrote:
>>> On Fri, Apr 18, 2025 at 05:58:32PM -0500, David Lechner wrote:
> 
> ...
> 
>>>> +/**
>>>> + * IIO_DECLARE_BUFFER_WITH_TS() - Declare a buffer with timestamp
>>>> + * @type: element type of the buffer
>>>> + * @name: identifier name of the buffer
>>>> + * @count: number of elements in the buffer
>>>> + *
>>>> + * Declares a buffer that is safe to use with iio_push_to_buffer_with_ts(). In
>>>> + * addition to allocating enough space for @count elements of @type, it also
>>>> + * allocates space for a s64 timestamp at the end of the buffer and ensures
>>>> + * proper alignment of the timestamp.
>>>> + */
>>>> +#define IIO_DECLARE_BUFFER_WITH_TS(type, name, count) \
>>>> +	type name[ALIGN((count), sizeof(s64) / sizeof(type)) \
>>>> +		  + sizeof(s64)/ sizeof(type)] __aligned(sizeof(s64))
>>>
>>> Missing space
>>
>> Sorry, but my eyes can't find any missing space. Can you be more specific?
> 
> As far as I can see it's missed after sizeof(s64)
> Also I don't like to see the leading operators (like +, -, *, &).

:facepalm: yes, I see it now.

> 
>>> and I would rather to see [...] on the same line independently on> the size as it will give better impression on what's going on here.
>>
>> As long as Jonathan doesn't mind the long line. :-)
>