[PATCH RFT 1/2] regulator: twl: Fix the formula to calculate vsel and voltage for twl6030ldo

[PATCH RFT 1/2] regulator: twl: Fix the formula to calculate vsel and voltage for twl6030ldo

[Axel Lin]

In twl6030ldo_set_voltage, current code use below formula to calculate vsel:
        vsel = (min_uV/1000 - 1000)/100 + 1;
This is worng because when min_uV is 1000000 uV, vsel is 1.
It should be 0 in this case.
Fix it by change the equation to: (This equation is common for linear mapping)
        vsel = DIV_ROUND_UP(min_uV - rdev->desc->min_uV, rdev->desc->uV_step);

In twl6030ldo_get_voltage, current code use below formula to calculate voltage:
        mV = 1000mv + 100mv * (vsel - 1)
This is worng because when vsel is 0, mV is 900mV. Note the min_uV is 1000mV.
Fix it by change the equation to: (This equation is common for linear mapping)
        return rdev->desc->min_uV + vsel * rdev->desc->uV_step;

Signed-off-by: Axel Lin <axel.lin@gmail.com>
---
 drivers/regulator/twl-regulator.c |   17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index 8f0bd56..bb51dec 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -569,30 +569,21 @@ twl6030ldo_set_voltage(struct regulator_dev *rdev, int min_uV, int max_uV,
 	if ((min_uV/1000 < info->min_mV) || (max_uV/1000 > info->max_mV))
 		return -EDOM;
 
-	/*
-	 * Use the below formula to calculate vsel
-	 * mV = 1000mv + 100mv * (vsel - 1)
-	 */
-	vsel = (min_uV/1000 - 1000)/100 + 1;
+	vsel = DIV_ROUND_UP(min_uV - rdev->desc->min_uV, rdev->desc->uV_step);
 	*selector = vsel;
-	return twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_VOLTAGE, vsel);
 
+	return twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_VOLTAGE, vsel);
 }
 
 static int twl6030ldo_get_voltage(struct regulator_dev *rdev)
 {
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
-	int		vsel = twlreg_read(info, TWL_MODULE_PM_RECEIVER,
-								VREG_VOLTAGE);
+	int vsel = twlreg_read(info, TWL_MODULE_PM_RECEIVER, VREG_VOLTAGE);
 
 	if (vsel < 0)
 		return vsel;
 
-	/*
-	 * Use the below formula to calculate vsel
-	 * mV = 1000mv + 100mv * (vsel - 1)
-	 */
-	return (1000 + (100 * (vsel - 1))) * 1000;
+	return rdev->desc->min_uV + vsel * rdev->desc->uV_step;
 }
 
 static struct regulator_ops twl6030ldo_ops = {
-- 
1.7.9.5

[RESEND][PATCH RFT 2/2] regulator: twl: Convert twl6030ldo_ops to [get|set]_voltage_sel

[Axel Lin]

Signed-off-by: Axel Lin <axel.lin@gmail.com>
---
Sorry, I CCed wrong guys. So here is the resend.
Axel
 drivers/regulator/twl-regulator.c |   24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index bb51dec..de99b78 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -560,37 +560,27 @@ static struct regulator_ops twl6030coresmps_ops = {
 };
 
 static int
-twl6030ldo_set_voltage(struct regulator_dev *rdev, int min_uV, int max_uV,
-		       unsigned *selector)
+twl6030ldo_set_voltage_sel(struct regulator_dev *rdev, unsigned selector)
 {
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
-	int			vsel;
 
-	if ((min_uV/1000 < info->min_mV) || (max_uV/1000 > info->max_mV))
-		return -EDOM;
-
-	vsel = DIV_ROUND_UP(min_uV - rdev->desc->min_uV, rdev->desc->uV_step);
-	*selector = vsel;
-
-	return twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_VOLTAGE, vsel);
+	return twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_VOLTAGE,
+			    selector);
 }
 
-static int twl6030ldo_get_voltage(struct regulator_dev *rdev)
+static int twl6030ldo_get_voltage_sel(struct regulator_dev *rdev)
 {
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
 	int vsel = twlreg_read(info, TWL_MODULE_PM_RECEIVER, VREG_VOLTAGE);
 
-	if (vsel < 0)
-		return vsel;
-
-	return rdev->desc->min_uV + vsel * rdev->desc->uV_step;
+	return vsel;
 }
 
 static struct regulator_ops twl6030ldo_ops = {
 	.list_voltage	= regulator_list_voltage_linear,
 
-	.set_voltage	= twl6030ldo_set_voltage,
-	.get_voltage	= twl6030ldo_get_voltage,
+	.set_voltage_sel = twl6030ldo_set_voltage_sel,
+	.get_voltage_sel = twl6030ldo_get_voltage_sel,
 
 	.enable		= twl6030reg_enable,
 	.disable	= twl6030reg_disable,
-- 
1.7.9.5

Re: [PATCH RFT 1/2] regulator: twl: Fix the formula to calculate vsel and voltage for twl6030ldo

[Axel Lin]

於 一，2012-07-09 於 11:22 +0800，Axel Lin 提到：
> In twl6030ldo_set_voltage, current code use below formula to calculate vsel:
>         vsel = (min_uV/1000 - 1000)/100 + 1;
> This is worng because when min_uV is 1000000 uV, vsel is 1.
> It should be 0 in this case.
> Fix it by change the equation to: (This equation is common for linear mapping)
>         vsel = DIV_ROUND_UP(min_uV - rdev->desc->min_uV, rdev->desc->uV_step);
> 
> In twl6030ldo_get_voltage, current code use below formula to calculate voltage:
>         mV = 1000mv + 100mv * (vsel - 1)
> This is worng because when vsel is 0, mV is 900mV. Note the min_uV is 1000mV.
> Fix it by change the equation to: (This equation is common for linear mapping)
>         return rdev->desc->min_uV + vsel * rdev->desc->uV_step;

While I'm thinking I need to rework this patch so that it doesn't use
rdev->desc->min_uV and rdev->desc->uV_step and then can be applied
to current Linus' tree.

But while I am tracking back to commit 3e3d3be79c
Author: Rajendra Nayak <rnayak@ti.com>
Date:   Thu Apr 22 14:18:32 2010 +0530

    twl6030: regulator: Remove vsel tables and use formula for
calculation
    
    All twl6030 regulators can be programmed from 1.0v to 3.3v
    with 100mV steps.
    The below formula can 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.
    
    This patch removes all existing VSEL tables for twl6030 adjustable
    regulators and just uses the formula directly for vsel calculations
    after verifing they fall in the allowed range.
    
    Signed-off-by: Rajendra Nayak <rnayak@ti.com>

I found a problem that before commit 3e3d3be79c, the voltage tables were
not linear mapping. So why we can convert these voltage mapping table to
Voltage(in mV) = 1000mv + 100mv * (vsel - 1)?

Did I miss something?

Regards,
Axel

Re: [PATCH RFT 1/2] regulator: twl: Fix the formula to calculate vsel and voltage for twl6030ldo

[Mark Brown]

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

On Mon, Jul 09, 2012 at 07:01:26PM +0800, Axel Lin wrote:

> I found a problem that before commit 3e3d3be79c, the voltage tables were
> not linear mapping. So why we can convert these voltage mapping table to
> Voltage(in mV) = 1000mv + 100mv * (vsel - 1)?

Guys, it's pretty concerning that there's been no response at all to
this.  Looking at the changes Axel's statement above is accurate for at
least VAUX1 and VMMC, I didn't check all the regulators, so either the
original code was buggy or there's an issue that needs fixing in the
current code.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RFT 1/2] regulator: twl: Fix the formula to calculate vsel and voltage for twl6030ldo

[Mark Brown]

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

On Mon, Jul 09, 2012 at 11:22:31AM +0800, Axel Lin wrote:
> In twl6030ldo_set_voltage, current code use below formula to calculate vsel:
>         vsel = (min_uV/1000 - 1000)/100 + 1;
> This is worng because when min_uV is 1000000 uV, vsel is 1.

Applied both, thanks - even if we need to fix some of these to be using
tables again this set of changes at least maintains the status quo.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RFT 1/2] regulator: twl: Fix the formula to calculate vsel and voltage for twl6030ldo

[Rajendra Nayak]

Hi Axel,

My apologies for the delay in responding to this one.

On Monday 09 July 2012 04:31 PM, Axel Lin wrote:
> 於 一，2012-07-09 於 11:22 +0800，Axel Lin 提到：
>> In twl6030ldo_set_voltage, current code use below formula to calculate vsel:
>>          vsel = (min_uV/1000 - 1000)/100 + 1;
>> This is worng because when min_uV is 1000000 uV, vsel is 1.
>> It should be 0 in this case.

Why? Do you know of any documentation which states this?
I am referring to the twl6030 Top Functional spec version 1.18
and Table - 223 "LDO Output Voltage Selection Code" clearly
states that the vsel should be 1 when voltage expected is 1000000 uV.

Also refer to equation 3 "LDO Output Voltage Selection Code" in the
same document and you will see this mentioned..
Absolute Voltage value = 1.0V + 0.1V * (binary value – 00000001)

>> Fix it by change the equation to: (This equation is common for linear mapping)
>>          vsel = DIV_ROUND_UP(min_uV - rdev->desc->min_uV, rdev->desc->uV_step);
>>
>> In twl6030ldo_get_voltage, current code use below formula to calculate voltage:
>>          mV = 1000mv + 100mv * (vsel - 1)
>> This is worng because when vsel is 0, mV is 900mV. Note the min_uV is 1000mV.
>> Fix it by change the equation to: (This equation is common for linear mapping)
>>          return rdev->desc->min_uV + vsel * rdev->desc->uV_step;
>
> While I'm thinking I need to rework this patch so that it doesn't use
> rdev->desc->min_uV and rdev->desc->uV_step and then can be applied
> to current Linus' tree.
>
> But while I am tracking back to commit 3e3d3be79c
> Author: Rajendra Nayak<rnayak@ti.com>
> Date:   Thu Apr 22 14:18:32 2010 +0530
>
>      twl6030: regulator: Remove vsel tables and use formula for
> calculation
>
>      All twl6030 regulators can be programmed from 1.0v to 3.3v
>      with 100mV steps.
>      The below formula can 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.
>
>      This patch removes all existing VSEL tables for twl6030 adjustable
>      regulators and just uses the formula directly for vsel calculations
>      after verifing they fall in the allowed range.
>
>      Signed-off-by: Rajendra Nayak<rnayak@ti.com>
>
> I found a problem that before commit 3e3d3be79c, the voltage tables were
> not linear mapping. So why we can convert these voltage mapping table to
> Voltage(in mV) = 1000mv + 100mv * (vsel - 1)?
>
> Did I miss something?

All voltage tables before commit '3e3d3be79c' for twl6030 regulators
were clearly wrong. They assumed similarity with twl4030 regulators
which was not right.

regards,
Rajendra

>
> Regards,
> Axel
>
>

Re: [PATCH RFT 1/2] regulator: twl: Fix the formula to calculate vsel and voltage for twl6030ldo

[Axel Lin]

> On Monday 09 July 2012 04:31 PM, Axel Lin wrote:
> > 於 一，2012-07-09 於 11:22 +0800，Axel Lin 提到：
> >> In twl6030ldo_set_voltage, current code use below formula to calculate vsel:
> >>          vsel = (min_uV/1000 - 1000)/100 + 1;
> >> This is worng because when min_uV is 1000000 uV, vsel is 1.
> >> It should be 0 in this case.
> 
> Why? Do you know of any documentation which states this?

I double check with the datasheet again now.
You are right in this. So we cannot use linear mapping for
twl6030ldo_ops here.
I'm going to send a patch to fix it, I'd appreciate if someone can
review and test it.


> > I found a problem that before commit 3e3d3be79c, the voltage tables were
> > not linear mapping. So why we can convert these voltage mapping table to
> > Voltage(in mV) = 1000mv + 100mv * (vsel - 1)?
> >
> > Did I miss something?
> 
> All voltage tables before commit '3e3d3be79c' for twl6030 regulators
> were clearly wrong. They assumed similarity with twl4030 regulators
> which was not right.

Good to know that. 
Thanks,
Axel
> 
> regards,
> Rajendra
> 
> >
> > Regards,
> > Axel
> >
> >
>