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

[Koichiro Den <koichiro.den@xxxxxxxxxxxxx>]

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

Koichiro

> 
> > 
> > >  drivers/gpio/Kconfig       |  4 ++
> > >  drivers/gpio/Makefile      |  1 +
> > >  drivers/gpio/gpio-pseudo.c | 86 ++++++++++++++++++++++++++++++++++++++
> > >  drivers/gpio/gpio-pseudo.h | 24 +++++++++++
> > >  4 files changed, 115 insertions(+)
> > >  create mode 100644 drivers/gpio/gpio-pseudo.c
> > >  create mode 100644 drivers/gpio/gpio-pseudo.h
> > >
> > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > > index 56c1f30ac195..1e2c95e03a95 100644
> > > --- a/drivers/gpio/Kconfig
> > > +++ b/drivers/gpio/Kconfig
> > > @@ -1863,6 +1863,10 @@ config GPIO_MPSSE
> > >
> > >  endmenu
> > >
> > > +# This symbol is selected by some pseudo gpio device implementations
> > > +config GPIO_PSEUDO
> > > +       bool
> > 
> > Please make it tristate - modules that use it are already tristate.
> 
> Thanks for pointing that out. I'll fix it.
> 
> > 
> > > +
> > >  menu "Virtual GPIO drivers"
> > >
> > >  config GPIO_AGGREGATOR
> > > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > > index af3ba4d81b58..5eb54147a1ab 100644
> > > --- a/drivers/gpio/Makefile
> > > +++ b/drivers/gpio/Makefile
> > > @@ -136,6 +136,7 @@ obj-$(CONFIG_GPIO_PISOSR)           += gpio-pisosr.o
> > >  obj-$(CONFIG_GPIO_PL061)               += gpio-pl061.o
> > >  obj-$(CONFIG_GPIO_PMIC_EIC_SPRD)       += gpio-pmic-eic-sprd.o
> > >  obj-$(CONFIG_GPIO_POLARFIRE_SOC)       += gpio-mpfs.o
> > > +obj-$(CONFIG_GPIO_PSEUDO)              += gpio-pseudo.o
> > >  obj-$(CONFIG_GPIO_PXA)                 += gpio-pxa.o
> > >  obj-$(CONFIG_GPIO_RASPBERRYPI_EXP)     += gpio-raspberrypi-exp.o
> > >  obj-$(CONFIG_GPIO_RC5T583)             += gpio-rc5t583.o
> > > diff --git a/drivers/gpio/gpio-pseudo.c b/drivers/gpio/gpio-pseudo.c
> > > new file mode 100644
> > > index 000000000000..6e3da05440d8
> > > --- /dev/null
> > > +++ b/drivers/gpio/gpio-pseudo.c
> > > @@ -0,0 +1,86 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Helper functions for Pseudo GPIOs
> > > + *
> > > + * Copyright 2025 Canonical Ltd.
> > 
> > Well, I'd say that most of the code here is still mine, so I'll go
> > ahead and shamelessly claim some credit: it would make sense to keep
> > my copyright.
> 
> Sure, sorry about that. I'll fix it.
> 
> > 
> > > + */
> > > +
> > > +#include "gpio-pseudo.h"
> > > +
> > > +static int pseudo_gpio_notifier_call(struct notifier_block *nb,
> > > +                                    unsigned long action,
> > > +                                    void *data)
> > > +{
> > > +       struct pseudo_gpio_common *common;
> > > +       struct device *dev = data;
> > > +
> > > +       common = container_of(nb, struct pseudo_gpio_common, bus_notifier);
> > > +       if (!device_match_name(dev, common->name))
> > > +               return NOTIFY_DONE;
> > > +
> > > +       switch (action) {
> > > +       case BUS_NOTIFY_BOUND_DRIVER:
> > > +               common->driver_bound = true;
> > > +               break;
> > > +       case BUS_NOTIFY_DRIVER_NOT_BOUND:
> > > +               common->driver_bound = false;
> > > +               break;
> > > +       default:
> > > +               return NOTIFY_DONE;
> > > +       }
> > > +
> > > +       complete(&common->probe_completion);
> > > +       return NOTIFY_OK;
> > > +}
> > > +
> > > +void pseudo_gpio_init(struct pseudo_gpio_common *common)
> > > +{
> > > +       memset(common, 0, sizeof(*common));
> > > +       init_completion(&common->probe_completion);
> > > +       common->bus_notifier.notifier_call = pseudo_gpio_notifier_call;
> > > +}
> > > +EXPORT_SYMBOL_GPL(pseudo_gpio_init);
> > > +
> > > +int pseudo_gpio_register(struct pseudo_gpio_common *common,
> > > +                        struct platform_device_info *pdevinfo)
> > > +{
> > > +       struct platform_device *pdev;
> > > +       char *name;
> > > +
> > > +       name = kasprintf(GFP_KERNEL, "%s.%u", pdevinfo->name, pdevinfo->id);
> > > +       if (!name)
> > > +               return -ENOMEM;
> > > +
> > > +       common->driver_bound = false;
> > > +       common->name = name;
> > > +       reinit_completion(&common->probe_completion);
> > > +       bus_register_notifier(&platform_bus_type, &common->bus_notifier);
> > > +
> > > +       pdev = platform_device_register_full(pdevinfo);
> > > +       if (IS_ERR(pdev)) {
> > > +               bus_unregister_notifier(&platform_bus_type, &common->bus_notifier);
> > > +               kfree(common->name);
> > > +               return PTR_ERR(pdev);
> > > +       }
> > > +
> > > +       wait_for_completion(&common->probe_completion);
> > > +       bus_unregister_notifier(&platform_bus_type, &common->bus_notifier);
> > > +
> > > +       if (!common->driver_bound) {
> > > +               platform_device_unregister(pdev);
> > > +               kfree(common->name);
> > > +               return -ENXIO;
> > > +       }
> > > +
> > > +       common->pdev = pdev;
> > > +       return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(pseudo_gpio_register);
> > > +
> > > +void pseudo_gpio_unregister(struct pseudo_gpio_common *common)
> > > +{
> > > +       platform_device_unregister(common->pdev);
> > > +       kfree(common->name);
> > > +       common->pdev = NULL;
> > > +}
> > > +EXPORT_SYMBOL_GPL(pseudo_gpio_unregister);
> > > diff --git a/drivers/gpio/gpio-pseudo.h b/drivers/gpio/gpio-pseudo.h
> > > new file mode 100644
> > > index 000000000000..093112b6cce5
> > > --- /dev/null
> > > +++ b/drivers/gpio/gpio-pseudo.h
> > > @@ -0,0 +1,24 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +
> > > +#ifndef GPIO_PSEUDO_H
> > > +#define GPIO_PSEUDO_H
> > > +
> > > +#include <linux/completion.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +struct pseudo_gpio_common {
> > 
> > After moving to lib/ this could be named "pdev_sync_probe_data"?
> 
> Hm, I can't think of anything else right now. I'll use the name. Thanks!
> 
> > 
> > > +       struct platform_device *pdev;
> > > +       const char *name;
> > > +
> > > +       /* Synchronize with probe */
> > > +       struct notifier_block bus_notifier;
> > > +       struct completion probe_completion;
> > > +       bool driver_bound;
> > > +};
> > > +
> > > +void pseudo_gpio_init(struct pseudo_gpio_common *common);
> > > +int pseudo_gpio_register(struct pseudo_gpio_common *common,
> > > +                        struct platform_device_info *pdevinfo);
> > > +void pseudo_gpio_unregister(struct pseudo_gpio_common *common);
> > > +
> > > +#endif /* GPIO_PSEUDO_H */
> > > --
> > > 2.45.2
> > >
> > 
> > Bartosz