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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

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

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

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

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

> + */
> +
> +#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"?

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