RE: [PATCH v1 1/2] regulator: pca9450: add enable_val for all bucks

[Robin Gong]

On 2020/09/01 9:58 Robin Gong <yibin.gong@xxxxxxx> wrote:
> On 2020/08/31 18:53 Marco Felsch <m.felsch@xxxxxxxxxxxxxx> wrote:
> > Hi Robin,
> >
> > On 20-09-01 00:48, Robin Gong wrote:
> > > BuckX enable mode
> > > 00b = OFF
> > > 01b = ON by PMIC_ON_REQ = H
> > > 10b = ON by PMIC_ON_REQ = H && PMIC_STBY_REQ = L 11b = Always ON
> > >
> > > For such enable mode, enable_value should be clearly set in
> > > requlator desc,
> > > 00/11 is not enough, correct it now for different bucks. For
> > > example,
> > > buck2 is designed for vddarm which could be off in 'PMIC_STBY_REQ = H'
> > > after kernel enter suspend, so should be set '10b' as ON, while others is
> '01b'
> > as ON.
> > > All are the same as the default setting which means bucks no need to
> > > be enabled again during kernel boot even if they have been enabled
> > > already after pmic on.
> >
> > I wouldn't hard-code the regulator behaviour because the behaviour
> > comes from the system design which in most cases are comming from our
> hw-guys.
> > Till now I saw a few intelligent designs don't following the pmic user
> > recommendations to save money. I would love to specify the regulator
> > behaviour/mode within the dt or acpi.
> Well, if so the better way is moving into dts. Will implement it in v2.
Hi Marco,
	Sorry, I'm afraid that big changes have to be made for moving it to dts, because
current pca9450 driver is using common regulator_enable_regmap() which means
rdev->desc is const and 'enable_value' can't be changed runtime with the value in
dts. Regards to pca9450 is the specific pmic designed for i.mx8m family rather than
generic pmic, the power rails defining should be the same as NXP MEK reference
board, especially for the BUCKs which match well with SOC's power. So may I leave it
at this time, and pick it up if the 'unbelievable trouble' really come out in the future?   

> 
> >
> > > Signed-off-by: Robin Gong <yibin.gong@xxxxxxx>
> > > ---
> > >  drivers/regulator/pca9450-regulator.c | 15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/regulator/pca9450-regulator.c
> > > b/drivers/regulator/pca9450-regulator.c
> > > index eb5822b..79f2a5a 100644
> > > --- a/drivers/regulator/pca9450-regulator.c
> > > +++ b/drivers/regulator/pca9450-regulator.c
> > > @@ -249,6 +249,7 @@ static const struct pca9450_regulator_desc
> > pca9450a_regulators[] = {
> > >  			.vsel_mask = BUCK1OUT_DVS0_MASK,
> > >  			.enable_reg = PCA9450_REG_BUCK1CTRL,
> > >  			.enable_mask = BUCK1_ENMODE_MASK,
> > > +			.enable_val = BUCK_ENMODE_ONREQ,
> > >  			.owner = THIS_MODULE,
> > >  			.of_parse_cb = pca9450_set_dvs_levels,
> > >  		},
> > > @@ -273,7 +274,8 @@ static const struct pca9450_regulator_desc
> > pca9450a_regulators[] = {
> > >  			.vsel_reg = PCA9450_REG_BUCK2OUT_DVS0,
> > >  			.vsel_mask = BUCK2OUT_DVS0_MASK,
> > >  			.enable_reg = PCA9450_REG_BUCK2CTRL,
> > > -			.enable_mask = BUCK1_ENMODE_MASK,
> > > +			.enable_mask = BUCK2_ENMODE_MASK,
> >
> > Unrelated change?
> Yes, that's just correct it minor literal since that's BUCK2 although they're the
> same, will split it anyway.