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

[Jonathan Cameron <jic23@kernel.org>]

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)
>  {
>