The regulator always on with high power mode if info->cfg->hwreq is set.
If we allow set idle mode when info->cfg->hwreq is set, get_mode() returns
REGULATOR_MODE_IDLE but the regulator actually is in REGULATOR_MODE_NORMAL mode.
This patch avoid inconsistent status return by get_mode().
Signed-off-by: Axel Lin <[email protected]>
---
drivers/regulator/ab8500-ext.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
index 9aee21c..f0a1bbf 100644
--- a/drivers/regulator/ab8500-ext.c
+++ b/drivers/regulator/ab8500-ext.c
@@ -192,6 +192,10 @@ static int ab8500_ext_regulator_set_mode(struct regulator_dev *rdev,
info->update_val = info->update_val_hp;
break;
case REGULATOR_MODE_IDLE:
+ /* Always on with high power mode if info->cfg->hwreq is set */
+ if (info->cfg && info->cfg->hwreq)
+ return -EINVAL;
+
info->update_val = info->update_val_lp;
break;
--
1.7.10.4
This ensures info->update_val status is still correct if set_mode() call fails.
Otherwise, get_mode() may return wrong status if a set_mode() call fails.
Signed-off-by: Axel Lin <[email protected]>
---
drivers/regulator/ab8500-ext.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
index f0a1bbf..1efe702 100644
--- a/drivers/regulator/ab8500-ext.c
+++ b/drivers/regulator/ab8500-ext.c
@@ -181,6 +181,7 @@ static int ab8500_ext_regulator_set_mode(struct regulator_dev *rdev,
{
int ret = 0;
struct ab8500_ext_regulator_info *info = rdev_get_drvdata(rdev);
+ u8 regval;
if (info == NULL) {
dev_err(rdev_get_dev(rdev), "regulator info null pointer\n");
@@ -189,14 +190,14 @@ static int ab8500_ext_regulator_set_mode(struct regulator_dev *rdev,
switch (mode) {
case REGULATOR_MODE_NORMAL:
- info->update_val = info->update_val_hp;
+ regval = info->update_val_hp;
break;
case REGULATOR_MODE_IDLE:
/* Always on with high power mode if info->cfg->hwreq is set */
if (info->cfg && info->cfg->hwreq)
return -EINVAL;
- info->update_val = info->update_val_lp;
+ regval = info->update_val_lp;
break;
default:
@@ -204,12 +205,14 @@ static int ab8500_ext_regulator_set_mode(struct regulator_dev *rdev,
}
if (ab8500_ext_regulator_is_enabled(rdev)) {
- u8 regval;
-
- ret = enable(info, ®val);
- if (ret < 0)
+ ret = abx500_mask_and_set_register_interruptible(info->dev,
+ info->update_bank, info->update_reg,
+ info->update_mask, regval);
+ if (ret < 0) {
dev_err(rdev_get_dev(rdev),
"Could not set regulator mode.\n");
+ return ret;
+ }
dev_dbg(rdev_get_dev(rdev),
"%s-set_mode (bank, reg, mask, value): "
@@ -218,7 +221,9 @@ static int ab8500_ext_regulator_set_mode(struct regulator_dev *rdev,
info->update_mask, regval);
}
- return ret;
+ info->update_val = regval;
+
+ return 0;
}
static unsigned int ab8500_ext_regulator_get_mode(struct regulator_dev *rdev)
--
1.7.10.4
Both enable() and disable() functions have only one caller, thus remove them.
Signed-off-by: Axel Lin <[email protected]>
---
drivers/regulator/ab8500-ext.c | 62 ++++++++++++++--------------------------
1 file changed, 22 insertions(+), 40 deletions(-)
diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
index 1efe702..dd582be 100644
--- a/drivers/regulator/ab8500-ext.c
+++ b/drivers/regulator/ab8500-ext.c
@@ -54,32 +54,44 @@ struct ab8500_ext_regulator_info {
u8 update_val_hw;
};
-static int enable(struct ab8500_ext_regulator_info *info, u8 *regval)
+static int ab8500_ext_regulator_enable(struct regulator_dev *rdev)
{
int ret;
+ struct ab8500_ext_regulator_info *info = rdev_get_drvdata(rdev);
+ u8 regval;
- *regval = info->update_val;
+ if (info == NULL) {
+ dev_err(rdev_get_dev(rdev), "regulator info null pointer\n");
+ return -EINVAL;
+ }
/*
* To satisfy both HW high power request and SW request, the regulator
* must be on in high power.
*/
if (info->cfg && info->cfg->hwreq)
- *regval = info->update_val_hp;
+ regval = info->update_val_hp;
+ else
+ regval = info->update_val;
ret = abx500_mask_and_set_register_interruptible(info->dev,
info->update_bank, info->update_reg,
- info->update_mask, *regval);
+ info->update_mask, regval);
if (ret < 0) {
dev_err(rdev_get_dev(info->rdev),
"couldn't set enable bits for regulator\n");
return ret;
}
- return ret;
+ dev_dbg(rdev_get_dev(rdev), "%s-enable (bank, reg, mask, value):"
+ " 0x%02x, 0x%02x, 0x%02x, 0x%02x\n",
+ info->desc.name, info->update_bank, info->update_reg,
+ info->update_mask, regval);
+
+ return 0;
}
-static int ab8500_ext_regulator_enable(struct regulator_dev *rdev)
+static int ab8500_ext_regulator_disable(struct regulator_dev *rdev)
{
int ret;
struct ab8500_ext_regulator_info *info = rdev_get_drvdata(rdev);
@@ -90,53 +102,23 @@ static int ab8500_ext_regulator_enable(struct regulator_dev *rdev)
return -EINVAL;
}
- ret = enable(info, ®val);
-
- dev_dbg(rdev_get_dev(rdev), "%s-enable (bank, reg, mask, value):"
- " 0x%02x, 0x%02x, 0x%02x, 0x%02x\n",
- info->desc.name, info->update_bank, info->update_reg,
- info->update_mask, regval);
-
- return ret;
-}
-
-static int disable(struct ab8500_ext_regulator_info *info, u8 *regval)
-{
- int ret;
-
- *regval = 0x0;
-
/*
* Set the regulator in HW request mode if configured
*/
if (info->cfg && info->cfg->hwreq)
- *regval = info->update_val_hw;
+ regval = info->update_val_hw;
+ else
+ regval = 0;
ret = abx500_mask_and_set_register_interruptible(info->dev,
info->update_bank, info->update_reg,
- info->update_mask, *regval);
+ info->update_mask, regval);
if (ret < 0) {
dev_err(rdev_get_dev(info->rdev),
"couldn't set disable bits for regulator\n");
return ret;
}
- return ret;
-}
-
-static int ab8500_ext_regulator_disable(struct regulator_dev *rdev)
-{
- int ret;
- struct ab8500_ext_regulator_info *info = rdev_get_drvdata(rdev);
- u8 regval;
-
- if (info == NULL) {
- dev_err(rdev_get_dev(rdev), "regulator info null pointer\n");
- return -EINVAL;
- }
-
- ret = disable(info, ®val);
-
dev_dbg(rdev_get_dev(rdev), "%s-disable (bank, reg, mask, value):"
" 0x%02x, 0x%02x, 0x%02x, 0x%02x\n",
info->desc.name, info->update_bank, info->update_reg,
--
1.7.10.4
On 04/10/2013 02:54 PM, Axel Lin wrote:
> The regulator always on with high power mode if info->cfg->hwreq is set.
>
> If we allow set idle mode when info->cfg->hwreq is set, get_mode() returns
> REGULATOR_MODE_IDLE but the regulator actually is in REGULATOR_MODE_NORMAL mode.
> This patch avoid inconsistent status return by get_mode().
>
> Signed-off-by: Axel Lin <[email protected]>z
> ---
> drivers/regulator/ab8500-ext.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
> index 9aee21c..f0a1bbf 100644
> --- a/drivers/regulator/ab8500-ext.c
> +++ b/drivers/regulator/ab8500-ext.c
> @@ -192,6 +192,10 @@ static int ab8500_ext_regulator_set_mode(struct regulator_dev *rdev,
> info->update_val = info->update_val_hp;
> break;
> case REGULATOR_MODE_IDLE:
> + /* Always on with high power mode if info->cfg->hwreq is set */
> + if (info->cfg && info->cfg->hwreq)
> + return -EINVAL;
> +
> info->update_val = info->update_val_lp;
> break;
>
I prefer to have info->update_val updated to reflect requested mode (in
get_mode) instead of returning -EINVAL. This is how I interpret Mark's
comment on the other patch ([PATCH] regulator: ab8500: Fix get_mode for
shared mode regulators).Otherwise, the patch should work fine.
Regards,
Bengt
On 04/10/2013 02:55 PM, Axel Lin wrote:
> This ensures info->update_val status is still correct if set_mode() call fails.
> Otherwise, get_mode() may return wrong status if a set_mode() call fails.
>
> Signed-off-by: Axel Lin <[email protected]>
> ---
> drivers/regulator/ab8500-ext.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
> index f0a1bbf..1efe702 100644
> --- a/drivers/regulator/ab8500-ext.c
> +++ b/drivers/regulator/ab8500-ext.c
> @@ -181,6 +181,7 @@ static int ab8500_ext_regulator_set_mode(struct regulator_dev *rdev,
> {
> int ret = 0;
> struct ab8500_ext_regulator_info *info = rdev_get_drvdata(rdev);
> + u8 regval;
>
> if (info == NULL) {
> dev_err(rdev_get_dev(rdev), "regulator info null pointer\n");
> @@ -189,14 +190,14 @@ static int ab8500_ext_regulator_set_mode(struct regulator_dev *rdev,
>
> switch (mode) {
> case REGULATOR_MODE_NORMAL:
> - info->update_val = info->update_val_hp;
> + regval = info->update_val_hp;
> break;
> case REGULATOR_MODE_IDLE:
> /* Always on with high power mode if info->cfg->hwreq is set */
> if (info->cfg && info->cfg->hwreq)
> return -EINVAL;
>
> - info->update_val = info->update_val_lp;
> + regval = info->update_val_lp;
> break;
>
> default:
> @@ -204,12 +205,14 @@ static int ab8500_ext_regulator_set_mode(struct regulator_dev *rdev,
> }
>
> if (ab8500_ext_regulator_is_enabled(rdev)) {
> - u8 regval;
> -
> - ret = enable(info, ®val);
> - if (ret < 0)
> + ret = abx500_mask_and_set_register_interruptible(info->dev,
> + info->update_bank, info->update_reg,
> + info->update_mask, regval);
> + if (ret < 0) {
> dev_err(rdev_get_dev(rdev),
> "Could not set regulator mode.\n");
> + return ret;
> + }
>
> dev_dbg(rdev_get_dev(rdev),
> "%s-set_mode (bank, reg, mask, value): "
> @@ -218,7 +221,9 @@ static int ab8500_ext_regulator_set_mode(struct regulator_dev *rdev,
> info->update_mask, regval);
> }
>
> - return ret;
> + info->update_val = regval;
> +
> + return 0;
> }
>
> static unsigned int ab8500_ext_regulator_get_mode(struct regulator_dev *rdev)
This patch is needed to fix the error path but it partly depends on the
previous patch so I would like to get that one sorted out before ack-ing
this patch.
Regards,
Bengt
On 04/10/2013 02:56 PM, Axel Lin wrote:
> Both enable() and disable() functions have only one caller, thus remove them.
>
> Signed-off-by: Axel Lin <[email protected]>
This patch depends on the first one in the series so I would like to get
first patch sorted out first.
If the first patch goes in, I am fine with this one too.
Regards,
Bengt
> ---
> drivers/regulator/ab8500-ext.c | 62 ++++++++++++++--------------------------
> 1 file changed, 22 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
> index 1efe702..dd582be 100644
> --- a/drivers/regulator/ab8500-ext.c
> +++ b/drivers/regulator/ab8500-ext.c
> @@ -54,32 +54,44 @@ struct ab8500_ext_regulator_info {
> u8 update_val_hw;
> };
>
> -static int enable(struct ab8500_ext_regulator_info *info, u8 *regval)
> +static int ab8500_ext_regulator_enable(struct regulator_dev *rdev)
> {
> int ret;
> + struct ab8500_ext_regulator_info *info = rdev_get_drvdata(rdev);
> + u8 regval;
>
> - *regval = info->update_val;
> + if (info == NULL) {
> + dev_err(rdev_get_dev(rdev), "regulator info null pointer\n");
> + return -EINVAL;
> + }
>
> /*
> * To satisfy both HW high power request and SW request, the regulator
> * must be on in high power.
> */
> if (info->cfg && info->cfg->hwreq)
> - *regval = info->update_val_hp;
> + regval = info->update_val_hp;
> + else
> + regval = info->update_val;
>
> ret = abx500_mask_and_set_register_interruptible(info->dev,
> info->update_bank, info->update_reg,
> - info->update_mask, *regval);
> + info->update_mask, regval);
> if (ret < 0) {
> dev_err(rdev_get_dev(info->rdev),
> "couldn't set enable bits for regulator\n");
> return ret;
> }
>
> - return ret;
> + dev_dbg(rdev_get_dev(rdev), "%s-enable (bank, reg, mask, value):"
> + " 0x%02x, 0x%02x, 0x%02x, 0x%02x\n",
> + info->desc.name, info->update_bank, info->update_reg,
> + info->update_mask, regval);
> +
> + return 0;
> }
>
> -static int ab8500_ext_regulator_enable(struct regulator_dev *rdev)
> +static int ab8500_ext_regulator_disable(struct regulator_dev *rdev)
> {
> int ret;
> struct ab8500_ext_regulator_info *info = rdev_get_drvdata(rdev);
> @@ -90,53 +102,23 @@ static int ab8500_ext_regulator_enable(struct regulator_dev *rdev)
> return -EINVAL;
> }
>
> - ret = enable(info, ®val);
> -
> - dev_dbg(rdev_get_dev(rdev), "%s-enable (bank, reg, mask, value):"
> - " 0x%02x, 0x%02x, 0x%02x, 0x%02x\n",
> - info->desc.name, info->update_bank, info->update_reg,
> - info->update_mask, regval);
> -
> - return ret;
> -}
> -
> -static int disable(struct ab8500_ext_regulator_info *info, u8 *regval)
> -{
> - int ret;
> -
> - *regval = 0x0;
> -
> /*
> * Set the regulator in HW request mode if configured
> */
> if (info->cfg && info->cfg->hwreq)
> - *regval = info->update_val_hw;
> + regval = info->update_val_hw;
> + else
> + regval = 0;
>
> ret = abx500_mask_and_set_register_interruptible(info->dev,
> info->update_bank, info->update_reg,
> - info->update_mask, *regval);
> + info->update_mask, regval);
> if (ret < 0) {
> dev_err(rdev_get_dev(info->rdev),
> "couldn't set disable bits for regulator\n");
> return ret;
> }
>
> - return ret;
> -}
> -
> -static int ab8500_ext_regulator_disable(struct regulator_dev *rdev)
> -{
> - int ret;
> - struct ab8500_ext_regulator_info *info = rdev_get_drvdata(rdev);
> - u8 regval;
> -
> - if (info == NULL) {
> - dev_err(rdev_get_dev(rdev), "regulator info null pointer\n");
> - return -EINVAL;
> - }
> -
> - ret = disable(info, ®val);
> -
> dev_dbg(rdev_get_dev(rdev), "%s-disable (bank, reg, mask, value):"
> " 0x%02x, 0x%02x, 0x%02x, 0x%02x\n",
> info->desc.name, info->update_bank, info->update_reg,
2013/4/16 Bengt J?nsson <[email protected]>:
> On 04/10/2013 02:54 PM, Axel Lin wrote:
>>
>> The regulator always on with high power mode if info->cfg->hwreq is set.
>>
>> If we allow set idle mode when info->cfg->hwreq is set, get_mode() returns
>> REGULATOR_MODE_IDLE but the regulator actually is in REGULATOR_MODE_NORMAL
>> mode.
>> This patch avoid inconsistent status return by get_mode().
>>
>> Signed-off-by: Axel Lin <[email protected]>z
>>
>> ---
>> drivers/regulator/ab8500-ext.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/regulator/ab8500-ext.c
>> b/drivers/regulator/ab8500-ext.c
>> index 9aee21c..f0a1bbf 100644
>> --- a/drivers/regulator/ab8500-ext.c
>> +++ b/drivers/regulator/ab8500-ext.c
>> @@ -192,6 +192,10 @@ static int ab8500_ext_regulator_set_mode(struct
>> regulator_dev *rdev,
>> info->update_val = info->update_val_hp;
>> break;
>> case REGULATOR_MODE_IDLE:
>> + /* Always on with high power mode if info->cfg->hwreq is
>> set */
>> + if (info->cfg && info->cfg->hwreq)
>> + return -EINVAL;
>> +
>> info->update_val = info->update_val_lp;
>> break;
>>
>
> I prefer to have info->update_val updated to reflect requested mode (in
> get_mode) instead of returning -EINVAL. This is how I interpret Mark's
> comment on the other patch ([PATCH] regulator: ab8500: Fix get_mode for
> shared mode regulators).Otherwise, the patch should work fine.
Well, I drop this path and send v2 for other patches now.
Thanks for the review,
Axel