Re: [PATCH v3 5/5] PM / devfreq: Add required OPPs support to passive governor

[Saravana Kannan]

On Wed, Jul 24, 2019 at 8:01 PM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>
> On 23-07-19, 17:26, Saravana Kannan wrote:
> > On Tue, Jul 23, 2019 at 3:04 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> > > On 17-07-19, 15:23, Saravana Kannan wrote:
> > > >       /*
> > > > @@ -56,13 +56,20 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
> > > >        * list of parent device. Because in this case, *freq is temporary
> > > >        * value which is decided by ondemand governor.
> > > >        */
> > > > -     opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0);
> > > > -     if (IS_ERR(opp)) {
> > > > -             ret = PTR_ERR(opp);
> > > > +     p_opp = devfreq_recommended_opp(parent_devfreq->dev.parent, freq, 0);
> > > > +     if (IS_ERR(p_opp)) {
> > > > +             ret = PTR_ERR(p_opp);
> > > >               goto out;
> > > >       }
> > > >
> > > > -     dev_pm_opp_put(opp);
> > > > +     if (devfreq->opp_table && parent_devfreq->opp_table)
> > > > +             opp = dev_pm_opp_xlate_opp(parent_devfreq->opp_table,
> > > > +                                        devfreq->opp_table, p_opp);
> > >
> > > you put p_opp right here.
>
> What about this comment ?

Sorry, mixed this up with the other comment. Good point. Fixed. This
unintentionally fixed a copy-pasta bug I had in the "out" path.

-Saravana

>
> > >
> > > Also shouldn't you try to get p_opp under the above if block only? As
> > > that is the only user of it ?
> >
> > No, p_opp (used to be called opp) was used even before my changes. If
> > there's no required-opps mapping this falls back to assuming the slave
> > device OPP to pick should be the same index as the master device's
> > opp.
> >
> > So I believe this patch is correct as-is.
>
> Right.
>
> --
> viresh