Re: [PATCH 13/16] OPP: Extend dev_pm_opp_data with OPP provider support

[Ulf Hansson]

On Thu, 8 Jun 2023 at 07:34, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>
> On 07-06-23, 14:46, Ulf Hansson wrote:
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 79b4b44ced3e..81a3418e2eaf 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -1112,6 +1112,15 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
> >                       return ret;
> >               }
> >
> > +             if (opp->provider == DEV_PM_OPP_TYPE_GENPD) {
> > +                     ret = dev_pm_genpd_set_performance_state(dev, opp->level);
> > +                     if (ret) {
> > +                             dev_err(dev, "Failed to set performance level: %d\n",
> > +                                     ret);
> > +                             return ret;
> > +                     }
> > +             }
> > +
>
> I don't like this :)
>
> We already have these calls in place from within _set_required_opps(), and we
> should try to get this done in a way that those calls themselves get the
> performance state configured.

I was looking at that, but wanted to keep things as simple as possible
in the $subject series.

The required opps are also different, as it's getting parsed from DT
both for the genpd provider and the consumer. The point is, there are
more code involved but just _set_required_opps().

For example, _set_performance_state() (which is the one that calls
dev_pm_genpd_set_performance_state()) is designed to be used for
required opps. Does it really make sense to rework
_set_performance_state() so it can be used for this case too, just to
avoid another call to dev_pm_genpd_set_performance_state() somewhere
in the code?

One improvement we can make though, is to add a helper function,
"_set_opp_level()", which we call from _set_opp(). This can then
replace the call to _set_required_opps() and the code above that I am
adding for DEV_PM_OPP_TYPE_GENPD. At least that should keep the code
_set_opp() a bit more readable.

What do you think?

Kind regards
Uffe