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.
Attachment:
signature.asc
Description: PGP signature
