Message-ID: <saszavmizjwhzechspy6otune2xwtgjjygaitxminzclgj7zep@ofwfb5jdfcam>
Date: Tue, 18 Feb 2025 14:01:41 +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 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.

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.

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?

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

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

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