[RFT][PATCH 1/2] regulator: lm363x: Fix off-by-one n_voltages for lm3632 ldo_vpos/ldo_vneg

[RFT][PATCH 1/2] regulator: lm363x: Fix off-by-one n_voltages for lm3632 ldo_vpos/ldo_vneg

[Axel Lin]

According to the datasheet https://www.ti.com/lit/ds/symlink/lm3632a.pdf
Table 20. VPOS Bias Register Field Descriptions VPOS[5:0]
Sets the Positive Display Bias (LDO) Voltage (50 mV per step)
000000: 4 V
000001: 4.05 V
000010: 4.1 V
....................
011101: 5.45 V
011110: 5.5 V (Default)
011111: 5.55 V
....................
100111: 5.95 V
101000: 6 V
Note: Codes 101001 to 111111 map to 6 V

The LM3632_LDO_VSEL_MAX should be 0b101000 (0x28), so the maximum voltage
can match the datasheet.

Fixes: 3a8d1a73a037 ("regulator: add LM363X driver")
Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 drivers/regulator/lm363x-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/lm363x-regulator.c b/drivers/regulator/lm363x-regulator.c
index 5647e2f97ff8..e4a27d63bf90 100644
--- a/drivers/regulator/lm363x-regulator.c
+++ b/drivers/regulator/lm363x-regulator.c
@@ -30,7 +30,7 @@
 
 /* LM3632 */
 #define LM3632_BOOST_VSEL_MAX		0x26
-#define LM3632_LDO_VSEL_MAX		0x29
+#define LM3632_LDO_VSEL_MAX		0x28
 #define LM3632_VBOOST_MIN		4500000
 #define LM3632_VLDO_MIN			4000000
 
-- 
2.20.1

[RFT][PATCH 2/2] regulator: lm363x: Fix n_voltages setting for lm36274

[Axel Lin]

According to the datasheet http://www.ti.com/lit/ds/symlink/lm36274.pdf:
Table 23. VPOS Bias Register Field Descriptions VPOS[5:0]:
VPOS voltage (50-mV steps): VPOS = 4 V + (Code × 50 mV), 6.5 V max
000000 = 4 V
000001 = 4.05 V
:
011110 = 5.5 V (Default)
:
110010 = 6.5 V
110011 to 111111 map to 6.5 V

So the LM36274_LDO_VSEL_MAX should be 0b110010 (0x32).
The valid selectors are 0 ... LM36274_LDO_VSEL_MAX, n_voltages should be
LM36274_LDO_VSEL_MAX + 1. Similarly, the n_voltages should be
LM36274_BOOST_VSEL_MAX + 1 for LM36274_BOOST.

Fixes: bff5e8071533 ("regulator: lm363x: Add support for LM36274")
Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 drivers/regulator/lm363x-regulator.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/lm363x-regulator.c b/drivers/regulator/lm363x-regulator.c
index e4a27d63bf90..4b9f618b07e9 100644
--- a/drivers/regulator/lm363x-regulator.c
+++ b/drivers/regulator/lm363x-regulator.c
@@ -36,7 +36,7 @@
 
 /* LM36274 */
 #define LM36274_BOOST_VSEL_MAX		0x3f
-#define LM36274_LDO_VSEL_MAX		0x34
+#define LM36274_LDO_VSEL_MAX		0x32
 #define LM36274_VOLTAGE_MIN		4000000
 
 /* Common */
@@ -226,7 +226,7 @@ static const struct regulator_desc lm363x_regulator_desc[] = {
 		.of_match	= "vboost",
 		.id             = LM36274_BOOST,
 		.ops            = &lm363x_boost_voltage_table_ops,
-		.n_voltages     = LM36274_BOOST_VSEL_MAX,
+		.n_voltages     = LM36274_BOOST_VSEL_MAX + 1,
 		.min_uV         = LM36274_VOLTAGE_MIN,
 		.uV_step        = LM363X_STEP_50mV,
 		.type           = REGULATOR_VOLTAGE,
@@ -239,7 +239,7 @@ static const struct regulator_desc lm363x_regulator_desc[] = {
 		.of_match	= "vpos",
 		.id             = LM36274_LDO_POS,
 		.ops            = &lm363x_regulator_voltage_table_ops,
-		.n_voltages     = LM36274_LDO_VSEL_MAX,
+		.n_voltages     = LM36274_LDO_VSEL_MAX + 1,
 		.min_uV         = LM36274_VOLTAGE_MIN,
 		.uV_step        = LM363X_STEP_50mV,
 		.type           = REGULATOR_VOLTAGE,
@@ -254,7 +254,7 @@ static const struct regulator_desc lm363x_regulator_desc[] = {
 		.of_match	= "vneg",
 		.id             = LM36274_LDO_NEG,
 		.ops            = &lm363x_regulator_voltage_table_ops,
-		.n_voltages     = LM36274_LDO_VSEL_MAX,
+		.n_voltages     = LM36274_LDO_VSEL_MAX + 1,
 		.min_uV         = LM36274_VOLTAGE_MIN,
 		.uV_step        = LM363X_STEP_50mV,
 		.type           = REGULATOR_VOLTAGE,
-- 
2.20.1

Re: [RFT][PATCH 2/2] regulator: lm363x: Fix n_voltages setting for lm36274

[Dan Murphy]

Hello

On 6/26/19 8:26 AM, Axel Lin wrote:
> According to the datasheet http://www.ti.com/lit/ds/symlink/lm36274.pdf:
> Table 23. VPOS Bias Register Field Descriptions VPOS[5:0]:
> VPOS voltage (50-mV steps): VPOS = 4 V + (Code × 50 mV), 6.5 V max
> 000000 = 4 V
> 000001 = 4.05 V
> :
> 011110 = 5.5 V (Default)
> :
> 110010 = 6.5 V
> 110011 to 111111 map to 6.5 V
>
> So the LM36274_LDO_VSEL_MAX should be 0b110010 (0x32).
> The valid selectors are 0 ... LM36274_LDO_VSEL_MAX, n_voltages should be
> LM36274_LDO_VSEL_MAX + 1. Similarly, the n_voltages should be
> LM36274_BOOST_VSEL_MAX + 1 for LM36274_BOOST.
>
> Fixes: bff5e8071533 ("regulator: lm363x: Add support for LM36274")
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> ---
>   drivers/regulator/lm363x-regulator.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/regulator/lm363x-regulator.c b/drivers/regulator/lm363x-regulator.c
> index e4a27d63bf90..4b9f618b07e9 100644
> --- a/drivers/regulator/lm363x-regulator.c
> +++ b/drivers/regulator/lm363x-regulator.c
> @@ -36,7 +36,7 @@
>   
>   /* LM36274 */
>   #define LM36274_BOOST_VSEL_MAX		0x3f
> -#define LM36274_LDO_VSEL_MAX		0x34
> +#define LM36274_LDO_VSEL_MAX		0x32

This does not seem correct the max number of voltages are 0x34.

The register is zero based so you can have 33 voltage select levels and 
+ 1 is 34 total selectors

Liam/Mark correct me if I am incorrect.

Dan


>   #define LM36274_VOLTAGE_MIN		4000000
>   
>   /* Common */
> @@ -226,7 +226,7 @@ static const struct regulator_desc lm363x_regulator_desc[] = {
>   		.of_match	= "vboost",
>   		.id             = LM36274_BOOST,
>   		.ops            = &lm363x_boost_voltage_table_ops,
> -		.n_voltages     = LM36274_BOOST_VSEL_MAX,
> +		.n_voltages     = LM36274_BOOST_VSEL_MAX + 1,
>   		.min_uV         = LM36274_VOLTAGE_MIN,
>   		.uV_step        = LM363X_STEP_50mV,
>   		.type           = REGULATOR_VOLTAGE,
> @@ -239,7 +239,7 @@ static const struct regulator_desc lm363x_regulator_desc[] = {
>   		.of_match	= "vpos",
>   		.id             = LM36274_LDO_POS,
>   		.ops            = &lm363x_regulator_voltage_table_ops,
> -		.n_voltages     = LM36274_LDO_VSEL_MAX,
> +		.n_voltages     = LM36274_LDO_VSEL_MAX + 1,
>   		.min_uV         = LM36274_VOLTAGE_MIN,
>   		.uV_step        = LM363X_STEP_50mV,
>   		.type           = REGULATOR_VOLTAGE,
> @@ -254,7 +254,7 @@ static const struct regulator_desc lm363x_regulator_desc[] = {
>   		.of_match	= "vneg",
>   		.id             = LM36274_LDO_NEG,
>   		.ops            = &lm363x_regulator_voltage_table_ops,
> -		.n_voltages     = LM36274_LDO_VSEL_MAX,
> +		.n_voltages     = LM36274_LDO_VSEL_MAX + 1,
>   		.min_uV         = LM36274_VOLTAGE_MIN,
>   		.uV_step        = LM363X_STEP_50mV,
>   		.type           = REGULATOR_VOLTAGE,

Re: [RFT][PATCH 2/2] regulator: lm363x: Fix n_voltages setting for lm36274

[Axel Lin]

Dan Murphy <dmurphy@ti.com> 於 2019年6月26日 週三 下午11:07寫道：
>
> Hello
>
> On 6/26/19 8:26 AM, Axel Lin wrote:
> > According to the datasheet http://www.ti.com/lit/ds/symlink/lm36274.pdf:
> > Table 23. VPOS Bias Register Field Descriptions VPOS[5:0]:
> > VPOS voltage (50-mV steps): VPOS = 4 V + (Code × 50 mV), 6.5 V max
> > 000000 = 4 V
> > 000001 = 4.05 V
> > :
> > 011110 = 5.5 V (Default)
> > :
> > 110010 = 6.5 V
> > 110011 to 111111 map to 6.5 V
> >
> > So the LM36274_LDO_VSEL_MAX should be 0b110010 (0x32).
> > The valid selectors are 0 ... LM36274_LDO_VSEL_MAX, n_voltages should be
> > LM36274_LDO_VSEL_MAX + 1. Similarly, the n_voltages should be
> > LM36274_BOOST_VSEL_MAX + 1 for LM36274_BOOST.
> >
> > Fixes: bff5e8071533 ("regulator: lm363x: Add support for LM36274")
> > Signed-off-by: Axel Lin <axel.lin@ingics.com>
> > ---
> >   drivers/regulator/lm363x-regulator.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/regulator/lm363x-regulator.c b/drivers/regulator/lm363x-regulator.c
> > index e4a27d63bf90..4b9f618b07e9 100644
> > --- a/drivers/regulator/lm363x-regulator.c
> > +++ b/drivers/regulator/lm363x-regulator.c
> > @@ -36,7 +36,7 @@
> >
> >   /* LM36274 */
> >   #define LM36274_BOOST_VSEL_MAX              0x3f
> > -#define LM36274_LDO_VSEL_MAX         0x34
> > +#define LM36274_LDO_VSEL_MAX         0x32
>
> This does not seem correct the max number of voltages are 0x34.
>
> The register is zero based so you can have 33 voltage select levels and
> + 1 is 34 total selectors
>
> Liam/Mark correct me if I am incorrect.

From the datasheet, the maximum voltage 110010 = 6.5 V, the 0b110010 is 0x32.
I know it is 0 based, so .n_voltages     = LM36274_LDO_VSEL_MAX + 1,
(And that coding style is to match the original code.)

With your current code where LM36274_LDO_VSEL_MAX and n_voltages is 0x34,
the maximum voltage will become 400000 + 50000 * 0x34 = 6.6V which
does not match the datasheet.

Would you mind double check again?

Re: [RFT][PATCH 2/2] regulator: lm363x: Fix n_voltages setting for lm36274

[Dan Murphy]

Axel

On 6/26/19 10:20 AM, Axel Lin wrote:
> Dan Murphy <dmurphy@ti.com> 於 2019年6月26日 週三 下午11:07寫道：
>> Hello
>>
>> On 6/26/19 8:26 AM, Axel Lin wrote:
>>> According to the datasheet http://www.ti.com/lit/ds/symlink/lm36274.pdf:
>>> Table 23. VPOS Bias Register Field Descriptions VPOS[5:0]:
>>> VPOS voltage (50-mV steps): VPOS = 4 V + (Code × 50 mV), 6.5 V max
>>> 000000 = 4 V
>>> 000001 = 4.05 V
>>> :
>>> 011110 = 5.5 V (Default)
>>> :
>>> 110010 = 6.5 V
>>> 110011 to 111111 map to 6.5 V
>>>
>>> So the LM36274_LDO_VSEL_MAX should be 0b110010 (0x32).
>>> The valid selectors are 0 ... LM36274_LDO_VSEL_MAX, n_voltages should be
>>> LM36274_LDO_VSEL_MAX + 1. Similarly, the n_voltages should be
>>> LM36274_BOOST_VSEL_MAX + 1 for LM36274_BOOST.
>>>
>>> Fixes: bff5e8071533 ("regulator: lm363x: Add support for LM36274")
>>> Signed-off-by: Axel Li
>>>
>>> 6.5 V
>>> DISPLAY BIAS POSITIVE OUTPUT (VPOS)
>>> Programmable output voltage
>>> range
>>>
>>>
>>> n <axel.lin@ingics.com>
>>> ---
>>>    drivers/regulator/lm363x-regulator.c | 8 ++++----
>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/regulator/lm363x-regulator.c b/drivers/regulator/lm363x-regulator.c
>>> index e4a27d63bf90..4b9f618b07e9 100644
>>> --- a/drivers/regulator/lm363x-regulator.c
>>> +++ b/drivers/regulator/lm363x-regulator.c
>>> @@ -36,7 +36,7 @@
>>>
>>>    /* LM36274 */
>>>    #define LM36274_BOOST_VSEL_MAX              0x3f
>>> -#define LM36274_LDO_VSEL_MAX         0x34
>>> +#define LM36274_LDO_VSEL_MAX         0x32
>>>
>>> 6.5 V
>>> DISPLAY BIAS POSITIVE OUTPUT (VPOS)
>>> Programmable output voltage
>>> range
>>>
>>>
>> This does not seem correct the max number of voltages are 0x34.
>>
>> The register is zero based so you can have 33 voltage select levels and
>> + 1 is 34 total selectors
>>
>> Liam/Mark correct me if I am incorrect.
>  From the datasheet, the maximum voltage 110010 = 6.5 V, the 0b110010 is 0x32.
> I know it is 0 based, so .n_voltages     = LM36274_LDO_VSEL_MAX + 1,
> (And that coding style is to match the original code.)
>
> With your current code where LM36274_LDO_VSEL_MAX and n_voltages is 0x34,
> the maximum voltage will become 400000 + 50000 * 0x34 = 6.6V which
> does not match the datasheet.

Not sure how you get 6.6v the LDO max is 6.5v.

After 0x32->0x7f maps to 6.5v

000000 = 4 V
000001 = 4.05 V
:
011110 = 5.5 V (Default)
:
110010 = 6.5 V

110011 to 111111 map to 6.5 V <- Should never see 6.6v from LDO

Page 7 of the Datasheet says range is 4v->6.5v

Dan

> Would you mind double check again?

Re: [RFT][PATCH 2/2] regulator: lm363x: Fix n_voltages setting for lm36274

[Axel Lin]

> > With your current code where LM36274_LDO_VSEL_MAX and n_voltages is 0x34,
> > the maximum voltage will become 400000 + 50000 * 0x34 = 6.6V which
> > does not match the datasheet.
>
> Not sure how you get 6.6v the LDO max is 6.5v.
>
> After 0x32->0x7f maps to 6.5v
>
> 000000 = 4 V
> 000001 = 4.05 V
> :
> 011110 = 5.5 V (Default)
> :
> 110010 = 6.5 V
>
> 110011 to 111111 map to 6.5 V <- Should never see 6.6v from LDO
>
> Page 7 of the Datasheet says range is 4v->6.5v
Hi Dan,

The device indeed can only support up to 6.5V, the point is you are using
linear equation to calculate the voltage of each selecter.
In your current code:
#define LM36274_LDO_VSEL_MAX           0x34 (and it's .n_voltages)
So it supports selector 0 ... 0x33.
For selector 0x33 in the linear equation is
4000000 + 50000 * 51 = 6550000 (i.e. 6.55V)
i.e. The device actually only support up to 6.5V but the driver
reports it support up to 6.55V
     because regulator_list_voltage() will return 6.55V for selector 0x33.
(I have off-by-one in my previous reply because when .n_voltages is
0x34, it supports up to 0x33)

Regards,
AXel

Re: [RFT][PATCH 2/2] regulator: lm363x: Fix n_voltages setting for lm36274

[Axel Lin]

Axel Lin <axel.lin@ingics.com> 於 2019年6月27日 週四 上午7:58寫道：
>
> > > With your current code where LM36274_LDO_VSEL_MAX and n_voltages is 0x34,
> > > the maximum voltage will become 400000 + 50000 * 0x34 = 6.6V which
> > > does not match the datasheet.
> >
> > Not sure how you get 6.6v the LDO max is 6.5v.
> >
> > After 0x32->0x7f maps to 6.5v
> >
> > 000000 = 4 V
> > 000001 = 4.05 V
> > :
> > 011110 = 5.5 V (Default)
> > :
> > 110010 = 6.5 V
> >
> > 110011 to 111111 map to 6.5 V <- Should never see 6.6v from LDO
> >
> > Page 7 of the Datasheet says range is 4v->6.5v
> Hi Dan,
>
> The device indeed can only support up to 6.5V, the point is you are using
> linear equation to calculate the voltage of each selecter.
> In your current code:
> #define LM36274_LDO_VSEL_MAX           0x34 (and it's .n_voltages)
> So it supports selector 0 ... 0x33.
> For selector 0x33 in the linear equation is
> 4000000 + 50000 * 51 = 6550000 (i.e. 6.55V)
> i.e. The device actually only support up to 6.5V but the driver
> reports it support up to 6.55V
>      because regulator_list_voltage() will return 6.55V for selector 0x33.
> (I have off-by-one in my previous reply because when .n_voltages is
> 0x34, it supports up to 0x33)

Similar comment as I mentioned in another path.
Did you check regulator_list_voltage() output for the boundary case
with and without this patch?

Applied "regulator: lm363x: Fix n_voltages setting for lm36274" to the regulator tree

[Mark Brown]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3814 bytes --]

The patch

   regulator: lm363x: Fix n_voltages setting for lm36274

has been applied to the regulator tree at

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

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

From 962f170d9344e5d9edb3903971c591f42d55e226 Mon Sep 17 00:00:00 2001
From: Axel Lin <axel.lin@ingics.com>
Date: Wed, 26 Jun 2019 21:26:32 +0800
Subject: [PATCH] regulator: lm363x: Fix n_voltages setting for lm36274
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

According to the datasheet http://www.ti.com/lit/ds/symlink/lm36274.pdf:
Table 23. VPOS Bias Register Field Descriptions VPOS[5:0]:
VPOS voltage (50-mV steps): VPOS = 4 V + (Code × 50 mV), 6.5 V max
000000 = 4 V
000001 = 4.05 V
:
011110 = 5.5 V (Default)
:
110010 = 6.5 V
110011 to 111111 map to 6.5 V

So the LM36274_LDO_VSEL_MAX should be 0b110010 (0x32).
The valid selectors are 0 ... LM36274_LDO_VSEL_MAX, n_voltages should be
LM36274_LDO_VSEL_MAX + 1. Similarly, the n_voltages should be
LM36274_BOOST_VSEL_MAX + 1 for LM36274_BOOST.

Fixes: bff5e8071533 ("regulator: lm363x: Add support for LM36274")
Signed-off-by: Axel Lin <axel.lin@ingics.com>
Link: https://lore.kernel.org/r/20190626132632.32629-2-axel.lin@ingics.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/lm363x-regulator.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/lm363x-regulator.c b/drivers/regulator/lm363x-regulator.c
index e4a27d63bf90..4b9f618b07e9 100644
--- a/drivers/regulator/lm363x-regulator.c
+++ b/drivers/regulator/lm363x-regulator.c
@@ -36,7 +36,7 @@
 
 /* LM36274 */
 #define LM36274_BOOST_VSEL_MAX		0x3f
-#define LM36274_LDO_VSEL_MAX		0x34
+#define LM36274_LDO_VSEL_MAX		0x32
 #define LM36274_VOLTAGE_MIN		4000000
 
 /* Common */
@@ -226,7 +226,7 @@ static const struct regulator_desc lm363x_regulator_desc[] = {
 		.of_match	= "vboost",
 		.id             = LM36274_BOOST,
 		.ops            = &lm363x_boost_voltage_table_ops,
-		.n_voltages     = LM36274_BOOST_VSEL_MAX,
+		.n_voltages     = LM36274_BOOST_VSEL_MAX + 1,
 		.min_uV         = LM36274_VOLTAGE_MIN,
 		.uV_step        = LM363X_STEP_50mV,
 		.type           = REGULATOR_VOLTAGE,
@@ -239,7 +239,7 @@ static const struct regulator_desc lm363x_regulator_desc[] = {
 		.of_match	= "vpos",
 		.id             = LM36274_LDO_POS,
 		.ops            = &lm363x_regulator_voltage_table_ops,
-		.n_voltages     = LM36274_LDO_VSEL_MAX,
+		.n_voltages     = LM36274_LDO_VSEL_MAX + 1,
 		.min_uV         = LM36274_VOLTAGE_MIN,
 		.uV_step        = LM363X_STEP_50mV,
 		.type           = REGULATOR_VOLTAGE,
@@ -254,7 +254,7 @@ static const struct regulator_desc lm363x_regulator_desc[] = {
 		.of_match	= "vneg",
 		.id             = LM36274_LDO_NEG,
 		.ops            = &lm363x_regulator_voltage_table_ops,
-		.n_voltages     = LM36274_LDO_VSEL_MAX,
+		.n_voltages     = LM36274_LDO_VSEL_MAX + 1,
 		.min_uV         = LM36274_VOLTAGE_MIN,
 		.uV_step        = LM363X_STEP_50mV,
 		.type           = REGULATOR_VOLTAGE,
-- 
2.20.1

Re: [RFT][PATCH 1/2] regulator: lm363x: Fix off-by-one n_voltages for lm3632 ldo_vpos/ldo_vneg

[Dan Murphy]

Hello

On 6/26/19 8:26 AM, Axel Lin wrote:
> According to the datasheet https://www.ti.com/lit/ds/symlink/lm3632a.pdf
> Table 20. VPOS Bias Register Field Descriptions VPOS[5:0]
> Sets the Positive Display Bias (LDO) Voltage (50 mV per step)
> 000000: 4 V
> 000001: 4.05 V
> 000010: 4.1 V
> ....................
> 011101: 5.45 V
> 011110: 5.5 V (Default)
> 011111: 5.55 V
> ....................
> 100111: 5.95 V
> 101000: 6 V
> Note: Codes 101001 to 111111 map to 6 V
>
> The LM3632_LDO_VSEL_MAX should be 0b101000 (0x28), so the maximum voltage
> can match the datasheet.
>
> Fixes: 3a8d1a73a037 ("regulator: add LM363X driver")
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> ---
>   drivers/regulator/lm363x-regulator.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/regulator/lm363x-regulator.c b/drivers/regulator/lm363x-regulator.c
> index 5647e2f97ff8..e4a27d63bf90 100644
> --- a/drivers/regulator/lm363x-regulator.c
> +++ b/drivers/regulator/lm363x-regulator.c
> @@ -30,7 +30,7 @@
>   
>   /* LM3632 */
>   #define LM3632_BOOST_VSEL_MAX		0x26
> -#define LM3632_LDO_VSEL_MAX		0x29
> +#define LM3632_LDO_VSEL_MAX		0x28

Similar comment as I made on the LM36274

These are 0 based registers so it is 28 + 1

Dan


>   #define LM3632_VBOOST_MIN		4500000
>   #define LM3632_VLDO_MIN			4000000
>   

Re: [RFT][PATCH 1/2] regulator: lm363x: Fix off-by-one n_voltages for lm3632 ldo_vpos/ldo_vneg

[Axel Lin]

Dan Murphy <dmurphy@ti.com> 於 2019年6月26日 週三 下午11:07寫道：
>
> Hello
>
> On 6/26/19 8:26 AM, Axel Lin wrote:
> > According to the datasheet https://www.ti.com/lit/ds/symlink/lm3632a.pdf
> > Table 20. VPOS Bias Register Field Descriptions VPOS[5:0]
> > Sets the Positive Display Bias (LDO) Voltage (50 mV per step)
> > 000000: 4 V
> > 000001: 4.05 V
> > 000010: 4.1 V
> > ....................
> > 011101: 5.45 V
> > 011110: 5.5 V (Default)
> > 011111: 5.55 V
> > ....................
> > 100111: 5.95 V
> > 101000: 6 V
> > Note: Codes 101001 to 111111 map to 6 V
> >
> > The LM3632_LDO_VSEL_MAX should be 0b101000 (0x28), so the maximum voltage
> > can match the datasheet.
> >
> > Fixes: 3a8d1a73a037 ("regulator: add LM363X driver")
> > Signed-off-by: Axel Lin <axel.lin@ingics.com>
> > ---
> >   drivers/regulator/lm363x-regulator.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/regulator/lm363x-regulator.c b/drivers/regulator/lm363x-regulator.c
> > index 5647e2f97ff8..e4a27d63bf90 100644
> > --- a/drivers/regulator/lm363x-regulator.c
> > +++ b/drivers/regulator/lm363x-regulator.c
> > @@ -30,7 +30,7 @@
> >
> >   /* LM3632 */
> >   #define LM3632_BOOST_VSEL_MAX               0x26
> > -#define LM3632_LDO_VSEL_MAX          0x29
> > +#define LM3632_LDO_VSEL_MAX          0x28
>
> Similar comment as I made on the LM36274
>
> These are 0 based registers so it is 28 + 1
The code shows:  .n_voltages     = LM3632_LDO_VSEL_MAX + 1
so LM3632_LDO_VSEL_MAX needs to be 0x28.

                .name           = "ldo_vpos",
                .of_match       = "vpos",
                .id             = LM3632_LDO_POS,
                .ops            = &lm363x_regulator_voltage_table_ops,
                .n_voltages     = LM3632_LDO_VSEL_MAX + 1,

Re: [RFT][PATCH 1/2] regulator: lm363x: Fix off-by-one n_voltages for lm3632 ldo_vpos/ldo_vneg

[Axel Lin]

Axel Lin <axel.lin@ingics.com> 於 2019年6月26日 週三 下午11:12寫道：
>
> Dan Murphy <dmurphy@ti.com> 於 2019年6月26日 週三 下午11:07寫道：
> >
> > Hello
> >
> > On 6/26/19 8:26 AM, Axel Lin wrote:
> > > According to the datasheet https://www.ti.com/lit/ds/symlink/lm3632a.pdf
> > > Table 20. VPOS Bias Register Field Descriptions VPOS[5:0]
> > > Sets the Positive Display Bias (LDO) Voltage (50 mV per step)
> > > 000000: 4 V
> > > 000001: 4.05 V
> > > 000010: 4.1 V
> > > ....................
> > > 011101: 5.45 V
> > > 011110: 5.5 V (Default)
> > > 011111: 5.55 V
> > > ....................
> > > 100111: 5.95 V
> > > 101000: 6 V
> > > Note: Codes 101001 to 111111 map to 6 V
> > >
> > > The LM3632_LDO_VSEL_MAX should be 0b101000 (0x28), so the maximum voltage
> > > can match the datasheet.
> > >
> > > Fixes: 3a8d1a73a037 ("regulator: add LM363X driver")
> > > Signed-off-by: Axel Lin <axel.lin@ingics.com>
> > > ---
> > >   drivers/regulator/lm363x-regulator.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/regulator/lm363x-regulator.c b/drivers/regulator/lm363x-regulator.c
> > > index 5647e2f97ff8..e4a27d63bf90 100644
> > > --- a/drivers/regulator/lm363x-regulator.c
> > > +++ b/drivers/regulator/lm363x-regulator.c
> > > @@ -30,7 +30,7 @@
> > >
> > >   /* LM3632 */
> > >   #define LM3632_BOOST_VSEL_MAX               0x26
> > > -#define LM3632_LDO_VSEL_MAX          0x29
> > > +#define LM3632_LDO_VSEL_MAX          0x28
> >
> > Similar comment as I made on the LM36274
> >
> > These are 0 based registers so it is 28 + 1
> The code shows:  .n_voltages     = LM3632_LDO_VSEL_MAX + 1
> so LM3632_LDO_VSEL_MAX needs to be 0x28.
>
>                 .name           = "ldo_vpos",
>                 .of_match       = "vpos",
>                 .id             = LM3632_LDO_POS,
>                 .ops            = &lm363x_regulator_voltage_table_ops,
>                 .n_voltages     = LM3632_LDO_VSEL_MAX + 1,

Hi Dan,
I'm wondering if you read my previous reply.
You can try to call regulator_list_voltage() for selector 0x29 with
current code,
I believe it will return 6.05V which is wrong because the h/w only
support up to 6V.
And that is exactly the issue this patch try to fix.

BTW, you seem mixes the meaning of latest valid selector
(LM3632_LDO_VSEL_MAX) with n_voltage
since you mentioned it's 0 based registers.
From the context all the LM3632_LDO_xxx_MAX are defined as latest
valid selector because you
can find the code: .n_voltages     = LM3632_LDO_VSEL_MAX + 1.

Regards,
Axel

Re: [RFT][PATCH 1/2] regulator: lm363x: Fix off-by-one n_voltages for lm3632 ldo_vpos/ldo_vneg

[Dan Murphy]

Axel

On 7/7/19 9:02 PM, Axel Lin wrote:
> Axel Lin <axel.lin@ingics.com> 於 2019年6月26日 週三 下午11:12寫道：
>> Dan Murphy <dmurphy@ti.com> 於 2019年6月26日 週三 下午11:07寫道：
>>> Hello
>>>
>>> On 6/26/19 8:26 AM, Axel Lin wrote:
>>>> According to the datasheet https://www.ti.com/lit/ds/symlink/lm3632a.pdf
>>>> Table 20. VPOS Bias Register Field Descriptions VPOS[5:0]
>>>> Sets the Positive Display Bias (LDO) Voltage (50 mV per step)
>>>> 000000: 4 V
>>>> 000001: 4.05 V
>>>> 000010: 4.1 V
>>>> ....................
>>>> 011101: 5.45 V
>>>> 011110: 5.5 V (Default)
>>>> 011111: 5.55 V
>>>> ....................
>>>> 100111: 5.95 V
>>>> 101000: 6 V
>>>> Note: Codes 101001 to 111111 map to 6 V
>>>>
>>>> The LM3632_LDO_VSEL_MAX should be 0b101000 (0x28), so the maximum voltage
>>>> can match the datasheet.
>>>>
>>>> Fixes: 3a8d1a73a037 ("regulator: add LM363X driver")
>>>> Signed-off-by: Axel Lin <axel.lin@ingics.com>
>>>> ---
>>>>    drivers/regulator/lm363x-regulator.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/regulator/lm363x-regulator.c b/drivers/regulator/lm363x-regulator.c
>>>> index 5647e2f97ff8..e4a27d63bf90 100644
>>>> --- a/drivers/regulator/lm363x-regulator.c
>>>> +++ b/drivers/regulator/lm363x-regulator.c
>>>> @@ -30,7 +30,7 @@
>>>>
>>>>    /* LM3632 */
>>>>    #define LM3632_BOOST_VSEL_MAX               0x26
>>>> -#define LM3632_LDO_VSEL_MAX          0x29
>>>> +#define LM3632_LDO_VSEL_MAX          0x28
>>> Similar comment as I made on the LM36274
>>>
>>> These are 0 based registers so it is 28 + 1
>> The code shows:  .n_voltages     = LM3632_LDO_VSEL_MAX + 1
>> so LM3632_LDO_VSEL_MAX needs to be 0x28.
>>
>>                  .name           = "ldo_vpos",
>>                  .of_match       = "vpos",
>>                  .id             = LM3632_LDO_POS,
>>                  .ops            = &lm363x_regulator_voltage_table_ops,
>>                  .n_voltages     = LM3632_LDO_VSEL_MAX + 1,
> Hi Dan,
> I'm wondering if you read my previous reply.

Yes I just got to it I was buried with other work.  Thanks for the bump 
on the list.

I will have to try this on my board.

FYI this is not really my code Milo K was the original author.

I just added another entry to the driver.  But since Milo is MIA I will

give it a look once I finish up my LED work next week

Dan

<snip>

> Regards,
> Axel

Applied "regulator: lm363x: Fix off-by-one n_voltages for lm3632 ldo_vpos/ldo_vneg" to the regulator tree

[Mark Brown]

The patch

   regulator: lm363x: Fix off-by-one n_voltages for lm3632 ldo_vpos/ldo_vneg

has been applied to the regulator tree at

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

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

From 1e2cc8c5e0745b545d4974788dc606d678b6e564 Mon Sep 17 00:00:00 2001
From: Axel Lin <axel.lin@ingics.com>
Date: Wed, 26 Jun 2019 21:26:31 +0800
Subject: [PATCH] regulator: lm363x: Fix off-by-one n_voltages for lm3632
 ldo_vpos/ldo_vneg

According to the datasheet https://www.ti.com/lit/ds/symlink/lm3632a.pdf
Table 20. VPOS Bias Register Field Descriptions VPOS[5:0]
Sets the Positive Display Bias (LDO) Voltage (50 mV per step)
000000: 4 V
000001: 4.05 V
000010: 4.1 V
....................
011101: 5.45 V
011110: 5.5 V (Default)
011111: 5.55 V
....................
100111: 5.95 V
101000: 6 V
Note: Codes 101001 to 111111 map to 6 V

The LM3632_LDO_VSEL_MAX should be 0b101000 (0x28), so the maximum voltage
can match the datasheet.

Fixes: 3a8d1a73a037 ("regulator: add LM363X driver")
Signed-off-by: Axel Lin <axel.lin@ingics.com>
Link: https://lore.kernel.org/r/20190626132632.32629-1-axel.lin@ingics.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/lm363x-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/lm363x-regulator.c b/drivers/regulator/lm363x-regulator.c
index 5647e2f97ff8..e4a27d63bf90 100644
--- a/drivers/regulator/lm363x-regulator.c
+++ b/drivers/regulator/lm363x-regulator.c
@@ -30,7 +30,7 @@
 
 /* LM3632 */
 #define LM3632_BOOST_VSEL_MAX		0x26
-#define LM3632_LDO_VSEL_MAX		0x29
+#define LM3632_LDO_VSEL_MAX		0x28
 #define LM3632_VBOOST_MIN		4500000
 #define LM3632_VLDO_MIN			4000000
 
-- 
2.20.1