* [PATCH 2/2] regulator: ab8500: Don't update lp_mode_req flag in set_mode() error paths
2013-04-09 12:15 [PATCH 1/2] regulator: ab8500: Don't update info->update_val if write to register fails Axel Lin
@ 2013-04-09 12:17 ` Axel Lin
2013-04-17 9:49 ` Bengt Jönsson
2013-04-16 16:51 ` [PATCH 1/2] regulator: ab8500: Don't update info->update_val if write to register fails Axel Lin
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Axel Lin @ 2013-04-09 12:17 UTC (permalink / raw)
To: Mark Brown
Cc: Bengt Jonsson, Lee Jones, Yvan FILLION, Liam Girdwood, linux-kernel
Currently, set invalid mode setting for shared mode regulators may change
sm->lp_mode_req flag. This patch ensures we don't set lp_mode_req flag to wrong
status if set_mode() fails.
This patch includes some clean up, and these changes makes this patch looks like
code refactor. The clean up is mainly to avoid adding ugly code to handle
failure paths.
Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
drivers/regulator/ab8500.c | 98 +++++++++++++++++++-------------------------
1 file changed, 43 insertions(+), 55 deletions(-)
diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
index 1866dbf..88ab408 100644
--- a/drivers/regulator/ab8500.c
+++ b/drivers/regulator/ab8500.c
@@ -348,11 +348,8 @@ static int ab8500_regulator_set_mode(struct regulator_dev *rdev,
unsigned int mode)
{
int ret = 0;
- u8 bank;
- u8 reg;
- u8 mask;
- u8 val;
- bool dmr = false; /* Dedicated mode register */
+ u8 bank, reg, mask, val;
+ bool lp_mode_req = false;
struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
if (info == NULL) {
@@ -360,66 +357,54 @@ static int ab8500_regulator_set_mode(struct regulator_dev *rdev,
return -EINVAL;
}
- if (info->shared_mode) {
- /*
- * Special case where mode is shared between two regulators.
- */
- struct ab8500_shared_mode *sm = info->shared_mode;
- mutex_lock(&shared_mode_mutex);
-
- if (mode == REGULATOR_MODE_IDLE) {
- sm->lp_mode_req = true; /* Low power mode requested */
- if (!((sm->shared_regulator)->
- shared_mode->lp_mode_req)) {
- mutex_unlock(&shared_mode_mutex);
- return 0; /* Other regulator prevent LP mode */
- }
- } else {
- sm->lp_mode_req = false;
- }
- }
-
if (info->mode_mask) {
- /* Dedicated register for handling mode */
-
- dmr = true;
-
- switch (mode) {
- case REGULATOR_MODE_NORMAL:
- val = info->mode_val_normal;
- break;
- case REGULATOR_MODE_IDLE:
- val = info->mode_val_idle;
- break;
- default:
- ret = -EINVAL;
- goto out_unlock;
- }
-
bank = info->mode_bank;
reg = info->mode_reg;
mask = info->mode_mask;
} else {
- /* Mode register same as enable register */
+ bank = info->update_bank;
+ reg = info->update_reg;
+ mask = info->update_mask;
+ }
+
+ if (info->shared_mode)
+ mutex_lock(&shared_mode_mutex);
- switch (mode) {
- case REGULATOR_MODE_NORMAL:
+ switch (mode) {
+ case REGULATOR_MODE_NORMAL:
+ if (info->shared_mode)
+ lp_mode_req = false;
+
+ if (info->mode_mask)
+ val = info->mode_val_normal;
+ else
val = info->update_val_normal;
- break;
- case REGULATOR_MODE_IDLE:
- val = info->update_val_idle;
- break;
- default:
- ret = -EINVAL;
- goto out_unlock;
+ break;
+ case REGULATOR_MODE_IDLE:
+ if (info->shared_mode) {
+ struct ab8500_regulator_info *shared_regulator;
+
+ shared_regulator = info->shared_mode->shared_regulator;
+ if (!shared_regulator->shared_mode->lp_mode_req) {
+ /* Other regulator prevent LP mode */
+ info->shared_mode->lp_mode_req = true;
+ goto out_unlock;
+ }
+
+ lp_mode_req = true;
}
- bank = info->update_bank;
- reg = info->update_reg;
- mask = info->update_mask;
+ if (info->mode_mask)
+ val = info->mode_val_idle;
+ else
+ val = info->update_val_idle;
+ break;
+ default:
+ ret = -EINVAL;
+ goto out_unlock;
}
- if (dmr || ab8500_regulator_is_enabled(rdev)) {
+ if (info->mode_mask || ab8500_regulator_is_enabled(rdev)) {
ret = abx500_mask_and_set_register_interruptible(info->dev,
bank, reg, mask, val);
if (ret < 0) {
@@ -435,9 +420,12 @@ static int ab8500_regulator_set_mode(struct regulator_dev *rdev,
mask, val);
}
- if (!dmr)
+ if (!info->mode_mask)
info->update_val = val;
+ if (info->shared_mode)
+ info->shared_mode->lp_mode_req = lp_mode_req;
+
out_unlock:
if (info->shared_mode)
mutex_unlock(&shared_mode_mutex);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] regulator: ab8500: Don't update lp_mode_req flag in set_mode() error paths
2013-04-09 12:17 ` [PATCH 2/2] regulator: ab8500: Don't update lp_mode_req flag in set_mode() error paths Axel Lin
@ 2013-04-17 9:49 ` Bengt Jönsson
0 siblings, 0 replies; 6+ messages in thread
From: Bengt Jönsson @ 2013-04-17 9:49 UTC (permalink / raw)
To: Axel Lin; +Cc: Mark Brown, Lee Jones, Yvan FILLION, Liam Girdwood, linux-kernel
On 04/09/2013 02:17 PM, Axel Lin wrote:
> Currently, set invalid mode setting for shared mode regulators may change
> sm->lp_mode_req flag. This patch ensures we don't set lp_mode_req flag to wrong
> status if set_mode() fails.
>
> This patch includes some clean up, and these changes makes this patch looks like
> code refactor. The clean up is mainly to avoid adding ugly code to handle
> failure paths.
>
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
Looks fine.
Acked-by: Bengt Jonsson <bengt.g.jonsson@stericsson.com>
> ---
> drivers/regulator/ab8500.c | 98 +++++++++++++++++++-------------------------
> 1 file changed, 43 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
> index 1866dbf..88ab408 100644
> --- a/drivers/regulator/ab8500.c
> +++ b/drivers/regulator/ab8500.c
> @@ -348,11 +348,8 @@ static int ab8500_regulator_set_mode(struct regulator_dev *rdev,
> unsigned int mode)
> {
> int ret = 0;
> - u8 bank;
> - u8 reg;
> - u8 mask;
> - u8 val;
> - bool dmr = false; /* Dedicated mode register */
> + u8 bank, reg, mask, val;
> + bool lp_mode_req = false;
> struct ab8500_regulator_info *info = rdev_get_drvdata(rdev);
>
> if (info == NULL) {
> @@ -360,66 +357,54 @@ static int ab8500_regulator_set_mode(struct regulator_dev *rdev,
> return -EINVAL;
> }
>
> - if (info->shared_mode) {
> - /*
> - * Special case where mode is shared between two regulators.
> - */
> - struct ab8500_shared_mode *sm = info->shared_mode;
> - mutex_lock(&shared_mode_mutex);
> -
> - if (mode == REGULATOR_MODE_IDLE) {
> - sm->lp_mode_req = true; /* Low power mode requested */
> - if (!((sm->shared_regulator)->
> - shared_mode->lp_mode_req)) {
> - mutex_unlock(&shared_mode_mutex);
> - return 0; /* Other regulator prevent LP mode */
> - }
> - } else {
> - sm->lp_mode_req = false;
> - }
> - }
> -
> if (info->mode_mask) {
> - /* Dedicated register for handling mode */
> -
> - dmr = true;
> -
> - switch (mode) {
> - case REGULATOR_MODE_NORMAL:
> - val = info->mode_val_normal;
> - break;
> - case REGULATOR_MODE_IDLE:
> - val = info->mode_val_idle;
> - break;
> - default:
> - ret = -EINVAL;
> - goto out_unlock;
> - }
> -
> bank = info->mode_bank;
> reg = info->mode_reg;
> mask = info->mode_mask;
> } else {
> - /* Mode register same as enable register */
> + bank = info->update_bank;
> + reg = info->update_reg;
> + mask = info->update_mask;
> + }
> +
> + if (info->shared_mode)
> + mutex_lock(&shared_mode_mutex);
>
> - switch (mode) {
> - case REGULATOR_MODE_NORMAL:
> + switch (mode) {
> + case REGULATOR_MODE_NORMAL:
> + if (info->shared_mode)
> + lp_mode_req = false;
> +
> + if (info->mode_mask)
> + val = info->mode_val_normal;
> + else
> val = info->update_val_normal;
> - break;
> - case REGULATOR_MODE_IDLE:
> - val = info->update_val_idle;
> - break;
> - default:
> - ret = -EINVAL;
> - goto out_unlock;
> + break;
> + case REGULATOR_MODE_IDLE:
> + if (info->shared_mode) {
> + struct ab8500_regulator_info *shared_regulator;
> +
> + shared_regulator = info->shared_mode->shared_regulator;
> + if (!shared_regulator->shared_mode->lp_mode_req) {
> + /* Other regulator prevent LP mode */
> + info->shared_mode->lp_mode_req = true;
> + goto out_unlock;
> + }
> +
> + lp_mode_req = true;
> }
>
> - bank = info->update_bank;
> - reg = info->update_reg;
> - mask = info->update_mask;
> + if (info->mode_mask)
> + val = info->mode_val_idle;
> + else
> + val = info->update_val_idle;
> + break;
> + default:
> + ret = -EINVAL;
> + goto out_unlock;
> }
>
> - if (dmr || ab8500_regulator_is_enabled(rdev)) {
> + if (info->mode_mask || ab8500_regulator_is_enabled(rdev)) {
> ret = abx500_mask_and_set_register_interruptible(info->dev,
> bank, reg, mask, val);
> if (ret < 0) {
> @@ -435,9 +420,12 @@ static int ab8500_regulator_set_mode(struct regulator_dev *rdev,
> mask, val);
> }
>
> - if (!dmr)
> + if (!info->mode_mask)
> info->update_val = val;
>
> + if (info->shared_mode)
> + info->shared_mode->lp_mode_req = lp_mode_req;
> +
> out_unlock:
> if (info->shared_mode)
> mutex_unlock(&shared_mode_mutex);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] regulator: ab8500: Don't update info->update_val if write to register fails
2013-04-09 12:15 [PATCH 1/2] regulator: ab8500: Don't update info->update_val if write to register fails Axel Lin
2013-04-09 12:17 ` [PATCH 2/2] regulator: ab8500: Don't update lp_mode_req flag in set_mode() error paths Axel Lin
@ 2013-04-16 16:51 ` Axel Lin
2013-04-17 9:48 ` Bengt Jönsson
2013-04-17 14:09 ` Mark Brown
3 siblings, 0 replies; 6+ messages in thread
From: Axel Lin @ 2013-04-16 16:51 UTC (permalink / raw)
To: Mark Brown
Cc: Bengt Jonsson, Lee Jones, Yvan FILLION, Liam Girdwood, linux-kernel
Hi Bengt,
This serial also needs your review.
Thanks,
Axel
2013/4/9 Axel Lin <axel.lin@ingics.com>:
> This patch ensures info->update_val is consistent with current register value.
>
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> ---
> drivers/regulator/ab8500.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
> index acdffc5..1866dbf 100644
> --- a/drivers/regulator/ab8500.c
> +++ b/drivers/regulator/ab8500.c
> @@ -392,9 +392,8 @@ static int ab8500_regulator_set_mode(struct regulator_dev *rdev,
> val = info->mode_val_idle;
> break;
> default:
> - if (info->shared_mode)
> - mutex_unlock(&shared_mode_mutex);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out_unlock;
> }
>
> bank = info->mode_bank;
> @@ -405,17 +404,14 @@ static int ab8500_regulator_set_mode(struct regulator_dev *rdev,
>
> switch (mode) {
> case REGULATOR_MODE_NORMAL:
> - info->update_val = info->update_val_normal;
> val = info->update_val_normal;
> break;
> case REGULATOR_MODE_IDLE:
> - info->update_val = info->update_val_idle;
> val = info->update_val_idle;
> break;
> default:
> - if (info->shared_mode)
> - mutex_unlock(&shared_mode_mutex);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out_unlock;
> }
>
> bank = info->update_bank;
> @@ -426,9 +422,11 @@ static int ab8500_regulator_set_mode(struct regulator_dev *rdev,
> if (dmr || ab8500_regulator_is_enabled(rdev)) {
> ret = abx500_mask_and_set_register_interruptible(info->dev,
> bank, reg, mask, val);
> - if (ret < 0)
> + if (ret < 0) {
> dev_err(rdev_get_dev(rdev),
> "couldn't set regulator mode\n");
> + goto out_unlock;
> + }
>
> dev_vdbg(rdev_get_dev(rdev),
> "%s-set_mode (bank, reg, mask, value): "
> @@ -437,6 +435,10 @@ static int ab8500_regulator_set_mode(struct regulator_dev *rdev,
> mask, val);
> }
>
> + if (!dmr)
> + info->update_val = val;
> +
> +out_unlock:
> if (info->shared_mode)
> mutex_unlock(&shared_mode_mutex);
>
> --
> 1.7.10.4
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] regulator: ab8500: Don't update info->update_val if write to register fails
2013-04-09 12:15 [PATCH 1/2] regulator: ab8500: Don't update info->update_val if write to register fails Axel Lin
2013-04-09 12:17 ` [PATCH 2/2] regulator: ab8500: Don't update lp_mode_req flag in set_mode() error paths Axel Lin
2013-04-16 16:51 ` [PATCH 1/2] regulator: ab8500: Don't update info->update_val if write to register fails Axel Lin
@ 2013-04-17 9:48 ` Bengt Jönsson
2013-04-17 14:09 ` Mark Brown
3 siblings, 0 replies; 6+ messages in thread
From: Bengt Jönsson @ 2013-04-17 9:48 UTC (permalink / raw)
To: Axel Lin; +Cc: Mark Brown, Lee Jones, Yvan FILLION, Liam Girdwood, linux-kernel
On 04/09/2013 02:15 PM, Axel Lin wrote:
> This patch ensures info->update_val is consistent with current register value.
>
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
Looks good to me.
Acked-by: Bengt Jonsson <bengt.g.jonsson@stericsson.com>
> ---
> drivers/regulator/ab8500.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
> index acdffc5..1866dbf 100644
> --- a/drivers/regulator/ab8500.c
> +++ b/drivers/regulator/ab8500.c
> @@ -392,9 +392,8 @@ static int ab8500_regulator_set_mode(struct regulator_dev *rdev,
> val = info->mode_val_idle;
> break;
> default:
> - if (info->shared_mode)
> - mutex_unlock(&shared_mode_mutex);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out_unlock;
> }
>
> bank = info->mode_bank;
> @@ -405,17 +404,14 @@ static int ab8500_regulator_set_mode(struct regulator_dev *rdev,
>
> switch (mode) {
> case REGULATOR_MODE_NORMAL:
> - info->update_val = info->update_val_normal;
> val = info->update_val_normal;
> break;
> case REGULATOR_MODE_IDLE:
> - info->update_val = info->update_val_idle;
> val = info->update_val_idle;
> break;
> default:
> - if (info->shared_mode)
> - mutex_unlock(&shared_mode_mutex);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out_unlock;
> }
>
> bank = info->update_bank;
> @@ -426,9 +422,11 @@ static int ab8500_regulator_set_mode(struct regulator_dev *rdev,
> if (dmr || ab8500_regulator_is_enabled(rdev)) {
> ret = abx500_mask_and_set_register_interruptible(info->dev,
> bank, reg, mask, val);
> - if (ret < 0)
> + if (ret < 0) {
> dev_err(rdev_get_dev(rdev),
> "couldn't set regulator mode\n");
> + goto out_unlock;
> + }
>
> dev_vdbg(rdev_get_dev(rdev),
> "%s-set_mode (bank, reg, mask, value): "
> @@ -437,6 +435,10 @@ static int ab8500_regulator_set_mode(struct regulator_dev *rdev,
> mask, val);
> }
>
> + if (!dmr)
> + info->update_val = val;
> +
> +out_unlock:
> if (info->shared_mode)
> mutex_unlock(&shared_mode_mutex);
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] regulator: ab8500: Don't update info->update_val if write to register fails
2013-04-09 12:15 [PATCH 1/2] regulator: ab8500: Don't update info->update_val if write to register fails Axel Lin
` (2 preceding siblings ...)
2013-04-17 9:48 ` Bengt Jönsson
@ 2013-04-17 14:09 ` Mark Brown
3 siblings, 0 replies; 6+ messages in thread
From: Mark Brown @ 2013-04-17 14:09 UTC (permalink / raw)
To: Axel Lin
Cc: Bengt Jonsson, Lee Jones, Yvan FILLION, Liam Girdwood, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 281 bytes --]
On Tue, Apr 09, 2013 at 08:15:06PM +0800, Axel Lin wrote:
> This patch ensures info->update_val is consistent with current register value.
Applied both, thanks. Bengt, please delete unneeded context from your
mails - it makes them easier to read and your content easier to find.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread