Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   James Calligeros <jcalligeros99@gmail.com>
To:     James Calligeros <jcalligeros99@gmail.com>,
        Viresh Kumar <viresh.kumar@linaro.org>
Cc:     linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, nm@ti.com,
        rafael@kernel.org, sboyd@kernel.org, vincent.guittot@linaro.org,
        vireshk@kernel.org
Subject: Re: [PATCH V2 5/5] OPP: decouple dt properties in opp_parse_supplies()
Date:   Fri, 04 Nov 2022 01:23:23 +1000
Message-ID: <2379445.NG923GbCHz@makoto>
In-Reply-To: <20221103131051.2ivkhibbsdarbewt@vireshk-i7>
References: <cover.1667473008.git.viresh.kumar@linaro.org> <3519763.iIbC2pHGDl@makoto> <20221103131051.2ivkhibbsdarbewt@vireshk-i7>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Thursday, 3 November 2022 11:10:51 PM AEST Viresh Kumar wrote:
> On 03-11-22, 22:24, James Calligeros wrote:
> > On Thursday, 3 November 2022 9:01:08 PM AEST Viresh Kumar wrote:
> > 
> > > @@ -674,7 +677,7 @@ static int opp_parse_supplies(struct dev_pm_opp *opp, struct device *dev,
> > >  	bool triplet;
> > >  
> > >  	microvolt = opp_parse_microvolt(opp, dev, opp_table, &triplet);
> > > -	if (IS_ERR_OR_NULL(microvolt))
> > > +	if (IS_ERR(microvolt))
> > >  		return PTR_ERR(microvolt);
> >  
> > Erroring out here will still block EM registration on platforms without 
> > the opp-microvolt prop so we're back to square one, which means the 
> > patch does not do what the description says it does. It behaves
> > almost identically to the current code.
> 
> I am confused.
> 
> With the current code, the following will work:
> - all three available
> - microvolt available and nothing else.
> - only microamp available
> - only microwatt available
> - both microamp and microwatt available but no microvolt
> - and other combinations
> 
> Isn't this all we want ?

You're right, I misinterpreted an error. Details as below.

> What did you test exactly ? As I thought you will be testing the only microwatt
> case here and you say it won't work.
> 
> Sorry, I just got a little bit confused :(
> 

I did test on the Apple machine with only opp-microwatt, but I misinterpreted
the error. We end up here upon parsing the second OPP:

>if (list_empty(&opp_table->opp_list) &&
>    opp_table->regulator_count > 0) {
>	dev_err(dev, "%s: opp-microvolt missing although OPP managing regulators\n",
>		__func__);
>	return ERR_PTR(-EINVAL);
>}

When this path is removed, things work as expected. Could it be that opp_list has
not been updated by the time we're parsing the next OPP? Seems unlikely, but
at the same time if we're ending up taking this branch then there's not much else
that could have gone wrong.

- James