[PATCH v4 0/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS

[PATCH v4 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 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: 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: 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           | 16 ++++++----------
 drivers/iio/adc/ad7380.c           |  3 +--
 drivers/iio/adc/at91-sama5d2_adc.c | 13 ++-----------
 include/linux/iio/iio.h            | 37 +++++++++++++++++++++++++++++++++++++
 5 files changed, 48 insertions(+), 39 deletions(-)
---
base-commit: aff301f37e220970c2f301b5c65a8bfedf52058e
change-id: 20250418-iio-introduce-iio_declare_buffer_with_ts-2f8773f7dad6

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

[PATCH v4 1/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>
---

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

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 638cf2420fbd85cf2924d09d061df601d1d4bb2a..95d90740b4ffb3ebd819d03ad2bd05ecf40d4384 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -7,9 +7,12 @@
 #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>
+#include <linux/minmax.h>
 #include <linux/slab.h>
 #include <linux/iio/types.h>
 /* IIO TODO LIST */
@@ -777,6 +780,40 @@ 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) \
+	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)		\
+	/* IIO_DMA_MINALIGN may be 4 on some 32-bit arches. */	\
+	__aligned(MAX(IIO_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

[PATCH v4 2/7] iio: adc: ad4695: use u16 for buffer elements

[David Lechner]

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

[PATCH v4 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.

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

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

[PATCH v4 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>
---
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

Re: [PATCH v4 2/7] iio: adc: ad4695: use u16 for buffer elements

[Trevor Gamblin]

On 2025-04-28 16:23, David Lechner wrote:
> 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>
Reviewed-by: Trevor Gamblin <tgamblin@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;
>

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

[Trevor Gamblin]

On 2025-04-28 16:23, David Lechner 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>
Reviewed-by: Trevor Gamblin <tgamblin@baylibre.com>

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

[Trevor Gamblin]

On 2025-04-28 16:23, David Lechner wrote:
> 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.
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Reviewed-by: Trevor Gamblin <tgamblin@baylibre.com>

Re: [PATCH v4 1/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros

[David Lechner]

On 4/28/25 3:23 PM, David Lechner 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().
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---

...

> +/**
> + * 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)		\
> +	/* IIO_DMA_MINALIGN may be 4 on some 32-bit arches. */	\
> +	__aligned(MAX(IIO_DMA_MINALIGN, sizeof(s64)))

I just realized my logic behind this is faulty. It assumes sizeof(s64) ==
__alignof__(s64), but that isn't always true and that is what caused the builds
to hit the static_assert() on v3.

We should be able to leave this as __aligned(IIO_DMA_MINALIGN)

And have this (with better error message):

static assert(IIO_DMA_MINALIGN % __alignof__(s64) == 0);

Re: [PATCH v4 1/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros

[David Lechner]

On 4/28/25 9:12 PM, David Lechner wrote:
> On 4/28/25 3:23 PM, David Lechner 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().
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
> 
> ...
> 
>> +/**
>> + * 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)		\
>> +	/* IIO_DMA_MINALIGN may be 4 on some 32-bit arches. */	\
>> +	__aligned(MAX(IIO_DMA_MINALIGN, sizeof(s64)))
> 
> I just realized my logic behind this is faulty. It assumes sizeof(s64) ==
> __alignof__(s64), but that isn't always true and that is what caused the builds
> to hit the static_assert() on v3.
> 
> We should be able to leave this as __aligned(IIO_DMA_MINALIGN)
> 
> And have this (with better error message):
> 
> static assert(IIO_DMA_MINALIGN % __alignof__(s64) == 0);

I was working late yesterday and should have saved that reply until morning
to think about it more!

We do want to align to to sizeof(s64) instead of __alignof__(s64) to avoid
issues with, e.g. 32-bit kernel and 64-bit userspace (same reason that
aligned_s64 exists and always uses 8-byte alignment).

So I think this patch is correct as-is after all.

Re: [PATCH v4 1/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros

[Andy Shevchenko]

On Tue, Apr 29, 2025 at 10:31 PM David Lechner <dlechner@baylibre.com> wrote:
> On 4/28/25 9:12 PM, David Lechner wrote:
> > On 4/28/25 3:23 PM, David Lechner 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().

...

> >> +/**
> >> + * 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)         \
> >> +    /* IIO_DMA_MINALIGN may be 4 on some 32-bit arches. */  \
> >> +    __aligned(MAX(IIO_DMA_MINALIGN, sizeof(s64)))
> >
> > I just realized my logic behind this is faulty. It assumes sizeof(s64) ==
> > __alignof__(s64), but that isn't always true and that is what caused the builds
> > to hit the static_assert() on v3.
> >
> > We should be able to leave this as __aligned(IIO_DMA_MINALIGN)
> >
> > And have this (with better error message):
> >
> > static assert(IIO_DMA_MINALIGN % __alignof__(s64) == 0);
>
> I was working late yesterday and should have saved that reply until morning
> to think about it more!
>
> We do want to align to to sizeof(s64) instead of __alignof__(s64) to avoid
> issues with, e.g. 32-bit kernel and 64-bit userspace (same reason that
> aligned_s64 exists and always uses 8-byte alignment).
>
> So I think this patch is correct as-is after all.

I'm wondering, shouldn't it be better just to make sure that
IIO_DMA_MINALIGN is always bigger or equal to sizeof(s64)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros

[David Lechner]

On 4/29/25 2:36 PM, Andy Shevchenko wrote:
> On Tue, Apr 29, 2025 at 10:31 PM David Lechner <dlechner@baylibre.com> wrote:
>> On 4/28/25 9:12 PM, David Lechner wrote:
>>> On 4/28/25 3:23 PM, David Lechner 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().
> 
> ...
> 
>>>> +/**
>>>> + * 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)         \
>>>> +    /* IIO_DMA_MINALIGN may be 4 on some 32-bit arches. */  \
>>>> +    __aligned(MAX(IIO_DMA_MINALIGN, sizeof(s64)))
>>>
>>> I just realized my logic behind this is faulty. It assumes sizeof(s64) ==
>>> __alignof__(s64), but that isn't always true and that is what caused the builds
>>> to hit the static_assert() on v3.
>>>
>>> We should be able to leave this as __aligned(IIO_DMA_MINALIGN)
>>>
>>> And have this (with better error message):
>>>
>>> static assert(IIO_DMA_MINALIGN % __alignof__(s64) == 0);
>>
>> I was working late yesterday and should have saved that reply until morning
>> to think about it more!
>>
>> We do want to align to to sizeof(s64) instead of __alignof__(s64) to avoid
>> issues with, e.g. 32-bit kernel and 64-bit userspace (same reason that
>> aligned_s64 exists and always uses 8-byte alignment).
>>
>> So I think this patch is correct as-is after all.
> 
> I'm wondering, shouldn't it be better just to make sure that
> IIO_DMA_MINALIGN is always bigger or equal to sizeof(s64)?
> 

Sounds reasonable to me. From what I have seen while working on this is that
there are quite a few drivers using IIO_DMA_MINALIGN expecting it to be
sufficient for timestamp alignment, which as it seems is not always the case.

I'll wait for Jonathan to weigh in though before spinning up a new patch.

Re: [PATCH v4 1/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros

[Nuno Sá]

On Tue, 2025-04-29 at 14:31 -0500, David Lechner wrote:
> On 4/28/25 9:12 PM, David Lechner wrote:
> > On 4/28/25 3:23 PM, David Lechner 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().
> > > 
> > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > > ---
> > 
> > ...
> > 
> > > +/**
> > > + * 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)		\
> > > +	/* IIO_DMA_MINALIGN may be 4 on some 32-bit arches. */	\
> > > +	__aligned(MAX(IIO_DMA_MINALIGN, sizeof(s64)))
> > 
> > I just realized my logic behind this is faulty. It assumes sizeof(s64) ==
> > __alignof__(s64), but that isn't always true and that is what caused the
> > builds
> > to hit the static_assert() on v3.
> > 
> > We should be able to leave this as __aligned(IIO_DMA_MINALIGN)
> > 
> > And have this (with better error message):
> > 
> > static assert(IIO_DMA_MINALIGN % __alignof__(s64) == 0);
> 
> I was working late yesterday and should have saved that reply until morning
> to think about it more!
> 
> We do want to align to to sizeof(s64) instead of __alignof__(s64) to avoid
> issues with, e.g. 32-bit kernel and 64-bit userspace (same reason that
> aligned_s64 exists and always uses 8-byte alignment).

What issues could we have? In your inner macros I think you still make sure we
pad everything up to sizeof(s64) right? Am I missing any subtle issue?

but...

> 
> So I think this patch is correct as-is after all.

FWIW, I do prefer this approach or what Andy suggest (min as sizeof(s64)).

- Nuno Sá

Re: [PATCH v4 1/7] iio: introduce IIO_DECLARE_BUFFER_WITH_TS macros

[Jonathan Cameron]

On Tue, 29 Apr 2025 14:47:41 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 4/29/25 2:36 PM, Andy Shevchenko wrote:
> > On Tue, Apr 29, 2025 at 10:31 PM David Lechner <dlechner@baylibre.com> wrote:  
> >> On 4/28/25 9:12 PM, David Lechner wrote:  
> >>> On 4/28/25 3:23 PM, David Lechner 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().  
> > 
> > ...
> >   
> >>>> +/**
> >>>> + * 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)         \
> >>>> +    /* IIO_DMA_MINALIGN may be 4 on some 32-bit arches. */  \
> >>>> +    __aligned(MAX(IIO_DMA_MINALIGN, sizeof(s64)))  
> >>>
> >>> I just realized my logic behind this is faulty. It assumes sizeof(s64) ==
> >>> __alignof__(s64), but that isn't always true and that is what caused the builds
> >>> to hit the static_assert() on v3.
> >>>
> >>> We should be able to leave this as __aligned(IIO_DMA_MINALIGN)
> >>>
> >>> And have this (with better error message):
> >>>
> >>> static assert(IIO_DMA_MINALIGN % __alignof__(s64) == 0);  
> >>
> >> I was working late yesterday and should have saved that reply until morning
> >> to think about it more!
> >>
> >> We do want to align to to sizeof(s64) instead of __alignof__(s64) to avoid
> >> issues with, e.g. 32-bit kernel and 64-bit userspace (same reason that
> >> aligned_s64 exists and always uses 8-byte alignment).
> >>
> >> So I think this patch is correct as-is after all.  
> > 
> > I'm wondering, shouldn't it be better just to make sure that
> > IIO_DMA_MINALIGN is always bigger or equal to sizeof(s64)?
> >   
> 
> Sounds reasonable to me. From what I have seen while working on this is that
> there are quite a few drivers using IIO_DMA_MINALIGN expecting it to be
> sufficient for timestamp alignment, which as it seems is not always the case.
> 
> I'll wait for Jonathan to weigh in though before spinning up a new patch.
> 
It would be very odd if we ever see a platform with a DMA alignment requirement
that is not a multiple of sizeof(s64) just forcing IIO_DMA_MINALIGN to max
of the arch constraint and sizeof(s64) seems fine to me and fixes up
all those other drivers that were assuming this was true already...
I thought it was and only got fussy for this macro :(


Jonathan

Re: [PATCH v4 2/7] iio: adc: ad4695: use u16 for buffer elements

[Jonathan Cameron]

On Mon, 28 Apr 2025 16:33:21 -0400
Trevor Gamblin <tgamblin@baylibre.com> wrote:

> On 2025-04-28 16:23, David Lechner wrote:
> > 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>  
> Reviewed-by: Trevor Gamblin <tgamblin@baylibre.com>
Mostly because there are two many patches for me to keep track of
at the moment I'm going to apply the ones in here that stand on their own
without the macro and your v5 with that size thing can be a smaller series :)

Applied this one.

Thanks,

Jonathan

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

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

[Jonathan Cameron]

On Mon, 28 Apr 2025 16:37:19 -0400
Trevor Gamblin <tgamblin@baylibre.com> wrote:

> On 2025-04-28 16:23, David Lechner wrote:
> > 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.
> >
> > Signed-off-by: David Lechner <dlechner@baylibre.com>  
> Reviewed-by: Trevor Gamblin <tgamblin@baylibre.com>
> 

Drat. I was thinking I could pick up this as well.
Obviously I can't as it was after the one adding the code that is being
changed...

Ah well. I already picked up that other patch so I'll keep that.
If you send the next series with it in as well not a problem.

Thanks,

Jonathan