| Message ID | 1607686060-17448-5-git-send-email-yoshihiro.shimoda.uh@renesas.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | treewide: bd9571mwv: Add support for BD9574MWF | expand |
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.) > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > 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@gmail.com> > > * > > @@ -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 > 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. ------------------------ > 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? Best regards, Yoshihiro Shimoda
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@renesas.com> > > > --- > > > 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@gmail.com > > > > > > > * > > > @@ -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@fi.rohmeurope.com> 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)
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@renesas.com> > > > > --- > > > > 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@gmail.com > > > > > > > > > * > > > > @@ -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@fi.rohmeurope.com> Thank you very much for your review! Best regards, Yoshihiro Shimoda
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@gmail.com> * @@ -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; rdev = devm_regulator_register(&pdev->dev, ®ulators[i], &config); if (IS_ERR(rdev)) { @@ -339,7 +344,8 @@ static int bd9571mwv_regulator_probe(struct platform_device *pdev) } static const struct platform_device_id bd9571mwv_regulator_id_table[] = { - { "bd9571mwv-regulator", }, + { "bd9571mwv-regulator", ROHM_CHIP_TYPE_BD9571 }, + { "bd9574mwf-regulator", ROHM_CHIP_TYPE_BD9574 }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(platform, bd9571mwv_regulator_id_table);
Add support for BD9574MWF which is silimar chip with BD9571MWV. Note that BD9574MWF doesn't support AVS and VID. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/regulator/bd9571mwv-regulator.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)