Message-ID: <zfyzhahnhtozrbtgdod7wyfhcdtwuofx4hl3ndtxglmzacd2at@q4hmczbylwcw>
Date: Tue, 18 Feb 2025 21:58:35 +0900
From: Koichiro Den <koichiro.den@...onical.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: linux-gpio@...r.kernel.org, geert+renesas@...der.be, 
	linus.walleij@...aro.org, maciej.borzecki@...onical.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] gpio: pseudo: common helper functions for pseudo
 gpio devices

On Tue, Feb 18, 2025 at 09:57:05AM GMT, Bartosz Golaszewski wrote:
> On Tue, Feb 18, 2025 at 6:01 AM Koichiro Den <koichiro.den@...onical.com> 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@...onical.com> 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@...onical.com> 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@...onical.com>
> > > > > > > ---
> > > > > >
> > > > >
> > > > > 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.

I think we're on the same page.

> 
> > 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.

Thanks for sharing the background history. My view/feeling remains unchanged.

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

Alright.

> 
> >
> > 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.

I have no intention of dropping the current OF support, as I wrote in my
last email like ".. understandable and obviously has already been accepted
..". In that sense, I think we're on the same page here as well.

> 
> > 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 totally agree with "dev-sync-probe" and the overall idea. Thanks for the
suggestion. I'll send v2 later.

> [...] 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?

Although I'm not sure whether there are many justifiable demand for such
generic lib, probably that's just because I don't know relevant topics.

Thanks,

Koichiro

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

Powered by blists - more mailing lists