Re: [PATCH v2 04/10] regulator: bd9571mwv: Add BD9574MWF support

[Vaittinen, Matti]

Hello Shimoda-san,

On Mon, 2020-12-14 at 04:57 +0000, Yoshihiro Shimoda wrote:
> Hello Matti-san,
> 
> > From: Vaittinen, Matti, Sent: Friday, December 11, 2020 9:34 PM
> > 
> > Hello again Shimada-san,
> > 
> > On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> > > Add support for BD9574MWF which is silimar chip with BD9571MWV.
> > > Note that BD9574MWF doesn't support AVS and VID.
> > 
> > I'd like to understand what is VID?
> 
> It seems reading some voltages from registers.
> For example, BD9571 has "VD18_VID" register which
> is prohibit to write. But, BD9574 doesn't have this
> register. Also, the driver names "vid_ops",
> so I described "VID" here. Perhaps, we should revise
> the description to clear. (Please look "Updated description" in this
> email.)

Thank you for detailed explanation. So as far as I understood - VID is
a register which displays the current output voltage, right? If I am
not mistaken, this is different from register where (initial) voltage
is set?

> 
> > > Signed-off-by: Yoshihiro Shimoda <
> > > yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > > ---
> > >  drivers/regulator/bd9571mwv-regulator.c | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/regulator/bd9571mwv-regulator.c
> > > b/drivers/regulator/bd9571mwv-regulator.c
> > > index 02120b0..041339b 100644
> > > --- a/drivers/regulator/bd9571mwv-regulator.c
> > > +++ b/drivers/regulator/bd9571mwv-regulator.c
> > > @@ -1,6 +1,6 @@
> > >  // SPDX-License-Identifier: GPL-2.0
> > >  /*
> > > - * ROHM BD9571MWV-M regulator driver
> > > + * ROHM BD9571MWV-M and BD9574MWF-M regulator driver
> > >   *
> > >   * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@xxxxxxxxx
> > > >
> > >   *
> > > @@ -9,6 +9,7 @@
> > >   * NOTE: VD09 is missing
> > >   */
> > > 
> > > +#include <linux/mfd/rohm-generic.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > >  #include <linux/platform_device.h>
> > > @@ -277,6 +278,7 @@ static int bd9571mwv_regulator_probe(struct
> > > platform_device *pdev)
> > >  	struct regulator_dev *rdev;
> > >  	unsigned int val;
> > >  	int i;
> > > +	enum rohm_chip_type chip = platform_get_device_id(pdev)-
> > > > driver_data;
> > > 
> > >  	bdreg = devm_kzalloc(&pdev->dev, sizeof(*bdreg), GFP_KERNEL);
> > >  	if (!bdreg)
> > > @@ -292,6 +294,9 @@ static int bd9571mwv_regulator_probe(struct
> > > platform_device *pdev)
> > >  	config.regmap = bdreg->regmap;
> > > 
> > >  	for (i = 0; i < ARRAY_SIZE(regulators); i++) {
> > > +		/* BD9574MWF supports DVFS only */
> > > +		if (chip == ROHM_CHIP_TYPE_BD9574 && regulators[i].id
> > > != DVFS)
> > > +			continue;
> > 
> > Does this mean that reading VD09 voltage is not supported by
> > driver?
> 
> Yes. Also, reading VD{18,25,33} voltage are not supported.

I think that would be excellent comment here. Maybe something like: "We
don't support voltage rails VD{09,18,25,33} by this driver on BD9574."

> 
> > (I
> > assumed VD09 initial voltage can be set from eeprom(?) and read by
> > driver - I may be wrong though). Perhaps it is worth mentioning in
> > the
> > commit message as a driver restriction?
> 
> Yes, these voltage can be set from eeprom and read by driver.
> So, I updated the description like below.
> 
> -- Updated description --
> Add support for BD9574MWF which is similar chip with BD9571MWV.
> Note that since BD9574MWF doesn't have avs_ops and vid_ops
> related registers, this driver avoids to use these registers
> if BD9574MWF is used.
> ------------------------

Thank you :) What I was after is that I would like to leave a note
about 'what could be improved' or about what is the 'software
limitation' here so that if anyone later needs the other voltage rails
he would have a hint about what could be done.

Do you think mentioning that "the VD09 voltage could be read from PMIC
but that is not supported by this commit" in commit message would be
Ok?

> 
> > And just asking out of the curiosity - are the other voltage rails
> > listed in data-sheet (VD18, DDR0, VD33, VD09 and LDO1,...,LDO4)
> > set-up
> > from DT as fixed-regulators? Any reason why they are not set-up
> > here?
> 
> TBH, I don't know why. Perhaps, the driver cannot read DDR0, LDO[1-4] 
> values?

I also think that all voltages can't be read. I was just thinking that
it might make sense to always create the fixed regulators from PMIC
driver - because if PMIC is used - then these voltage rails do exist.
(This was just a question so that I could learn - not so much of a
review comment.)

If you re-spin the series for other fixups - then I would appreciate
some comment about omitting the rest of the voltage outputs.

Other than that - for what it is worth:

Reviewed-by: Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>


Best Regards
	Matti


--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland
SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~

Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit

(Thanks for the translation Simon)