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

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>]

Hello Matti-san,

> From: Vaittinen, Matti, Sent: Monday, December 14, 2020 4:13 PM
> 
> 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?

Yes.

> If I am
> not mistaken, this is different from register where (initial) voltage
> is set?

Yes. I checked on my environment (H3 Salvator-XS).

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

Thank you for the suggestion! I'll use this comment.

> > > (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?

I think OK because VD09 could be read from "BD9574MWF_VD09_VINIT"
register, but that is not supported but this commit.

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

Thank you very much for your review!

Best regards,
Yoshihiro Shimoda