[PATCH RESEND] regulator: bd718x7, bd71828, Fix dvs voltage levels

[PATCH RESEND] regulator: bd718x7, bd71828, Fix dvs voltage levels

[Matti Vaittinen]

The ROHM BD718x7 and BD71828 drivers support setting HW state
specific voltages from device-tree. This is used also by various
in-tree DTS files.

These drivers do incorrectly try to compose bit-map using enum
values. By a chance this works for first two valid levels having
values 1 and 2 - but setting values for the rest of the levels
do indicate capability of setting values for first levels as
well. Luckily the regulators which support setting values for
SUSPEND/LPSR do usually also support setting values for RUN
and IDLE too - thus this has not been such a fatal issue.

Fix this by defining the old enum values as bits and fixing the
parsing code. This allows keeping existing IC specific drivers
intact and only slightly changing the rohm-regulator.c

Fixes: 21b72156ede8b ("regulator: bd718x7: Split driver to common and bd718x7 specific parts")
Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---

I just noticed this fix never made it in-tree. So this is a resend of a
resend :)

 drivers/regulator/rohm-regulator.c |  9 ++++++---
 include/linux/mfd/rohm-generic.h   | 14 ++++++--------
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/regulator/rohm-regulator.c b/drivers/regulator/rohm-regulator.c
index 399002383b28..5c558b153d55 100644
--- a/drivers/regulator/rohm-regulator.c
+++ b/drivers/regulator/rohm-regulator.c
@@ -52,9 +52,12 @@ int rohm_regulator_set_dvs_levels(const struct rohm_dvs_config *dvs,
 	char *prop;
 	unsigned int reg, mask, omask, oreg = desc->enable_reg;
 
-	for (i = 0; i < ROHM_DVS_LEVEL_MAX && !ret; i++) {
-		if (dvs->level_map & (1 << i)) {
-			switch (i + 1) {
+	for (i = 0; i < ROHM_DVS_LEVEL_VALID_AMOUNT && !ret; i++) {
+		int bit;
+
+		bit = BIT(i);
+		if (dvs->level_map & bit) {
+			switch (bit) {
 			case ROHM_DVS_LEVEL_RUN:
 				prop = "rohm,dvs-run-voltage";
 				reg = dvs->run_reg;
diff --git a/include/linux/mfd/rohm-generic.h b/include/linux/mfd/rohm-generic.h
index 4283b5b33e04..2b85b9deb03a 100644
--- a/include/linux/mfd/rohm-generic.h
+++ b/include/linux/mfd/rohm-generic.h
@@ -20,14 +20,12 @@ struct rohm_regmap_dev {
 	struct regmap *regmap;
 };
 
-enum {
-	ROHM_DVS_LEVEL_UNKNOWN,
-	ROHM_DVS_LEVEL_RUN,
-	ROHM_DVS_LEVEL_IDLE,
-	ROHM_DVS_LEVEL_SUSPEND,
-	ROHM_DVS_LEVEL_LPSR,
-	ROHM_DVS_LEVEL_MAX = ROHM_DVS_LEVEL_LPSR,
-};
+#define ROHM_DVS_LEVEL_RUN		BIT(0)
+#define ROHM_DVS_LEVEL_IDLE		BIT(1)
+#define ROHM_DVS_LEVEL_SUSPEND		BIT(2)
+#define ROHM_DVS_LEVEL_LPSR		BIT(3)
+#define ROHM_DVS_LEVEL_VALID_AMOUNT	4
+#define ROHM_DVS_LEVEL_UNKNOWN		0
 
 /**
  * struct rohm_dvs_config - dynamic voltage scaling register descriptions

base-commit: 92bf22614b21a2706f4993b278017e437f7785b3
-- 
2.25.4


-- 
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 to Simon Glass for the translation =] 

Re: [PATCH RESEND] regulator: bd718x7, bd71828, Fix dvs voltage levels

[Lee Jones]

On Fri, 12 Feb 2021, Matti Vaittinen wrote:

> The ROHM BD718x7 and BD71828 drivers support setting HW state
> specific voltages from device-tree. This is used also by various
> in-tree DTS files.
> 
> These drivers do incorrectly try to compose bit-map using enum
> values. By a chance this works for first two valid levels having
> values 1 and 2 - but setting values for the rest of the levels
> do indicate capability of setting values for first levels as
> well. Luckily the regulators which support setting values for
> SUSPEND/LPSR do usually also support setting values for RUN
> and IDLE too - thus this has not been such a fatal issue.
> 
> Fix this by defining the old enum values as bits and fixing the
> parsing code. This allows keeping existing IC specific drivers
> intact and only slightly changing the rohm-regulator.c
> 
> Fixes: 21b72156ede8b ("regulator: bd718x7: Split driver to common and bd718x7 specific parts")
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
> 
> I just noticed this fix never made it in-tree. So this is a resend of a
> resend :)

Not sure what you mean.  Isn't this patch part of:

  [PATCH v2 00/17] Support ROHM BD71815 PMIC

... which has just been reviewed and is awaiting rework?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RESEND] regulator: bd718x7, bd71828, Fix dvs voltage levels

[Matti Vaittinen]

Hello Lee,

On Fri, 2021-02-12 at 09:16 +0000, Lee Jones wrote:
> On Fri, 12 Feb 2021, Matti Vaittinen wrote:
> 
> > The ROHM BD718x7 and BD71828 drivers support setting HW state
> > specific voltages from device-tree. This is used also by various
> > in-tree DTS files.
> > 
> > These drivers do incorrectly try to compose bit-map using enum
> > values. By a chance this works for first two valid levels having
> > values 1 and 2 - but setting values for the rest of the levels
> > do indicate capability of setting values for first levels as
> > well. Luckily the regulators which support setting values for
> > SUSPEND/LPSR do usually also support setting values for RUN
> > and IDLE too - thus this has not been such a fatal issue.
> > 
> > Fix this by defining the old enum values as bits and fixing the
> > parsing code. This allows keeping existing IC specific drivers
> > intact and only slightly changing the rohm-regulator.c
> > 
> > Fixes: 21b72156ede8b ("regulator: bd718x7: Split driver to common
> > and bd718x7 specific parts")
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > ---
> > 
> > I just noticed this fix never made it in-tree. So this is a resend
> > of a
> > resend :)
> 
> Not sure what you mean.  Isn't this patch part of:
> 
>   [PATCH v2 00/17] Support ROHM BD71815 PMIC
> 
> ... which has just been reviewed and is awaiting rework?

Sorry for the confusion Lee.
It was originally part of the series but I did intend to submit it
separately. That's because it is a bugfix for existing drivers - and
because the  series "[PATCH v2 00/17] Support ROHM BD71815 PMIC"
is expected to take some time as it needs to wait the dependency
patches to get merged in relevant sub-systems. (The parent-data removal
patches to gpio, regulator and clk).

So my thinking was that that fix could've been merged in as it's own
patch to get existing things fixed in next release. I could then rebase
the "PATCH v2 00/17] Support ROHM BD71815 PMIC" - series on top of this
when it is in-tree.

I think I should have communicated this better. Sorry.

Please let me know what works for you guys the best.

Best Regards
    Matti Vaittinen

Re: [PATCH RESEND] regulator: bd718x7, bd71828, Fix dvs voltage levels

[Lee Jones]

On Fri, 12 Feb 2021, Matti Vaittinen wrote:

> The ROHM BD718x7 and BD71828 drivers support setting HW state
> specific voltages from device-tree. This is used also by various
> in-tree DTS files.
> 
> These drivers do incorrectly try to compose bit-map using enum
> values. By a chance this works for first two valid levels having
> values 1 and 2 - but setting values for the rest of the levels
> do indicate capability of setting values for first levels as
> well. Luckily the regulators which support setting values for
> SUSPEND/LPSR do usually also support setting values for RUN
> and IDLE too - thus this has not been such a fatal issue.
> 
> Fix this by defining the old enum values as bits and fixing the
> parsing code. This allows keeping existing IC specific drivers
> intact and only slightly changing the rohm-regulator.c
> 
> Fixes: 21b72156ede8b ("regulator: bd718x7: Split driver to common and bd718x7 specific parts")
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
> 
> I just noticed this fix never made it in-tree. So this is a resend of a
> resend :)
> 
>  drivers/regulator/rohm-regulator.c |  9 ++++++---
>  include/linux/mfd/rohm-generic.h   | 14 ++++++--------
>  2 files changed, 12 insertions(+), 11 deletions(-)

Happy for Mark to take this in:

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RESEND] regulator: bd718x7, bd71828, Fix dvs voltage levels

[Mark Brown]

On Fri, 12 Feb 2021 10:00:23 +0200, Matti Vaittinen wrote:
> The ROHM BD718x7 and BD71828 drivers support setting HW state
> specific voltages from device-tree. This is used also by various
> in-tree DTS files.
> 
> These drivers do incorrectly try to compose bit-map using enum
> values. By a chance this works for first two valid levels having
> values 1 and 2 - but setting values for the rest of the levels
> do indicate capability of setting values for first levels as
> well. Luckily the regulators which support setting values for
> SUSPEND/LPSR do usually also support setting values for RUN
> and IDLE too - thus this has not been such a fatal issue.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/1] regulator: bd718x7, bd71828, Fix dvs voltage levels
      commit: c294554111a835598b557db789d9ad2379b512a2

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark