When setting voltage for AB8540_LDO_AUX3, current code only updates one of
info->voltage_reg and info->expand_register registers which is wrong.
To ensure we set to correct voltage, it always needs to clear or set
expand_register.voltage_mask bit of expand_register.
Signed-off-by: Axel Lin <[email protected]>
---
drivers/regulator/ab8500.c | 55 ++++++++++++++++++++++++++++----------------
1 file changed, 35 insertions(+), 20 deletions(-)
diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
index 9ebd131..222a63e 100644
--- a/drivers/regulator/ab8500.c
+++ b/drivers/regulator/ab8500.c
@@ -605,39 +605,54 @@ static int ab8540_aux3_regulator_set_voltage_sel(struct regulator_dev *rdev,
{
int ret;
struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
- u8 regval;
+ u8 regval, regval_expand;
if (info == NULL) {
dev_err(rdev_get_dev(rdev), "regulator info null pointer\n");
return -EINVAL;
}
- if (selector >= info->expand_register.voltage_limit) {
- /* Vaux3 bit4 has different layout */
- regval = (u8)selector << info->expand_register.voltage_shift;
- ret = abx500_mask_and_set_register_interruptible(info->dev,
- info->expand_register.voltage_bank,
- info->expand_register.voltage_reg,
- info->expand_register.voltage_mask,
- regval);
- } else {
- /* set the registers for the request */
- regval = (u8)selector << info->voltage_shift;
- ret = abx500_mask_and_set_register_interruptible(info->dev,
+ if (selector >= info->expand_register.voltage_limit)
+ regval_expand = info->expand_register.voltage_mask;
+ else
+ regval_expand = 0;
+
+ ret = abx500_mask_and_set_register_interruptible(info->dev,
+ info->expand_register.voltage_bank,
+ info->expand_register.voltage_reg,
+ info->expand_register.voltage_mask,
+ regval_expand);
+ if (ret < 0) {
+ dev_err(rdev_get_dev(rdev),
+ "couldn't set expand voltage reg for regulator\n");
+ return ret;
+ }
+
+ dev_vdbg(rdev_get_dev(rdev),
+ "%s-set_voltage expand (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n",
+ info->desc.name, info->expand_register.voltage_bank,
+ info->expand_register.voltage_reg,
+ info->expand_register.voltage_mask, regval_expand);
+
+ if (selector >= info->expand_register.voltage_limit)
+ return 0;
+
+ regval = (u8)selector << info->voltage_shift;
+ ret = abx500_mask_and_set_register_interruptible(info->dev,
info->voltage_bank, info->voltage_reg,
info->voltage_mask, regval);
- }
- if (ret < 0)
+ if (ret < 0) {
dev_err(rdev_get_dev(rdev),
"couldn't set voltage reg for regulator\n");
+ return ret;
+ }
dev_vdbg(rdev_get_dev(rdev),
- "%s-set_voltage (bank, reg, mask, value): 0x%x, 0x%x, 0x%x,"
- " 0x%x\n",
- info->desc.name, info->voltage_bank, info->voltage_reg,
- info->voltage_mask, regval);
+ "%s-set_voltage (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n",
+ info->desc.name, info->voltage_bank, info->voltage_reg,
+ info->voltage_mask, regval);
- return ret;
+ return 0;
}
static struct regulator_ops ab8500_regulator_volt_mode_ops = {
--
1.7.10.4
We can save a register read operation in some case if read
expand_register first.
If info->expand_register.voltage_mask bit is set, no need to read
voltage_reg.
Signed-off-by: Axel Lin <[email protected]>
---
drivers/regulator/ab8500.c | 42 ++++++++++++++++++------------------------
1 file changed, 18 insertions(+), 24 deletions(-)
diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
index 222a63e..7a86fe6 100644
--- a/drivers/regulator/ab8500.c
+++ b/drivers/regulator/ab8500.c
@@ -521,7 +521,7 @@ static int ab8500_regulator_get_voltage_sel(struct regulator_dev *rdev)
static int ab8540_aux3_regulator_get_voltage_sel(struct regulator_dev *rdev)
{
- int ret, val;
+ int ret;
struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
u8 regval, regval_expand;
@@ -531,18 +531,25 @@ static int ab8540_aux3_regulator_get_voltage_sel(struct regulator_dev *rdev)
}
ret = abx500_get_register_interruptible(info->dev,
- info->voltage_bank, info->voltage_reg, ®val);
-
+ info->expand_register.voltage_bank,
+ info->expand_register.voltage_reg, ®val_expand);
if (ret < 0) {
dev_err(rdev_get_dev(rdev),
- "couldn't read voltage reg for regulator\n");
+ "couldn't read voltage expand reg for regulator\n");
return ret;
}
- ret = abx500_get_register_interruptible(info->dev,
- info->expand_register.voltage_bank,
- info->expand_register.voltage_reg, ®val_expand);
+ dev_vdbg(rdev_get_dev(rdev),
+ "%s-get_voltage expand (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n",
+ info->desc.name, info->expand_register.voltage_bank,
+ info->expand_register.voltage_reg,
+ info->expand_register.voltage_mask, regval_expand);
+
+ if (regval_expand & info->expand_register.voltage_mask)
+ return info->expand_register.voltage_limit;
+ ret = abx500_get_register_interruptible(info->dev,
+ info->voltage_bank, info->voltage_reg, ®val);
if (ret < 0) {
dev_err(rdev_get_dev(rdev),
"couldn't read voltage reg for regulator\n");
@@ -550,24 +557,11 @@ static int ab8540_aux3_regulator_get_voltage_sel(struct regulator_dev *rdev)
}
dev_vdbg(rdev_get_dev(rdev),
- "%s-get_voltage (bank, reg, mask, value): 0x%x, 0x%x, 0x%x,"
- " 0x%x\n",
- info->desc.name, info->voltage_bank, info->voltage_reg,
- info->voltage_mask, regval);
- dev_vdbg(rdev_get_dev(rdev),
- "%s-get_voltage expand (bank, reg, mask, value): 0x%x, 0x%x, 0x%x,"
- " 0x%x\n",
- info->desc.name, info->expand_register.voltage_bank,
- info->expand_register.voltage_reg,
- info->expand_register.voltage_mask, regval_expand);
-
- if (regval_expand&(info->expand_register.voltage_mask))
- /* Vaux3 has a different layout */
- val = info->expand_register.voltage_limit;
- else
- val = (regval & info->voltage_mask) >> info->voltage_shift;
+ "%s-get_voltage (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n",
+ info->desc.name, info->voltage_bank, info->voltage_reg,
+ info->voltage_mask, regval);
- return val;
+ return (regval & info->voltage_mask) >> info->voltage_shift;
}
static int ab8500_regulator_set_voltage_sel(struct regulator_dev *rdev,
--
1.7.10.4
On 04/10/2013 08:40 AM, Axel Lin wrote:
> When setting voltage for AB8540_LDO_AUX3, current code only updates one of
> info->voltage_reg and info->expand_register registers which is wrong.
> To ensure we set to correct voltage, it always needs to clear or set
> expand_register.voltage_mask bit of expand_register.
The original code is wrong. It is not possible to leave 3.05 V once set.
The function of the expand register bit is the following (from the user
manual):
0: VAUX3 output voltage is determined by Vaux3Sel bit settings in
register VldoCVaux3Sel
1: VAUX3 output voltage is set to 3.05 V regardless of Vaux3Sel settings
in register VldoCVaux3Sel
(VldoCVaux3Sel is the register at 0x0421)
So when going to 3.05 V I would like to set the bit as you are doing.
But when leaving 3.05 V for another voltage I would prefer setting the
target voltage before clearing the expand register bit.
>
> Signed-off-by: Axel Lin <[email protected]>
> ---
> drivers/regulator/ab8500.c | 55 ++++++++++++++++++++++++++++----------------
> 1 file changed, 35 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
> index 9ebd131..222a63e 100644
> --- a/drivers/regulator/ab8500.c
> +++ b/drivers/regulator/ab8500.c
> @@ -605,39 +605,54 @@ static int ab8540_aux3_regulator_set_voltage_sel(struct regulator_dev *rdev,
> {
> int ret;
> struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
> - u8 regval;
> + u8 regval, regval_expand;
>
> if (info == NULL) {
> dev_err(rdev_get_dev(rdev), "regulator info null pointer\n");
> return -EINVAL;
> }
>
> - if (selector >= info->expand_register.voltage_limit) {
> - /* Vaux3 bit4 has different layout */
> - regval = (u8)selector << info->expand_register.voltage_shift;
> - ret = abx500_mask_and_set_register_interruptible(info->dev,
> - info->expand_register.voltage_bank,
> - info->expand_register.voltage_reg,
> - info->expand_register.voltage_mask,
> - regval);
> - } else {
> - /* set the registers for the request */
> - regval = (u8)selector << info->voltage_shift;
> - ret = abx500_mask_and_set_register_interruptible(info->dev,
> + if (selector >= info->expand_register.voltage_limit)
> + regval_expand = info->expand_register.voltage_mask;
> + else
> + regval_expand = 0;
> +
> + ret = abx500_mask_and_set_register_interruptible(info->dev,
> + info->expand_register.voltage_bank,
> + info->expand_register.voltage_reg,
> + info->expand_register.voltage_mask,
> + regval_expand);
> + if (ret < 0) {
> + dev_err(rdev_get_dev(rdev),
> + "couldn't set expand voltage reg for regulator\n");
> + return ret;
> + }
> +
> + dev_vdbg(rdev_get_dev(rdev),
> + "%s-set_voltage expand (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n",
> + info->desc.name, info->expand_register.voltage_bank,
> + info->expand_register.voltage_reg,
> + info->expand_register.voltage_mask, regval_expand);
> +
> + if (selector >= info->expand_register.voltage_limit)
> + return 0;
> +
> + regval = (u8)selector << info->voltage_shift;
> + ret = abx500_mask_and_set_register_interruptible(info->dev,
> info->voltage_bank, info->voltage_reg,
> info->voltage_mask, regval);
> - }
> - if (ret < 0)
> + if (ret < 0) {
> dev_err(rdev_get_dev(rdev),
> "couldn't set voltage reg for regulator\n");
> + return ret;
> + }
>
> dev_vdbg(rdev_get_dev(rdev),
> - "%s-set_voltage (bank, reg, mask, value): 0x%x, 0x%x, 0x%x,"
> - " 0x%x\n",
> - info->desc.name, info->voltage_bank, info->voltage_reg,
> - info->voltage_mask, regval);
> + "%s-set_voltage (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n",
> + info->desc.name, info->voltage_bank, info->voltage_reg,
> + info->voltage_mask, regval);
>
> - return ret;
> + return 0;
> }
>
> static struct regulator_ops ab8500_regulator_volt_mode_ops = {
On 04/10/2013 08:46 AM, Axel Lin wrote:
> We can save a register read operation in some case if read
> expand_register first.
> If info->expand_register.voltage_mask bit is set, no need to read
> voltage_reg.
>
> Signed-off-by: Axel Lin <[email protected]>
Looks good.
Acked-by: Bengt Jonsson <[email protected]>
> ---
> drivers/regulator/ab8500.c | 42 ++++++++++++++++++------------------------
> 1 file changed, 18 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
> index 222a63e..7a86fe6 100644
> --- a/drivers/regulator/ab8500.c
> +++ b/drivers/regulator/ab8500.c
> @@ -521,7 +521,7 @@ static int ab8500_regulator_get_voltage_sel(struct regulator_dev *rdev)
>
> static int ab8540_aux3_regulator_get_voltage_sel(struct regulator_dev *rdev)
> {
> - int ret, val;
> + int ret;
> struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
> u8 regval, regval_expand;
>
> @@ -531,18 +531,25 @@ static int ab8540_aux3_regulator_get_voltage_sel(struct regulator_dev *rdev)
> }
>
> ret = abx500_get_register_interruptible(info->dev,
> - info->voltage_bank, info->voltage_reg, ®val);
> -
> + info->expand_register.voltage_bank,
> + info->expand_register.voltage_reg, ®val_expand);
> if (ret < 0) {
> dev_err(rdev_get_dev(rdev),
> - "couldn't read voltage reg for regulator\n");
> + "couldn't read voltage expand reg for regulator\n");
> return ret;
> }
>
> - ret = abx500_get_register_interruptible(info->dev,
> - info->expand_register.voltage_bank,
> - info->expand_register.voltage_reg, ®val_expand);
> + dev_vdbg(rdev_get_dev(rdev),
> + "%s-get_voltage expand (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n",
> + info->desc.name, info->expand_register.voltage_bank,
> + info->expand_register.voltage_reg,
> + info->expand_register.voltage_mask, regval_expand);
> +
> + if (regval_expand & info->expand_register.voltage_mask)
> + return info->expand_register.voltage_limit;
>
> + ret = abx500_get_register_interruptible(info->dev,
> + info->voltage_bank, info->voltage_reg, ®val);
> if (ret < 0) {
> dev_err(rdev_get_dev(rdev),
> "couldn't read voltage reg for regulator\n");
> @@ -550,24 +557,11 @@ static int ab8540_aux3_regulator_get_voltage_sel(struct regulator_dev *rdev)
> }
>
> dev_vdbg(rdev_get_dev(rdev),
> - "%s-get_voltage (bank, reg, mask, value): 0x%x, 0x%x, 0x%x,"
> - " 0x%x\n",
> - info->desc.name, info->voltage_bank, info->voltage_reg,
> - info->voltage_mask, regval);
> - dev_vdbg(rdev_get_dev(rdev),
> - "%s-get_voltage expand (bank, reg, mask, value): 0x%x, 0x%x, 0x%x,"
> - " 0x%x\n",
> - info->desc.name, info->expand_register.voltage_bank,
> - info->expand_register.voltage_reg,
> - info->expand_register.voltage_mask, regval_expand);
> -
> - if (regval_expand&(info->expand_register.voltage_mask))
> - /* Vaux3 has a different layout */
> - val = info->expand_register.voltage_limit;
> - else
> - val = (regval & info->voltage_mask) >> info->voltage_shift;
> + "%s-get_voltage (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n",
> + info->desc.name, info->voltage_bank, info->voltage_reg,
> + info->voltage_mask, regval);
>
> - return val;
> + return (regval & info->voltage_mask) >> info->voltage_shift;
> }
>
> static int ab8500_regulator_set_voltage_sel(struct regulator_dev *rdev,
On Wed, Apr 10, 2013 at 02:46:20PM +0800, Axel Lin wrote:
> We can save a register read operation in some case if read
> expand_register first.
> If info->expand_register.voltage_mask bit is set, no need to read
> voltage_reg.
Applied, thanks.