[PATCH v5 0/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS

[PATCH v5 0/7] 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. To
handle these, we 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 new macros 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.

---
Changes in v5:
- Add new patch to set minimum alignment to 8 for IIO_DMA_MINALIGN.
- Adjust IIO_DECLARE_DMA_BUFFER_WITH_TS() macro for above change.
- Drop one ad4695 patch that was already applied.
- Link to v4: https://lore.kernel.org/r/20250428-iio-introduce-iio_declare_buffer_with_ts-v4-0-6f7f6126f1cb@baylibre.com

Changes in v4:
- Dropped static_assert()s from the first patch.
- Handle case when IIO_DMA_MINALIGN < sizeof(timestamp).
- Added one more patch for ad4695 to rename a confusing macro.
- Link to v3: https://lore.kernel.org/r/20250425-iio-introduce-iio_declare_buffer_with_ts-v3-0-f12df1bff248@baylibre.com

Changes in v3:
- Fixed a few mistakes, style issues and incorporate other feedback (see
  individual commit message changelogs for details).
- Link to v2: https://lore.kernel.org/r/20250422-iio-introduce-iio_declare_buffer_with_ts-v2-0-3fd36475c706@baylibre.com

Changes in v2:
- Add 2nd macro for case where we need DMA alignment.
- Add new patch for ad4695 to convert buffer from u8 to u16 before
  making use of the new macro.
- Drop the bmp280 patch since it was determined to have a better
  alternative not using these macros.
- Add a few more examples to show the non-DMA case, both in a struct and
  stack allocated.
- Link to v1: https://lore.kernel.org/r/20250418-iio-introduce-iio_declare_buffer_with_ts-v1-0-ee0c62a33a0f@baylibre.com

---
David Lechner (7):
      iio: make IIO_DMA_MINALIGN minimum of 8 bytes
      iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros
      iio: adc: ad4695: use IIO_DECLARE_DMA_BUFFER_WITH_TS
      iio: adc: ad4695: rename AD4695_MAX_VIN_CHANNELS
      iio: adc: ad7380: use IIO_DECLARE_DMA_BUFFER_WITH_TS
      iio: accel: sca3300: use IIO_DECLARE_BUFFER_WITH_TS
      iio: adc: at91-sama5d2: use IIO_DECLARE_BUFFER_WITH_TS

 drivers/iio/accel/sca3300.c        | 18 ++--------------
 drivers/iio/adc/ad4695.c           | 11 +++++-----
 drivers/iio/adc/ad7380.c           |  3 +--
 drivers/iio/adc/at91-sama5d2_adc.c | 13 ++----------
 include/linux/iio/iio.h            | 42 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 52 insertions(+), 35 deletions(-)
---
base-commit: 7e9a82ab5b861d3c33c99a22c1245a5b262ee502
change-id: 20250418-iio-introduce-iio_declare_buffer_with_ts-2f8773f7dad6

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

[PATCH v5 1/7] iio: make IIO_DMA_MINALIGN minimum of 8 bytes

[David Lechner]

Add a condition to ensure that IIO_DMA_MINALIGN is at least 8 bytes.
On some 32-bit architectures, IIO_DMA_MINALIGN is 4. In many cases,
drivers are using this alignment for buffers that include a 64-bit
timestamp that is used with iio_push_to_buffers_with_ts(), which expects
the timestamp to be aligned to 8 bytes. To handle this, we can just make
IIO_DMA_MINALIGN at least 8 bytes.

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

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 638cf2420fbd85cf2924d09d061df601d1d4bb2a..7e1e3739328d103262071bd34ba5f6631163c122 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -775,8 +775,18 @@ static inline void *iio_device_get_drvdata(const struct iio_dev *indio_dev)
  * to in turn include IIO_DMA_MINALIGN'd elements such as buffers which
  * must not share  cachelines with the rest of the structure, thus making
  * them safe for use with non-coherent DMA.
+ *
+ * A number of drivers also use this on buffers that include a 64-bit timestamp
+ * that is used with iio_push_to_buffer_with_ts(). Therefore, in the case where
+ * DMA alignment is not sufficient for proper timestamp alignment, we align to
+ * 8 bytes instead.
  */
+#if ARCH_DMA_MINALIGN < sizeof(s64)
+#define IIO_DMA_MINALIGN sizeof(s64)
+#else
 #define IIO_DMA_MINALIGN ARCH_DMA_MINALIGN
+#endif
+
 struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv);
 
 /* The information at the returned address is guaranteed to be cacheline aligned */

-- 
2.43.0

Re: [PATCH v5 1/7] iio: make IIO_DMA_MINALIGN minimum of 8 bytes

[Jonathan Cameron]

On Mon, 05 May 2025 11:31:42 -0500
David Lechner <dlechner@baylibre.com> wrote:

> Add a condition to ensure that IIO_DMA_MINALIGN is at least 8 bytes.
> On some 32-bit architectures, IIO_DMA_MINALIGN is 4. In many cases,
> drivers are using this alignment for buffers that include a 64-bit
> timestamp that is used with iio_push_to_buffers_with_ts(), which expects
> the timestamp to be aligned to 8 bytes. To handle this, we can just make
> IIO_DMA_MINALIGN at least 8 bytes.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Dropped again...

In file included from drivers/input/touchscreen/tsc2007_iio.c:8:
./include/linux/iio/iio.h:784:5: warning: '__alignof__' is not defined, evaluates to 0 [-Wundef]
  784 | #if ARCH_DMA_MINALIGN < sizeof(s64)
      |     ^
./include/linux/cache.h:181:27: note: expanded from macro 'ARCH_DMA_MINALIGN'
  181 | #define ARCH_DMAmake[3]: *** [scripts/Makefile.build:461: drivers/hid] Error 2
_MINALIGN __alignof__(unsigned long long)
      |                           ^
In file included from drivers/input/touchscreen/tsc2007_iio.c:8:
./include/linux/iio/iio.h:784:5: error: function-like macro '__alignof__' is not defined
./include/linux/cache.h:181:27: note: expanded from macro 'ARCH_DMA_MINALIGN'
  181 | #define ARCH_DMA_MINALIGN __alignof__(unsigned long long)
      |   

With an LLVM build on x86.

> ---
>  include/linux/iio/iio.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 638cf2420fbd85cf2924d09d061df601d1d4bb2a..7e1e3739328d103262071bd34ba5f6631163c122 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -775,8 +775,18 @@ static inline void *iio_device_get_drvdata(const struct iio_dev *indio_dev)
>   * to in turn include IIO_DMA_MINALIGN'd elements such as buffers which
>   * must not share  cachelines with the rest of the structure, thus making
>   * them safe for use with non-coherent DMA.
> + *
> + * A number of drivers also use this on buffers that include a 64-bit timestamp
> + * that is used with iio_push_to_buffer_with_ts(). Therefore, in the case where
> + * DMA alignment is not sufficient for proper timestamp alignment, we align to
> + * 8 bytes instead.
>   */
> +#if ARCH_DMA_MINALIGN < sizeof(s64)
> +#define IIO_DMA_MINALIGN sizeof(s64)
> +#else
>  #define IIO_DMA_MINALIGN ARCH_DMA_MINALIGN
> +#endif
> +
>  struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv);
>  
>  /* The information at the returned address is guaranteed to be cacheline aligned */
> 

Re: [PATCH v5 1/7] iio: make IIO_DMA_MINALIGN minimum of 8 bytes

[David Laight]

On Mon, 05 May 2025 11:31:42 -0500
David Lechner <dlechner@baylibre.com> wrote:

> Add a condition to ensure that IIO_DMA_MINALIGN is at least 8 bytes.
> On some 32-bit architectures, IIO_DMA_MINALIGN is 4. In many cases,
> drivers are using this alignment for buffers that include a 64-bit
> timestamp that is used with iio_push_to_buffers_with_ts(), which expects
> the timestamp to be aligned to 8 bytes. To handle this, we can just make
> IIO_DMA_MINALIGN at least 8 bytes.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  include/linux/iio/iio.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 638cf2420fbd85cf2924d09d061df601d1d4bb2a..7e1e3739328d103262071bd34ba5f6631163c122 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -775,8 +775,18 @@ static inline void *iio_device_get_drvdata(const struct iio_dev *indio_dev)
>   * to in turn include IIO_DMA_MINALIGN'd elements such as buffers which
>   * must not share  cachelines with the rest of the structure, thus making
>   * them safe for use with non-coherent DMA.
> + *
> + * A number of drivers also use this on buffers that include a 64-bit timestamp
> + * that is used with iio_push_to_buffer_with_ts(). Therefore, in the case where
> + * DMA alignment is not sufficient for proper timestamp alignment, we align to
> + * 8 bytes instead.
>   */
> +#if ARCH_DMA_MINALIGN < sizeof(s64)
> +#define IIO_DMA_MINALIGN sizeof(s64)
> +#else
>  #define IIO_DMA_MINALIGN ARCH_DMA_MINALIGN
> +#endif

Did you actually test this?
You can't use sizeof() in a pre-processor conditional.

	David

> +
>  struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv);
>  
>  /* The information at the returned address is guaranteed to be cacheline aligned */
> 

Re: [PATCH v5 1/7] iio: make IIO_DMA_MINALIGN minimum of 8 bytes

[David Lechner]

On 5/8/25 4:39 PM, David Laight wrote:
> On Mon, 05 May 2025 11:31:42 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> Add a condition to ensure that IIO_DMA_MINALIGN is at least 8 bytes.
>> On some 32-bit architectures, IIO_DMA_MINALIGN is 4. In many cases,
>> drivers are using this alignment for buffers that include a 64-bit
>> timestamp that is used with iio_push_to_buffers_with_ts(), which expects
>> the timestamp to be aligned to 8 bytes. To handle this, we can just make
>> IIO_DMA_MINALIGN at least 8 bytes.
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
>>  include/linux/iio/iio.h | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>> index 638cf2420fbd85cf2924d09d061df601d1d4bb2a..7e1e3739328d103262071bd34ba5f6631163c122 100644
>> --- a/include/linux/iio/iio.h
>> +++ b/include/linux/iio/iio.h
>> @@ -775,8 +775,18 @@ static inline void *iio_device_get_drvdata(const struct iio_dev *indio_dev)
>>   * to in turn include IIO_DMA_MINALIGN'd elements such as buffers which
>>   * must not share  cachelines with the rest of the structure, thus making
>>   * them safe for use with non-coherent DMA.
>> + *
>> + * A number of drivers also use this on buffers that include a 64-bit timestamp
>> + * that is used with iio_push_to_buffer_with_ts(). Therefore, in the case where
>> + * DMA alignment is not sufficient for proper timestamp alignment, we align to
>> + * 8 bytes instead.
>>   */
>> +#if ARCH_DMA_MINALIGN < sizeof(s64)
>> +#define IIO_DMA_MINALIGN sizeof(s64)
>> +#else
>>  #define IIO_DMA_MINALIGN ARCH_DMA_MINALIGN
>> +#endif
> 
> Did you actually test this?
> You can't use sizeof() in a pre-processor conditional.

Pretty sure I did, but maybe I was asleep at the wheel. Anyway, this got caught
by a build bot and I realized the mistake.

> 
> 	David
> 
>> +
>>  struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv);
>>  
>>  /* The information at the returned address is guaranteed to be cacheline aligned */
>>
> 

[PATCH v5 2/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros

[David Lechner]

Add new macros 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.

To avoid double __align() attributes in cases where we also need DMA
alignment, add a 2nd variant IIO_DECLARE_DMA_BUFFER_WITH_TS().

Signed-off-by: David Lechner <dlechner@baylibre.com>
---

v5 changes:
* Revert back to __align(IIO_DMA_MINALIGN) as IIO_DMA_MINALIGN now has
  a minimum of 8 bytes.

v4 changes:
* Drop the static_asserts(). Some 32-bit arches were triggering one, so
  we had to address the problem instead of hoping that it didn't exist.
  The other made a multi-statement macro, which isn't the best practice
  and didn't have a way to make a really helpful error message. The
  condition we were trying to catch is still caught by -Wvla.
* Changed __align(IIO_DMA_MINALIGN) to __align(MAX(IIO_DMA_MINALIGN,
  sizeof(s64))). As the build-bots found, there are some 32-bit arches
  where IIO_DMA_MINALIGN is 4, but we still need 8-byte alignment for
  the timestamp.

v3 changes:
* Use leading double-underscore for "private" macro to match "private"
  functions that do the same.
* Use static_assert() from linux/build_bug.h instead of _Static_assert()
* Fix incorrectly using sizeof(IIO_DMA_MINALIGN).
* Add check that count argument is constant. (Note, I didn't include a
  message in this static assert because it already gives a reasonable
  message.)

/home/david/work/bl/linux/drivers/iio/accel/sca3300.c:482:51: error: expression in static assertion is not constant
  482 |         IIO_DECLARE_BUFFER_WITH_TS(s16, channels, val);
      |                                                   ^~~

v2 changes:
* Add 2nd macro for DMA alignment
---
 include/linux/iio/iio.h | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 7e1e3739328d103262071bd34ba5f6631163c122..703ade71483803c0a335343d5a7ecaefd8bc93b6 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>
@@ -787,6 +788,37 @@ static inline void *iio_device_get_drvdata(const struct iio_dev *indio_dev)
 #define IIO_DMA_MINALIGN ARCH_DMA_MINALIGN
 #endif
 
+#define __IIO_DECLARE_BUFFER_WITH_TS(type, name, count) \
+	type name[ALIGN((count), sizeof(s64) / sizeof(type)) + sizeof(s64) / sizeof(type)]
+
+/**
+ * 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) \
+	__IIO_DECLARE_BUFFER_WITH_TS(type, name, count) __aligned(sizeof(s64))
+
+/**
+ * IIO_DECLARE_DMA_BUFFER_WITH_TS() - Declare a DMA-aligned buffer with timestamp
+ * @type: element type of the buffer
+ * @name: identifier name of the buffer
+ * @count: number of elements in the buffer
+ *
+ * Same as IIO_DECLARE_BUFFER_WITH_TS(), but is uses __aligned(IIO_DMA_MINALIGN)
+ * to ensure that the buffer doesn't share cachelines with anything that comes
+ * before it in a struct. This should not be used for stack-allocated buffers
+ * as stack memory cannot generally be used for DMA.
+ */
+#define IIO_DECLARE_DMA_BUFFER_WITH_TS(type, name, count) \
+	__IIO_DECLARE_BUFFER_WITH_TS(type, name, count) __aligned(IIO_DMA_MINALIGN)
+
 struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv);
 
 /* The information at the returned address is guaranteed to be cacheline aligned */

-- 
2.43.0

[PATCH v5 3/7] iio: adc: ad4695: use IIO_DECLARE_DMA_BUFFER_WITH_TS

[David Lechner]

Use IIO_DECLARE_DMA_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.

Reviewed-by: Trevor Gamblin <tgamblin@baylibre.com>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad4695.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index 0c633d43e480d5404074e9fa35f1d330b448f0a2..992abf6c63b51dee222caf624e172455fb9b9900 100644
--- a/drivers/iio/adc/ad4695.c
+++ b/drivers/iio/adc/ad4695.c
@@ -160,8 +160,7 @@ struct ad4695_state {
 	struct spi_transfer buf_read_xfer[AD4695_MAX_CHANNELS * 2 + 3];
 	struct spi_message buf_read_msg;
 	/* Raw conversion data received. */
-	u16 buf[ALIGN((AD4695_MAX_CHANNELS + 1) * sizeof(u16),
-		      sizeof(s64)) + sizeof(s64)] __aligned(IIO_DMA_MINALIGN);
+	IIO_DECLARE_DMA_BUFFER_WITH_TS(u16, buf, AD4695_MAX_CHANNELS + 1);
 	u16 raw_data;
 	/* Commands to send for single conversion. */
 	u16 cnv_cmd;

-- 
2.43.0

[PATCH v5 4/7] iio: adc: ad4695: rename AD4695_MAX_VIN_CHANNELS

[David Lechner]

Rename AD4695_MAX_CHANNELS to AD4695_MAX_VIN_CHANNELS. It has been a
point of confusion that this macro is only the voltage input channels
and not all channels.

Reviewed-by: Trevor Gamblin <tgamblin@baylibre.com>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad4695.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index 992abf6c63b51dee222caf624e172455fb9b9900..cda419638d9a88debb3501d05a513b17a4ecde95 100644
--- a/drivers/iio/adc/ad4695.c
+++ b/drivers/iio/adc/ad4695.c
@@ -105,7 +105,7 @@
 #define AD4695_REG_ACCESS_SCLK_HZ	(10 * MEGA)
 
 /* Max number of voltage input channels. */
-#define AD4695_MAX_CHANNELS		16
+#define AD4695_MAX_VIN_CHANNELS		16
 
 enum ad4695_in_pair {
 	AD4695_IN_PAIR_REFGND,
@@ -143,8 +143,8 @@ struct ad4695_state {
 	/* offload also requires separate gpio to manually control CNV */
 	struct gpio_desc *cnv_gpio;
 	/* voltages channels plus temperature and timestamp */
-	struct iio_chan_spec iio_chan[AD4695_MAX_CHANNELS + 2];
-	struct ad4695_channel_config channels_cfg[AD4695_MAX_CHANNELS];
+	struct iio_chan_spec iio_chan[AD4695_MAX_VIN_CHANNELS + 2];
+	struct ad4695_channel_config channels_cfg[AD4695_MAX_VIN_CHANNELS];
 	const struct ad4695_chip_info *chip_info;
 	int sample_freq_range[3];
 	/* Reference voltage. */
@@ -157,10 +157,10 @@ struct ad4695_state {
 	 * to control CS and add a delay between the last SCLK and next
 	 * CNV rising edges.
 	 */
-	struct spi_transfer buf_read_xfer[AD4695_MAX_CHANNELS * 2 + 3];
+	struct spi_transfer buf_read_xfer[AD4695_MAX_VIN_CHANNELS * 2 + 3];
 	struct spi_message buf_read_msg;
 	/* Raw conversion data received. */
-	IIO_DECLARE_DMA_BUFFER_WITH_TS(u16, buf, AD4695_MAX_CHANNELS + 1);
+	IIO_DECLARE_DMA_BUFFER_WITH_TS(u16, buf, AD4695_MAX_VIN_CHANNELS + 1);
 	u16 raw_data;
 	/* Commands to send for single conversion. */
 	u16 cnv_cmd;

-- 
2.43.0

[PATCH v5 5/7] iio: adc: ad7380: use IIO_DECLARE_DMA_BUFFER_WITH_TS

[David Lechner]

Use IIO_DECLARE_DMA_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>
---

As discussed in v1, this one stays u8 because it is used with both 16
and 32-bit word sizes.

v3 changes:
* Use IIO_DECLARE_DMA_BUFFER_WITH_TS() and drop __align()

v2 changes:
* None (but I messed up and there was supposed to be a change).
---
 drivers/iio/adc/ad7380.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index f93e6c67766aa89b18c1a7dec02ae8912f65261c..ed5e43c8c84cfcc9c4ce1866659a05787c1d6f5e 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -909,8 +909,7 @@ 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_DMA_BUFFER_WITH_TS(u8, scan_data, MAX_NUM_CHANNELS * sizeof(u32));
 	/* buffers for reading/writing registers */
 	u16 tx;
 	u16 rx;

-- 
2.43.0

[PATCH v5 6/7] iio: accel: sca3300: 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>
---
This is an alternative to [1]. Also, this serves as a test to see if we
can get a rule of thumb to decide how much is too much to put on the
stack vs. needing to put the buffer in a static struct. SCA3300_SCAN_MAX
is 7, so this add a bit over 64 bytes to the stack, make the stack now
roughly double what it was before.

[1]: https://lore.kernel.org/linux-iio/20250418-iio-prefer-aligned_s64-timestamp-v1-1-4c6080710516@baylibre.com/
---
 drivers/iio/accel/sca3300.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
index 1132bbaba75bcca525fac2f3e19f63546380fd4f..67416a406e2f43e4e417210410904d44c93111d2 100644
--- a/drivers/iio/accel/sca3300.c
+++ b/drivers/iio/accel/sca3300.c
@@ -58,15 +58,6 @@ enum sca3300_scan_indexes {
 	SCA3300_SCAN_MAX
 };
 
-/*
- * Buffer size max case:
- * Three accel channels, two bytes per channel.
- * Temperature channel, two bytes.
- * Three incli channels, two bytes per channel.
- * Timestamp channel, eight bytes.
- */
-#define SCA3300_MAX_BUFFER_SIZE (ALIGN(sizeof(s16) * SCA3300_SCAN_MAX, sizeof(s64)) + sizeof(s64))
-
 #define SCA3300_ACCEL_CHANNEL(index, reg, axis) {			\
 	.type = IIO_ACCEL,						\
 	.address = reg,							\
@@ -193,9 +184,6 @@ struct sca3300_chip_info {
  * @spi: SPI device structure
  * @lock: Data buffer lock
  * @chip: Sensor chip specific information
- * @buffer: Triggered buffer:
- *          -SCA3300: 4 channel 16-bit data + 64-bit timestamp
- *          -SCL3300: 7 channel 16-bit data + 64-bit timestamp
  * @txbuf: Transmit buffer
  * @rxbuf: Receive buffer
  */
@@ -203,7 +191,6 @@ struct sca3300_data {
 	struct spi_device *spi;
 	struct mutex lock;
 	const struct sca3300_chip_info *chip;
-	u8 buffer[SCA3300_MAX_BUFFER_SIZE] __aligned(sizeof(s64));
 	u8 txbuf[4] __aligned(IIO_DMA_MINALIGN);
 	u8 rxbuf[4];
 };
@@ -492,7 +479,7 @@ static irqreturn_t sca3300_trigger_handler(int irq, void *p)
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct sca3300_data *data = iio_priv(indio_dev);
 	int bit, ret, val, i = 0;
-	s16 *channels = (s16 *)data->buffer;
+	IIO_DECLARE_BUFFER_WITH_TS(s16, channels, SCA3300_SCAN_MAX);
 
 	iio_for_each_active_channel(indio_dev, bit) {
 		ret = sca3300_read_reg(data, indio_dev->channels[bit].address, &val);
@@ -505,8 +492,7 @@ static irqreturn_t sca3300_trigger_handler(int irq, void *p)
 		channels[i++] = val;
 	}
 
-	iio_push_to_buffers_with_ts(indio_dev, data->buffer,
-				    sizeof(data->buffer),
+	iio_push_to_buffers_with_ts(indio_dev, channels, sizeof(channels),
 				    iio_get_time_ns(indio_dev));
 out:
 	iio_trigger_notify_done(indio_dev->trig);

-- 
2.43.0

Re: [PATCH v5 6/7] iio: accel: sca3300: use IIO_DECLARE_BUFFER_WITH_TS

[Nuno Sá]

On Mon, 2025-05-05 at 11:31 -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.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> This is an alternative to [1]. Also, this serves as a test to see if we
> can get a rule of thumb to decide how much is too much to put on the
> stack vs. needing to put the buffer in a static struct. SCA3300_SCAN_MAX
> is 7, so this add a bit over 64 bytes to the stack, make the stack now
> roughly double what it was before.

IMHO, I think that for this buffer size, having it on the stack is very reasonable.

> 
> [1]:
> https://lore.kernel.org/linux-iio/20250418-iio-prefer-aligned_s64-timestamp-v1-1-4c6080710516@baylibre.com/
> ---
>  drivers/iio/accel/sca3300.c | 18 ++----------------
>  1 file changed, 2 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
> index
> 1132bbaba75bcca525fac2f3e19f63546380fd4f..67416a406e2f43e4e417210410904d44c93111d2
> 100644
> --- a/drivers/iio/accel/sca3300.c
> +++ b/drivers/iio/accel/sca3300.c
> @@ -58,15 +58,6 @@ enum sca3300_scan_indexes {
>  	SCA3300_SCAN_MAX
>  };
>  
> -/*
> - * Buffer size max case:
> - * Three accel channels, two bytes per channel.
> - * Temperature channel, two bytes.
> - * Three incli channels, two bytes per channel.
> - * Timestamp channel, eight bytes.
> - */
> -#define SCA3300_MAX_BUFFER_SIZE (ALIGN(sizeof(s16) * SCA3300_SCAN_MAX,
> sizeof(s64)) + sizeof(s64))
> -
>  #define SCA3300_ACCEL_CHANNEL(index, reg, axis) {			\
>  	.type = IIO_ACCEL,						\
>  	.address = reg,							\
> @@ -193,9 +184,6 @@ struct sca3300_chip_info {
>   * @spi: SPI device structure
>   * @lock: Data buffer lock
>   * @chip: Sensor chip specific information
> - * @buffer: Triggered buffer:
> - *          -SCA3300: 4 channel 16-bit data + 64-bit timestamp
> - *          -SCL3300: 7 channel 16-bit data + 64-bit timestamp
>   * @txbuf: Transmit buffer
>   * @rxbuf: Receive buffer
>   */
> @@ -203,7 +191,6 @@ struct sca3300_data {
>  	struct spi_device *spi;
>  	struct mutex lock;
>  	const struct sca3300_chip_info *chip;
> -	u8 buffer[SCA3300_MAX_BUFFER_SIZE] __aligned(sizeof(s64));
>  	u8 txbuf[4] __aligned(IIO_DMA_MINALIGN);
>  	u8 rxbuf[4];
>  };
> @@ -492,7 +479,7 @@ static irqreturn_t sca3300_trigger_handler(int irq, void *p)
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct sca3300_data *data = iio_priv(indio_dev);
>  	int bit, ret, val, i = 0;
> -	s16 *channels = (s16 *)data->buffer;
> +	IIO_DECLARE_BUFFER_WITH_TS(s16, channels, SCA3300_SCAN_MAX);

On top of what you mentioned I like this change. The pattern you're removing is the
typical one for getting into unaligned accesses.

>  
>  	iio_for_each_active_channel(indio_dev, bit) {
>  		ret = sca3300_read_reg(data, indio_dev->channels[bit].address,
> &val);
> @@ -505,8 +492,7 @@ static irqreturn_t sca3300_trigger_handler(int irq, void *p)
>  		channels[i++] = val;
>  	}
>  
> -	iio_push_to_buffers_with_ts(indio_dev, data->buffer,
> -				    sizeof(data->buffer),
> +	iio_push_to_buffers_with_ts(indio_dev, channels, sizeof(channels),
>  				    iio_get_time_ns(indio_dev));
>  out:
>  	iio_trigger_notify_done(indio_dev->trig);
> 

[PATCH v5 7/7] iio: adc: at91-sama5d2: 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>
---

v5 changes:
* Fixed missing space at end of comment.

This is an alternative to [1].

[1]: https://lore.kernel.org/linux-iio/20250418-iio-prefer-aligned_s64-timestamp-v1-2-4c6080710516@baylibre.com/
---
 drivers/iio/adc/at91-sama5d2_adc.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 414610afcb2c4128a63cf76767803c32cb01ac5e..c3450246730e08cdacc975ed19f46044dc76848f 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -586,15 +586,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;
@@ -616,8 +607,8 @@ struct at91_adc_state {
 	struct at91_adc_temp		temp_st;
 	struct iio_dev			*indio_dev;
 	struct device			*dev;
-	/* Ensure naturally aligned timestamp */
-	u16				buffer[AT91_BUFFER_MAX_HWORDS] __aligned(8);
+	/* We assume 32 channels for now, has to be increased if needed. */
+	IIO_DECLARE_BUFFER_WITH_TS(u16, buffer, 32);
 	/*
 	 * lock to prevent concurrent 'single conversion' requests through
 	 * sysfs.

-- 
2.43.0

Re: [PATCH v5 0/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS

[Jonathan Cameron]

On Mon, 05 May 2025 11:31:41 -0500
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. To
> handle these, we 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 new macros 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.

Looks good to me.  I'll leave it on list for a day or two though for
everyone else to take a final look!
> 
> ---
> Changes in v5:
> - Add new patch to set minimum alignment to 8 for IIO_DMA_MINALIGN.
> - Adjust IIO_DECLARE_DMA_BUFFER_WITH_TS() macro for above change.
> - Drop one ad4695 patch that was already applied.
> - Link to v4: https://lore.kernel.org/r/20250428-iio-introduce-iio_declare_buffer_with_ts-v4-0-6f7f6126f1cb@baylibre.com
> 
> Changes in v4:
> - Dropped static_assert()s from the first patch.
> - Handle case when IIO_DMA_MINALIGN < sizeof(timestamp).
> - Added one more patch for ad4695 to rename a confusing macro.
> - Link to v3: https://lore.kernel.org/r/20250425-iio-introduce-iio_declare_buffer_with_ts-v3-0-f12df1bff248@baylibre.com
> 
> Changes in v3:
> - Fixed a few mistakes, style issues and incorporate other feedback (see
>   individual commit message changelogs for details).
> - Link to v2: https://lore.kernel.org/r/20250422-iio-introduce-iio_declare_buffer_with_ts-v2-0-3fd36475c706@baylibre.com
> 
> Changes in v2:
> - Add 2nd macro for case where we need DMA alignment.
> - Add new patch for ad4695 to convert buffer from u8 to u16 before
>   making use of the new macro.
> - Drop the bmp280 patch since it was determined to have a better
>   alternative not using these macros.
> - Add a few more examples to show the non-DMA case, both in a struct and
>   stack allocated.
> - Link to v1: https://lore.kernel.org/r/20250418-iio-introduce-iio_declare_buffer_with_ts-v1-0-ee0c62a33a0f@baylibre.com
> 
> ---
> David Lechner (7):
>       iio: make IIO_DMA_MINALIGN minimum of 8 bytes
>       iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros
>       iio: adc: ad4695: use IIO_DECLARE_DMA_BUFFER_WITH_TS
>       iio: adc: ad4695: rename AD4695_MAX_VIN_CHANNELS
>       iio: adc: ad7380: use IIO_DECLARE_DMA_BUFFER_WITH_TS
>       iio: accel: sca3300: use IIO_DECLARE_BUFFER_WITH_TS
>       iio: adc: at91-sama5d2: use IIO_DECLARE_BUFFER_WITH_TS
> 
>  drivers/iio/accel/sca3300.c        | 18 ++--------------
>  drivers/iio/adc/ad4695.c           | 11 +++++-----
>  drivers/iio/adc/ad7380.c           |  3 +--
>  drivers/iio/adc/at91-sama5d2_adc.c | 13 ++----------
>  include/linux/iio/iio.h            | 42 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 52 insertions(+), 35 deletions(-)
> ---
> base-commit: 7e9a82ab5b861d3c33c99a22c1245a5b262ee502
> change-id: 20250418-iio-introduce-iio_declare_buffer_with_ts-2f8773f7dad6
> 
> Best regards,

Re: [PATCH v5 0/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS

[Nuno Sá]

On Mon, 2025-05-05 at 11:31 -0500, David Lechner 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. To
> handle these, we 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 new macros 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.
> 
> ---

LGTM

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

> Changes in v5:
> - Add new patch to set minimum alignment to 8 for IIO_DMA_MINALIGN.
> - Adjust IIO_DECLARE_DMA_BUFFER_WITH_TS() macro for above change.
> - Drop one ad4695 patch that was already applied.
> - Link to v4:
> https://lore.kernel.org/r/20250428-iio-introduce-iio_declare_buffer_with_ts-v4-0-6f7f6126f1cb@baylibre.com
> 
> Changes in v4:
> - Dropped static_assert()s from the first patch.
> - Handle case when IIO_DMA_MINALIGN < sizeof(timestamp).
> - Added one more patch for ad4695 to rename a confusing macro.
> - Link to v3:
> https://lore.kernel.org/r/20250425-iio-introduce-iio_declare_buffer_with_ts-v3-0-f12df1bff248@baylibre.com
> 
> Changes in v3:
> - Fixed a few mistakes, style issues and incorporate other feedback (see
>   individual commit message changelogs for details).
> - Link to v2:
> https://lore.kernel.org/r/20250422-iio-introduce-iio_declare_buffer_with_ts-v2-0-3fd36475c706@baylibre.com
> 
> Changes in v2:
> - Add 2nd macro for case where we need DMA alignment.
> - Add new patch for ad4695 to convert buffer from u8 to u16 before
>   making use of the new macro.
> - Drop the bmp280 patch since it was determined to have a better
>   alternative not using these macros.
> - Add a few more examples to show the non-DMA case, both in a struct and
>   stack allocated.
> - Link to v1:
> https://lore.kernel.org/r/20250418-iio-introduce-iio_declare_buffer_with_ts-v1-0-ee0c62a33a0f@baylibre.com
> 
> ---
> David Lechner (7):
>       iio: make IIO_DMA_MINALIGN minimum of 8 bytes
>       iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros
>       iio: adc: ad4695: use IIO_DECLARE_DMA_BUFFER_WITH_TS
>       iio: adc: ad4695: rename AD4695_MAX_VIN_CHANNELS
>       iio: adc: ad7380: use IIO_DECLARE_DMA_BUFFER_WITH_TS
>       iio: accel: sca3300: use IIO_DECLARE_BUFFER_WITH_TS
>       iio: adc: at91-sama5d2: use IIO_DECLARE_BUFFER_WITH_TS
> 
>  drivers/iio/accel/sca3300.c        | 18 ++--------------
>  drivers/iio/adc/ad4695.c           | 11 +++++-----
>  drivers/iio/adc/ad7380.c           |  3 +--
>  drivers/iio/adc/at91-sama5d2_adc.c | 13 ++----------
>  include/linux/iio/iio.h            | 42 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 52 insertions(+), 35 deletions(-)
> ---
> base-commit: 7e9a82ab5b861d3c33c99a22c1245a5b262ee502
> change-id: 20250418-iio-introduce-iio_declare_buffer_with_ts-2f8773f7dad6
> 
> Best regards,

Re: [PATCH v5 0/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS

[Jonathan Cameron]

On Wed, 07 May 2025 07:39:07 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Mon, 2025-05-05 at 11:31 -0500, David Lechner 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. To
> > handle these, we 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 new macros 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.
> > 
> > ---  
> 
> LGTM
> 
> Reviewed-by: Nuno Sá <nuno.sa@analog.com>

Thanks,

Still time for additional comments, but given where we are in the
cycle I'm keen to get this at least undergoing build tests.

Applied to the togreg branch of iio.git and pushed out as testing for
those build tests.

Thanks,

Jonathan

> 
> > Changes in v5:
> > - Add new patch to set minimum alignment to 8 for IIO_DMA_MINALIGN.
> > - Adjust IIO_DECLARE_DMA_BUFFER_WITH_TS() macro for above change.
> > - Drop one ad4695 patch that was already applied.
> > - Link to v4:
> > https://lore.kernel.org/r/20250428-iio-introduce-iio_declare_buffer_with_ts-v4-0-6f7f6126f1cb@baylibre.com
> > 
> > Changes in v4:
> > - Dropped static_assert()s from the first patch.
> > - Handle case when IIO_DMA_MINALIGN < sizeof(timestamp).
> > - Added one more patch for ad4695 to rename a confusing macro.
> > - Link to v3:
> > https://lore.kernel.org/r/20250425-iio-introduce-iio_declare_buffer_with_ts-v3-0-f12df1bff248@baylibre.com
> > 
> > Changes in v3:
> > - Fixed a few mistakes, style issues and incorporate other feedback (see
> >   individual commit message changelogs for details).
> > - Link to v2:
> > https://lore.kernel.org/r/20250422-iio-introduce-iio_declare_buffer_with_ts-v2-0-3fd36475c706@baylibre.com
> > 
> > Changes in v2:
> > - Add 2nd macro for case where we need DMA alignment.
> > - Add new patch for ad4695 to convert buffer from u8 to u16 before
> >   making use of the new macro.
> > - Drop the bmp280 patch since it was determined to have a better
> >   alternative not using these macros.
> > - Add a few more examples to show the non-DMA case, both in a struct and
> >   stack allocated.
> > - Link to v1:
> > https://lore.kernel.org/r/20250418-iio-introduce-iio_declare_buffer_with_ts-v1-0-ee0c62a33a0f@baylibre.com
> > 
> > ---
> > David Lechner (7):
> >       iio: make IIO_DMA_MINALIGN minimum of 8 bytes
> >       iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros
> >       iio: adc: ad4695: use IIO_DECLARE_DMA_BUFFER_WITH_TS
> >       iio: adc: ad4695: rename AD4695_MAX_VIN_CHANNELS
> >       iio: adc: ad7380: use IIO_DECLARE_DMA_BUFFER_WITH_TS
> >       iio: accel: sca3300: use IIO_DECLARE_BUFFER_WITH_TS
> >       iio: adc: at91-sama5d2: use IIO_DECLARE_BUFFER_WITH_TS
> > 
> >  drivers/iio/accel/sca3300.c        | 18 ++--------------
> >  drivers/iio/adc/ad4695.c           | 11 +++++-----
> >  drivers/iio/adc/ad7380.c           |  3 +--
> >  drivers/iio/adc/at91-sama5d2_adc.c | 13 ++----------
> >  include/linux/iio/iio.h            | 42 ++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 52 insertions(+), 35 deletions(-)
> > ---
> > base-commit: 7e9a82ab5b861d3c33c99a22c1245a5b262ee502
> > change-id: 20250418-iio-introduce-iio_declare_buffer_with_ts-2f8773f7dad6
> > 
> > Best regards,  
>