| Message ID | 20250417142513.312939-5-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | pmdomain: Add generic ->sync_state() support to genpd | expand |
On 25-04-17 16:25:02, Ulf Hansson 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. > > 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 035b65563947..da51a61a974c 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 { I'd replace "provider" substring and expand drv to driver everywhere. I think that's more in line with all other subsystems. > + 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) ... and then here drop the "provider" as well. Other than that, LGTM: Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
On Tue, 22 Apr 2025 at 16:03, Abel Vesa <abel.vesa@linaro.org> wrote: > > On 25-04-17 16:25:02, Ulf Hansson 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. > > > > 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 035b65563947..da51a61a974c 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 { > > I'd replace "provider" substring and expand drv to driver everywhere. > > I think that's more in line with all other subsystems. I understand your point, but it's not that straight-forward to find a proper name this time. We already have another bus_type for genpd consumer devices (virtual devices created when attaching a device to one of its multiple PM domains). That bus is already named "genpd". > > > + 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) > > ... and then here drop the "provider" as well. For the reason I pointed out above, I decided to use "provider" in the bus/driver's functions names to have a clear difference from the "consumer" genpd bus. I am worried that if we don't use "provider" we will mix up things with the existing genpd bus. Maybe there is a better option? > > Other than that, LGTM: > > Reviewed-by: Abel Vesa <abel.vesa@linaro.org> Thanks for reviewing! Kind regards Uffe
diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c index 035b65563947..da51a61a974c 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); +} + +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) +{ +} + +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);
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. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/pmdomain/core.c | 89 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 88 insertions(+), 1 deletion(-)