Re: [PATCH v2 3/5] regulator: qcom_spmi: Add support for new regulator types

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

> >> @@ -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.

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux