Re: [PATCH v2 02/21] pmdomain: core: Add a bus and a driver for genpd providers

[Saravana Kannan <saravanak@google.com>]

On Fri, May 23, 2025 at 6:40 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> When we create a genpd via pm_genpd_init() we are initializing a
> corresponding struct device for it, but we don't add the device to any
> bus_type. It has not really been needed as the device is used as cookie to
> help us manage OPP tables.
>
> However, to prepare to make better use of the device let's add a new genpd
> provider bus_type and a corresponding genpd provider driver. Subsequent
> changes will make use of this.
>
> Suggested-by: Saravana Kannan <saravanak@google.com>
> Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/pmdomain/core.c | 89 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 88 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index 9a66b728fbbf..da515350c65b 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -27,6 +27,11 @@
>  /* Provides a unique ID for each genpd device */
>  static DEFINE_IDA(genpd_ida);
>
> +/* The parent for genpd_provider devices. */
> +static struct device genpd_provider_bus = {
> +       .init_name = "genpd_provider",
> +};
> +
>  #define GENPD_RETRY_MAX_MS     250             /* Approximate */
>
>  #define GENPD_DEV_CALLBACK(genpd, type, callback, dev)         \
> @@ -44,6 +49,14 @@ static DEFINE_IDA(genpd_ida);
>  static LIST_HEAD(gpd_list);
>  static DEFINE_MUTEX(gpd_list_lock);
>
> +#define to_genpd_provider_drv(d) container_of(d, struct genpd_provider_drv, drv)
> +
> +struct genpd_provider_drv {
> +       struct device_driver drv;
> +       int (*probe)(struct device *dev);
> +       void (*remove)(struct device *dev);
> +};
> +
>  struct genpd_lock_ops {
>         void (*lock)(struct generic_pm_domain *genpd);
>         void (*lock_nested)(struct generic_pm_domain *genpd, int depth);
> @@ -2225,6 +2238,26 @@ static int genpd_set_default_power_state(struct generic_pm_domain *genpd)
>         return 0;
>  }
>
> +static int genpd_provider_bus_probe(struct device *dev)
> +{
> +       struct genpd_provider_drv *drv = to_genpd_provider_drv(dev->driver);
> +
> +       return drv->probe(dev);
> +}
> +
> +static void genpd_provider_bus_remove(struct device *dev)
> +{
> +       struct genpd_provider_drv *drv = to_genpd_provider_drv(dev->driver);
> +
> +       drv->remove(dev);
> +}

Not sure if I'm missing some corner case you found out, but you don't
need these stubs just to call the drv ops. Driver core does it anyway
if the bus probe/remove functions are missing.

> +
> +static const struct bus_type genpd_provider_bus_type = {
> +       .name           = "genpd_provider",
> +       .probe          = genpd_provider_bus_probe,
> +       .remove         = genpd_provider_bus_remove,
> +};
> +
>  static void genpd_provider_release(struct device *dev)
>  {
>         /* nothing to be done here */
> @@ -2262,6 +2295,8 @@ static int genpd_alloc_data(struct generic_pm_domain *genpd)
>         genpd->gd = gd;
>         device_initialize(&genpd->dev);
>         genpd->dev.release = genpd_provider_release;
> +       genpd->dev.bus = &genpd_provider_bus_type;
> +       genpd->dev.parent = &genpd_provider_bus;
>
>         if (!genpd_is_dev_name_fw(genpd)) {
>                 dev_set_name(&genpd->dev, "%s", genpd->name);
> @@ -3355,9 +3390,61 @@ int of_genpd_parse_idle_states(struct device_node *dn,
>  }
>  EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
>
> +static int genpd_provider_probe(struct device *dev)
> +{
> +       return 0;
> +}
> +
> +static void genpd_provider_remove(struct device *dev)
> +{
> +}

Same might apply here.

-Saravana

> +
> +static void genpd_provider_sync_state(struct device *dev)
> +{
> +}
> +
> +static struct genpd_provider_drv genpd_provider_drv = {
> +       .drv = {
> +               .name = "genpd_provider",
> +               .bus = &genpd_provider_bus_type,
> +               .sync_state = genpd_provider_sync_state,
> +               .suppress_bind_attrs = true,
> +       },
> +       .probe = genpd_provider_probe,
> +       .remove = genpd_provider_remove,
> +};
> +
>  static int __init genpd_bus_init(void)
>  {
> -       return bus_register(&genpd_bus_type);
> +       int ret;
> +
> +       ret = device_register(&genpd_provider_bus);
> +       if (ret) {
> +               put_device(&genpd_provider_bus);
> +               return ret;
> +       }
> +
> +       ret = bus_register(&genpd_provider_bus_type);
> +       if (ret)
> +               goto err_dev;
> +
> +       ret = bus_register(&genpd_bus_type);
> +       if (ret)
> +               goto err_prov_bus;
> +
> +       ret = driver_register(&genpd_provider_drv.drv);
> +       if (ret)
> +               goto err_bus;
> +
> +       return 0;
> +
> +err_bus:
> +       bus_unregister(&genpd_bus_type);
> +err_prov_bus:
> +       bus_unregister(&genpd_provider_bus_type);
> +err_dev:
> +       device_unregister(&genpd_provider_bus);
> +       return ret;
>  }
>  core_initcall(genpd_bus_init);
>
> --
> 2.43.0
>