[PATCH 1/2] twl6030: regulator: Fix vsel calculations in set/get voltage apis

[PATCH 1/2] twl6030: regulator: Fix vsel calculations in set/get voltage apis

[Rajendra Nayak]

TWL6030 has a formula to be used to calculate the vsel values
to be programmed in the VREG_VOLTAGE registers.

Voltage(in mV) = 1000mv + 100mv * (vsel - 1)

Ex: if vsel = 0x9, mV = 1000 + 100 * (9 -1) = 1800mV.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
Cc: Liam Girdwood <lrg@slimlogic.co.uk>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/regulator/twl-regulator.c |   23 +++++++++++++++++++----
 1 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index 7e67485..e7871d6 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -367,11 +367,17 @@ twlldo_set_voltage(struct regulator_dev *rdev, int min_uV, int max_uV)
 		/* REVISIT for VAUX2, first match may not be best/lowest */
 
 		/* use the first in-range value */
-		if (min_uV <= uV && uV <= max_uV)
+		if (min_uV <= uV && uV <= max_uV) {
+			if (twl_class_is_6030())
+				/*
+				 * Use the below formula to calculate vsel
+				 * mV = 1000mv + 100mv * (vsel - 1)
+				 */
+				vsel = (LDO_MV(mV) - 1000)/100 + 1;
 			return twlreg_write(info, TWL_MODULE_PM_RECEIVER,
 							VREG_VOLTAGE, vsel);
+		}
 	}
-
 	return -EDOM;
 }
 
@@ -384,8 +390,17 @@ static int twlldo_get_voltage(struct regulator_dev *rdev)
 	if (vsel < 0)
 		return vsel;
 
-	vsel &= info->table_len - 1;
-	return LDO_MV(info->table[vsel]) * 1000;
+	if (twl_class_is_4030()) {
+		vsel &= info->table_len - 1;
+		return LDO_MV(info->table[vsel]) * 1000;
+	} else if (twl_class_is_6030()) {
+		/*
+		 * Use the below formula to calculate vsel
+		 * mV = 1000mv + 100mv * (vsel - 1)
+		 */
+		return (1000 + (100 * (vsel - 1))) * 1000;
+	}
+	return -EDOM;
 }
 
 static struct regulator_ops twlldo_ops = {
-- 
1.5.4.7

[PATCH 2/2] twl6030: regulator: Configure STATE register instead of REMAP

[Rajendra Nayak]

This is no REMAP register on twl6030, instead there is a STATE register
to drive a resource to a given state.
The state register can be used to specify what state the resource should
enter when its associated with a GRP.
Register Bit field description is as below. The patch programmes the
corresponding STATE registers for all LDO's to turn ON when assocaited
with GRP_P1.

STATE REG:
Bit7   |Bit6   |Bit5   |Bit4  |Bit3  |Bit2  |Bit1   |Bit0
P3_GRP |P2_GRP |P1_GRP |RES   |RES   |RES   |State1 |State0

State can be specified as below
00: OFF
01: ON
10: OFF
11: SLEEP

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
Cc: Liam Girdwood <lrg@slimlogic.co.uk>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/regulator/twl-regulator.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index e7871d6..f4ccccc 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -534,16 +534,16 @@ static struct twlreg_info twl_regs[] = {
 	/* 6030 REG with base as PMC Slave Misc : 0x0030 */
 	/* Turnon-delay and remap configuration values for 6030 are not
 	   verified since the specification is not public */
-	TWL6030_ADJUSTABLE_LDO(VAUX1_6030, 0x54, 1, 0, 0x08),
-	TWL6030_ADJUSTABLE_LDO(VAUX2_6030, 0x58, 2, 0, 0x08),
-	TWL6030_ADJUSTABLE_LDO(VAUX3_6030, 0x5c, 3, 0, 0x08),
-	TWL6030_ADJUSTABLE_LDO(VMMC, 0x68, 4, 0, 0x08),
-	TWL6030_ADJUSTABLE_LDO(VPP, 0x6c, 5, 0, 0x08),
-	TWL6030_ADJUSTABLE_LDO(VUSIM, 0x74, 7, 0, 0x08),
-	TWL6030_FIXED_LDO(VANA, 0x50, 2100, 15, 0, 0x08),
-	TWL6030_FIXED_LDO(VCXIO, 0x60, 1800, 16, 0, 0x08),
-	TWL6030_FIXED_LDO(VDAC, 0x64, 1800, 17, 0, 0x08),
-	TWL6030_FIXED_LDO(VUSB, 0x70, 3300, 18, 0, 0x08)
+	TWL6030_ADJUSTABLE_LDO(VAUX1_6030, 0x54, 1, 0, 0x21),
+	TWL6030_ADJUSTABLE_LDO(VAUX2_6030, 0x58, 2, 0, 0x21),
+	TWL6030_ADJUSTABLE_LDO(VAUX3_6030, 0x5c, 3, 0, 0x21),
+	TWL6030_ADJUSTABLE_LDO(VMMC, 0x68, 4, 0, 0x21),
+	TWL6030_ADJUSTABLE_LDO(VPP, 0x6c, 5, 0, 0x21),
+	TWL6030_ADJUSTABLE_LDO(VUSIM, 0x74, 7, 0, 0x21),
+	TWL6030_FIXED_LDO(VANA, 0x50, 2100, 15, 0, 0x21),
+	TWL6030_FIXED_LDO(VCXIO, 0x60, 1800, 16, 0, 0x21),
+	TWL6030_FIXED_LDO(VDAC, 0x64, 1800, 17, 0, 0x21),
+	TWL6030_FIXED_LDO(VUSB, 0x70, 3300, 18, 0, 0x21)
 };
 
 static int twlreg_probe(struct platform_device *pdev)
-- 
1.5.4.7

Re: [PATCH 2/2] twl6030: regulator: Configure STATE register instead of REMAP

[Mark Brown]

On Wed, Feb 17, 2010 at 08:54:15PM +0530, Rajendra Nayak wrote:
> This is no REMAP register on twl6030, instead there is a STATE register
> to drive a resource to a given state.
> The state register can be used to specify what state the resource should
> enter when its associated with a GRP.
> Register Bit field description is as below. The patch programmes the
> corresponding STATE registers for all LDO's to turn ON when assocaited
> with GRP_P1.

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Re: [PATCH 2/2] twl6030: regulator: Configure STATE register instead of REMAP

[Liam Girdwood]

On Wed, 2010-02-17 at 20:54 +0530, Rajendra Nayak wrote:
> This is no REMAP register on twl6030, instead there is a STATE register
> to drive a resource to a given state.
> The state register can be used to specify what state the resource should
> enter when its associated with a GRP.
> Register Bit field description is as below. The patch programmes the
> corresponding STATE registers for all LDO's to turn ON when assocaited
> with GRP_P1.
> 
> STATE REG:
> Bit7   |Bit6   |Bit5   |Bit4  |Bit3  |Bit2  |Bit1   |Bit0
> P3_GRP |P2_GRP |P1_GRP |RES   |RES   |RES   |State1 |State0
> 
> State can be specified as below
> 00: OFF
> 01: ON
> 10: OFF
> 11: SLEEP
> 
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> Cc: Liam Girdwood <lrg@slimlogic.co.uk>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---

Applied.

Thanks

Liam

-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH 1/2] twl6030: regulator: Fix vsel calculations in set/get voltage apis

[Mark Brown]

On Wed, Feb 17, 2010 at 08:54:14PM +0530, Rajendra Nayak wrote:

>  		/* use the first in-range value */
> -		if (min_uV <= uV && uV <= max_uV)
> +		if (min_uV <= uV && uV <= max_uV) {
> +			if (twl_class_is_6030())
> +				/*
> +				 * Use the below formula to calculate vsel
> +				 * mV = 1000mv + 100mv * (vsel - 1)
> +				 */
> +				vsel = (LDO_MV(mV) - 1000)/100 + 1;
>  			return twlreg_write(info, TWL_MODULE_PM_RECEIVER,
>  							VREG_VOLTAGE, vsel);

This looks wrong - this code is inside a loop over vsel->voltage
mappings, if that mapping is wrong and needs to be ignored (which is
what this code is doing) then we shouldn't be looking at it at all.

RE: [PATCH 1/2] twl6030: regulator: Fix vsel calculations in set/get voltage apis

[Nayak, Rajendra]

 

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] 
> Sent: Wednesday, February 17, 2010 9:45 PM
> To: Nayak, Rajendra
> Cc: linux-omap@vger.kernel.org; Liam Girdwood; Samuel Ortiz
> Subject: Re: [PATCH 1/2] twl6030: regulator: Fix vsel 
> calculations in set/get voltage apis
> 
> On Wed, Feb 17, 2010 at 08:54:14PM +0530, Rajendra Nayak wrote:
> 
> >  		/* use the first in-range value */
> > -		if (min_uV <= uV && uV <= max_uV)
> > +		if (min_uV <= uV && uV <= max_uV) {
> > +			if (twl_class_is_6030())
> > +				/*
> > +				 * Use the below formula to 
> calculate vsel
> > +				 * mV = 1000mv + 100mv * (vsel - 1)
> > +				 */
> > +				vsel = (LDO_MV(mV) - 1000)/100 + 1;
> >  			return twlreg_write(info, 
> TWL_MODULE_PM_RECEIVER,
> >  							
> VREG_VOLTAGE, vsel);
> 
> This looks wrong - this code is inside a loop over vsel->voltage
> mappings, if that mapping is wrong and needs to be ignored (which is
> what this code is doing) then we shouldn't be looking at it at all.

Mark,

Not sure if I understood your comment completely, but here's how I think
it will work.

The for loop runs through all the valid voltages supported by the regulator
and and when it finds the first in-range value programs the VOLTAGE register.
For twl4030 the VOLTAGE register can be programmed with just the index
of the table since the mapping was something like
1800mV = 0x0
2800mV = 0x1
2900mV = 0x2
3000mV = 0x3

Now for twl6030 I just have added an additional vsel calulation part after
the in-range value is found, since using the table index directly as vsel
would not work.

regards,
Rajendra

> 

Re: [PATCH 1/2] twl6030: regulator: Fix vsel calculations in set/get voltage apis

[Mark Brown]

On Thu, Feb 18, 2010 at 12:02:49PM +0530, Nayak, Rajendra wrote:

> > This looks wrong - this code is inside a loop over vsel->voltage
> > mappings, if that mapping is wrong and needs to be ignored (which is
> > what this code is doing) then we shouldn't be looking at it at all.

> Not sure if I understood your comment completely, but here's how I think
> it will work.

> The for loop runs through all the valid voltages supported by the regulator
> and and when it finds the first in-range value programs the VOLTAGE register.
> For twl4030 the VOLTAGE register can be programmed with just the index
> of the table since the mapping was something like

> 1800mV = 0x0
> 2800mV = 0x1
> 2900mV = 0x2
> 3000mV = 0x3

> Now for twl6030 I just have added an additional vsel calulation part after
> the in-range value is found, since using the table index directly as vsel
> would not work.

Right, but this means that the code is looking at one table of values to
find a voltage it likes and then picking the actual selector from
another set of values.  This means that depending on the actual
differences it may either pick a value which isn't valid for the TWL6030
or omit values which the TWL6030 can actually support.  Either way from
a code inspection point of view the code doesn't look good due to the
mismatch between the two sets of selectors.

The TWL6030 case should be doing something more like other regulator
drivers with straightforward mappings between selector and voltage (most
of them) and not using the TWL4030 value table.

RE: [PATCH 1/2] twl6030: regulator: Fix vsel calculations in set/get voltage apis

[Nayak, Rajendra]

 

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] 
> Sent: Thursday, February 18, 2010 3:18 PM
> To: Nayak, Rajendra
> Cc: linux-omap@vger.kernel.org; Liam Girdwood; Samuel Ortiz
> Subject: Re: [PATCH 1/2] twl6030: regulator: Fix vsel 
> calculations in set/get voltage apis
> 
> On Thu, Feb 18, 2010 at 12:02:49PM +0530, Nayak, Rajendra wrote:
> 
> > > This looks wrong - this code is inside a loop over vsel->voltage
> > > mappings, if that mapping is wrong and needs to be 
> ignored (which is
> > > what this code is doing) then we shouldn't be looking at 
> it at all.
> 
> > Not sure if I understood your comment completely, but 
> here's how I think
> > it will work.
> 
> > The for loop runs through all the valid voltages supported 
> by the regulator
> > and and when it finds the first in-range value programs the 
> VOLTAGE register.
> > For twl4030 the VOLTAGE register can be programmed with 
> just the index
> > of the table since the mapping was something like
> 
> > 1800mV = 0x0
> > 2800mV = 0x1
> > 2900mV = 0x2
> > 3000mV = 0x3
> 
> > Now for twl6030 I just have added an additional vsel 
> calulation part after
> > the in-range value is found, since using the table index 
> directly as vsel
> > would not work.
> 
> Right, but this means that the code is looking at one table 
> of values to
> find a voltage it likes and then picking the actual selector from
> another set of values.  This means that depending on the actual
> differences it may either pick a value which isn't valid for 
> the TWL6030
> or omit values which the TWL6030 can actually support.  
> Either way from
> a code inspection point of view the code doesn't look good due to the
> mismatch between the two sets of selectors.
> 
> The TWL6030 case should be doing something more like other regulator
> drivers with straightforward mappings between selector and 
> voltage (most
> of them) and not using the TWL4030 value table.

The code is'nt looking into twl4030 tables for finding the valid
voltage for twl6030. There are seperate tables for twl4030 and twl6030
regulators with all the valid supported voltages.

The difference is, in case of twl4030 once you find a valid voltage
from the right table you could just use the table index to program
the register on twl4030.
In case of twl6030, once you find a valid voltage, again from the right
table (meant for twl6030) you still need to derive
what value needs to be programmed in the register, so twl6030 understands
it. Using the table index does not help.

Example:
In case of TWL4030
static const u16 VAUX1_VSEL_table[] = {
        2500, 2800, 3000, 3000, 3000, 3000,
};

If the request is to set 2800mv, look up the table,
once found program the VOLTAGE register with the table
index. In this case 0x1.

So the mapping is
VOLTAGE_REG=0x0, VAUX1=2500mv
VOLTAGE_REG=0x1, VAUX1=2800mv
VOLTAGE_REG=0x2, VAUX1=3000mv

In case of TWL6030
static const u16 VAUX1_6030_VSEL_table[] = {
        1000, 1300, 1800, 2500,
        2800, 2900, 3000, 3000,
};

if the request is to set 2800mv, look up the table,
once found calulate the value to be programmed
value = (2800 - 1000)/100 + 1 = 0x13

So here the mapping is
VOLTAGE_REG=0x10, VAUX1=2500mv
VOLTAGE_REG=0x13, VAUX1=2800mv
VOLTAGE_REG=0x15, VAUX1=3000mv

Without this patch the code goes ahead and programs
the table index (In this case 0x4) which is wrong 

> 

Re: [PATCH 1/2] twl6030: regulator: Fix vsel calculations in set/get voltage apis

[Mark Brown]

On Thu, Feb 18, 2010 at 03:49:29PM +0530, Nayak, Rajendra wrote:

> > The TWL6030 case should be doing something more like other regulator
> > drivers with straightforward mappings between selector and 
> > voltage (most
> > of them) and not using the TWL4030 value table.

> The code is'nt looking into twl4030 tables for finding the valid
> voltage for twl6030. There are seperate tables for twl4030 and twl6030
> regulators with all the valid supported voltages.

> The difference is, in case of twl4030 once you find a valid voltage
> from the right table you could just use the table index to program
> the register on twl4030.
> In case of twl6030, once you find a valid voltage, again from the right
> table (meant for twl6030) you still need to derive
> what value needs to be programmed in the register, so twl6030 understands
> it. Using the table index does not help.

In that case the tables for TWL6030 are not ideal as-is and should
either be fixed somehow, have a different function accessing them which
makes their meaning clear or removed and replaced with just the formula.

I know what the code is trying to do, it just seems to be trying to do
it at the wrong abstraction level and so looks buggy on code inspection.
My point here is that the code looks wrong since it's iterating over a
table giving voltage to selector mappings but then ignoring the selector
value it comes up with and instead using a formula which at least gives
way more options than most of the selector tables do.  It's readability
that's my focus here, the code possibly does do exactly what it ought to
but it looks wrong.

> In case of TWL6030
> static const u16 VAUX1_6030_VSEL_table[] = {
>         1000, 1300, 1800, 2500,
>         2800, 2900, 3000, 3000,
> };

> if the request is to set 2800mv, look up the table,
> once found calulate the value to be programmed
> value = (2800 - 1000)/100 + 1 = 0x13

Are these really the only supported values (obviously you can set other
selectors)?  Note also that the name says "_VSEL_table" which is
definitely no longer true, this isn't helping clarity at all.

RE: [PATCH 1/2] twl6030: regulator: Fix vsel calculations in set/get voltage apis

[Nayak, Rajendra]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] 
> Sent: Thursday, February 18, 2010 4:11 PM
> To: Nayak, Rajendra
> Cc: linux-omap@vger.kernel.org; Liam Girdwood; Samuel Ortiz
> Subject: Re: [PATCH 1/2] twl6030: regulator: Fix vsel 
> calculations in set/get voltage apis
> 
> On Thu, Feb 18, 2010 at 03:49:29PM +0530, Nayak, Rajendra wrote:
> 
> > > The TWL6030 case should be doing something more like 
> other regulator
> > > drivers with straightforward mappings between selector and 
> > > voltage (most
> > > of them) and not using the TWL4030 value table.
> 
> > The code is'nt looking into twl4030 tables for finding the valid
> > voltage for twl6030. There are seperate tables for twl4030 
> and twl6030
> > regulators with all the valid supported voltages.
> 
> > The difference is, in case of twl4030 once you find a valid voltage
> > from the right table you could just use the table index to program
> > the register on twl4030.
> > In case of twl6030, once you find a valid voltage, again 
> from the right
> > table (meant for twl6030) you still need to derive
> > what value needs to be programmed in the register, so 
> twl6030 understands
> > it. Using the table index does not help.
> 
> In that case the tables for TWL6030 are not ideal as-is and should
> either be fixed somehow, have a different function accessing 
> them which
> makes their meaning clear or removed and replaced with just 
> the formula.
> 
> I know what the code is trying to do, it just seems to be trying to do
> it at the wrong abstraction level and so looks buggy on code 
> inspection.
> My point here is that the code looks wrong since it's iterating over a
> table giving voltage to selector mappings but then ignoring 
> the selector
> value it comes up with and instead using a formula which at 
> least gives
> way more options than most of the selector tables do.

Mark, I get your point. The table values were derived from what the spec
suggests, but I agree that its confusing that in theory the formula
suggests there could be more selectors than whats present in the table.

I will check if its indeed possible to support all possible selectors
between say a min/max range. And then have maybe seperate kind of tables
for twl6030 which only specify min/max range and then use the formula to
generate the right selector.

>  It's 
> readability
> that's my focus here, the code possibly does do exactly what 
> it ought to
> but it looks wrong.
> 
> > In case of TWL6030
> > static const u16 VAUX1_6030_VSEL_table[] = {
> >         1000, 1300, 1800, 2500,
> >         2800, 2900, 3000, 3000,
> > };
> 
> > if the request is to set 2800mv, look up the table,
> > once found calulate the value to be programmed
> > value = (2800 - 1000)/100 + 1 = 0x13
> 
> Are these really the only supported values (obviously you can 
> set other
> selectors)?  Note also that the name says "_VSEL_table" which is
> definitely no longer true, this isn't helping clarity at all.
> 

Re: [PATCH 1/2] twl6030: regulator: Fix vsel calculations in set/get voltage apis

[Mark Brown]

On Thu, Feb 18, 2010 at 05:09:53PM +0530, Nayak, Rajendra wrote:

> Mark, I get your point. The table values were derived from what the spec
> suggests, but I agree that its confusing that in theory the formula
> suggests there could be more selectors than whats present in the table.

> I will check if its indeed possible to support all possible selectors
> between say a min/max range. And then have maybe seperate kind of tables
> for twl6030 which only specify min/max range and then use the formula to
> generate the right selector.

Another option if you do need the supported voltage list would be to
rename the table and have a separate function to do set on twl6030 which
makes the intended usage clear.

RE: [PATCH 1/2] twl6030: regulator: Fix vsel calculations in set/get voltage apis

[Nayak, Rajendra]

 

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] 
> Sent: Thursday, February 18, 2010 5:13 PM
> To: Nayak, Rajendra
> Cc: linux-omap@vger.kernel.org; Liam Girdwood; Samuel Ortiz
> Subject: Re: [PATCH 1/2] twl6030: regulator: Fix vsel 
> calculations in set/get voltage apis
> 
> On Thu, Feb 18, 2010 at 05:09:53PM +0530, Nayak, Rajendra wrote:
> 
> > Mark, I get your point. The table values were derived from 
> what the spec
> > suggests, but I agree that its confusing that in theory the formula
> > suggests there could be more selectors than whats present 
> in the table.
> 
> > I will check if its indeed possible to support all possible 
> selectors
> > between say a min/max range. And then have maybe seperate 
> kind of tables
> > for twl6030 which only specify min/max range and then use 
> the formula to
> > generate the right selector.
> 
> Another option if you do need the supported voltage list would be to
> rename the table and have a separate function to do set on 
> twl6030 which
> makes the intended usage clear.

Mark, It turns out, all twl6030 regulators can be programmed from
1.0v to 3.3v with 100mV steps. The spec I was referring to was specfic
to the OMAP4 platform I am working on. Hence, it does not prevent
someone using the PMIC on a different platform to have different
voltage requirements.
So I was thinking of removing all the twl6030 tables and just use
the formula to get the selector and make sure they fall in the
allowed range (1.0v to 3.3v).
If that sounds good then I can post a patch for this.

> 

Re: [PATCH 1/2] twl6030: regulator: Fix vsel calculations in set/get voltage apis

[Mark Brown]

On Fri, Feb 19, 2010 at 12:09:57PM +0530, Nayak, Rajendra wrote:

> So I was thinking of removing all the twl6030 tables and just use
> the formula to get the selector and make sure they fall in the
> allowed range (1.0v to 3.3v).
> If that sounds good then I can post a patch for this.

Yes, that sounds good to me.