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

From: Saravana Kannan <saravanak@google.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: MyungJoo Ham <myungjoo.ham@samsung.com>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Chanwoo Choi <cw00.choi@samsung.com>,
	Viresh Kumar <vireshk@kernel.org>, Nishanth Menon <nm@ti.com>,
	Stephen Boyd <sboyd@kernel.org>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Sibi Sankar <sibis@codeaurora.org>,
	Android Kernel Team <kernel-team@android.com>,
	Linux PM <linux-pm@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 5/5] PM / devfreq: Add required OPPs support to passive governor
Date: Wed, 24 Jul 2019 20:58:34 -0700	[thread overview]
Message-ID: <CAGETcx_b96teeKoYpJoi8=jhMVrNCKCw9Ef10AU8=mVXGXVPHg@mail.gmail.com> (raw)
In-Reply-To: <20190725030137.uz22iwvdv37fsw56@vireshk-i7>

On Wed, Jul 24, 2019 at 8:01 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 23-07-19, 17:26, Saravana Kannan wrote:
> > On Tue, Jul 23, 2019 at 3:04 AM Viresh Kumar <viresh.kumar@linaro.org> 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