Re: [PATCH 06/10] OPP: Add dev_pm_opp_{set|put}_required_device() helper

[Ulf Hansson]

On 29 June 2018 at 08:19, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> Multiple generic power domains for a device are supported with the help
> of virtual devices, which are created for each device-genpd pair. These

What "device-genpd" pair are you referring to?

> are the device structures which are attached to the power domain and are
> required by the OPP core to set the performance state of the genpd.
>
> The helpers added by this commit are required to be called once for each
> genpd of a device. These are required only if multiple domains are
> present for a device, otherwise the actual device structure will be used
> instead by the OPP core.
>
> This commit also updates the genpd core to automatically call the new
> helper to set the required devices with the OPP layer, whenever the
> virtual devices are created for multiple genpd. The prototype of
> __genpd_dev_pm_attach() is slightly updated for this.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
>  drivers/base/power/domain.c | 31 ++++++++++++++---
>  drivers/opp/core.c          | 69 +++++++++++++++++++++++++++++++++++++
>  drivers/opp/of.c            | 12 +++++++
>  drivers/opp/opp.h           |  2 ++
>  include/linux/pm_opp.h      |  8 +++++
>  5 files changed, 118 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index c298de8a8308..e9c85c96580c 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2219,8 +2219,14 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
>         genpd_queue_power_off_work(pd);
>
>         /* Unregister the device if it was created by genpd. */
> -       if (dev->bus == &genpd_bus_type)
> +       if (dev->bus == &genpd_bus_type) {
> +               struct opp_table *opp_table = dev_get_drvdata(dev);

This feels wrong, to me. Genpd is not a driver, but rather a
subsystem, hence I think it's a bad idea to "abuse" the driver data
pointer like this.

Instead, in genpd we store device specific data, through the
dev->power.subsys_data.

Seems like a better option is to extend the subsys_data, to also
include data needed to be shared for opp/performance states.

In that way, we might not even need to call into the opp library
(dev_pm_opp_set_required_device()) API, but rather the opp library can
itself check what is in the subsys_data for the device that is being
targeted. Would that work?

[...]

Kind regards
Uffe