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

[Koichiro Den <koichiro.den@xxxxxxxxxxxxx>]

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.

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