public inbox for linux-arm-kernel@lists.infradead.org
 help / color / mirror / Atom feed
* [PATCH v3 0/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS
@ 2025-04-25 21:08 David Lechner
  2025-04-25 21:08 ` [PATCH v3 1/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros David Lechner
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: David Lechner @ 2025-04-25 21:08 UTC (permalink / raw)
  To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
	Lars-Peter Clausen, Michael Hennerich, Eugen Hristev,
	Nicolas Ferre, Alexandre Belloni, Claudiu Beznea
  Cc: linux-iio, linux-kernel, linux-arm-kernel

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 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 (6):
      iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros
      iio: adc: ad4695: use u16 for buffer elements
      iio: adc: ad4695: use IIO_DECLARE_DMA_BUFFER_WITH_TS
      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           |  8 ++------
 drivers/iio/adc/ad7380.c           |  3 +--
 drivers/iio/adc/at91-sama5d2_adc.c | 13 ++-----------
 include/linux/iio/iio.h            | 38 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 45 insertions(+), 35 deletions(-)
---
base-commit: aff301f37e220970c2f301b5c65a8bfedf52058e
change-id: 20250418-iio-introduce-iio_declare_buffer_with_ts-2f8773f7dad6

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



^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3 1/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros
  2025-04-25 21:08 [PATCH v3 0/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS David Lechner
@ 2025-04-25 21:08 ` David Lechner
  2025-04-26 11:35   ` Jonathan Cameron
                     ` (2 more replies)
  2025-04-25 21:08 ` [PATCH v3 2/6] iio: adc: ad4695: use u16 for buffer elements David Lechner
                   ` (4 subsequent siblings)
  5 siblings, 3 replies; 13+ messages in thread
From: David Lechner @ 2025-04-25 21:08 UTC (permalink / raw)
  To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
	Lars-Peter Clausen, Michael Hennerich, Eugen Hristev,
	Nicolas Ferre, Alexandre Belloni, Claudiu Beznea
  Cc: linux-iio, linux-kernel, linux-arm-kernel

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

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 | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 638cf2420fbd85cf2924d09d061df601d1d4bb2a..1115b219271b76792539931edc404a67549bd8b1 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -7,6 +7,8 @@
 #ifndef _INDUSTRIAL_IO_H_
 #define _INDUSTRIAL_IO_H_
 
+#include <linux/align.h>
+#include <linux/build_bug.h>
 #include <linux/device.h>
 #include <linux/cdev.h>
 #include <linux/compiler_types.h>
@@ -777,6 +779,42 @@ static inline void *iio_device_get_drvdata(const struct iio_dev *indio_dev)
  * them safe for use with non-coherent DMA.
  */
 #define IIO_DMA_MINALIGN ARCH_DMA_MINALIGN
+
+#define __IIO_DECLARE_BUFFER_WITH_TS(type, name, count) \
+	static_assert(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)
+
+static_assert(IIO_DMA_MINALIGN % sizeof(s64) == 0,
+	"macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment");
+
 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



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 2/6] iio: adc: ad4695: use u16 for buffer elements
  2025-04-25 21:08 [PATCH v3 0/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS David Lechner
  2025-04-25 21:08 ` [PATCH v3 1/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros David Lechner
@ 2025-04-25 21:08 ` David Lechner
  2025-04-25 21:08 ` [PATCH v3 3/6] iio: adc: ad4695: use IIO_DECLARE_DMA_BUFFER_WITH_TS David Lechner
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: David Lechner @ 2025-04-25 21:08 UTC (permalink / raw)
  To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
	Lars-Peter Clausen, Michael Hennerich, Eugen Hristev,
	Nicolas Ferre, Alexandre Belloni, Claudiu Beznea
  Cc: linux-iio, linux-kernel, linux-arm-kernel

Change the type of the buffer elements to u16 since we currently only
support 16-bit word size. The code was originally written to also allow
for 32-bit word size when oversampling is enabled, but so far,
oversampling is only implemented when using SPI offload and therefore
doesn't use this buffer.

AD4695_MAX_CHANNEL_SIZE macro is dropped since it no longer adds any
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 | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index 68c6625db0d75f4cade7cb029e94191118dbcaa0..0c633d43e480d5404074e9fa35f1d330b448f0a2 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);
+	u16 buf[ALIGN((AD4695_MAX_CHANNELS + 1) * sizeof(u16),
+		      sizeof(s64)) + sizeof(s64)] __aligned(IIO_DMA_MINALIGN);
 	u16 raw_data;
 	/* Commands to send for single conversion. */
 	u16 cnv_cmd;
@@ -660,9 +658,8 @@ static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
 	iio_for_each_active_channel(indio_dev, bit) {
 		xfer = &st->buf_read_xfer[num_xfer];
 		xfer->bits_per_word = 16;
-		xfer->rx_buf = &st->buf[rx_buf_offset];
+		xfer->rx_buf = &st->buf[rx_buf_offset++];
 		xfer->len = 2;
-		rx_buf_offset += xfer->len;
 
 		if (bit == temp_chan_bit) {
 			temp_en = 1;

-- 
2.43.0



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 3/6] iio: adc: ad4695: use IIO_DECLARE_DMA_BUFFER_WITH_TS
  2025-04-25 21:08 [PATCH v3 0/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS David Lechner
  2025-04-25 21:08 ` [PATCH v3 1/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros David Lechner
  2025-04-25 21:08 ` [PATCH v3 2/6] iio: adc: ad4695: use u16 for buffer elements David Lechner
@ 2025-04-25 21:08 ` David Lechner
  2025-04-26 11:24   ` Jonathan Cameron
  2025-04-25 21:08 ` [PATCH v3 4/6] iio: adc: ad7380: " David Lechner
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: David Lechner @ 2025-04-25 21:08 UTC (permalink / raw)
  To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
	Lars-Peter Clausen, Michael Hennerich, Eugen Hristev,
	Nicolas Ferre, Alexandre Belloni, Claudiu Beznea
  Cc: linux-iio, linux-kernel, linux-arm-kernel

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



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 4/6] iio: adc: ad7380: use IIO_DECLARE_DMA_BUFFER_WITH_TS
  2025-04-25 21:08 [PATCH v3 0/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS David Lechner
                   ` (2 preceding siblings ...)
  2025-04-25 21:08 ` [PATCH v3 3/6] iio: adc: ad4695: use IIO_DECLARE_DMA_BUFFER_WITH_TS David Lechner
@ 2025-04-25 21:08 ` David Lechner
  2025-04-25 21:08 ` [PATCH v3 5/6] iio: accel: sca3300: use IIO_DECLARE_BUFFER_WITH_TS David Lechner
  2025-04-25 21:08 ` [PATCH v3 6/6] iio: adc: at91-sama5d2: " David Lechner
  5 siblings, 0 replies; 13+ messages in thread
From: David Lechner @ 2025-04-25 21:08 UTC (permalink / raw)
  To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
	Lars-Peter Clausen, Michael Hennerich, Eugen Hristev,
	Nicolas Ferre, Alexandre Belloni, Claudiu Beznea
  Cc: linux-iio, linux-kernel, linux-arm-kernel

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



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 5/6] iio: accel: sca3300: use IIO_DECLARE_BUFFER_WITH_TS
  2025-04-25 21:08 [PATCH v3 0/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS David Lechner
                   ` (3 preceding siblings ...)
  2025-04-25 21:08 ` [PATCH v3 4/6] iio: adc: ad7380: " David Lechner
@ 2025-04-25 21:08 ` David Lechner
  2025-04-25 21:08 ` [PATCH v3 6/6] iio: adc: at91-sama5d2: " David Lechner
  5 siblings, 0 replies; 13+ messages in thread
From: David Lechner @ 2025-04-25 21:08 UTC (permalink / raw)
  To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
	Lars-Peter Clausen, Michael Hennerich, Eugen Hristev,
	Nicolas Ferre, Alexandre Belloni, Claudiu Beznea
  Cc: linux-iio, linux-kernel, linux-arm-kernel

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



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 6/6] iio: adc: at91-sama5d2: use IIO_DECLARE_BUFFER_WITH_TS
  2025-04-25 21:08 [PATCH v3 0/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS David Lechner
                   ` (4 preceding siblings ...)
  2025-04-25 21:08 ` [PATCH v3 5/6] iio: accel: sca3300: use IIO_DECLARE_BUFFER_WITH_TS David Lechner
@ 2025-04-25 21:08 ` David Lechner
  5 siblings, 0 replies; 13+ messages in thread
From: David Lechner @ 2025-04-25 21:08 UTC (permalink / raw)
  To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
	Lars-Peter Clausen, Michael Hennerich, Eugen Hristev,
	Nicolas Ferre, Alexandre Belloni, Claudiu Beznea
  Cc: linux-iio, linux-kernel, linux-arm-kernel

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].

[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..4ebaeb41aa4568e2461506471af0540af9d1a041 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



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 3/6] iio: adc: ad4695: use IIO_DECLARE_DMA_BUFFER_WITH_TS
  2025-04-25 21:08 ` [PATCH v3 3/6] iio: adc: ad4695: use IIO_DECLARE_DMA_BUFFER_WITH_TS David Lechner
@ 2025-04-26 11:24   ` Jonathan Cameron
  0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2025-04-26 11:24 UTC (permalink / raw)
  To: David Lechner
  Cc: Nuno Sá, Andy Shevchenko, Lars-Peter Clausen,
	Michael Hennerich, Eugen Hristev, Nicolas Ferre,
	Alexandre Belloni, Claudiu Beznea, linux-iio, linux-kernel,
	linux-arm-kernel

On Fri, 25 Apr 2025 16:08:45 -0500
David Lechner <dlechner@baylibre.com> wrote:

> 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>
> ---
>  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);

As a follow up, maybe we can rename that AD4695_MAX_CHANNELS to
AD4695_MAX_ADC_CHANNELS so I don't wonder why there is a + 1?

>  	u16 raw_data;
>  	/* Commands to send for single conversion. */
>  	u16 cnv_cmd;
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros
  2025-04-25 21:08 ` [PATCH v3 1/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros David Lechner
@ 2025-04-26 11:35   ` Jonathan Cameron
  2025-04-26 22:34     ` David Lechner
  2025-04-26 19:15   ` kernel test robot
  2025-04-27  1:54   ` kernel test robot
  2 siblings, 1 reply; 13+ messages in thread
From: Jonathan Cameron @ 2025-04-26 11:35 UTC (permalink / raw)
  To: David Lechner
  Cc: Nuno Sá, Andy Shevchenko, Lars-Peter Clausen,
	Michael Hennerich, Eugen Hristev, Nicolas Ferre,
	Alexandre Belloni, Claudiu Beznea, linux-iio, linux-kernel,
	linux-arm-kernel

On Fri, 25 Apr 2025 16:08:43 -0500
David Lechner <dlechner@baylibre.com> wrote:

> 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().
> 
Generally good.  A few little things though...

> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> 
> 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 | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 638cf2420fbd85cf2924d09d061df601d1d4bb2a..1115b219271b76792539931edc404a67549bd8b1 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -7,6 +7,8 @@
>  #ifndef _INDUSTRIAL_IO_H_
>  #define _INDUSTRIAL_IO_H_
>  
> +#include <linux/align.h>
> +#include <linux/build_bug.h>
>  #include <linux/device.h>
>  #include <linux/cdev.h>
>  #include <linux/compiler_types.h>
> @@ -777,6 +779,42 @@ static inline void *iio_device_get_drvdata(const struct iio_dev *indio_dev)
>   * them safe for use with non-coherent DMA.
>   */
>  #define IIO_DMA_MINALIGN ARCH_DMA_MINALIGN
> +
> +#define __IIO_DECLARE_BUFFER_WITH_TS(type, name, count) \
> +	static_assert(count); \

Why do we care if count is 0?  Or is intent to check if is constant?
If the thought is we don't care either way about 0 (as rather nonsensical)
and this will fail to compile if not constant, then perhaps a comment would
avoid future confusion?

> +	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)
> +
> +static_assert(IIO_DMA_MINALIGN % sizeof(s64) == 0,
That message isn't super helpful if seen in a compile log as we aren't reading the code here
"IIO_DECLARE_DMA_BUFFER_WITH_TS() assumes that ...

> +	"macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment");
> +
>  struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv);
>  
>  /* The information at the returned address is guaranteed to be cacheline aligned */
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros
  2025-04-25 21:08 ` [PATCH v3 1/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros David Lechner
  2025-04-26 11:35   ` Jonathan Cameron
@ 2025-04-26 19:15   ` kernel test robot
  2025-04-27  1:54   ` kernel test robot
  2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2025-04-26 19:15 UTC (permalink / raw)
  To: David Lechner, Jonathan Cameron, Nuno Sá, Andy Shevchenko,
	Lars-Peter Clausen, Michael Hennerich, Eugen Hristev,
	Nicolas Ferre, Alexandre Belloni, Claudiu Beznea
  Cc: oe-kbuild-all, linux-iio, linux-kernel, linux-arm-kernel

Hi David,

kernel test robot noticed the following build errors:

[auto build test ERROR on aff301f37e220970c2f301b5c65a8bfedf52058e]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Lechner/iio-introduce-IIO_DECLARE_BUFFER_WITH_TS-macros/20250426-051240
base:   aff301f37e220970c2f301b5c65a8bfedf52058e
patch link:    https://lore.kernel.org/r/20250425-iio-introduce-iio_declare_buffer_with_ts-v3-1-f12df1bff248%40baylibre.com
patch subject: [PATCH v3 1/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros
config: openrisc-randconfig-r053-20250427 (https://download.01.org/0day-ci/archive/20250427/202504270311.4lppXI1u-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 10.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250427/202504270311.4lppXI1u-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504270311.4lppXI1u-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/container_of.h:5,
                    from include/linux/list.h:5,
                    from include/linux/kobject.h:19,
                    from include/linux/cdev.h:5,
                    from drivers/iio/industrialio-core.c:13:
>> include/linux/build_bug.h:78:41: error: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
      78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
         |                                         ^~~~~~~~~~~~~~
   include/linux/build_bug.h:77:34: note: in expansion of macro '__static_assert'
      77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
         |                                  ^~~~~~~~~~~~~~~
   include/linux/iio/iio.h:815:1: note: in expansion of macro 'static_assert'
     815 | static_assert(IIO_DMA_MINALIGN % sizeof(s64) == 0,
         | ^~~~~~~~~~~~~


vim +78 include/linux/build_bug.h

bc6245e5efd70c Ian Abbott       2017-07-10  60  
6bab69c65013be Rasmus Villemoes 2019-03-07  61  /**
6bab69c65013be Rasmus Villemoes 2019-03-07  62   * static_assert - check integer constant expression at build time
6bab69c65013be Rasmus Villemoes 2019-03-07  63   *
6bab69c65013be Rasmus Villemoes 2019-03-07  64   * static_assert() is a wrapper for the C11 _Static_assert, with a
6bab69c65013be Rasmus Villemoes 2019-03-07  65   * little macro magic to make the message optional (defaulting to the
6bab69c65013be Rasmus Villemoes 2019-03-07  66   * stringification of the tested expression).
6bab69c65013be Rasmus Villemoes 2019-03-07  67   *
6bab69c65013be Rasmus Villemoes 2019-03-07  68   * Contrary to BUILD_BUG_ON(), static_assert() can be used at global
6bab69c65013be Rasmus Villemoes 2019-03-07  69   * scope, but requires the expression to be an integer constant
6bab69c65013be Rasmus Villemoes 2019-03-07  70   * expression (i.e., it is not enough that __builtin_constant_p() is
6bab69c65013be Rasmus Villemoes 2019-03-07  71   * true for expr).
6bab69c65013be Rasmus Villemoes 2019-03-07  72   *
6bab69c65013be Rasmus Villemoes 2019-03-07  73   * Also note that BUILD_BUG_ON() fails the build if the condition is
6bab69c65013be Rasmus Villemoes 2019-03-07  74   * true, while static_assert() fails the build if the expression is
6bab69c65013be Rasmus Villemoes 2019-03-07  75   * false.
6bab69c65013be Rasmus Villemoes 2019-03-07  76   */
6bab69c65013be Rasmus Villemoes 2019-03-07  77  #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
6bab69c65013be Rasmus Villemoes 2019-03-07 @78  #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
6bab69c65013be Rasmus Villemoes 2019-03-07  79  
07a368b3f55a79 Maxim Levitsky   2022-10-25  80  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros
  2025-04-26 11:35   ` Jonathan Cameron
@ 2025-04-26 22:34     ` David Lechner
  2025-04-27 10:20       ` Jonathan Cameron
  0 siblings, 1 reply; 13+ messages in thread
From: David Lechner @ 2025-04-26 22:34 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Nuno Sá, Andy Shevchenko, Lars-Peter Clausen,
	Michael Hennerich, Eugen Hristev, Nicolas Ferre,
	Alexandre Belloni, Claudiu Beznea, linux-iio, linux-kernel,
	linux-arm-kernel

On 4/26/25 6:35 AM, Jonathan Cameron wrote:
> On Fri, 25 Apr 2025 16:08:43 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 

...

>> @@ -777,6 +779,42 @@ static inline void *iio_device_get_drvdata(const struct iio_dev *indio_dev)
>>   * them safe for use with non-coherent DMA.
>>   */
>>  #define IIO_DMA_MINALIGN ARCH_DMA_MINALIGN
>> +
>> +#define __IIO_DECLARE_BUFFER_WITH_TS(type, name, count) \
>> +	static_assert(count); \
> 
> Why do we care if count is 0?  Or is intent to check if is constant?
> If the thought is we don't care either way about 0 (as rather nonsensical)
> and this will fail to compile if not constant, then perhaps a comment would
> avoid future confusion?

I would be inclined to just leave out the check. But yes, it is just checking
that count is constant and we don't expect 0.

> 
>> +	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)
>> +
>> +static_assert(IIO_DMA_MINALIGN % sizeof(s64) == 0,
> That message isn't super helpful if seen in a compile log as we aren't reading the code here
> "IIO_DECLARE_DMA_BUFFER_WITH_TS() assumes that ...
> 
>> +	"macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment");
>> +

Seems we actually have an arch (openrisc) that triggers this [1]. This arch
doesn't define ARCH_DMA_MINALIGN so it falls back to:

#define ARCH_DMA_MINALIGN __alignof__(unsigned long long)

Apparently this is only of those 32-bit arches that only does 4 byte alignment.
From the official docs [2]:

	Current OR32 implementations (OR1200) do not implement 8 byte alignment,
	but do require 4 byte alignment. Therefore the Application Binary
	Interface (chapter 16) uses 4 byte alignment for 8 byte types. Future
	extensions such as ORVDX64 may require natural alignment.

[1]: https://lore.kernel.org/linux-iio/20250425-iio-introduce-iio_declare_buffer_with_ts-v3-0-f12df1bff248@baylibre.com/T/#m91e0332673438793ff76949037ff40a34765ca30
[2]: https://openrisc.io/or1k.html


It looks like this could work (it compiles for me):

	__aligned(MAX(IIO_DMA_MINALIGN, sizeof(s64)))

If that is OK we could leave out the static_assert(), unless we think there
could be an arch with IIO_DMA_MINALIGN not a power of 2?!



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros
  2025-04-25 21:08 ` [PATCH v3 1/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros David Lechner
  2025-04-26 11:35   ` Jonathan Cameron
  2025-04-26 19:15   ` kernel test robot
@ 2025-04-27  1:54   ` kernel test robot
  2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2025-04-27  1:54 UTC (permalink / raw)
  To: David Lechner, Jonathan Cameron, Nuno Sá, Andy Shevchenko,
	Lars-Peter Clausen, Michael Hennerich, Eugen Hristev,
	Nicolas Ferre, Alexandre Belloni, Claudiu Beznea
  Cc: oe-kbuild-all, linux-iio, linux-kernel, linux-arm-kernel

Hi David,

kernel test robot noticed the following build warnings:

[auto build test WARNING on aff301f37e220970c2f301b5c65a8bfedf52058e]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Lechner/iio-introduce-IIO_DECLARE_BUFFER_WITH_TS-macros/20250426-051240
base:   aff301f37e220970c2f301b5c65a8bfedf52058e
patch link:    https://lore.kernel.org/r/20250425-iio-introduce-iio_declare_buffer_with_ts-v3-1-f12df1bff248%40baylibre.com
patch subject: [PATCH v3 1/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros
config: i386-randconfig-r133-20250427 (https://download.01.org/0day-ci/archive/20250427/202504270919.3FGvikEj-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250427/202504270919.3FGvikEj-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504270919.3FGvikEj-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
   drivers/iio/accel/adxl313_core.c: note: in included file (through drivers/iio/accel/adxl313.h):
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/adxl345_core.c: note: in included file:
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/adxl355_core.c: note: in included file (through include/linux/iio/buffer.h):
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/adxl313_spi.c: note: in included file (through drivers/iio/accel/adxl313.h):
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/adis16201.c: note: in included file:
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/adis16209.c: note: in included file:
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/dmard09.c: note: in included file:
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/da311.c: note: in included file:
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/adxl313_i2c.c: note: in included file (through drivers/iio/accel/adxl313.h):
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/adxl367_spi.c: note: in included file:
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/da280.c: note: in included file:
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/kxsd9.c: note: in included file:
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/bma220_spi.c: note: in included file (through include/linux/iio/buffer.h):
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/kionix-kx022a.c: note: in included file:
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/adxl372.c: note: in included file:
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/kxcjk-1013.c: note: in included file:
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/fxls8962af-core.c: note: in included file (through include/linux/iio/buffer.h):
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/adxl380.c: note: in included file (through include/linux/iio/buffer.h):
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/adxl367.c: note: in included file (through include/linux/iio/buffer.h):
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/mma7455_core.c: note: in included file:
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/mc3230.c: note: in included file:
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/ssp_accel_sensor.c: note: in included file (through include/linux/iio/common/ssp_sensors.h):
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/mma7660.c: note: in included file:
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/sca3300.c: note: in included file (through include/linux/iio/buffer.h):
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/mma9551.c: note: in included file:
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/mma9551_core.c: note: in included file:
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/mxc4005.c: note: in included file:
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/stk8312.c: note: in included file (through include/linux/iio/buffer.h):
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/stk8ba50.c: note: in included file (through include/linux/iio/buffer.h):
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/sca3000.c: note: in included file:
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/msa311.c: note: in included file (through include/linux/iio/buffer.h):
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"
--
   drivers/iio/accel/mma9553.c: note: in included file:
>> include/linux/iio/iio.h:815:1: sparse: sparse: static assertion failed: "macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment"

vim +815 include/linux/iio/iio.h

   782	
   783	#define __IIO_DECLARE_BUFFER_WITH_TS(type, name, count) \
   784		static_assert(count); \
   785		type name[ALIGN((count), sizeof(s64) / sizeof(type)) + sizeof(s64) / sizeof(type)]
   786	
   787	/**
   788	 * IIO_DECLARE_BUFFER_WITH_TS() - Declare a buffer with timestamp
   789	 * @type: element type of the buffer
   790	 * @name: identifier name of the buffer
   791	 * @count: number of elements in the buffer
   792	 *
   793	 * Declares a buffer that is safe to use with iio_push_to_buffer_with_ts(). In
   794	 * addition to allocating enough space for @count elements of @type, it also
   795	 * allocates space for a s64 timestamp at the end of the buffer and ensures
   796	 * proper alignment of the timestamp.
   797	 */
   798	#define IIO_DECLARE_BUFFER_WITH_TS(type, name, count) \
   799		__IIO_DECLARE_BUFFER_WITH_TS(type, name, count) __aligned(sizeof(s64))
   800	
   801	/**
   802	 * IIO_DECLARE_DMA_BUFFER_WITH_TS() - Declare a DMA-aligned buffer with timestamp
   803	 * @type: element type of the buffer
   804	 * @name: identifier name of the buffer
   805	 * @count: number of elements in the buffer
   806	 *
   807	 * Same as IIO_DECLARE_BUFFER_WITH_TS(), but is uses __aligned(IIO_DMA_MINALIGN)
   808	 * to ensure that the buffer doesn't share cachelines with anything that comes
   809	 * before it in a struct. This should not be used for stack-allocated buffers
   810	 * as stack memory cannot generally be used for DMA.
   811	 */
   812	#define IIO_DECLARE_DMA_BUFFER_WITH_TS(type, name, count) \
   813		__IIO_DECLARE_BUFFER_WITH_TS(type, name, count) __aligned(IIO_DMA_MINALIGN)
   814	
 > 815	static_assert(IIO_DMA_MINALIGN % sizeof(s64) == 0,
   816		"macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment");
   817	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros
  2025-04-26 22:34     ` David Lechner
@ 2025-04-27 10:20       ` Jonathan Cameron
  0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2025-04-27 10:20 UTC (permalink / raw)
  To: David Lechner
  Cc: Nuno Sá, Andy Shevchenko, Lars-Peter Clausen,
	Michael Hennerich, Eugen Hristev, Nicolas Ferre,
	Alexandre Belloni, Claudiu Beznea, linux-iio, linux-kernel,
	linux-arm-kernel

On Sat, 26 Apr 2025 17:34:10 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 4/26/25 6:35 AM, Jonathan Cameron wrote:
> > On Fri, 25 Apr 2025 16:08:43 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> 
> ...
> 
> >> @@ -777,6 +779,42 @@ static inline void *iio_device_get_drvdata(const struct iio_dev *indio_dev)
> >>   * them safe for use with non-coherent DMA.
> >>   */
> >>  #define IIO_DMA_MINALIGN ARCH_DMA_MINALIGN
> >> +
> >> +#define __IIO_DECLARE_BUFFER_WITH_TS(type, name, count) \
> >> +	static_assert(count); \  
> > 
> > Why do we care if count is 0?  Or is intent to check if is constant?
> > If the thought is we don't care either way about 0 (as rather nonsensical)
> > and this will fail to compile if not constant, then perhaps a comment would
> > avoid future confusion?  
> 
> I would be inclined to just leave out the check. But yes, it is just checking
> that count is constant and we don't expect 0.
> 
> >   
> >> +	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)
> >> +
> >> +static_assert(IIO_DMA_MINALIGN % sizeof(s64) == 0,  
> > That message isn't super helpful if seen in a compile log as we aren't reading the code here
> > "IIO_DECLARE_DMA_BUFFER_WITH_TS() assumes that ...
> >   
> >> +	"macros above assume that IIO_DMA_MINALIGN also ensures s64 timestamp alignment");
> >> +  
> 
> Seems we actually have an arch (openrisc) that triggers this [1]. This arch
> doesn't define ARCH_DMA_MINALIGN so it falls back to:
> 
> #define ARCH_DMA_MINALIGN __alignof__(unsigned long long)
> 
> Apparently this is only of those 32-bit arches that only does 4 byte alignment.
> From the official docs [2]:
> 
> 	Current OR32 implementations (OR1200) do not implement 8 byte alignment,
> 	but do require 4 byte alignment. Therefore the Application Binary
> 	Interface (chapter 16) uses 4 byte alignment for 8 byte types. Future
> 	extensions such as ORVDX64 may require natural alignment.
> 
> [1]: https://lore.kernel.org/linux-iio/20250425-iio-introduce-iio_declare_buffer_with_ts-v3-0-f12df1bff248@baylibre.com/T/#m91e0332673438793ff76949037ff40a34765ca30
> [2]: https://openrisc.io/or1k.html
> 
> 
> It looks like this could work (it compiles for me):
> 
> 	__aligned(MAX(IIO_DMA_MINALIGN, sizeof(s64)))
> 
> If that is OK we could leave out the static_assert(), unless we think there
> could be an arch with IIO_DMA_MINALIGN not a power of 2?!
> 
That change seems fine.  Non power of 2 arch would be fun but implausible any time soon :)



^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2025-04-27 10:22 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-25 21:08 [PATCH v3 0/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS David Lechner
2025-04-25 21:08 ` [PATCH v3 1/6] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros David Lechner
2025-04-26 11:35   ` Jonathan Cameron
2025-04-26 22:34     ` David Lechner
2025-04-27 10:20       ` Jonathan Cameron
2025-04-26 19:15   ` kernel test robot
2025-04-27  1:54   ` kernel test robot
2025-04-25 21:08 ` [PATCH v3 2/6] iio: adc: ad4695: use u16 for buffer elements David Lechner
2025-04-25 21:08 ` [PATCH v3 3/6] iio: adc: ad4695: use IIO_DECLARE_DMA_BUFFER_WITH_TS David Lechner
2025-04-26 11:24   ` Jonathan Cameron
2025-04-25 21:08 ` [PATCH v3 4/6] iio: adc: ad7380: " David Lechner
2025-04-25 21:08 ` [PATCH v3 5/6] iio: accel: sca3300: use IIO_DECLARE_BUFFER_WITH_TS David Lechner
2025-04-25 21:08 ` [PATCH v3 6/6] iio: adc: at91-sama5d2: " David Lechner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox