* [PATCH v6 0/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS
@ 2025-05-07 20:42 David Lechner
2025-05-07 20:42 ` [PATCH v6 1/7] iio: make IIO_DMA_MINALIGN minimum of 8 bytes David Lechner
` (7 more replies)
0 siblings, 8 replies; 9+ messages in thread
From: David Lechner @ 2025-05-07 20:42 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, Trevor Gamblin
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 v6:
- Rework IIO_DMA_MINALIGN to work around clang compiler issue.
- Link to v5: https://lore.kernel.org/r/20250505-iio-introduce-iio_declare_buffer_with_ts-v5-0-814b72b1cae3@baylibre.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 | 41 +++++++++++++++++++++++++++++++++++++-
5 files changed, 50 insertions(+), 36 deletions(-)
---
base-commit: 7e9a82ab5b861d3c33c99a22c1245a5b262ee502
change-id: 20250418-iio-introduce-iio_declare_buffer_with_ts-2f8773f7dad6
Best regards,
--
David Lechner <dlechner@baylibre.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v6 1/7] iio: make IIO_DMA_MINALIGN minimum of 8 bytes
2025-05-07 20:42 [PATCH v6 0/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS David Lechner
@ 2025-05-07 20:42 ` David Lechner
2025-05-07 20:42 ` [PATCH v6 2/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros David Lechner
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: David Lechner @ 2025-05-07 20:42 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 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.
Reviewed-by: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
v6 changes:
- Removed the #if and use MAX instead since apparently clang raises an
error that __alignof__ isn't defined when used in a #if condition.
---
include/linux/iio/iio.h | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 638cf2420fbd85cf2924d09d061df601d1d4bb2a..a574f22398e45cad1ea741d20d302f88756a1b13 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -10,6 +10,7 @@
#include <linux/device.h>
#include <linux/cdev.h>
#include <linux/compiler_types.h>
+#include <linux/minmax.h>
#include <linux/slab.h>
#include <linux/iio/types.h>
/* IIO TODO LIST */
@@ -775,8 +776,14 @@ 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.
*/
-#define IIO_DMA_MINALIGN ARCH_DMA_MINALIGN
+#define IIO_DMA_MINALIGN MAX(ARCH_DMA_MINALIGN, sizeof(s64))
+
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] 9+ messages in thread
* [PATCH v6 2/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros
2025-05-07 20:42 [PATCH v6 0/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS David Lechner
2025-05-07 20:42 ` [PATCH v6 1/7] iio: make IIO_DMA_MINALIGN minimum of 8 bytes David Lechner
@ 2025-05-07 20:42 ` David Lechner
2025-05-07 20:42 ` [PATCH v6 3/7] iio: adc: ad4695: use IIO_DECLARE_DMA_BUFFER_WITH_TS David Lechner
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: David Lechner @ 2025-05-07 20:42 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().
Reviewed-by: Nuno Sá <nuno.sa@analog.com>
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 a574f22398e45cad1ea741d20d302f88756a1b13..d11668f14a3e17654fcf17a4e853d4b493205019 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>
@@ -784,6 +785,37 @@ static inline void *iio_device_get_drvdata(const struct iio_dev *indio_dev)
*/
#define IIO_DMA_MINALIGN MAX(ARCH_DMA_MINALIGN, sizeof(s64))
+#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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v6 3/7] iio: adc: ad4695: use IIO_DECLARE_DMA_BUFFER_WITH_TS
2025-05-07 20:42 [PATCH v6 0/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS David Lechner
2025-05-07 20:42 ` [PATCH v6 1/7] iio: make IIO_DMA_MINALIGN minimum of 8 bytes David Lechner
2025-05-07 20:42 ` [PATCH v6 2/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros David Lechner
@ 2025-05-07 20:42 ` David Lechner
2025-05-07 20:42 ` [PATCH v6 4/7] iio: adc: ad4695: rename AD4695_MAX_VIN_CHANNELS David Lechner
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: David Lechner @ 2025-05-07 20:42 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, Trevor Gamblin
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>
Reviewed-by: Nuno Sá <nuno.sa@analog.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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v6 4/7] iio: adc: ad4695: rename AD4695_MAX_VIN_CHANNELS
2025-05-07 20:42 [PATCH v6 0/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS David Lechner
` (2 preceding siblings ...)
2025-05-07 20:42 ` [PATCH v6 3/7] iio: adc: ad4695: use IIO_DECLARE_DMA_BUFFER_WITH_TS David Lechner
@ 2025-05-07 20:42 ` David Lechner
2025-05-07 20:42 ` [PATCH v6 5/7] iio: adc: ad7380: use IIO_DECLARE_DMA_BUFFER_WITH_TS David Lechner
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: David Lechner @ 2025-05-07 20:42 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, Trevor Gamblin
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>
Reviewed-by: Nuno Sá <nuno.sa@analog.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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v6 5/7] iio: adc: ad7380: use IIO_DECLARE_DMA_BUFFER_WITH_TS
2025-05-07 20:42 [PATCH v6 0/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS David Lechner
` (3 preceding siblings ...)
2025-05-07 20:42 ` [PATCH v6 4/7] iio: adc: ad4695: rename AD4695_MAX_VIN_CHANNELS David Lechner
@ 2025-05-07 20:42 ` David Lechner
2025-05-07 20:42 ` [PATCH v6 6/7] iio: accel: sca3300: use IIO_DECLARE_BUFFER_WITH_TS David Lechner
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: David Lechner @ 2025-05-07 20:42 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.
Reviewed-by: Nuno Sá <nuno.sa@analog.com>
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] 9+ messages in thread
* [PATCH v6 6/7] iio: accel: sca3300: use IIO_DECLARE_BUFFER_WITH_TS
2025-05-07 20:42 [PATCH v6 0/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS David Lechner
` (4 preceding siblings ...)
2025-05-07 20:42 ` [PATCH v6 5/7] iio: adc: ad7380: use IIO_DECLARE_DMA_BUFFER_WITH_TS David Lechner
@ 2025-05-07 20:42 ` David Lechner
2025-05-07 20:42 ` [PATCH v6 7/7] iio: adc: at91-sama5d2: " David Lechner
2025-05-08 19:09 ` [PATCH v6 0/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS Jonathan Cameron
7 siblings, 0 replies; 9+ messages in thread
From: David Lechner @ 2025-05-07 20:42 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.
Reviewed-by: Nuno Sá <nuno.sa@analog.com>
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] 9+ messages in thread
* [PATCH v6 7/7] iio: adc: at91-sama5d2: use IIO_DECLARE_BUFFER_WITH_TS
2025-05-07 20:42 [PATCH v6 0/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS David Lechner
` (5 preceding siblings ...)
2025-05-07 20:42 ` [PATCH v6 6/7] iio: accel: sca3300: use IIO_DECLARE_BUFFER_WITH_TS David Lechner
@ 2025-05-07 20:42 ` David Lechner
2025-05-08 19:09 ` [PATCH v6 0/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS Jonathan Cameron
7 siblings, 0 replies; 9+ messages in thread
From: David Lechner @ 2025-05-07 20:42 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.
Reviewed-by: Nuno Sá <nuno.sa@analog.com>
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v6 0/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS
2025-05-07 20:42 [PATCH v6 0/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS David Lechner
` (6 preceding siblings ...)
2025-05-07 20:42 ` [PATCH v6 7/7] iio: adc: at91-sama5d2: " David Lechner
@ 2025-05-08 19:09 ` Jonathan Cameron
7 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2025-05-08 19:09 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, Trevor Gamblin
On Wed, 07 May 2025 15:42:39 -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.
>
Seems better. Applied and pushed out as testing to see if any other
compiler versions don't like it!
J
> ---
> Changes in v6:
> - Rework IIO_DMA_MINALIGN to work around clang compiler issue.
> - Link to v5: https://lore.kernel.org/r/20250505-iio-introduce-iio_declare_buffer_with_ts-v5-0-814b72b1cae3@baylibre.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 | 41 +++++++++++++++++++++++++++++++++++++-
> 5 files changed, 50 insertions(+), 36 deletions(-)
> ---
> base-commit: 7e9a82ab5b861d3c33c99a22c1245a5b262ee502
> change-id: 20250418-iio-introduce-iio_declare_buffer_with_ts-2f8773f7dad6
>
> Best regards,
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-05-08 19:15 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-07 20:42 [PATCH v6 0/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS David Lechner
2025-05-07 20:42 ` [PATCH v6 1/7] iio: make IIO_DMA_MINALIGN minimum of 8 bytes David Lechner
2025-05-07 20:42 ` [PATCH v6 2/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros David Lechner
2025-05-07 20:42 ` [PATCH v6 3/7] iio: adc: ad4695: use IIO_DECLARE_DMA_BUFFER_WITH_TS David Lechner
2025-05-07 20:42 ` [PATCH v6 4/7] iio: adc: ad4695: rename AD4695_MAX_VIN_CHANNELS David Lechner
2025-05-07 20:42 ` [PATCH v6 5/7] iio: adc: ad7380: use IIO_DECLARE_DMA_BUFFER_WITH_TS David Lechner
2025-05-07 20:42 ` [PATCH v6 6/7] iio: accel: sca3300: use IIO_DECLARE_BUFFER_WITH_TS David Lechner
2025-05-07 20:42 ` [PATCH v6 7/7] iio: adc: at91-sama5d2: " David Lechner
2025-05-08 19:09 ` [PATCH v6 0/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).