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

[Ulf Hansson]

On Tue, 3 Jun 2025 at 02:23, Saravana Kannan <saravanak at google.com> wrote:
>
> On Fri, May 23, 2025 at 6:40 AM Ulf Hansson <ulf.hansson at 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 at google.com>
> > Reviewed-by: Abel Vesa <abel.vesa at linaro.org>
> > Signed-off-by: Ulf Hansson <ulf.hansson at 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.

Right, they are probably just leftovers used for debug-prints during
development.

Thanks for spotting this!

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

Let me check and see if/what can be dropped.

I think you told me that I needed the ->probe() function to keep
fw_devlink happy, but maybe I got that wrong.

[...]

Kind regards
Uffe