Re: [PATCH 0/5] iio: accel: adxl3xx: Add timestamps to FIFO data

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 10 May 2026 23:02:00 +0300
Md Shofiqul Islam <shofiqtest@gmail.com> wrote:

> On the ABI question: none of the five drivers in this series define
> IIO_CHAN_SOFT_TIMESTAMP in their channel arrays, and their
> available_scan_masks do not include a timestamp bit.
> As a result, indio_dev->scan_timestamp is never set for any of them, which
> means iio_push_to_buffers_with_timestamp() writes no timestamp and the call
> reduces to iio_push_to_buffers().
> The scan buffer content seen by userspace is byte-for-byte identical before
> and after this series. No ABI is changed.
> 
> That said, this also means the series does not actually deliver timestamps
> to userspace. To do that, a follow-on patch adding
> IIO_CHAN_SOFT_TIMESTAMP(N) to each driver's channel array is required.
> That would be an intentional, explicit ABI addition. Should I include that
> in a v2 of this series?
> 
Yes if you take this forward. However, so far I'm giving this a hard no.

> On hardware testing: I do not have any of the ADXL313/345/367/372/380
> devices. Given that the patches are currently a no-op for userspace the
> risk of a silent regression is low, but real-hardware confirmation is
> needed before this can be considered complete.
> If any maintainer or user of these parts on the list can run a test, I
> would welcome that.
> 
> I also verified that the scan struct alignment is correct for all five
> drivers: the aligned_s64 ts field sits at offset 8 in each case, satisfying
> the 8-byte alignment required by iio_push_to_buffers_with_timestamp().

It's no where near this simple.  Think about how a fifo is working.
You are adding a timestamp based on when the last element that triggered
the fifo watershed was added.  That is wrong for all the other data pulled
off the fifo which should have earlier timestamps.

It is possible to approximate the correct timestamps but it's complex.
See the support form the invensense IMUs which may not even be appropriate
to port to these drivers.

You definitely don't want to try getting timestamps working for a fifo
equipped part without real hardware so you can plot them over time and
verify there are no inconsistencies of bugs in your timestamp approximation
algorithm.

Note the invensense one has been rewritten several times - that should give
you an idea how hard this is to do.

Thanks,

Jonathan

> 
> Thanks
> Shofiq
> 
> On Sun, May 10, 2026 at 3:58 PM Andy Shevchenko <andriy.shevchenko@intel.com>
> wrote:
> 
> > On Sun, May 10, 2026 at 11:25:51AM +0300, Md Shofiqul Islam wrote:  
> > > Five ADXL-family accelerometer drivers (ADXL313, ADXL345, ADXL367,
> > > ADXL372, ADXL380) push buffered samples using iio_push_to_buffers(),
> > > which does not attach a hardware timestamp to the scan data.
> > > Userspace consumers therefore receive samples with no timing
> > > information.
> > >
> > > This series adds timestamp support uniformly across the family:
> > >
> > >   - A scan buffer struct with an aligned_s64 ts field is added to each
> > >     driver's state struct.  The struct layout ensures the timestamp
> > >     field sits at an 8-byte aligned offset as required by
> > >     iio_push_to_buffers_with_timestamp().
> > >
> > >   - In the FIFO push loop, FIFO data is copied into scan.channels via
> > >     memcpy(), then iio_push_to_buffers_with_timestamp() is called with
> > >     a single timestamp captured once per interrupt with
> > >     iio_get_time_ns().  Using one timestamp per IRQ is consistent with
> > >     the existing approach in the same handlers for event timestamps.
> > >
> > >   - For ADXL367, the helper adxl367_push_fifo_data() gains a s64 ts
> > >     parameter so the timestamp captured in the IRQ handler is passed
> > >     through instead of calling iio_get_time_ns() a second time.
> > >
> > >   - For ADXL372 and ADXL380, where the IRQ handler already called
> > >     iio_get_time_ns() for the event push, the same captured timestamp
> > >     is now also passed to the FIFO push, removing the duplicate call.
> > >
> > > The ADXL313 and ADXL345 drivers always scan all three axes together
> > > (available_scan_masks contains only the full X|Y|Z mask), so their
> > > scan buffer layout is fixed.  The ADXL367, ADXL372, and ADXL380
> > > drivers support variable scan masks; fifo_set_size tracks the number
> > > of enabled channels per sample set and is used as the memcpy length.  
> >
> > This is sensitive change. Do we have any confirmation that this
> > - does work as expected on real HW and platforms that use these devices
> > - does not break any ABI
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
> >