[PATCH v2] iio: accel: adxl372: Add timestamp to FIFO data

[PATCH v2] iio: accel: adxl372: Add timestamp to FIFO data

[Md Shofiqul Islam]

The driver pushes FIFO samples using iio_push_to_buffers() which does
not attach a hardware timestamp to the data.  Capture a single timestamp
per IRQ with iio_get_time_ns() and reuse it for both the event push and
the FIFO sample loop.

Use IIO_DECLARE_BUFFER_WITH_TS() for fifo_buf to guarantee the required
s64 alignment, and switch the FIFO push loop to
iio_push_to_buffers_with_ts() which accepts an explicit data size and
timestamp without requiring a separate scan struct or memcpy.

Add IIO_CHAN_SOFT_TIMESTAMP(3) to the channel spec so the IIO core
enables scan_timestamp and includes the timestamp slot in the buffer.

Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
---
Changes in v2:
  - Use IIO_DECLARE_BUFFER_WITH_TS() + iio_push_to_buffers_with_ts() instead
    of a separate scan struct + memcpy, per review from David Lechner
  - Add IIO_CHAN_SOFT_TIMESTAMP(3) to channel spec (missing in v1)
 drivers/iio/accel/adxl372.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
index e375d068a..266e80e27 100644
--- a/drivers/iio/accel/adxl372.c
+++ b/drivers/iio/accel/adxl372.c
@@ -344,6 +344,7 @@ static const struct iio_chan_spec adxl372_channels[] = {
 	ADXL372_ACCEL_CHANNEL(0, ADXL372_X_DATA_H, X),
 	ADXL372_ACCEL_CHANNEL(1, ADXL372_Y_DATA_H, Y),
 	ADXL372_ACCEL_CHANNEL(2, ADXL372_Z_DATA_H, Z),
+	IIO_CHAN_SOFT_TIMESTAMP(3),
 };
 
 struct adxl372_state {
@@ -365,7 +366,7 @@ struct adxl372_state {
 	u8				fifo_set_size;
 	unsigned long			int1_bitmask;
 	u16				watermark;
-	__be16				fifo_buf[ADXL372_FIFO_SIZE];
+	IIO_DECLARE_BUFFER_WITH_TS(__be16, fifo_buf, ADXL372_FIFO_SIZE);
 	bool				peak_fifo_mode_en;
 	struct mutex			threshold_m; /* lock for threshold */
 };
@@ -703,13 +704,15 @@ static irqreturn_t adxl372_trigger_handler(int irq, void  *p)
 	struct adxl372_state *st = iio_priv(indio_dev);
 	u8 status1, status2;
 	u16 fifo_entries;
+	s64 ts;
 	int i, ret;
 
 	ret = adxl372_get_status(st, &status1, &status2, &fifo_entries);
 	if (ret < 0)
 		goto err;
 
-	adxl372_push_event(indio_dev, iio_get_time_ns(indio_dev), status2);
+	ts = iio_get_time_ns(indio_dev);
+	adxl372_push_event(indio_dev, ts, status2);
 
 	if (st->fifo_mode != ADXL372_FIFO_BYPASSED &&
 	    ADXL372_STATUS_1_FIFO_FULL(status1)) {
@@ -733,7 +736,9 @@ static irqreturn_t adxl372_trigger_handler(int irq, void  *p)
 			/* filter peak detection data */
 			if (st->peak_fifo_mode_en)
 				adxl372_arrange_axis_data(st, &st->fifo_buf[i]);
-			iio_push_to_buffers(indio_dev, &st->fifo_buf[i]);
+			iio_push_to_buffers_with_ts(indio_dev, &st->fifo_buf[i],
+						    st->fifo_set_size * sizeof(__be16),
+						    ts);
 		}
 	}
 err:
-- 
2.51.1

Re: [PATCH v2] iio: accel: adxl372: Add timestamp to FIFO data

[David Lechner]

On 5/16/26 4:19 PM, Md Shofiqul Islam wrote:
> The driver pushes FIFO samples using iio_push_to_buffers() which does
> not attach a hardware timestamp to the data.  Capture a single timestamp
> per IRQ with iio_get_time_ns() and reuse it for both the event push and
> the FIFO sample loop.
> 
> Use IIO_DECLARE_BUFFER_WITH_TS() for fifo_buf to guarantee the required
> s64 alignment, and switch the FIFO push loop to
> iio_push_to_buffers_with_ts() which accepts an explicit data size and
> timestamp without requiring a separate scan struct or memcpy.
> 
> Add IIO_CHAN_SOFT_TIMESTAMP(3) to the channel spec so the IIO core
> enables scan_timestamp and includes the timestamp slot in the buffer.
> 
> Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
> ---
> Changes in v2:
>   - Use IIO_DECLARE_BUFFER_WITH_TS() + iio_push_to_buffers_with_ts() instead
>     of a separate scan struct + memcpy, per review from David Lechner
>   - Add IIO_CHAN_SOFT_TIMESTAMP(3) to channel spec (missing in v1)

There was other discussion on v1 that said adding a timestamp channel
to FIFO data is not practical since we are receiving multiple data points
all at one time. The timestamp will not be accurate other than the last
data point in each burst.

Did you respond to that?

If there is a good reason for it, the commit message should explain that.

Re: [PATCH v2] iio: accel: adxl372: Add timestamp to FIFO data

[David Lechner]

On 5/16/26 4:19 PM, Md Shofiqul Islam wrote:
> The driver pushes FIFO samples using iio_push_to_buffers() which does
> not attach a hardware timestamp to the data.  Capture a single timestamp
> per IRQ with iio_get_time_ns() and reuse it for both the event push and
> the FIFO sample loop.
> 
> Use IIO_DECLARE_BUFFER_WITH_TS() for fifo_buf to guarantee the required
> s64 alignment, and switch the FIFO push loop to
> iio_push_to_buffers_with_ts() which accepts an explicit data size and
> timestamp without requiring a separate scan struct or memcpy.
> 
> Add IIO_CHAN_SOFT_TIMESTAMP(3) to the channel spec so the IIO core
> enables scan_timestamp and includes the timestamp slot in the buffer.
> 
> Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
> ---
> Changes in v2:
>   - Use IIO_DECLARE_BUFFER_WITH_TS() + iio_push_to_buffers_with_ts() instead
>     of a separate scan struct + memcpy, per review from David Lechner
>   - Add IIO_CHAN_SOFT_TIMESTAMP(3) to channel spec (missing in v1)
>  drivers/iio/accel/adxl372.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
> index e375d068a..266e80e27 100644
> --- a/drivers/iio/accel/adxl372.c
> +++ b/drivers/iio/accel/adxl372.c
> @@ -344,6 +344,7 @@ static const struct iio_chan_spec adxl372_channels[] = {
>  	ADXL372_ACCEL_CHANNEL(0, ADXL372_X_DATA_H, X),
>  	ADXL372_ACCEL_CHANNEL(1, ADXL372_Y_DATA_H, Y),
>  	ADXL372_ACCEL_CHANNEL(2, ADXL372_Z_DATA_H, Z),
> +	IIO_CHAN_SOFT_TIMESTAMP(3),
>  };
>  
>  struct adxl372_state {
> @@ -365,7 +366,7 @@ struct adxl372_state {
>  	u8				fifo_set_size;
>  	unsigned long			int1_bitmask;
>  	u16				watermark;
> -	__be16				fifo_buf[ADXL372_FIFO_SIZE];
> +	IIO_DECLARE_BUFFER_WITH_TS(__be16, fifo_buf, ADXL372_FIFO_SIZE);
>  	bool				peak_fifo_mode_en;
>  	struct mutex			threshold_m; /* lock for threshold */
>  };
> @@ -703,13 +704,15 @@ static irqreturn_t adxl372_trigger_handler(int irq, void  *p)
>  	struct adxl372_state *st = iio_priv(indio_dev);
>  	u8 status1, status2;
>  	u16 fifo_entries;
> +	s64 ts;
>  	int i, ret;
>  
>  	ret = adxl372_get_status(st, &status1, &status2, &fifo_entries);
>  	if (ret < 0)
>  		goto err;
>  
> -	adxl372_push_event(indio_dev, iio_get_time_ns(indio_dev), status2);
> +	ts = iio_get_time_ns(indio_dev);
> +	adxl372_push_event(indio_dev, ts, status2);
>  
>  	if (st->fifo_mode != ADXL372_FIFO_BYPASSED &&
>  	    ADXL372_STATUS_1_FIFO_FULL(status1)) {
> @@ -733,7 +736,9 @@ static irqreturn_t adxl372_trigger_handler(int irq, void  *p)
>  			/* filter peak detection data */
>  			if (st->peak_fifo_mode_en)
>  				adxl372_arrange_axis_data(st, &st->fifo_buf[i]);
> -			iio_push_to_buffers(indio_dev, &st->fifo_buf[i]);
> +			iio_push_to_buffers_with_ts(indio_dev, &st->fifo_buf[i],
> +						    st->fifo_set_size * sizeof(__be16),

Should just be sizeof(st->fifo_buf), otherwise it doesn't
actually protect against buffer overflow.

> +						    ts);
>  		}
>  	}
>  err:

Re: [PATCH v2] iio: accel: adxl372: Add timestamp to FIFO data

[Stepan Ionichev]

On Sat, May 16, 2026, Md Shofiqul Islam wrote:
> +			iio_push_to_buffers_with_ts(indio_dev, &st->fifo_buf[i],
> +						    st->fifo_set_size * sizeof(__be16),
> +						    ts);

On top of David's note about the size argument, the bigger issue is
that iio_push_to_buffers_with_ts() writes the timestamp into the
buffer it was given, at offset (scan_bytes - sizeof(s64)) from the
passed pointer. For the 3-channel scan here scan_bytes is 16, so the
s64 lands at &fifo_buf[i + 4]. With fifo_set_size == 3 that overlaps
the next iteration's samples in the same array, so the following
push reads partially overwritten data.

iio_push_to_buffers_with_ts_unaligned() copies into an internal
aligned bounce buffer instead and does not touch the caller's data,
which fits this looped FIFO-drain pattern. Alternatively, the v1
shape with a small local scan struct and a memcpy per iteration
works too.

Stepan

[PATCH v3] iio: accel: adxl372: Add timestamp to FIFO data

[Md Shofiqul Islam]

Add a software timestamp to each sample set pushed to the IIO buffer
during FIFO readout. IIO_CHAN_SOFT_TIMESTAMP(3) is added to the channel
spec so the IIO core includes the timestamp slot in scan_bytes.

The timestamp is captured once per FIFO watermark interrupt via
iio_get_time_ns() and reused for every sample set drained from the FIFO
in that invocation. This is an approximation: the single timestamp
reflects when the interrupt was handled, which is closest in time to the
last sample in the burst. Earlier samples in the same burst carry an
increasingly stale timestamp. This is acceptable for coarse event
sequencing and consistent with watermark-based FIFO readout in other
IIO accelerometer drivers.

Use iio_push_to_buffers_with_ts_unaligned() rather than
iio_push_to_buffers_with_ts(): the latter writes the s64 timestamp
directly into the source buffer at offset ALIGN(data_sz - 8, 8), which
in a multi-sample FIFO drain loop overwrites FIFO entries not yet
consumed by the next iteration. The _unaligned() variant copies each
sample into an internal bounce buffer and leaves the callers data
intact.

Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
---
Changes since v2:
- Use iio_push_to_buffers_with_ts_unaligned() instead of
  iio_push_to_buffers_with_ts() to avoid writing the timestamp into the
  source buffer during the FIFO drain loop (Stepan Ionichev)
- Update commit message to explain the single-timestamp-per-IRQ
  approximation and why _unaligned() is needed (David Lechner)
 drivers/iio/accel/adxl372.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
index e375d068a..b108b214b 100644
--- a/drivers/iio/accel/adxl372.c
+++ b/drivers/iio/accel/adxl372.c
@@ -344,6 +344,7 @@ static const struct iio_chan_spec adxl372_channels[] = {
 	ADXL372_ACCEL_CHANNEL(0, ADXL372_X_DATA_H, X),
 	ADXL372_ACCEL_CHANNEL(1, ADXL372_Y_DATA_H, Y),
 	ADXL372_ACCEL_CHANNEL(2, ADXL372_Z_DATA_H, Z),
+	IIO_CHAN_SOFT_TIMESTAMP(3),
 };
 
 struct adxl372_state {
@@ -365,7 +366,7 @@ struct adxl372_state {
 	u8				fifo_set_size;
 	unsigned long			int1_bitmask;
 	u16				watermark;
-	__be16				fifo_buf[ADXL372_FIFO_SIZE];
+	IIO_DECLARE_BUFFER_WITH_TS(__be16, fifo_buf, ADXL372_FIFO_SIZE);
 	bool				peak_fifo_mode_en;
 	struct mutex			threshold_m; /* lock for threshold */
 };
@@ -703,13 +704,15 @@ static irqreturn_t adxl372_trigger_handler(int irq, void  *p)
 	struct adxl372_state *st = iio_priv(indio_dev);
 	u8 status1, status2;
 	u16 fifo_entries;
+	s64 ts;
 	int i, ret;
 
 	ret = adxl372_get_status(st, &status1, &status2, &fifo_entries);
 	if (ret < 0)
 		goto err;
 
-	adxl372_push_event(indio_dev, iio_get_time_ns(indio_dev), status2);
+	ts = iio_get_time_ns(indio_dev);
+	adxl372_push_event(indio_dev, ts, status2);
 
 	if (st->fifo_mode != ADXL372_FIFO_BYPASSED &&
 	    ADXL372_STATUS_1_FIFO_FULL(status1)) {
@@ -733,7 +736,9 @@ static irqreturn_t adxl372_trigger_handler(int irq, void  *p)
 			/* filter peak detection data */
 			if (st->peak_fifo_mode_en)
 				adxl372_arrange_axis_data(st, &st->fifo_buf[i]);
-			iio_push_to_buffers(indio_dev, &st->fifo_buf[i]);
+			iio_push_to_buffers_with_ts_unaligned(indio_dev, &st->fifo_buf[i],
+						    st->fifo_set_size * sizeof(__be16),
+						    ts);
 		}
 	}
 err:
-- 
2.51.1

Re: [PATCH v2] iio: accel: adxl372: Add timestamp to FIFO data

[Md Shofiqul Islam]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

Hi David,

Thank you for the two review comments.

Regarding the timestamp accuracy concern from the v1 discussion: the commit
message in v3 now explicitly documents this. The timestamp is captured once
per FIFO watermark interrupt and reused for all samples drained in that
invocation. It is most accurate for the last sample in the burst and
progressively stale for earlier ones. This is noted as an acceptable
approximation for coarse event sequencing, consistent with watermark-based
FIFO readout in other IIO accelerometer drivers.

Regarding the function choice: Stepan Ionichev identified that
iio_push_to_buffers_with_ts() writes the timestamp directly into the source
buffer during the loop, corrupting subsequent FIFO entries. v3 switches to
iio_push_to_buffers_with_ts_unaligned() which copies into an internal bounce
buffer without modifying the caller's data. The size argument remains
st->fifo_set_size * sizeof(__be16) -- the actual per-sample data length --
which _unaligned() caps at min(scan_bytes, data_sz) internally.

v3 is at:
  [PATCH v3] iio: accel: adxl372: Add timestamp to FIFO data
  Message-ID: <20260517091735.135610-1-shofiqtest@gmail.com>

Md Shofiqul Islam

Re: [PATCH v2] iio: accel: adxl372: Add timestamp to FIFO data

[Md Shofiqul Islam]

[-- Attachment #1: Type: text/plain, Size: 949 bytes --]

Hi Stepan,

Thank you for the careful analysis. You are correct -- iio_push_to_buffers_with_ts()
writes the s64 timestamp into the source buffer at offset ALIGN(data_sz - 8, 8),
which in the FIFO drain loop means the write lands in data that has not been
consumed by the next iteration yet. This is indeed a data corruption bug.

v3 switches to iio_push_to_buffers_with_ts_unaligned() which copies the sample
into an internal bounce buffer before adding the timestamp, leaving fifo_buf[]
untouched across iterations. The size argument stays as
st->fifo_set_size * sizeof(__be16) -- the actual per-sample data size -- which
_unaligned() clamps internally to min(scan_bytes, data_sz).

The fix and the explanation of why _unaligned() is needed are in the commit
message of v3:
  [PATCH v3] iio: accel: adxl372: Add timestamp to FIFO data
  Message-ID: <20260517091735.135610-1-shofiqtest@gmail.com>

Thanks again for catching this.

Md Shofiqul Islam

Re: [PATCH v2] iio: accel: adxl372: Add timestamp to FIFO data

[Jonathan Cameron]

On Sun, 17 May 2026 02:19:24 -0700 (PDT)
Md Shofiqul Islam <shofiqtest@gmail.com> wrote:

> Hi David,
> 
> Thank you for the two review comments.
> 
> Regarding the timestamp accuracy concern from the v1 discussion: the commit
> message in v3 now explicitly documents this. The timestamp is captured once
> per FIFO watermark interrupt and reused for all samples drained in that
> invocation. It is most accurate for the last sample in the burst and
> progressively stale for earlier ones. This is noted as an acceptable
> approximation for coarse event sequencing, consistent with watermark-based
> FIFO readout in other IIO accelerometer drivers.

Hi,

No this is not valid under the standard way we handle timestamps for FIFOs
and if we made this approximation in a given driver we would then be more
or less stuck wrt to fixing it in future.  If you wanted to discuss
that more you should have replied to the earlier feedback saying no
to this approach, not wasted time on new versions when such a fundamental
objection ha been raised.

So I'll be ingoring this series and all other series adding timestamps
to FIFOs that repeat a timestamp. 

If you have a usecase for adding this then you need to look at how to
do a decent approximation of timestamps. If this is feature addition
because you feel it might be useful to someone else, then find that
user and work with them.

Also, replies to reviews need to the to the review not to the thread.
Fix that before replying to other reviews as it is making it very hard
to follow the flow of discussion.

Jonathan

> 
> Regarding the function choice: Stepan Ionichev identified that
> iio_push_to_buffers_with_ts() writes the timestamp directly into the source
> buffer during the loop, corrupting subsequent FIFO entries. v3 switches to
> iio_push_to_buffers_with_ts_unaligned() which copies into an internal bounce
> buffer without modifying the caller's data. The size argument remains
> st->fifo_set_size * sizeof(__be16) -- the actual per-sample data length --
> which _unaligned() caps at min(scan_bytes, data_sz) internally.
> 
> v3 is at:
>   [PATCH v3] iio: accel: adxl372: Add timestamp to FIFO data
>   Message-ID: <20260517091735.135610-1-shofiqtest@gmail.com>
> 
> Md Shofiqul Islam