[PATCH 1/2] regulator: bd70528: Fix off-by-one for buck123 .n_voltages setting

[PATCH 1/2] regulator: bd70528: Fix off-by-one for buck123 .n_voltages setting

[Axel Lin]

The valid selectors for bd70528 bucks are 0 ~ 0xf, so the .n_voltages
should be 16 (0x10). Use 0x10 to make it consistent with BD70528_LDO_VOLTS.
Also remove redundant defines for BD70528_BUCK_VOLTS.

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
I think this fix does not need "Fixes" tag because in original code the
.n_voltage is greater than correct one. The latest selector is not valid
in the linear range setting anyway.
 include/linux/mfd/rohm-bd70528.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/mfd/rohm-bd70528.h b/include/linux/mfd/rohm-bd70528.h
index a57af878fd0c..4a5966475a35 100644
--- a/include/linux/mfd/rohm-bd70528.h
+++ b/include/linux/mfd/rohm-bd70528.h
@@ -26,9 +26,7 @@ struct bd70528_data {
 	struct mutex rtc_timer_lock;
 };
 
-#define BD70528_BUCK_VOLTS 17
-#define BD70528_BUCK_VOLTS 17
-#define BD70528_BUCK_VOLTS 17
+#define BD70528_BUCK_VOLTS 0x10
 #define BD70528_LDO_VOLTS 0x20
 
 #define BD70528_REG_BUCK1_EN	0x0F
-- 
2.25.1

[PATCH 2/2] regulator: bd71828: Fix .n_voltages settings

[Axel Lin]

Current .n_voltages settings do not cover the latest 2 valid selectors,
so it fails to set voltage for the hightest voltage support.
The latest linear range has step_uV = 0, so it does not matter if we
count the .n_voltages to maximum selector + 1 or the first selector of
latest linear range + 1.
To simplify calculating the n_voltages, let's just set the
.n_voltages to maximum selector + 1.

Fixes: 522498f8cb8c ("regulator: bd71828: Basic support for ROHM bd71828 PMIC regulators")
Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 include/linux/mfd/rohm-bd71828.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/mfd/rohm-bd71828.h b/include/linux/mfd/rohm-bd71828.h
index c7ab69c87ee8..3b5f3a7db4bd 100644
--- a/include/linux/mfd/rohm-bd71828.h
+++ b/include/linux/mfd/rohm-bd71828.h
@@ -26,11 +26,11 @@ enum {
 	BD71828_REGULATOR_AMOUNT,
 };
 
-#define BD71828_BUCK1267_VOLTS		0xEF
-#define BD71828_BUCK3_VOLTS		0x10
-#define BD71828_BUCK4_VOLTS		0x20
-#define BD71828_BUCK5_VOLTS		0x10
-#define BD71828_LDO_VOLTS		0x32
+#define BD71828_BUCK1267_VOLTS		0x100
+#define BD71828_BUCK3_VOLTS		0x20
+#define BD71828_BUCK4_VOLTS		0x40
+#define BD71828_BUCK5_VOLTS		0x20
+#define BD71828_LDO_VOLTS		0x40
 /* LDO6 is fixed 1.8V voltage */
 #define BD71828_LDO_6_VOLTAGE		1800000
 
-- 
2.25.1

Re: [PATCH 1/2] regulator: bd70528: Fix off-by-one for buck123 .n_voltages setting

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1815 bytes --]

Hi Axel,

On Sun, 2021-05-23 at 15:10 +0800, Axel Lin wrote:
> The valid selectors for bd70528 bucks are 0 ~ 0xf, so the .n_voltages
> should be 16 (0x10). Use 0x10 to make it consistent with
> BD70528_LDO_VOLTS.
> Also remove redundant defines for BD70528_BUCK_VOLTS.
> 
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> ---
> I think this fix does not need "Fixes" tag because in original code
> the
> .n_voltage is greater than correct one. The latest selector is not
> valid
> in the linear range setting anyway.
>  include/linux/mfd/rohm-bd70528.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/include/linux/mfd/rohm-bd70528.h
> b/include/linux/mfd/rohm-bd70528.h
> index a57af878fd0c..4a5966475a35 100644
> --- a/include/linux/mfd/rohm-bd70528.h
> +++ b/include/linux/mfd/rohm-bd70528.h
> @@ -26,9 +26,7 @@ struct bd70528_data {
>  	struct mutex rtc_timer_lock;
>  };
>  
> -#define BD70528_BUCK_VOLTS 17
> -#define BD70528_BUCK_VOLTS 17
> -#define BD70528_BUCK_VOLTS 17
> +#define BD70528_BUCK_VOLTS 0x10

Thank you for fixing this. There really is only 16 valid voltage
settings as you pointed out. Regarding changing the define to hex - I
would prefer seeing the amount in decimal as it is easier to
understand. (I do understand bit-patterns better when in HEX - but
"real world" values like voltages, currents or amounts are easier (for
me) to understand when in decimals)


Best Regards
	Matti Vaittinen

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] regulator: bd70528: Fix off-by-one for buck123 .n_voltages setting

[Axel Lin]

Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> 於 2021年5月24日 週一 下午1:41寫道：
>
> Hi Axel,
>
> On Sun, 2021-05-23 at 15:10 +0800, Axel Lin wrote:
> > The valid selectors for bd70528 bucks are 0 ~ 0xf, so the .n_voltages
> > should be 16 (0x10). Use 0x10 to make it consistent with
> > BD70528_LDO_VOLTS.
> > Also remove redundant defines for BD70528_BUCK_VOLTS.
> >
> > Signed-off-by: Axel Lin <axel.lin@ingics.com>
> > ---
> > I think this fix does not need "Fixes" tag because in original code
> > the
> > .n_voltage is greater than correct one. The latest selector is not
> > valid
> > in the linear range setting anyway.
> >  include/linux/mfd/rohm-bd70528.h | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/include/linux/mfd/rohm-bd70528.h
> > b/include/linux/mfd/rohm-bd70528.h
> > index a57af878fd0c..4a5966475a35 100644
> > --- a/include/linux/mfd/rohm-bd70528.h
> > +++ b/include/linux/mfd/rohm-bd70528.h
> > @@ -26,9 +26,7 @@ struct bd70528_data {
> >       struct mutex rtc_timer_lock;
> >  };
> >
> > -#define BD70528_BUCK_VOLTS 17
> > -#define BD70528_BUCK_VOLTS 17
> > -#define BD70528_BUCK_VOLTS 17
> > +#define BD70528_BUCK_VOLTS 0x10
>
> Thank you for fixing this. There really is only 16 valid voltage
> settings as you pointed out. Regarding changing the define to hex - I
> would prefer seeing the amount in decimal as it is easier to
> understand. (I do understand bit-patterns better when in HEX - but
> "real world" values like voltages, currents or amounts are easier (for
> me) to understand when in decimals)

Current code already uses hex-decimal (which is the reason I use
hex-decimal for BD70528_BUCK_VOLTS):
#define BD70528_LDO_VOLTS 0x20

So do you suggest to change BD70528_LDO_VOLTS to decimal as well?

Regards,
Axel

Re: [PATCH 2/2] regulator: bd71828: Fix .n_voltages settings

[Matti Vaittinen]

On Sun, 2021-05-23 at 15:10 +0800, Axel Lin wrote:
> Current .n_voltages settings do not cover the latest 2 valid
> selectors,
> so it fails to set voltage for the hightest voltage support.
> The latest linear range has step_uV = 0, so it does not matter if we
> count the .n_voltages to maximum selector + 1 or the first selector
> of
> latest linear range + 1.
> To simplify calculating the n_voltages, let's just set the
> .n_voltages to maximum selector + 1.
> 
> Fixes: 522498f8cb8c ("regulator: bd71828: Basic support for ROHM
> bd71828 PMIC regulators")
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> ---

Thank you Axel. I never stop being surprized by your accuracy what
comes spotting errors like this. I had to look-up my calculator and the
data-sheet to verify your fix - and you did find this just by reviewing
the existing code(?) Really impressive. It seems the biggest supported
voltage (2V) was really not reachable as it only belonged to the last
(step 0) range. Big thanks!

Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>

>  include/linux/mfd/rohm-bd71828.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/mfd/rohm-bd71828.h
> b/include/linux/mfd/rohm-bd71828.h
> index c7ab69c87ee8..3b5f3a7db4bd 100644
> --- a/include/linux/mfd/rohm-bd71828.h
> +++ b/include/linux/mfd/rohm-bd71828.h
> @@ -26,11 +26,11 @@ enum {
>  	BD71828_REGULATOR_AMOUNT,
>  };
>  
> -#define BD71828_BUCK1267_VOLTS		0xEF
> -#define BD71828_BUCK3_VOLTS		0x10
> -#define BD71828_BUCK4_VOLTS		0x20
> -#define BD71828_BUCK5_VOLTS		0x10
> -#define BD71828_LDO_VOLTS		0x32
> +#define BD71828_BUCK1267_VOLTS		0x100
> +#define BD71828_BUCK3_VOLTS		0x20
> +#define BD71828_BUCK4_VOLTS		0x40
> +#define BD71828_BUCK5_VOLTS		0x20
> +#define BD71828_LDO_VOLTS		0x40
>  /* LDO6 is fixed 1.8V voltage */
>  #define BD71828_LDO_6_VOLTAGE		1800000
>  

Re: [PATCH 1/2] regulator: bd70528: Fix off-by-one for buck123 .n_voltages setting

[Vaittinen, Matti]

Morning Axel,

On Mon, 2021-05-24 at 14:06 +0800, Axel Lin wrote:
> Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> 於 2021年5月24日 週一
> 下午1:41寫道：
> > Hi Axel,
> > 
> > On Sun, 2021-05-23 at 15:10 +0800, Axel Lin wrote:
> > > The valid selectors for bd70528 bucks are 0 ~ 0xf, so the
> > > .n_voltages
> > > should be 16 (0x10). Use 0x10 to make it consistent with
> > > BD70528_LDO_VOLTS.
> > > Also remove redundant defines for BD70528_BUCK_VOLTS.
> > > 
> > > Signed-off-by: Axel Lin <axel.lin@ingics.com>
> > > ---
> > > I think this fix does not need "Fixes" tag because in original
> > > code
> > > the
> > > .n_voltage is greater than correct one. The latest selector is
> > > not
> > > valid
> > > in the linear range setting anyway.
> > >  include/linux/mfd/rohm-bd70528.h | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/include/linux/mfd/rohm-bd70528.h
> > > b/include/linux/mfd/rohm-bd70528.h
> > > index a57af878fd0c..4a5966475a35 100644
> > > --- a/include/linux/mfd/rohm-bd70528.h
> > > +++ b/include/linux/mfd/rohm-bd70528.h
> > > @@ -26,9 +26,7 @@ struct bd70528_data {
> > >       struct mutex rtc_timer_lock;
> > >  };
> > > 
> > > -#define BD70528_BUCK_VOLTS 17
> > > -#define BD70528_BUCK_VOLTS 17
> > > -#define BD70528_BUCK_VOLTS 17
> > > +#define BD70528_BUCK_VOLTS 0x10
> > 
> > Thank you for fixing this. There really is only 16 valid voltage
> > settings as you pointed out. Regarding changing the define to hex -
> > I
> > would prefer seeing the amount in decimal as it is easier to
> > understand. (I do understand bit-patterns better when in HEX - but
> > "real world" values like voltages, currents or amounts are easier
> > (for
> > me) to understand when in decimals)
> 
> Current code already uses hex-decimal (which is the reason I use
> hex-decimal for BD70528_BUCK_VOLTS):
> #define BD70528_LDO_VOLTS 0x20
> 
> So do you suggest to change BD70528_LDO_VOLTS to decimal as well?
> 

In my opinion that would be better. So if you feel like - then yes
please. After that being said - I am not expecting you to do it - I
appreciate the fix as it is.

Acked-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>


> Regards,
> Axel

Re: [PATCH 1/2] regulator: bd70528: Fix off-by-one for buck123 .n_voltages setting

[Mark Brown]

On Sun, 23 May 2021 15:10:44 +0800, Axel Lin wrote:
> The valid selectors for bd70528 bucks are 0 ~ 0xf, so the .n_voltages
> should be 16 (0x10). Use 0x10 to make it consistent with BD70528_LDO_VOLTS.
> Also remove redundant defines for BD70528_BUCK_VOLTS.

Applied to

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

Thanks!

[1/2] regulator: bd70528: Fix off-by-one for buck123 .n_voltages setting
      commit: 0514582a1a5b4ac1a3fd64792826d392d7ae9ddc
[2/2] regulator: bd71828: Fix .n_voltages settings
      commit: 4c668630bf8ea90a041fc69c9984486e0f56682d

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

Re: [PATCH 2/2] regulator: bd71828: Fix .n_voltages settings

[Lee Jones]

On Sun, 23 May 2021, Axel Lin wrote:

> Current .n_voltages settings do not cover the latest 2 valid selectors,
> so it fails to set voltage for the hightest voltage support.
> The latest linear range has step_uV = 0, so it does not matter if we
> count the .n_voltages to maximum selector + 1 or the first selector of
> latest linear range + 1.
> To simplify calculating the n_voltages, let's just set the
> .n_voltages to maximum selector + 1.
> 
> Fixes: 522498f8cb8c ("regulator: bd71828: Basic support for ROHM bd71828 PMIC regulators")
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> ---
>  include/linux/mfd/rohm-bd71828.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Applied, thanks.

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