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;
Date:   Thu, 3 Nov 2022 18:40:51 +0530
From:   Viresh Kumar <viresh.kumar@linaro.org>
To:     James Calligeros <jcalligeros99@gmail.com>
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()
Message-ID: <20221103131051.2ivkhibbsdarbewt@vireshk-i7>
References: <cover.1667473008.git.viresh.kumar@linaro.org>
 <5acd93462bff6c108d65802fd39f6002dfadd1a0.1667473008.git.viresh.kumar@linaro.org>
 <3519763.iIbC2pHGDl@makoto>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3519763.iIbC2pHGDl@makoto>
Precedence: bulk

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 ?

We error out here when there is a problem with DT entries, i.e. incorrect
entries, etc. Else we will get NULL here and we continue as we don't check for
IS_ERR_OR_NULL() anymore after this patch.

> I assume this is an intentional choice given the comments in
> opp_parse_microvolt(), so the commit message should drop 
> references to fixing such platforms.
> 
> If this is a hard sticking point and opp_parse_supplies() must return
> a regulator with opp-microvolt, then I am of the opinion that the parsing
> of opp-microwatt should be detangled entirely from the regulator
> infrastructure.
> 
> Otherwise for the whole series,
> 
> Tested-by: James Calligeros <jcalligeros99@gmail.com>

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 :(

-- 
viresh