Re: [PATCH 1/3] gpio: pseudo: common helper functions for pseudo gpio devices

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Tue, Feb 18, 2025 at 6:01 AM Koichiro Den <koichiro.den@xxxxxxxxxxxxx> wrote:
>
> On Mon, Feb 17, 2025 at 06:29:27PM GMT, Bartosz Golaszewski wrote:
> > On Mon, Feb 17, 2025 at 5:21 PM Koichiro Den <koichiro.den@xxxxxxxxxxxxx> wrote:
> > >
> > > On Tue, Feb 18, 2025 at 01:12:17AM GMT, Koichiro Den wrote:
> > > > On Mon, Feb 17, 2025 at 04:46:30PM GMT, Bartosz Golaszewski wrote:
> > > > > On Mon, Feb 17, 2025 at 3:28 PM Koichiro Den <koichiro.den@xxxxxxxxxxxxx> wrote:
> > > > > >
> > > > > > Both gpio-sim and gpio-virtuser share a mechanism to instantiate a
> > > > > > platform device and wait synchronously for probe completion.
> > > > > > With gpio-aggregator adopting the same approach in a later commit for
> > > > > > its configfs interface, it's time to factor out the common code.
> > > > > >
> > > > > > Add gpio-pseudo.[ch] to house helper functions used by all the pseudo
> > > > > > GPIO device implementations.
> > > > > >
> > > > > > No functional change.
> > > > > >
> > > > > > Signed-off-by: Koichiro Den <koichiro.den@xxxxxxxxxxxxx>
> > > > > > ---
> > > > >
> > > >
> > > > Thanks for the review.
> > > >
> > > > > Looking at this patch now, I've realized that there is nothing
> > > > > GPIO-specific here. It's a mechanism for synchronous platform device
> > > > > probing. I don't think its place is in drivers/gpio/ if we're making
> > > > > it a set of library functions. Can I suggest moving it to lib/ and
> > > > > renaming the module as pdev_sync_probe or something else that's
> > > > > expressive enough to tell users what it does? You can make me the
> > > > > maintainer of that module if you wish (feel free to add yourself
> > > > > too!).
> > > >
> > > > I had vaguely envisioned that this might eventually contain some
> > > > GPIO-specific code for some reason, and also it's just a tiny utility to
> > > > reduce code duplication, which is why I placed it in the neighborhood,
> > > > drivers/gpio/. However, of course you’re right, there’s nothing
> > > > GPIO-specific here, so moving it to lib/ makes sense.
> > > >
> > > > I'm not really sure if this method for synchronous platform device probing
> > > > can be broadly accepted as a general solution, but I have no objections to
> > > > making the change. I'll move it as you suggested and send v2, setting you
> > > > as its maintainer.
> > >
> > > Regarding this series, I feel that it might make discussions smoother if
> > > you submit it directly. So if you're okay with it, please go ahead. In
> > > that case, there's even no need to mention me or CC me - I can track it on
> > > ML :)
> >
> > I'm not sure I'm following. Why would I submit it myself? You did most
> > of the work already. If you want the changes to gpio-aggregator
> > merged, then I think that it's time to refactor this code before we do
> > that because repeating it three times is just bad programming. I
> > probably wouldn't have done it otherwise at this point.
>
> As I mentioned earlier, I'm not really sure if this particular usage of
> platform devices will be generally acceptable. gpio-pseudo was intended
> solely to reduce code duplication in methods already accepted by the GPIO
> subsystem. Moving it to lib/ would shift the approach, effectively trying
> to promote this method as a standard solution.
>

Promote it as a solution for this specific use-case - the need to
probe "faux" platform devices synchonously.

> For example, if for any reason drivers_autoprobe is set to 0 on the
> platform bus, the synchronous mechanism might be blocked indefinitely.
> Moreover, in the first place, I'm not sure whether employing the platform
> bus in this way is appropriate.
>

It's sketchy, I know. Back in the day I was advised by Greg to use the
auxiliary bus but I realized very fast that if I want to support
device-tree too, then I would end up reimplementing all the code that
already exists for supporting the platform bus. He eventually agreed
that it's better to use the platform bus. We had the same issue with
PCI pwrctrl recently and also ended up using the platform bus.

> For drivers like gpio-virtuser, which we can define virtual GPIO consumers
> via DT, or for gpio-aggregator, which we can use as a generic GPIO driver,
> the expectation is to use the platform bus/device mechanism as usual. In
> those cases, adding a synchronous mechanism via the platform bus notifier
> to piggyback on the existing platform bus probe implementation is
> understandable and obviously has already been accepted in the GPIO
> subsystem. However, moving just the synchronous mechanism into lib/ can
> potentially be perceived as an abuse of the platform device concept?

That's actually a good point. I guess this code could be reworked to
work with any bus (that would be specified by the user).

>
> Incidentally, Greg K-H’s faux bus work was recently merged into mainline:
> commit 35fa2d88ca94 ("driver core: add a faux bus for use when a simple
> device/bus is needed").
>

Thanks for bringing this to my attention, I wasn't aware this existed.
However it's not useful here - I still want to support OF.

> Correct me where I'm wrong. And I'd appreciate if you could share your
> thoughts.
>

I don't want to block the aggregator work but I also don't want to
have code triplication under drivers/gpio/. Let's do the following: I
don't like the sound of the word "gpio-pseudo" in this context. Let's
keep it under drivers/gpio/ but rename the module already to
"dev-sync-probe.c" and use the
dev_sync_probe_init/register/unregister/data naming scheme. Stick to
supporting platform devices exclusively for now. I don't have the time
just yet but maybe in the next release cycle, I'll try to make it more
generic (work for all device types) and move it out of drivers/gpio/.
How does it sound?

Bartosz

> Koichiro
>
> >
> > The code looks good other than that, just put it under lib/, rename
> > functions to pdev_sync_probe_init/register/unregister() and send it to
> > the list as usual. With that it's good to go. Just make sure to
> > mention to Andrew Morton the need for this to go through the GPIO
> > tree, I don't think he'll mind.
> >
> > Bart