[PATCH RFC 0/9] iio: Introduce new timestamp grabbing APIs

Gyeyoung Baek posted 9 patches 7 months ago
drivers/iio/buffer/industrialio-triggered-buffer.c |  84 ++++++++++++-
drivers/iio/industrialio-trigger.c                 | 135 ++++++++++++++++++++-
drivers/iio/light/rpr0521.c                        |  22 +---
include/linux/iio/trigger.h                        |  16 ++-
include/linux/iio/trigger_consumer.h               |  23 ++++
include/linux/iio/triggered_buffer.h               |  25 ++++
6 files changed, 283 insertions(+), 22 deletions(-)
[PATCH RFC 0/9] iio: Introduce new timestamp grabbing APIs
Posted by Gyeyoung Baek 7 months ago
Support automatic timestamp grabbing by passing `true` to the `timestamp_enabled` parameter of `iio_triggered_buffer_setup_new()`.
So consumer drivers don't need to set `iio_pollfunc_store_time()` as either the tophalf or bottomhalf manually.

For this, triggers must indicate whether they will call `poll()`, `poll_nested()`, or both before
calling `iio_trigger_register()`. This is necessary because the consumer's handler does not know
in advance which trigger will be attached.

Once `iio_trigger_attach_poll_func()` is called, a timestamp is grabbed in either the
tophalf or bottomhalf based on the trigger's type (POLL or POLL_NESTED). If the trigger
supports both (e.g., at91-sama5d2-adc.c), it is treated as POLL_NESTED since the consumer's
tophalf is not invoked in poll_nested(), but the bottomhalf always is.

If the attached trigger supports timestamp grabbing itself, the consumer does not need to handle it.
Instead, the consumer's `poll_func` pointer is passed to the trigger, which can then store the
timestamp directly into consumer. Trigger drivers can pass timestamp values to consumers in a consistent
interface using the new API `iio_trigger_store_time()`.

Tested on qemu, with dummy and trig-sysfs drivers tweaked for testing.

Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
Gyeyoung Baek (9):
      iio: buffer: Fix checkpatch.pl warning
      iio: consumer: Define timestamp-related structures and constants
      iio: consumer: Add new APIs of triggered_buffer_setup() family
      iio: consumer: Add new API iio_poll_func_register()
      iio: consumer: Add new API iio_pollfunc_get_timestamp()
      iio: trigger: Define timetamp-related structures and constants
      iio: trigger: Add new API iio_trigger_attach_timestamp()
      iio: trigger: Add new API iio_trigger_store_time()
      iio: rpr0521: Use new timestamp-related APIs

 drivers/iio/buffer/industrialio-triggered-buffer.c |  84 ++++++++++++-
 drivers/iio/industrialio-trigger.c                 | 135 ++++++++++++++++++++-
 drivers/iio/light/rpr0521.c                        |  22 +---
 include/linux/iio/trigger.h                        |  16 ++-
 include/linux/iio/trigger_consumer.h               |  23 ++++
 include/linux/iio/triggered_buffer.h               |  25 ++++
 6 files changed, 283 insertions(+), 22 deletions(-)
---
base-commit: 43a9eee06bf8a8535d8709b29379bec8cafcab56
change-id: 20250518-timestamp-a899e78e07e3

Best regards,
-- 
Gyeyoung Baek <gye976@gmail.com>
Re: [PATCH RFC 0/9] iio: Introduce new timestamp grabbing APIs
Posted by Jonathan Cameron 6 months, 3 weeks ago
On Mon, 19 May 2025 23:25:52 +0900
Gyeyoung Baek <gye976@gmail.com> wrote:

> Support automatic timestamp grabbing by passing `true` to the `timestamp_enabled` parameter of `iio_triggered_buffer_setup_new()`.
> So consumer drivers don't need to set `iio_pollfunc_store_time()` as either the tophalf or bottomhalf manually.
> 
> For this, triggers must indicate whether they will call `poll()`, `poll_nested()`, or both before
> calling `iio_trigger_register()`. This is necessary because the consumer's handler does not know
> in advance which trigger will be attached.
> 
> Once `iio_trigger_attach_poll_func()` is called, a timestamp is grabbed in either the
> tophalf or bottomhalf based on the trigger's type (POLL or POLL_NESTED). If the trigger
> supports both (e.g., at91-sama5d2-adc.c), it is treated as POLL_NESTED since the consumer's
> tophalf is not invoked in poll_nested(), but the bottomhalf always is.
> 
> If the attached trigger supports timestamp grabbing itself, the consumer does not need to handle it.
> Instead, the consumer's `poll_func` pointer is passed to the trigger, which can then store the
> timestamp directly into consumer. Trigger drivers can pass timestamp values to consumers in a consistent
> interface using the new API `iio_trigger_store_time()`.

This trigger grabbing timestamps thing seems to me to a potential future
optimization.  I'm not seeing why we need it for the fundamental thing we
are addressing here and it is making the patch set more confusing for me
at least.

> 
> Tested on qemu, with dummy and trig-sysfs drivers tweaked for testing.
> 
> Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
> ---
> Gyeyoung Baek (9):
>       iio: buffer: Fix checkpatch.pl warning
>       iio: consumer: Define timestamp-related structures and constants
>       iio: consumer: Add new APIs of triggered_buffer_setup() family
>       iio: consumer: Add new API iio_poll_func_register()
>       iio: consumer: Add new API iio_pollfunc_get_timestamp()
>       iio: trigger: Define timetamp-related structures and constants
>       iio: trigger: Add new API iio_trigger_attach_timestamp()
>       iio: trigger: Add new API iio_trigger_store_time()
>       iio: rpr0521: Use new timestamp-related APIs
> 
>  drivers/iio/buffer/industrialio-triggered-buffer.c |  84 ++++++++++++-
>  drivers/iio/industrialio-trigger.c                 | 135 ++++++++++++++++++++-
>  drivers/iio/light/rpr0521.c                        |  22 +---
>  include/linux/iio/trigger.h                        |  16 ++-
>  include/linux/iio/trigger_consumer.h               |  23 ++++
>  include/linux/iio/triggered_buffer.h               |  25 ++++
>  6 files changed, 283 insertions(+), 22 deletions(-)
> ---
> base-commit: 43a9eee06bf8a8535d8709b29379bec8cafcab56
> change-id: 20250518-timestamp-a899e78e07e3
> 
> Best regards,
Re: [PATCH RFC 0/9] iio: Introduce new timestamp grabbing APIs
Posted by David Lechner 7 months ago
On 5/19/25 9:25 AM, Gyeyoung Baek wrote:
> Support automatic timestamp grabbing by passing `true` to the `timestamp_enabled` parameter of `iio_triggered_buffer_setup_new()`.
> So consumer drivers don't need to set `iio_pollfunc_store_time()` as either the tophalf or bottomhalf manually.
> 
> For this, triggers must indicate whether they will call `poll()`, `poll_nested()`, or both before
> calling `iio_trigger_register()`. This is necessary because the consumer's handler does not know
> in advance which trigger will be attached.
> 
> Once `iio_trigger_attach_poll_func()` is called, a timestamp is grabbed in either the
> tophalf or bottomhalf based on the trigger's type (POLL or POLL_NESTED). If the trigger
> supports both (e.g., at91-sama5d2-adc.c), it is treated as POLL_NESTED since the consumer's
> tophalf is not invoked in poll_nested(), but the bottomhalf always is.
> 
> If the attached trigger supports timestamp grabbing itself, the consumer does not need to handle it.
> Instead, the consumer's `poll_func` pointer is passed to the trigger, which can then store the
> timestamp directly into consumer. Trigger drivers can pass timestamp values to consumers in a consistent
> interface using the new API `iio_trigger_store_time()`.

This is explaining what it does and how it works, but we really want to
know first _why_ we need this and why it is better that what we already
have or what sort of problem this is fixing that the current situation
can't handle.
Re: [PATCH RFC 0/9] iio: Introduce new timestamp grabbing APIs
Posted by Gyeyoung Baek 7 months ago
On Tue, May 20, 2025 at 12:28 AM David Lechner <dlechner@baylibre.com> wrote:
>
> On 5/19/25 9:25 AM, Gyeyoung Baek wrote:
> > Support automatic timestamp grabbing by passing `true` to the `timestamp_enabled` parameter of `iio_triggered_buffer_setup_new()`.
> > So consumer drivers don't need to set `iio_pollfunc_store_time()` as either the tophalf or bottomhalf manually.
> >
> > For this, triggers must indicate whether they will call `poll()`, `poll_nested()`, or both before
> > calling `iio_trigger_register()`. This is necessary because the consumer's handler does not know
> > in advance which trigger will be attached.
> >
> > Once `iio_trigger_attach_poll_func()` is called, a timestamp is grabbed in either the
> > tophalf or bottomhalf based on the trigger's type (POLL or POLL_NESTED). If the trigger
> > supports both (e.g., at91-sama5d2-adc.c), it is treated as POLL_NESTED since the consumer's
> > tophalf is not invoked in poll_nested(), but the bottomhalf always is.
> >
> > If the attached trigger supports timestamp grabbing itself, the consumer does not need to handle it.
> > Instead, the consumer's `poll_func` pointer is passed to the trigger, which can then store the
> > timestamp directly into consumer. Trigger drivers can pass timestamp values to consumers in a consistent
> > interface using the new API `iio_trigger_store_time()`.
>
> This is explaining what it does and how it works, but we really want to
> know first _why_ we need this and why it is better that what we already
> have or what sort of problem this is fixing that the current situation
> can't handle.

Hello David, thanks for the review.
I see that I didn’t explain the reason properly.
The following explains the reason for these patch series.

There are three cases when a timestamp can be grabbed:
1. In the consumer’s top half (which is the most common case, using
`iio_pollfunc_store_time()`),
2. In the consumer’s bottom half,
3. Directly by the trigger before polling the consumer (for drivers
using their own trigger).

Since the consumer can't know what type of trigger will be attached at
runtime, the following two problems can arise:

1. When a trigger that calls `iio_trigger_poll_nested()` instead of
`iio_trigger_poll()`is attached:
most consumer register `iio_pollfunc_store_time()` as top-half
expecting a timestamp, but top-half is not invoked.
And this is not the intended behavior of consumer devices.
2. When a trigger directly provides a timestamp:
The consumer’s handler checks whether a timestamp has already been
grabbed using if statement (like light/rpr0521.c),
or overwrites the existing timestamp even though it was already
provided by the trigger.

This patch series addresses these two issues.

--
Best regards,
Gyeyoung