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

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

[Gyeyoung Baek]

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>

[PATCH RFC 1/9] iio: buffer: Fix checkpatch.pl warning

[Gyeyoung Baek]

Remove the following trivial warning:
"WARNING: Block comments should align the * on each line"

Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
 drivers/iio/buffer/industrialio-triggered-buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c b/drivers/iio/buffer/industrialio-triggered-buffer.c
index c06515987e7a..9bf75dee7ff8 100644
--- a/drivers/iio/buffer/industrialio-triggered-buffer.c
+++ b/drivers/iio/buffer/industrialio-triggered-buffer.c
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0-only
- /*
+/*
  * Copyright (c) 2012 Analog Devices, Inc.
  *  Author: Lars-Peter Clausen <lars@metafoo.de>
  */

-- 
2.43.0

[PATCH RFC 2/9] iio: consumer: Define timestamp-related structures and constants

[Gyeyoung Baek]

Define the required constants and structures on the consumer side.

The `timestamp_enabled` indicates whether a timestamp is grabbed or not.
This is passed to `iio_triggered_buffer_setup_new()` as an argument.

The `timestamp_type` indicates which handler grabs the timestamp.
This value is set by `iio_poll_func_register()`.

Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
 include/linux/iio/trigger_consumer.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/iio/trigger_consumer.h b/include/linux/iio/trigger_consumer.h
index 2c05dfad88d7..5e6ff8738386 100644
--- a/include/linux/iio/trigger_consumer.h
+++ b/include/linux/iio/trigger_consumer.h
@@ -13,6 +13,13 @@
 struct iio_dev;
 struct iio_trigger;
 
+enum iio_timestamp_type {
+	IIO_TIMESTAMP_TYPE_NONE,
+	IIO_TIMESTAMP_TYPE_CONSUMER_TOP_HALF,
+	IIO_TIMESTAMP_TYPE_CONSUMER_BOTTOM_HALF,
+	IIO_TIMESTAMP_TYPE_TRIGGER,
+};
+
 /**
  * struct iio_poll_func - poll function pair
  *
@@ -26,7 +33,10 @@ struct iio_trigger;
  * @timestamp:			some devices need a timestamp grabbed as soon
  *				as possible after the trigger - hence handler
  *				passes it via here.
+ * @timestamp_type:		indicates which handler grabs the timestamp.
+ * @timestamp_enabled:		if true, automatically grabs the timestamp.
  **/
+
 struct iio_poll_func {
 	struct iio_dev *indio_dev;
 	irqreturn_t (*h)(int irq, void *p);
@@ -35,6 +45,9 @@ struct iio_poll_func {
 	char *name;
 	int irq;
 	s64 timestamp;
+
+	enum iio_timestamp_type timestamp_type;
+	bool timestamp_enabled;
 };
 
 

-- 
2.43.0

[PATCH RFC 3/9] iio: consumer: Add new APIs of triggered_buffer_setup() family

[Gyeyoung Baek]

Add new versions of the `iio_triggered_buffer_setup_ext()` APIs.
(API names are tentative)
	iio_triggered_buffer_setup_new
	iio_triggered_buffer_setup_ext_new
	devm_iio_triggered_buffer_setup_new
	devm_iio_triggered_buffer_setup_ext_new
	iio_alloc_pollfunc_new
these APIs take a bool parameter named `timestamp_enabled`.

Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
 drivers/iio/buffer/industrialio-triggered-buffer.c | 82 ++++++++++++++++++++++
 drivers/iio/industrialio-trigger.c                 | 33 +++++++++
 include/linux/iio/trigger_consumer.h               |  7 ++
 include/linux/iio/triggered_buffer.h               | 25 +++++++
 4 files changed, 147 insertions(+)

diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c b/drivers/iio/buffer/industrialio-triggered-buffer.c
index 9bf75dee7ff8..9b99bf884ccb 100644
--- a/drivers/iio/buffer/industrialio-triggered-buffer.c
+++ b/drivers/iio/buffer/industrialio-triggered-buffer.c
@@ -14,6 +14,68 @@
 #include <linux/iio/triggered_buffer.h>
 #include <linux/iio/trigger_consumer.h>
 
+int iio_triggered_buffer_setup_ext_new(struct iio_dev *indio_dev,
+					irqreturn_t (*thread)(int irq, void *p),
+					bool timestamp_enabled,
+					enum iio_buffer_direction direction,
+					const struct iio_buffer_setup_ops *setup_ops,
+					const struct iio_dev_attr **buffer_attrs)
+{
+	struct iio_buffer *buffer;
+	int ret;
+
+	/*
+	 * iio_triggered_buffer_cleanup() assumes that the buffer allocated here
+	 * is assigned to indio_dev->buffer but this is only the case if this
+	 * function is the first caller to iio_device_attach_buffer(). If
+	 * indio_dev->buffer is already set then we can't proceed otherwise the
+	 * cleanup function will try to free a buffer that was not allocated here.
+	 */
+	if (indio_dev->buffer)
+		return -EADDRINUSE;
+
+	buffer = iio_kfifo_allocate();
+	if (!buffer) {
+		ret = -ENOMEM;
+		goto error_ret;
+	}
+
+	indio_dev->pollfunc = iio_alloc_pollfunc_new(thread,
+							timestamp_enabled,
+							IRQF_ONESHOT,
+							indio_dev,
+							"%s_consumer%d",
+							indio_dev->name,
+							iio_device_id(indio_dev));
+	if (indio_dev->pollfunc == NULL) {
+		ret = -ENOMEM;
+		goto error_kfifo_free;
+	}
+
+	/* Ring buffer functions - here trigger setup related */
+	indio_dev->setup_ops = setup_ops;
+
+	/* Flag that polled ring buffering is possible */
+	indio_dev->modes |= INDIO_BUFFER_TRIGGERED;
+
+	buffer->direction = direction;
+	buffer->attrs = buffer_attrs;
+
+	ret = iio_device_attach_buffer(indio_dev, buffer);
+	if (ret < 0)
+		goto error_dealloc_pollfunc;
+
+	return 0;
+
+error_dealloc_pollfunc:
+	iio_dealloc_pollfunc(indio_dev->pollfunc);
+error_kfifo_free:
+	iio_kfifo_free(buffer);
+error_ret:
+	return ret;
+}
+EXPORT_SYMBOL(iio_triggered_buffer_setup_ext_new);
+
 /**
  * iio_triggered_buffer_setup_ext() - Setup triggered buffer and pollfunc
  * @indio_dev:		IIO device structure
@@ -114,6 +176,26 @@ static void devm_iio_triggered_buffer_clean(void *indio_dev)
 	iio_triggered_buffer_cleanup(indio_dev);
 }
 
+int devm_iio_triggered_buffer_setup_ext_new(struct device *dev,
+						struct iio_dev *indio_dev,
+						irqreturn_t (*thread)(int irq, void *p),
+						bool timestamp_enabled,
+						enum iio_buffer_direction direction,
+						const struct iio_buffer_setup_ops *ops,
+						const struct iio_dev_attr **buffer_attrs)
+{
+	int ret;
+
+	ret = iio_triggered_buffer_setup_ext_new(indio_dev, thread, timestamp_enabled, direction,
+						     ops, buffer_attrs);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, devm_iio_triggered_buffer_clean,
+					indio_dev);
+}
+EXPORT_SYMBOL_GPL(devm_iio_triggered_buffer_setup_ext_new);
+
 int devm_iio_triggered_buffer_setup_ext(struct device *dev,
 					struct iio_dev *indio_dev,
 					irqreturn_t (*h)(int irq, void *p),
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index 54416a384232..527c3cf84be0 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -361,6 +361,39 @@ irqreturn_t iio_pollfunc_store_time(int irq, void *p)
 }
 EXPORT_SYMBOL(iio_pollfunc_store_time);
 
+struct iio_poll_func
+*iio_alloc_pollfunc_new(irqreturn_t (*thread)(int irq, void *p),
+			bool timestamp_enabled,
+			int type,
+			struct iio_dev *indio_dev,
+			const char *fmt,
+			...)
+{
+	va_list vargs;
+	struct iio_poll_func *pf;
+
+	pf = kmalloc(sizeof(*pf), GFP_KERNEL);
+	if (!pf)
+		return NULL;
+	va_start(vargs, fmt);
+	pf->name = kvasprintf(GFP_KERNEL, fmt, vargs);
+	va_end(vargs);
+	if (pf->name == NULL) {
+		kfree(pf);
+		return NULL;
+	}
+	pf->timestamp_enabled = timestamp_enabled;
+	pf->h = NULL;
+	pf->thread = thread;
+	pf->type = type;
+	pf->indio_dev = indio_dev;
+
+	pf->timestamp = 0;
+	pf->timestamp_type = 0;
+	return pf;
+}
+EXPORT_SYMBOL_GPL(iio_alloc_pollfunc_new);
+
 struct iio_poll_func
 *iio_alloc_pollfunc(irqreturn_t (*h)(int irq, void *p),
 		    irqreturn_t (*thread)(int irq, void *p),
diff --git a/include/linux/iio/trigger_consumer.h b/include/linux/iio/trigger_consumer.h
index 5e6ff8738386..213cd8560518 100644
--- a/include/linux/iio/trigger_consumer.h
+++ b/include/linux/iio/trigger_consumer.h
@@ -50,6 +50,13 @@ struct iio_poll_func {
 	bool timestamp_enabled;
 };
 
+__printf(5, 6) struct iio_poll_func
+*iio_alloc_pollfunc_new(irqreturn_t (*thread)(int irq, void *p),
+			bool timestamp_enabled,
+			int type,
+			struct iio_dev *indio_dev,
+			const char *fmt,
+			...);
 
 __printf(5, 6) struct iio_poll_func
 *iio_alloc_pollfunc(irqreturn_t (*h)(int irq, void *p),
diff --git a/include/linux/iio/triggered_buffer.h b/include/linux/iio/triggered_buffer.h
index 29e1fe146879..5648c382a506 100644
--- a/include/linux/iio/triggered_buffer.h
+++ b/include/linux/iio/triggered_buffer.h
@@ -9,6 +9,13 @@ struct iio_dev;
 struct iio_dev_attr;
 struct iio_buffer_setup_ops;
 
+int iio_triggered_buffer_setup_ext_new(struct iio_dev *indio_dev,
+	irqreturn_t (*thread)(int irq, void *p),
+	bool timestamp_enabled,
+	enum iio_buffer_direction direction,
+	const struct iio_buffer_setup_ops *setup_ops,
+	const struct iio_dev_attr **buffer_attrs);
+
 int iio_triggered_buffer_setup_ext(struct iio_dev *indio_dev,
 	irqreturn_t (*h)(int irq, void *p),
 	irqreturn_t (*thread)(int irq, void *p),
@@ -17,11 +24,24 @@ int iio_triggered_buffer_setup_ext(struct iio_dev *indio_dev,
 	const struct iio_dev_attr **buffer_attrs);
 void iio_triggered_buffer_cleanup(struct iio_dev *indio_dev);
 
+#define iio_triggered_buffer_setup_new(indio_dev, h, timestamp_enabled, setup_ops)	\
+	iio_triggered_buffer_setup_ext_new((indio_dev), (h), (timestamp_enabled),	\
+					IIO_BUFFER_DIRECTION_IN, (setup_ops),		\
+					NULL)
+
 #define iio_triggered_buffer_setup(indio_dev, h, thread, setup_ops)		\
 	iio_triggered_buffer_setup_ext((indio_dev), (h), (thread),		\
 					IIO_BUFFER_DIRECTION_IN, (setup_ops),	\
 					NULL)
 
+int devm_iio_triggered_buffer_setup_ext_new(struct device *dev,
+					struct iio_dev *indio_dev,
+					irqreturn_t (*thread)(int irq, void *p),
+					bool timestamp_enabled,
+					enum iio_buffer_direction direction,
+					const struct iio_buffer_setup_ops *ops,
+					const struct iio_dev_attr **buffer_attrs);
+
 int devm_iio_triggered_buffer_setup_ext(struct device *dev,
 					struct iio_dev *indio_dev,
 					irqreturn_t (*h)(int irq, void *p),
@@ -30,6 +50,11 @@ int devm_iio_triggered_buffer_setup_ext(struct device *dev,
 					const struct iio_buffer_setup_ops *ops,
 					const struct iio_dev_attr **buffer_attrs);
 
+#define devm_iio_triggered_buffer_setup_new(dev, indio_dev, thread, timestamp_enabled, setup_ops)	\
+	devm_iio_triggered_buffer_setup_ext_new((dev), (indio_dev), (thread), (timestamp_enabled),	\
+					    IIO_BUFFER_DIRECTION_IN,					\
+					    (setup_ops), NULL)
+
 #define devm_iio_triggered_buffer_setup(dev, indio_dev, h, thread, setup_ops)	\
 	devm_iio_triggered_buffer_setup_ext((dev), (indio_dev), (h), (thread),	\
 					    IIO_BUFFER_DIRECTION_IN,		\

-- 
2.43.0

[PATCH RFC 4/9] iio: consumer: Add new API iio_poll_func_register()

[Gyeyoung Baek]

This API wraps either the tophalf or bottomhalf handler to grab a timestamp,
based on the consumer's `timestamp_enabled` and the trigger's `trig_type` and `early_timestamp`.

Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
 drivers/iio/industrialio-trigger.c | 58 +++++++++++++++++++++++++++++++++++---
 1 file changed, 54 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index 527c3cf84be0..1a7bab2741af 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -65,6 +65,24 @@ ATTRIBUTE_GROUPS(iio_trig_dev);
 
 static struct iio_trigger *__iio_trigger_find_by_name(const char *name);
 
+/* Only be invoked in consumer top-half */
+static irqreturn_t iio_pollfunc_top_half_wrapper(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+
+	pf->timestamp = iio_get_time_ns(pf->indio_dev);
+	return IRQ_WAKE_THREAD;
+}
+
+/* Only be invoked in consumer botom-half */
+static irqreturn_t iio_pollfunc_bottom_half_wrapper(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+
+	pf->timestamp = iio_get_time_ns(pf->indio_dev);
+	return pf->thread(irq, p);
+}
+
 int iio_trigger_register(struct iio_trigger *trig_info)
 {
 	int ret;
@@ -270,6 +288,36 @@ static void iio_trigger_put_irq(struct iio_trigger *trig, int irq)
 	clear_bit(irq - trig->subirq_base, trig->pool);
 }
 
+static int iio_poll_func_register(struct iio_poll_func *pf,
+				  struct iio_trigger *trig)
+{
+	irq_handler_t tophalf = NULL;
+	irq_handler_t bottomhalf = pf->thread;
+	int ret;
+
+	/*
+	 * The consumer does not need timestamp.
+	 * Just request raw irq handler.
+	 */
+	if (!pf->timestamp_enabled)
+		goto out_request_irq;
+
+	if (trig->trig_type & IIO_TRIG_TYPE_POLL_NESTED) {
+		bottomhalf = iio_pollfunc_bottom_half_wrapper;
+		pf->timestamp_type = IIO_TIMESTAMP_TYPE_CONSUMER_BOTTOM_HALF;
+	} else if (trig->trig_type & IIO_TRIG_TYPE_POLL) {
+		tophalf = iio_pollfunc_top_half_wrapper;
+		pf->timestamp_type = IIO_TIMESTAMP_TYPE_CONSUMER_TOP_HALF;
+	} else {
+		pr_err("Trigger does not set valid trig_type.");
+		return -EINVAL;
+	}
+
+out_request_irq:
+	return request_threaded_irq(pf->irq, tophalf, bottomhalf,
+				    pf->type, pf->name, pf);
+}
+
 /* Complexity in here.  With certain triggers (datardy) an acknowledgement
  * may be needed if the pollfuncs do not include the data read for the
  * triggering device.
@@ -296,10 +344,8 @@ int iio_trigger_attach_poll_func(struct iio_trigger *trig,
 		goto out_put_module;
 	}
 
-	/* Request irq */
-	ret = request_threaded_irq(pf->irq, pf->h, pf->thread,
-				   pf->type, pf->name,
-				   pf);
+	/* Register consumer handlers */
+	ret = iio_poll_func_register(pf, trig);
 	if (ret < 0)
 		goto out_put_irq;
 
@@ -352,6 +398,10 @@ int iio_trigger_detach_poll_func(struct iio_trigger *trig,
 	return ret;
 }
 
+/*
+ * Will be deprecated.
+ * We do not need to set this as a top half manually to grab a timestamp.
+ */
 irqreturn_t iio_pollfunc_store_time(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;

-- 
2.43.0

[PATCH RFC 5/9] iio: consumer: Add new API iio_pollfunc_get_timestamp()

[Gyeyoung Baek]

As the timestamp is automatically grabbed,
the consumer can simply read it using this API.

Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
 drivers/iio/industrialio-trigger.c   | 6 ++++++
 include/linux/iio/trigger_consumer.h | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index 1a7bab2741af..d6b0e1ec4153 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -398,6 +398,12 @@ int iio_trigger_detach_poll_func(struct iio_trigger *trig,
 	return ret;
 }
 
+s64 iio_pollfunc_get_timestamp(struct iio_poll_func *pf)
+{
+	return pf->timestamp;
+}
+EXPORT_SYMBOL(iio_pollfunc_get_timestamp);
+
 /*
  * Will be deprecated.
  * We do not need to set this as a top half manually to grab a timestamp.
diff --git a/include/linux/iio/trigger_consumer.h b/include/linux/iio/trigger_consumer.h
index 213cd8560518..279c88cae675 100644
--- a/include/linux/iio/trigger_consumer.h
+++ b/include/linux/iio/trigger_consumer.h
@@ -65,8 +65,11 @@ __printf(5, 6) struct iio_poll_func
 		    struct iio_dev *indio_dev,
 		    const char *fmt,
 		    ...);
+
 void iio_dealloc_pollfunc(struct iio_poll_func *pf);
+
 irqreturn_t iio_pollfunc_store_time(int irq, void *p);
+s64 iio_pollfunc_get_timestamp(struct iio_poll_func *pf);
 
 void iio_trigger_notify_done(struct iio_trigger *trig);
 

-- 
2.43.0

[PATCH RFC 6/9] iio: trigger: Define timetamp-related structures and constants

[Gyeyoung Baek]

The `trig_type` indicates whether the trigger calls poll() or poll_nested().
The `early_timestamp` indicates whether the trigger grabs the timestamp at the trigger.
We need this to prevent the consumer from overwriting the timestamp.

To allow the trigger to directly write the timestamp into the consumer's poll_func,
add poll_func pointer member to the iio_trigger structure.

However, I'm not sure if having a poll_func pointer member
in iio_trigger is a good approach.
Would this approach be acceptable?

Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
 include/linux/iio/trigger.h | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h
index bce3b1788199..f3b89a1e0318 100644
--- a/include/linux/iio/trigger.h
+++ b/include/linux/iio/trigger.h
@@ -36,6 +36,10 @@ struct iio_trigger_ops {
 			       struct iio_dev *indio_dev);
 };
 
+#define IIO_TRIG_TYPE_POLL		BIT(0)
+#define IIO_TRIG_TYPE_POLL_NESTED	BIT(1)
+#define IIO_TRIG_TYPE_BOTH		(IIO_TRIG_TYPE_POLL | \
+					IIO_TRIG_TYPE_POLL_NESTED)
 
 /**
  * struct iio_trigger - industrial I/O trigger device
@@ -56,7 +60,10 @@ struct iio_trigger_ops {
  *			i.e. if we registered a poll function to the same
  *			device as the one providing the trigger.
  * @reenable_work:	[INTERN] work item used to ensure reenable can sleep.
+ * @trig_type:		[DRIVER] specifies whether the trigger calls poll(), poll_nested(), or both.
+ * @early_timestamp:	[DRIVER] set to true if the trigger supports grabbing timestamp.
  **/
+
 struct iio_trigger {
 	const struct iio_trigger_ops	*ops;
 	struct module			*owner;
@@ -76,8 +83,13 @@ struct iio_trigger {
 	struct mutex			pool_lock;
 	bool				attached_own_device;
 	struct work_struct		reenable_work;
-};
 
+	/* RFC, exists to access the consumer device’s pollfunc. */
+	struct iio_poll_func *consumer_pf[CONFIG_IIO_CONSUMERS_PER_TRIGGER];
+
+	int trig_type;
+	bool early_timestamp;
+};
 
 static inline struct iio_trigger *to_iio_trigger(struct device *d)
 {

-- 
2.43.0

[PATCH RFC 7/9] iio: trigger: Add new API iio_trigger_attach_timestamp()

[Gyeyoung Baek]

This new API is used in `iio_poll_func_register()` to handle the case
where the trigger writes a timestamp directly into the consumer (i.e. early_timestamp == true).

Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
 drivers/iio/industrialio-trigger.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index d6b0e1ec4153..f394933b9d0a 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -288,6 +288,15 @@ static void iio_trigger_put_irq(struct iio_trigger *trig, int irq)
 	clear_bit(irq - trig->subirq_base, trig->pool);
 }
 
+static int iio_trigger_attach_timestamp(struct iio_trigger *trig,
+					  struct iio_poll_func *pf)
+{
+	/* RFC */
+	trig->consumer_pf[pf->irq - trig->subirq_base] = pf;
+
+	return 0;
+}
+
 static int iio_poll_func_register(struct iio_poll_func *pf,
 				  struct iio_trigger *trig)
 {
@@ -302,6 +311,16 @@ static int iio_poll_func_register(struct iio_poll_func *pf,
 	if (!pf->timestamp_enabled)
 		goto out_request_irq;
 
+	/*
+	 * The trigger supports grabbing timestamp.
+	 * Just request raw irq handler.
+	 */
+	if (trig->early_timestamp) {
+		ret = iio_trigger_attach_timestamp(trig, pf);
+		pf->timestamp_type = IIO_TIMESTAMP_TYPE_TRIGGER;
+		goto out_request_irq;
+	}
+
 	if (trig->trig_type & IIO_TRIG_TYPE_POLL_NESTED) {
 		bottomhalf = iio_pollfunc_bottom_half_wrapper;
 		pf->timestamp_type = IIO_TIMESTAMP_TYPE_CONSUMER_BOTTOM_HALF;
@@ -395,6 +414,9 @@ int iio_trigger_detach_poll_func(struct iio_trigger *trig,
 	module_put(iio_dev_opaque->driver_module);
 	pf->irq = 0;
 
+	/* RFC */
+	trig->consumer_pf[pf->irq - trig->subirq_base] = NULL;
+
 	return ret;
 }
 

-- 
2.43.0

[PATCH RFC 8/9] iio: trigger: Add new API iio_trigger_store_time()

[Gyeyoung Baek]

Now the trigger can simply call `iio_trigger_store_time()`
to pass a timestamp to the consumer.

Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
 drivers/iio/industrialio-trigger.c | 16 ++++++++++++++++
 include/linux/iio/trigger.h        |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index f394933b9d0a..a961156f0eeb 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -288,6 +288,22 @@ static void iio_trigger_put_irq(struct iio_trigger *trig, int irq)
 	clear_bit(irq - trig->subirq_base, trig->pool);
 }
 
+void iio_trigger_store_time(struct iio_trigger *trig)
+{
+	WARN_ON(!trig->early_timestamp);
+
+	for (int i = 0; i < CONFIG_IIO_CONSUMERS_PER_TRIGGER; i++) {
+		struct iio_poll_func *pf = trig->consumer_pf[i];
+
+		if (pf) {
+			WARN_ON(pf->timestamp_type != IIO_TIMESTAMP_TYPE_TRIGGER);
+
+			pf->timestamp = iio_get_time_ns(pf->indio_dev);
+		}
+	}
+}
+EXPORT_SYMBOL(iio_trigger_store_time);
+
 static int iio_trigger_attach_timestamp(struct iio_trigger *trig,
 					  struct iio_poll_func *pf)
 {
diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h
index f3b89a1e0318..8048a2c69971 100644
--- a/include/linux/iio/trigger.h
+++ b/include/linux/iio/trigger.h
@@ -187,6 +187,8 @@ int iio_validate_own_trigger(struct iio_dev *idev, struct iio_trigger *trig);
 int iio_trigger_validate_own_device(struct iio_trigger *trig,
 				     struct iio_dev *indio_dev);
 
+void iio_trigger_store_time(struct iio_trigger *trig);
+
 #else
 struct iio_trigger;
 struct iio_trigger_ops;

-- 
2.43.0

[PATCH RFC 9/9] iio: rpr0521: Use new timestamp-related APIs

[Gyeyoung Baek]

This patch is an example that helps explain the previous series.
Now there is no need to handle timestamps differently depending on whether
the consumer is attached to its own trigger or to another trigger.
And the own trigger of rpr0521 can simply pass a timestamp to consumer,
using `iio_trigger_store_time()`

Not tested since I don't have the corresponding device.

Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
---
 drivers/iio/light/rpr0521.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
index 92e7552f3e39..32981db18428 100644
--- a/drivers/iio/light/rpr0521.c
+++ b/drivers/iio/light/rpr0521.c
@@ -186,7 +186,6 @@ struct rpr0521_data {
 	bool pxs_dev_en;
 
 	struct iio_trigger *drdy_trigger0;
-	s64 irq_timestamp;
 
 	/* optimize runtime pm ops - enable/disable device only if needed */
 	bool als_ps_need_en;
@@ -416,7 +415,7 @@ static irqreturn_t rpr0521_drdy_irq_handler(int irq, void *private)
 	struct iio_dev *indio_dev = private;
 	struct rpr0521_data *data = iio_priv(indio_dev);
 
-	data->irq_timestamp = iio_get_time_ns(indio_dev);
+	iio_trigger_store_time(data->drdy_trigger0);
 	/*
 	 * We need to wake the thread to read the interrupt reg. It
 	 * is not possible to do that here because regmap_read takes a
@@ -446,15 +445,6 @@ static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
 	struct rpr0521_data *data = iio_priv(indio_dev);
 	int err;
 
-	/* Use irq timestamp when reasonable. */
-	if (iio_trigger_using_own(indio_dev) && data->irq_timestamp) {
-		pf->timestamp = data->irq_timestamp;
-		data->irq_timestamp = 0;
-	}
-	/* Other chained trigger polls get timestamp only here. */
-	if (!pf->timestamp)
-		pf->timestamp = iio_get_time_ns(indio_dev);
-
 	err = regmap_bulk_read(data->regmap, RPR0521_REG_PXS_DATA,
 		data->scan.channels,
 		(3 * 2) + 1);	/* 3 * 16-bit + (discarded) int clear reg. */
@@ -464,7 +454,6 @@ static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
 	else
 		dev_err(&data->client->dev,
 			"Trigger consumer can't read from sensor.\n");
-	pf->timestamp = 0;
 
 	iio_trigger_notify_done(indio_dev->trig);
 
@@ -867,8 +856,6 @@ static int rpr0521_init(struct rpr0521_data *data)
 		return ret;
 #endif
 
-	data->irq_timestamp = 0;
-
 	return 0;
 }
 
@@ -984,6 +971,9 @@ static int rpr0521_probe(struct i2c_client *client)
 			goto err_pm_disable;
 		}
 		data->drdy_trigger0->ops = &rpr0521_trigger_ops;
+		data->drdy_trigger0->early_timestamp = true;
+		data->drdy_trigger0->trig_type = IIO_TRIG_TYPE_POLL_NESTED;
+
 		indio_dev->available_scan_masks = rpr0521_available_scan_masks;
 		iio_trigger_set_drvdata(data->drdy_trigger0, indio_dev);
 
@@ -1011,10 +1001,10 @@ static int rpr0521_probe(struct i2c_client *client)
 		 */
 
 		/* Trigger consumer setup */
-		ret = devm_iio_triggered_buffer_setup(indio_dev->dev.parent,
+		ret = devm_iio_triggered_buffer_setup_new(indio_dev->dev.parent,
 			indio_dev,
-			iio_pollfunc_store_time,
 			rpr0521_trigger_consumer_handler,
+			true,
 			&rpr0521_buffer_setup_ops);
 		if (ret < 0) {
 			dev_err(&client->dev, "iio triggered buffer setup failed\n");

-- 
2.43.0

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

[David Lechner]

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

[Gyeyoung Baek]

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

Re: [PATCH RFC 1/9] iio: buffer: Fix checkpatch.pl warning

[Jonathan Cameron]

On Mon, 19 May 2025 23:25:53 +0900
Gyeyoung Baek <gye976@gmail.com> wrote:

> Remove the following trivial warning:
> "WARNING: Block comments should align the * on each line"
> 
> Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
Applied.

As a general rule don't send unrelated cleanup in an RFC series
doing something interesting!  They might get missed.

Jonathan
> ---
>  drivers/iio/buffer/industrialio-triggered-buffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c b/drivers/iio/buffer/industrialio-triggered-buffer.c
> index c06515987e7a..9bf75dee7ff8 100644
> --- a/drivers/iio/buffer/industrialio-triggered-buffer.c
> +++ b/drivers/iio/buffer/industrialio-triggered-buffer.c
> @@ -1,5 +1,5 @@
>  // SPDX-License-Identifier: GPL-2.0-only
> - /*
> +/*
>   * Copyright (c) 2012 Analog Devices, Inc.
>   *  Author: Lars-Peter Clausen <lars@metafoo.de>
>   */
> 

Re: [PATCH RFC 1/9] iio: buffer: Fix checkpatch.pl warning

[Gyeyoung Baek]

On Mon, May 26, 2025 at 2:35 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 19 May 2025 23:25:53 +0900
> Gyeyoung Baek <gye976@gmail.com> wrote:
>
> > Remove the following trivial warning:
> > "WARNING: Block comments should align the * on each line"
> >
> > Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
> Applied.
>
> As a general rule don't send unrelated cleanup in an RFC series
> doing something interesting!  They might get missed.

Well, since the patches modify the same file, I considered them
dependent and grouped them into a single series. But now realize it
would be more appropriate to split patches logically.
Thanks for pointing it out.

Gyeyoung

> Jonathan
> > ---
> >  drivers/iio/buffer/industrialio-triggered-buffer.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c b/drivers/iio/buffer/industrialio-triggered-buffer.c
> > index c06515987e7a..9bf75dee7ff8 100644
> > --- a/drivers/iio/buffer/industrialio-triggered-buffer.c
> > +++ b/drivers/iio/buffer/industrialio-triggered-buffer.c
> > @@ -1,5 +1,5 @@
> >  // SPDX-License-Identifier: GPL-2.0-only
> > - /*
> > +/*
> >   * Copyright (c) 2012 Analog Devices, Inc.
> >   *  Author: Lars-Peter Clausen <lars@metafoo.de>
> >   */
> >
>

Re: [PATCH RFC 1/9] iio: buffer: Fix checkpatch.pl warning

[Jonathan Cameron]

On Mon, 26 May 2025 14:30:41 +0900
Gyeyoung Baek <gye976@gmail.com> wrote:

> On Mon, May 26, 2025 at 2:35 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 19 May 2025 23:25:53 +0900
> > Gyeyoung Baek <gye976@gmail.com> wrote:
> >  
> > > Remove the following trivial warning:
> > > "WARNING: Block comments should align the * on each line"
> > >
> > > Signed-off-by: Gyeyoung Baek <gye976@gmail.com>  
> > Applied.
> >
> > As a general rule don't send unrelated cleanup in an RFC series
> > doing something interesting!  They might get missed.  
> 
> Well, since the patches modify the same file, I considered them
> dependent and grouped them into a single series. But now realize it
> would be more appropriate to split patches logically.
> Thanks for pointing it out.
If there was a chance of a merge conflict I'd agree with you
but I'd be very surprised to see one with this change given where
it is in the file.

No problem though, the only result of too much grouping is
things might not go in as quickly!

Jonathan

> 
> Gyeyoung
> 
> > Jonathan  
> > > ---
> > >  drivers/iio/buffer/industrialio-triggered-buffer.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c b/drivers/iio/buffer/industrialio-triggered-buffer.c
> > > index c06515987e7a..9bf75dee7ff8 100644
> > > --- a/drivers/iio/buffer/industrialio-triggered-buffer.c
> > > +++ b/drivers/iio/buffer/industrialio-triggered-buffer.c
> > > @@ -1,5 +1,5 @@
> > >  // SPDX-License-Identifier: GPL-2.0-only
> > > - /*
> > > +/*
> > >   * Copyright (c) 2012 Analog Devices, Inc.
> > >   *  Author: Lars-Peter Clausen <lars@metafoo.de>
> > >   */
> > >  
> >  
> 

Re: [PATCH RFC 2/9] iio: consumer: Define timestamp-related structures and constants

[Jonathan Cameron]

On Mon, 19 May 2025 23:25:54 +0900
Gyeyoung Baek <gye976@gmail.com> wrote:

> Define the required constants and structures on the consumer side.
> 
> The `timestamp_enabled` indicates whether a timestamp is grabbed or not.
> This is passed to `iio_triggered_buffer_setup_new()` as an argument.
> 
> The `timestamp_type` indicates which handler grabs the timestamp.
> This value is set by `iio_poll_func_register()`.
> 
> Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
> ---
>  include/linux/iio/trigger_consumer.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/include/linux/iio/trigger_consumer.h b/include/linux/iio/trigger_consumer.h
> index 2c05dfad88d7..5e6ff8738386 100644
> --- a/include/linux/iio/trigger_consumer.h
> +++ b/include/linux/iio/trigger_consumer.h
> @@ -13,6 +13,13 @@
>  struct iio_dev;
>  struct iio_trigger;
>  
> +enum iio_timestamp_type {
> +	IIO_TIMESTAMP_TYPE_NONE,
> +	IIO_TIMESTAMP_TYPE_CONSUMER_TOP_HALF,
> +	IIO_TIMESTAMP_TYPE_CONSUMER_BOTTOM_HALF,
> +	IIO_TIMESTAMP_TYPE_TRIGGER,
> +};

This needs documentation. I'm struggling even with the series in front of me
to understand what each of these means. The comment below helps somewhat
but we should have it alongside the enum.

> +
>  /**
>   * struct iio_poll_func - poll function pair
>   *
> @@ -26,7 +33,10 @@ struct iio_trigger;
>   * @timestamp:			some devices need a timestamp grabbed as soon
>   *				as possible after the trigger - hence handler
>   *				passes it via here.
> + * @timestamp_type:		indicates which handler grabs the timestamp.
> + * @timestamp_enabled:		if true, automatically grabs the timestamp.
>   **/
> +
>  struct iio_poll_func {
>  	struct iio_dev *indio_dev;
>  	irqreturn_t (*h)(int irq, void *p);
> @@ -35,6 +45,9 @@ struct iio_poll_func {
>  	char *name;
>  	int irq;
>  	s64 timestamp;
> +
> +	enum iio_timestamp_type timestamp_type;
> +	bool timestamp_enabled;
>  };
>  
>  
> 

Re: [PATCH RFC 6/9] iio: trigger: Define timetamp-related structures and constants

[Jonathan Cameron]

On Mon, 19 May 2025 23:25:58 +0900
Gyeyoung Baek <gye976@gmail.com> wrote:

> The `trig_type` indicates whether the trigger calls poll() or poll_nested().
> The `early_timestamp` indicates whether the trigger grabs the timestamp at the trigger.
> We need this to prevent the consumer from overwriting the timestamp.
> 
> To allow the trigger to directly write the timestamp into the consumer's poll_func,
> add poll_func pointer member to the iio_trigger structure.
> 
> However, I'm not sure if having a poll_func pointer member
> in iio_trigger is a good approach.
> Would this approach be acceptable?

I need to think about that.  My initial thought was that it crosses boundaries
that we really don't want to cross.

I'm not sure I yet understand why we ever want the trigger to do that
write of the timestamp. Why are the wrapping handlers not enough?

> 
> Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
> ---
>  include/linux/iio/trigger.h | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/iio/trigger.h b/include/linux/iio/trigger.h
> index bce3b1788199..f3b89a1e0318 100644
> --- a/include/linux/iio/trigger.h
> +++ b/include/linux/iio/trigger.h
> @@ -36,6 +36,10 @@ struct iio_trigger_ops {
>  			       struct iio_dev *indio_dev);
>  };
>  
> +#define IIO_TRIG_TYPE_POLL		BIT(0)
> +#define IIO_TRIG_TYPE_POLL_NESTED	BIT(1)
> +#define IIO_TRIG_TYPE_BOTH		(IIO_TRIG_TYPE_POLL | \
> +					IIO_TRIG_TYPE_POLL_NESTED)

I'm lost. How does a given trigger do both?

Also didn't these get used in an earlier patch?  Even for an RFC make
sure it bisects.

>  
>  /**
>   * struct iio_trigger - industrial I/O trigger device
> @@ -56,7 +60,10 @@ struct iio_trigger_ops {
>   *			i.e. if we registered a poll function to the same
>   *			device as the one providing the trigger.
>   * @reenable_work:	[INTERN] work item used to ensure reenable can sleep.
> + * @trig_type:		[DRIVER] specifies whether the trigger calls poll(), poll_nested(), or both.
> + * @early_timestamp:	[DRIVER] set to true if the trigger supports grabbing timestamp.
>   **/
> +
Stray blank line.
>  struct iio_trigger {
>  	const struct iio_trigger_ops	*ops;
>  	struct module			*owner;
> @@ -76,8 +83,13 @@ struct iio_trigger {
>  	struct mutex			pool_lock;
>  	bool				attached_own_device;
>  	struct work_struct		reenable_work;
> -};
>  
> +	/* RFC, exists to access the consumer device’s pollfunc. */
> +	struct iio_poll_func *consumer_pf[CONFIG_IIO_CONSUMERS_PER_TRIGGER];
> +
> +	int trig_type;
> +	bool early_timestamp;
> +};
>  
>  static inline struct iio_trigger *to_iio_trigger(struct device *d)
>  {
> 

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

[Jonathan Cameron]

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 9/9] iio: rpr0521: Use new timestamp-related APIs

[Jonathan Cameron]

On Mon, 19 May 2025 23:26:01 +0900
Gyeyoung Baek <gye976@gmail.com> wrote:

> This patch is an example that helps explain the previous series.
> Now there is no need to handle timestamps differently depending on whether
> the consumer is attached to its own trigger or to another trigger.
> And the own trigger of rpr0521 can simply pass a timestamp to consumer,
> using `iio_trigger_store_time()`
> 
> Not tested since I don't have the corresponding device.
> 
> Signed-off-by: Gyeyoung Baek <gye976@gmail.com>

Ah. So this is what the trigger grabbing timestamp thing is about.

I'd just ignore this case for now and solve the simpler one
of what to do the the pf->timestamp stuff.

That part of the patch set looks promising to me.

Jonathan


> ---
>  drivers/iio/light/rpr0521.c | 22 ++++++----------------
>  1 file changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> index 92e7552f3e39..32981db18428 100644
> --- a/drivers/iio/light/rpr0521.c
> +++ b/drivers/iio/light/rpr0521.c
> @@ -186,7 +186,6 @@ struct rpr0521_data {
>  	bool pxs_dev_en;
>  
>  	struct iio_trigger *drdy_trigger0;
> -	s64 irq_timestamp;
>  
>  	/* optimize runtime pm ops - enable/disable device only if needed */
>  	bool als_ps_need_en;
> @@ -416,7 +415,7 @@ static irqreturn_t rpr0521_drdy_irq_handler(int irq, void *private)
>  	struct iio_dev *indio_dev = private;
>  	struct rpr0521_data *data = iio_priv(indio_dev);
>  
> -	data->irq_timestamp = iio_get_time_ns(indio_dev);
> +	iio_trigger_store_time(data->drdy_trigger0);
>  	/*
>  	 * We need to wake the thread to read the interrupt reg. It
>  	 * is not possible to do that here because regmap_read takes a
> @@ -446,15 +445,6 @@ static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
>  	struct rpr0521_data *data = iio_priv(indio_dev);
>  	int err;
>  
> -	/* Use irq timestamp when reasonable. */
> -	if (iio_trigger_using_own(indio_dev) && data->irq_timestamp) {
> -		pf->timestamp = data->irq_timestamp;
> -		data->irq_timestamp = 0;
> -	}
> -	/* Other chained trigger polls get timestamp only here. */
> -	if (!pf->timestamp)
> -		pf->timestamp = iio_get_time_ns(indio_dev);
> -
>  	err = regmap_bulk_read(data->regmap, RPR0521_REG_PXS_DATA,
>  		data->scan.channels,
>  		(3 * 2) + 1);	/* 3 * 16-bit + (discarded) int clear reg. */
> @@ -464,7 +454,6 @@ static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
>  	else
>  		dev_err(&data->client->dev,
>  			"Trigger consumer can't read from sensor.\n");
> -	pf->timestamp = 0;
>  
>  	iio_trigger_notify_done(indio_dev->trig);
>  
> @@ -867,8 +856,6 @@ static int rpr0521_init(struct rpr0521_data *data)
>  		return ret;
>  #endif
>  
> -	data->irq_timestamp = 0;
> -
>  	return 0;
>  }
>  
> @@ -984,6 +971,9 @@ static int rpr0521_probe(struct i2c_client *client)
>  			goto err_pm_disable;
>  		}
>  		data->drdy_trigger0->ops = &rpr0521_trigger_ops;
> +		data->drdy_trigger0->early_timestamp = true;
> +		data->drdy_trigger0->trig_type = IIO_TRIG_TYPE_POLL_NESTED;
> +
>  		indio_dev->available_scan_masks = rpr0521_available_scan_masks;
>  		iio_trigger_set_drvdata(data->drdy_trigger0, indio_dev);
>  
> @@ -1011,10 +1001,10 @@ static int rpr0521_probe(struct i2c_client *client)
>  		 */
>  
>  		/* Trigger consumer setup */
> -		ret = devm_iio_triggered_buffer_setup(indio_dev->dev.parent,
> +		ret = devm_iio_triggered_buffer_setup_new(indio_dev->dev.parent,
>  			indio_dev,
> -			iio_pollfunc_store_time,
>  			rpr0521_trigger_consumer_handler,
> +			true,
>  			&rpr0521_buffer_setup_ops);
>  		if (ret < 0) {
>  			dev_err(&client->dev, "iio triggered buffer setup failed\n");
> 

Re: [PATCH RFC 3/9] iio: consumer: Add new APIs of triggered_buffer_setup() family

[Jonathan Cameron]

On Mon, 19 May 2025 23:25:55 +0900
Gyeyoung Baek <gye976@gmail.com> wrote:

> Add new versions of the `iio_triggered_buffer_setup_ext()` APIs.
> (API names are tentative)
> 	iio_triggered_buffer_setup_new
> 	iio_triggered_buffer_setup_ext_new
> 	devm_iio_triggered_buffer_setup_new
> 	devm_iio_triggered_buffer_setup_ext_new
> 	iio_alloc_pollfunc_new
> these APIs take a bool parameter named `timestamp_enabled`.
> 
> Signed-off-by: Gyeyoung Baek <gye976@gmail.com>

See if you can find a way (probably a common shared function) to make it
more obvious that these are very nearly the same as the existing code.

Right now it is harder than I'd like to spot the differences.


> ---
>  drivers/iio/buffer/industrialio-triggered-buffer.c | 82 ++++++++++++++++++++++
>  drivers/iio/industrialio-trigger.c                 | 33 +++++++++
>  include/linux/iio/trigger_consumer.h               |  7 ++
>  include/linux/iio/triggered_buffer.h               | 25 +++++++
>  4 files changed, 147 insertions(+)
> 
> diff --git a/drivers/iio/buffer/industrialio-triggered-buffer.c b/drivers/iio/buffer/industrialio-triggered-buffer.c
> index 9bf75dee7ff8..9b99bf884ccb 100644
> --- a/drivers/iio/buffer/industrialio-triggered-buffer.c
> +++ b/drivers/iio/buffer/industrialio-triggered-buffer.c
> @@ -14,6 +14,68 @@
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/iio/trigger_consumer.h>
>  
> +int iio_triggered_buffer_setup_ext_new(struct iio_dev *indio_dev,
> +					irqreturn_t (*thread)(int irq, void *p),
> +					bool timestamp_enabled,
> +					enum iio_buffer_direction direction,
> +					const struct iio_buffer_setup_ops *setup_ops,
> +					const struct iio_dev_attr **buffer_attrs)
> +{
> +	struct iio_buffer *buffer;
> +	int ret;
> +
> +	/*
> +	 * iio_triggered_buffer_cleanup() assumes that the buffer allocated here
> +	 * is assigned to indio_dev->buffer but this is only the case if this
> +	 * function is the first caller to iio_device_attach_buffer(). If
> +	 * indio_dev->buffer is already set then we can't proceed otherwise the
> +	 * cleanup function will try to free a buffer that was not allocated here.
> +	 */
> +	if (indio_dev->buffer)
> +		return -EADDRINUSE;
> +
> +	buffer = iio_kfifo_allocate();
> +	if (!buffer) {
> +		ret = -ENOMEM;
> +		goto error_ret;
> +	}
> +
> +	indio_dev->pollfunc = iio_alloc_pollfunc_new(thread,
> +							timestamp_enabled,
> +							IRQF_ONESHOT,
> +							indio_dev,
> +							"%s_consumer%d",
> +							indio_dev->name,
> +							iio_device_id(indio_dev));
> +	if (indio_dev->pollfunc == NULL) {
> +		ret = -ENOMEM;
> +		goto error_kfifo_free;
> +	}
> +
> +	/* Ring buffer functions - here trigger setup related */
> +	indio_dev->setup_ops = setup_ops;
> +
> +	/* Flag that polled ring buffering is possible */
> +	indio_dev->modes |= INDIO_BUFFER_TRIGGERED;
> +
> +	buffer->direction = direction;
> +	buffer->attrs = buffer_attrs;
> +
> +	ret = iio_device_attach_buffer(indio_dev, buffer);
> +	if (ret < 0)
> +		goto error_dealloc_pollfunc;
> +
> +	return 0;
> +
> +error_dealloc_pollfunc:
> +	iio_dealloc_pollfunc(indio_dev->pollfunc);
> +error_kfifo_free:
> +	iio_kfifo_free(buffer);
> +error_ret:
> +	return ret;
> +}
> +EXPORT_SYMBOL(iio_triggered_buffer_setup_ext_new);
> +
>  /**
>   * iio_triggered_buffer_setup_ext() - Setup triggered buffer and pollfunc
>   * @indio_dev:		IIO device structure
> @@ -114,6 +176,26 @@ static void devm_iio_triggered_buffer_clean(void *indio_dev)
>  	iio_triggered_buffer_cleanup(indio_dev);
>  }
>  
> +int devm_iio_triggered_buffer_setup_ext_new(struct device *dev,
> +						struct iio_dev *indio_dev,
> +						irqreturn_t (*thread)(int irq, void *p),
> +						bool timestamp_enabled,
> +						enum iio_buffer_direction direction,
> +						const struct iio_buffer_setup_ops *ops,
> +						const struct iio_dev_attr **buffer_attrs)
> +{
> +	int ret;
> +
> +	ret = iio_triggered_buffer_setup_ext_new(indio_dev, thread, timestamp_enabled, direction,
> +						     ops, buffer_attrs);
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action_or_reset(dev, devm_iio_triggered_buffer_clean,
> +					indio_dev);
> +}
> +EXPORT_SYMBOL_GPL(devm_iio_triggered_buffer_setup_ext_new);
> +
>  int devm_iio_triggered_buffer_setup_ext(struct device *dev,
>  					struct iio_dev *indio_dev,
>  					irqreturn_t (*h)(int irq, void *p),
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index 54416a384232..527c3cf84be0 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -361,6 +361,39 @@ irqreturn_t iio_pollfunc_store_time(int irq, void *p)
>  }
>  EXPORT_SYMBOL(iio_pollfunc_store_time);
>  
> +struct iio_poll_func
> +*iio_alloc_pollfunc_new(irqreturn_t (*thread)(int irq, void *p),
> +			bool timestamp_enabled,
> +			int type,
> +			struct iio_dev *indio_dev,
> +			const char *fmt,
> +			...)
> +{
> +	va_list vargs;
> +	struct iio_poll_func *pf;
> +
> +	pf = kmalloc(sizeof(*pf), GFP_KERNEL);
> +	if (!pf)
> +		return NULL;
> +	va_start(vargs, fmt);
> +	pf->name = kvasprintf(GFP_KERNEL, fmt, vargs);
> +	va_end(vargs);
> +	if (pf->name == NULL) {
> +		kfree(pf);
> +		return NULL;
> +	}
> +	pf->timestamp_enabled = timestamp_enabled;
> +	pf->h = NULL;
> +	pf->thread = thread;
> +	pf->type = type;
> +	pf->indio_dev = indio_dev;
> +
> +	pf->timestamp = 0;
> +	pf->timestamp_type = 0;
> +	return pf;
> +}
> +EXPORT_SYMBOL_GPL(iio_alloc_pollfunc_new);
> +
>  struct iio_poll_func
>  *iio_alloc_pollfunc(irqreturn_t (*h)(int irq, void *p),
>  		    irqreturn_t (*thread)(int irq, void *p),
> diff --git a/include/linux/iio/trigger_consumer.h b/include/linux/iio/trigger_consumer.h
> index 5e6ff8738386..213cd8560518 100644
> --- a/include/linux/iio/trigger_consumer.h
> +++ b/include/linux/iio/trigger_consumer.h
> @@ -50,6 +50,13 @@ struct iio_poll_func {
>  	bool timestamp_enabled;
>  };
>  
> +__printf(5, 6) struct iio_poll_func
> +*iio_alloc_pollfunc_new(irqreturn_t (*thread)(int irq, void *p),
> +			bool timestamp_enabled,
> +			int type,
> +			struct iio_dev *indio_dev,
> +			const char *fmt,
> +			...);
>  
>  __printf(5, 6) struct iio_poll_func
>  *iio_alloc_pollfunc(irqreturn_t (*h)(int irq, void *p),
> diff --git a/include/linux/iio/triggered_buffer.h b/include/linux/iio/triggered_buffer.h
> index 29e1fe146879..5648c382a506 100644
> --- a/include/linux/iio/triggered_buffer.h
> +++ b/include/linux/iio/triggered_buffer.h
> @@ -9,6 +9,13 @@ struct iio_dev;
>  struct iio_dev_attr;
>  struct iio_buffer_setup_ops;
>  
> +int iio_triggered_buffer_setup_ext_new(struct iio_dev *indio_dev,
> +	irqreturn_t (*thread)(int irq, void *p),
> +	bool timestamp_enabled,
> +	enum iio_buffer_direction direction,
> +	const struct iio_buffer_setup_ops *setup_ops,
> +	const struct iio_dev_attr **buffer_attrs);
> +
>  int iio_triggered_buffer_setup_ext(struct iio_dev *indio_dev,
>  	irqreturn_t (*h)(int irq, void *p),
>  	irqreturn_t (*thread)(int irq, void *p),
> @@ -17,11 +24,24 @@ int iio_triggered_buffer_setup_ext(struct iio_dev *indio_dev,
>  	const struct iio_dev_attr **buffer_attrs);
>  void iio_triggered_buffer_cleanup(struct iio_dev *indio_dev);
>  
> +#define iio_triggered_buffer_setup_new(indio_dev, h, timestamp_enabled, setup_ops)	\
> +	iio_triggered_buffer_setup_ext_new((indio_dev), (h), (timestamp_enabled),	\
> +					IIO_BUFFER_DIRECTION_IN, (setup_ops),		\
> +					NULL)
> +
>  #define iio_triggered_buffer_setup(indio_dev, h, thread, setup_ops)		\
>  	iio_triggered_buffer_setup_ext((indio_dev), (h), (thread),		\
>  					IIO_BUFFER_DIRECTION_IN, (setup_ops),	\
>  					NULL)
>  
> +int devm_iio_triggered_buffer_setup_ext_new(struct device *dev,
> +					struct iio_dev *indio_dev,
> +					irqreturn_t (*thread)(int irq, void *p),
> +					bool timestamp_enabled,
> +					enum iio_buffer_direction direction,
> +					const struct iio_buffer_setup_ops *ops,
> +					const struct iio_dev_attr **buffer_attrs);
> +
>  int devm_iio_triggered_buffer_setup_ext(struct device *dev,
>  					struct iio_dev *indio_dev,
>  					irqreturn_t (*h)(int irq, void *p),
> @@ -30,6 +50,11 @@ int devm_iio_triggered_buffer_setup_ext(struct device *dev,
>  					const struct iio_buffer_setup_ops *ops,
>  					const struct iio_dev_attr **buffer_attrs);
>  
> +#define devm_iio_triggered_buffer_setup_new(dev, indio_dev, thread, timestamp_enabled, setup_ops)	\
> +	devm_iio_triggered_buffer_setup_ext_new((dev), (indio_dev), (thread), (timestamp_enabled),	\
> +					    IIO_BUFFER_DIRECTION_IN,					\
> +					    (setup_ops), NULL)
> +
>  #define devm_iio_triggered_buffer_setup(dev, indio_dev, h, thread, setup_ops)	\
>  	devm_iio_triggered_buffer_setup_ext((dev), (indio_dev), (h), (thread),	\
>  					    IIO_BUFFER_DIRECTION_IN,		\
> 

Re: [PATCH RFC 9/9] iio: rpr0521: Use new timestamp-related APIs

[Gyeyoung Baek]

On Sun, Jun 1, 2025 at 3:14 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 19 May 2025 23:26:01 +0900
> Gyeyoung Baek <gye976@gmail.com> wrote:
>
> > This patch is an example that helps explain the previous series.
> > Now there is no need to handle timestamps differently depending on whether
> > the consumer is attached to its own trigger or to another trigger.
> > And the own trigger of rpr0521 can simply pass a timestamp to consumer,
> > using `iio_trigger_store_time()`
> >
> > Not tested since I don't have the corresponding device.
> >
> > Signed-off-by: Gyeyoung Baek <gye976@gmail.com>
>
> Ah. So this is what the trigger grabbing timestamp thing is about.
>
> I'd just ignore this case for now and solve the simpler one
> of what to do the the pf->timestamp stuff.

Yes, since they can be logically separated, it seems much better to
split the patch series as well.
Thank you for the review.

> That part of the patch set looks promising to me.
>
> Jonathan
>
>
> > ---
> >  drivers/iio/light/rpr0521.c | 22 ++++++----------------
> >  1 file changed, 6 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> > index 92e7552f3e39..32981db18428 100644
> > --- a/drivers/iio/light/rpr0521.c
> > +++ b/drivers/iio/light/rpr0521.c
> > @@ -186,7 +186,6 @@ struct rpr0521_data {
> >       bool pxs_dev_en;
> >
> >       struct iio_trigger *drdy_trigger0;
> > -     s64 irq_timestamp;
> >
> >       /* optimize runtime pm ops - enable/disable device only if needed */
> >       bool als_ps_need_en;
> > @@ -416,7 +415,7 @@ static irqreturn_t rpr0521_drdy_irq_handler(int irq, void *private)
> >       struct iio_dev *indio_dev = private;
> >       struct rpr0521_data *data = iio_priv(indio_dev);
> >
> > -     data->irq_timestamp = iio_get_time_ns(indio_dev);
> > +     iio_trigger_store_time(data->drdy_trigger0);
> >       /*
> >        * We need to wake the thread to read the interrupt reg. It
> >        * is not possible to do that here because regmap_read takes a
> > @@ -446,15 +445,6 @@ static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
> >       struct rpr0521_data *data = iio_priv(indio_dev);
> >       int err;
> >
> > -     /* Use irq timestamp when reasonable. */
> > -     if (iio_trigger_using_own(indio_dev) && data->irq_timestamp) {
> > -             pf->timestamp = data->irq_timestamp;
> > -             data->irq_timestamp = 0;
> > -     }
> > -     /* Other chained trigger polls get timestamp only here. */
> > -     if (!pf->timestamp)
> > -             pf->timestamp = iio_get_time_ns(indio_dev);
> > -
> >       err = regmap_bulk_read(data->regmap, RPR0521_REG_PXS_DATA,
> >               data->scan.channels,
> >               (3 * 2) + 1);   /* 3 * 16-bit + (discarded) int clear reg. */
> > @@ -464,7 +454,6 @@ static irqreturn_t rpr0521_trigger_consumer_handler(int irq, void *p)
> >       else
> >               dev_err(&data->client->dev,
> >                       "Trigger consumer can't read from sensor.\n");
> > -     pf->timestamp = 0;
> >
> >       iio_trigger_notify_done(indio_dev->trig);
> >
> > @@ -867,8 +856,6 @@ static int rpr0521_init(struct rpr0521_data *data)
> >               return ret;
> >  #endif
> >
> > -     data->irq_timestamp = 0;
> > -
> >       return 0;
> >  }
> >
> > @@ -984,6 +971,9 @@ static int rpr0521_probe(struct i2c_client *client)
> >                       goto err_pm_disable;
> >               }
> >               data->drdy_trigger0->ops = &rpr0521_trigger_ops;
> > +             data->drdy_trigger0->early_timestamp = true;
> > +             data->drdy_trigger0->trig_type = IIO_TRIG_TYPE_POLL_NESTED;
> > +
> >               indio_dev->available_scan_masks = rpr0521_available_scan_masks;
> >               iio_trigger_set_drvdata(data->drdy_trigger0, indio_dev);
> >
> > @@ -1011,10 +1001,10 @@ static int rpr0521_probe(struct i2c_client *client)
> >                */
> >
> >               /* Trigger consumer setup */
> > -             ret = devm_iio_triggered_buffer_setup(indio_dev->dev.parent,
> > +             ret = devm_iio_triggered_buffer_setup_new(indio_dev->dev.parent,
> >                       indio_dev,
> > -                     iio_pollfunc_store_time,
> >                       rpr0521_trigger_consumer_handler,
> > +                     true,
> >                       &rpr0521_buffer_setup_ops);
> >               if (ret < 0) {
> >                       dev_err(&client->dev, "iio triggered buffer setup failed\n");
> >
>