Date:   Fri, 29 Jul 2022 13:04:57 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Iskren Chernev <iskren.chernev@...il.com>
Cc:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Adam Skladowski <a39.skl@...il.com>,
        Andy Gross <agross@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Robert Marko <robimarko@...il.com>,
        Jorge Ramirez-Ortiz <jorge@...ndries.io>,
        devicetree@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, phone-devel@...r.kernel.org,
        ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH v2 3/5] regulator: qcom_spmi: Add support for new
 regulator types

On Thu, Jul 28, 2022 at 11:59:03PM +0300, Iskren Chernev wrote:
> 
> 
> On 7/28/22 14:11, Mark Brown wrote:
> > On Thu, Jul 28, 2022 at 02:14:10AM +0300, Iskren Chernev wrote:
> >> On 7/27/22 14:57, Mark Brown wrote:
> >>> On Tue, Jul 26, 2022 at 09:11:31PM +0300, Iskren Chernev wrote:
> >
> >>>> Add support for some regulator types that are missing in this driver, all
> >>>> belonging to the FTSMPS426 register layout.  This is done in preparation
> >>>> for adding support for the PM6125 PMIC.
> >
> >>>> +	.set_mode		= spmi_regulator_ftsmps3_set_mode,
> >>>> +	.get_mode		= spmi_regulator_ftsmps426_get_mode,
> >
> >>> Why are set and get asymmetric?
> >
> >> Because the get method, only uses AUTO and HPM, which have the same value
> >> for ftsmps3 and ftsmps426 (so there is no need for a new function).
> >
> > This needs at least a comment.
> 
> I agree, I think to add the function with the right macros, and comment
> that it is the same now but might change in the future if support for mode
> modes is added.
> 
> >>>> @@ -1473,7 +1557,7 @@ static const struct spmi_regulator_mapping supported_regulators[] = {
> >>>>  	SPMI_VREG(LDO,   HT_P600,  0, INF, HFS430, hfs430, ht_p600, 10000),
> >>>>  	SPMI_VREG(LDO,   HT_P150,  0, INF, HFS430, hfs430, ht_p150, 10000),
> >>>>  	SPMI_VREG(BUCK,  GP_CTL,   0, INF, SMPS,   smps,   smps,   100000),
> >>>> -	SPMI_VREG(BUCK,  HFS430,   0, INF, HFS430, hfs430, hfs430,  10000),
> >>>> +	SPMI_VREG(BUCK,  HFS430,   0,   3, HFS430, hfs430, hfs430,  10000),
> >
> >>> The changelog said we were adding support for new types but this looks
> >>> like changing an existing type.
> >
> >> The code, as written now does a different thing for BUCK, HFS430 (on
> >> mainline (ML) and downstream (DS) linked in the commit message). Since DS
> >> only supports newer stuff, to be on safe side, I kept existing behavior for
> >> rev 0-3 on BUCK(3)+HFS430(10), so at least DS and ML agree on pm6125
> >> completely.
> >
> > This needs describing in the changelog, probably you need multiple
> > paches here since you are making a number of different changes each of
> > which needs some explanation.
> >
> >> The commit [1] that adds support for BUCK+HFS430 might be wrong, or it
> >> might be right for the time being (i.e initial revisions had different
> >> behavior). I'm CC-ing Jorge.
> >
> > If that's the case perhaps part of this needs to be sent as a fix.
> 
> The Downstream patch is adding 3 logical types:
> - LDO_510 -- these have new subtypes, so no existing PMICs are affected
> - FTSMPS3 -- this has a new subtype (0xb), so no existing PMICs are
>   affected
> - HFSMPS -- this has the same type and subtype (BUCK+HFS430) as an existing
>   mainline logical type (HFS430), both declaring 0-INF revisions.
> 
> So if we fully trust the downstream patch, I can make a fix for the
> existing BUCK+HFS430+0-INF, so it uses the slighly modified mode values.
> 
> Currently the set mode fn differs in LPM mode (5 in the common2 case and
> 4 in the common3 case), so if indeed downstream is correct it would mean
> this regulator (when turned off) was set to an invalid mode (5 has
> undefined meaning in common3 map) from 2019 onward.
> 
> On the other hand, if we assume downstream is wrong, then their code sets
> 4, which actually means RETENTION (not LPM). I really don't know how this
> could cause trouble. In fact downstream does a bunch of weird stuff, it
> doesn't "just" set to LPM (like mainline), instead there is complex logic
> per logical type and "initial mode". Or they're just masking this mistake
> ;-)
> 
> TL;DR Jorge's mail is gone, so we can't get info from the original author.

Jorge moved to foundries.io, copying him in in case he remembers
anything about this.

> Another issue is I can't really test any other PMIC (and even my PMIC
> I can't turn off most of the regs without loosing critical functionality,
> and the BUCKs are kinda important :)).
> 
> So we can:
> 1. politely ask for somebody with access to the secret sauce to say what is
>    correct, at least according to the docs (with a timeout)
> 2. assume downstream patch is right, and fix the existing HFS430 regulator
> 3. maintain the current (patch) behavior, which likely won't affect older
>    PMICs, but is still adhering to DS patch, because it adds support for
>    this particular PMIC, so presumably it was tested and works with it
> 4. drop the pmic patch and rely on SMD
> 
> Please advice.
> 
> In any case if we go with 2 or 3, I can split out this particular (BUCK)
> part in a separate patch with more information/comments.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists