Message-ID: <CAKbEznuG6-+cKOOVSvyw30Qra_6yVruA0cvpcK5Gqp2_kcPHcw@mail.gmail.com>
Date: Mon, 12 May 2025 00:47:39 +0900
From: Gyeyoung Baek <gye976@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: David Lechner <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>, 
	Andy Shevchenko <andy@...nel.org>, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: trigger: Add validation to reject devices requiring
 top half

Hello Jonathan,
I’ve referred to your previous comments and implemented the ideas.
Thank you for your earlier feedback.
I now have a few follow-up questions and would appreciate your
thoughts on the below points.

On Thu, May 8, 2025 at 4:40 AM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Wed, 7 May 2025 00:55:27 +0900
> Gyeyoung Baek <gye976@...il.com> wrote:

> I'd take a different approach (slightly) though it's more effort.
>
> Step 1. Tidy up current situation.
>
> Patch to convert all existing calls to devm_iio_triggered_buffer_setup()
> and iio_triggered_buffer_setup() to not take a top half function but replace
> that variable with a bool early_timestamp or something along those lines.
> Replace the h in struct iio_poll_func with a similarly named bool.
> Bunch of plumbing to make that all get filled in correctly.
>
> Then in iio_trigger_attach_pollfunc() check that bool and if appropriate
> pass iio_pollfunc_store_time() it to request_threaded_irq()

Now we have both the existing `devm_iio_triggered_buffer_setup()`,
and a new version with the additional arguments of that.
Should these two coexist for compatibility, or should the before one
be replaced by the new one?

> Step 2. Make what you want work cleanly now we only have that one handler.
>
> In iio_trigger_poll_nested() we can't know if that flag is set and I'm not
> really keen on trying to get to this from elsewhere. We have previously considered
> solving this case via whether the timestamp is set or not in the threaded
> handler. I've never like that much as in theory timestamp 0 is valid (was
> a while ago). The rpr0521 light sensor has handling for this.

What I'm trying to do is a mechanism where device drivers can
automatically get timestamps without manually handling them — simply
by setting a argument to indicate whether to capture the timestamp in
the tophalf or bottomhalf.

But there are cases like the rpr0521 where the driver sets the
timestamp manually within its own trigger.
Would it make sense to extend this to automatically set the timestamp
in cases where the driver uses its own trigger as well?
To that, I believe we would need a unified interface that can cover
all trigger types (e.g., interrupt, software trigger) that invoke
poll() or poll_nested().
Would it be the right direction?
Or would it be more appropriate to consider only the top/bottom of a
consumer device?

> I wonder if the following would work.
>
> In iio_trigger_attach_poll_func() we have access to the trigger and
> the pollfunc.  So if the pollfunc flag for wanting an early timestamp is set and
> we know the trigger is going to use iio_poll_trigger_nested() then we could
> wrap the registered handler in a local one that calls the iio_pollfunc_store_time()
>
> The additional magic needed here is that today we don't know that about the trigger.
> So we'd need to add a bool to the struct iio_trig to indicate it and set that
> for all drivers that use iio_trigger_poll_nested()   bool nested; will do.
>
> It's not perfect as there are driver that do iio_trigger_poll() and iio_trigger_poll_nested()
> depending on path. To handle those we'd need a flag to say don't overwrite my timestamp.
> at91-sama5d2-adc.c is the first one I found.

> There are ways to make even that work but lets skip that for now as they'd
> slightly complicate things. That driver won't call the timestamp capture in
> some paths but it doesn't today so we are no worse off.

--
Regards,
Gyeyoung

Powered by blists - more mailing lists