LinuxLists.cc - [PATCH 1/4] regulator: ab8500: Get rid of is_enabled from struct ab8500_regulator

2013-04-07 15:12:36

Subject: [PATCH 1/4] regulator: ab8500: Get rid of is_enabled from struct ab8500_regulator_info

The intention of this patch is to simplify the code.

Maintain the is_enabled flag is not trivial, it not only needs to set/clear the
flag in disable()/enable() but also needs to set the flag in is_enable() to get
initial status. The only benefit of keeping is_enabled flag is just save a
register read when set_mode(). Remove is_enabled flag makes the code simpler.

This patch also moves ab8500_regulator_is_enabled() close to
ab8500_regulator_[en|dis]able functions.
This is required to avoid a forward declaration because now we call
ab8500_regulator_is_enabled() in ab8500_regulator_set_mode().
This change also makes the code better in readability by moving similar
functions to one place.

Signed-off-by: Axel Lin <[email protected]>
---
drivers/regulator/ab8500.c | 72 ++++++++++++++++++++------------------------
1 file changed, 32 insertions(+), 40 deletions(-)

diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
index 517305e..9ebd131 100644
--- a/drivers/regulator/ab8500.c
+++ b/drivers/regulator/ab8500.c
@@ -46,7 +46,6 @@ struct ab8500_shared_mode {
* @desc: regulator description
* @regulator_dev: regulator device
* @shared_mode: used when mode is shared between two regulators
- * @is_enabled: status of regulator (on/off)
* @load_lp_uA: maximum load in idle (low power) mode
* @update_bank: bank to control on/off
* @update_reg: register to control on/off
@@ -69,7 +68,6 @@ struct ab8500_regulator_info {
struct regulator_desc desc;
struct regulator_dev *regulator;
struct ab8500_shared_mode *shared_mode;
- bool is_enabled;
int load_lp_uA;
u8 update_bank;
u8 update_reg;
@@ -259,8 +257,6 @@ static int ab8500_regulator_enable(struct regulator_dev *rdev)
return ret;
}

- info->is_enabled = true;
-
dev_vdbg(rdev_get_dev(rdev),
"%s-enable (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n",
info->desc.name, info->update_bank, info->update_reg,
@@ -288,8 +284,6 @@ static int ab8500_regulator_disable(struct regulator_dev *rdev)
return ret;
}

- info->is_enabled = false;
-
dev_vdbg(rdev_get_dev(rdev),
"%s-disable (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n",
info->desc.name, info->update_bank, info->update_reg,
@@ -298,6 +292,37 @@ static int ab8500_regulator_disable(struct regulator_dev *rdev)
return ret;
}

+static int ab8500_regulator_is_enabled(struct regulator_dev *rdev)
+{
+ int ret;
+ struct ab8500_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 = abx500_get_register_interruptible(info->dev,
+ info->update_bank, info->update_reg, &regval);
+ if (ret < 0) {
+ dev_err(rdev_get_dev(rdev),
+ "couldn't read 0x%x register\n", info->update_reg);
+ return ret;
+ }
+
+ dev_vdbg(rdev_get_dev(rdev),
+ "%s-is_enabled (bank, reg, mask, value): 0x%x, 0x%x, 0x%x,"
+ " 0x%x\n",
+ info->desc.name, info->update_bank, info->update_reg,
+ info->update_mask, regval);
+
+ if (regval & info->update_mask)
+ return 1;
+ else
+ return 0;
+}
+
static unsigned int ab8500_regulator_get_optimum_mode(
struct regulator_dev *rdev, int input_uV,
int output_uV, int load_uA)
@@ -398,7 +423,7 @@ static int ab8500_regulator_set_mode(struct regulator_dev *rdev,
mask = info->update_mask;
}

- if (info->is_enabled || dmr) {
+ if (dmr || ab8500_regulator_is_enabled(rdev)) {
ret = abx500_mask_and_set_register_interruptible(info->dev,
bank, reg, mask, val);
if (ret < 0)
@@ -464,39 +489,6 @@ static unsigned int ab8500_regulator_get_mode(struct regulator_dev *rdev)
return ret;
}

-static int ab8500_regulator_is_enabled(struct regulator_dev *rdev)
-{
- int ret;
- struct ab8500_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 = abx500_get_register_interruptible(info->dev,
- info->update_bank, info->update_reg, &regval);
- if (ret < 0) {
- dev_err(rdev_get_dev(rdev),
- "couldn't read 0x%x register\n", info->update_reg);
- return ret;
- }
-
- dev_vdbg(rdev_get_dev(rdev),
- "%s-is_enabled (bank, reg, mask, value): 0x%x, 0x%x, 0x%x,"
- " 0x%x\n",
- info->desc.name, info->update_bank, info->update_reg,
- info->update_mask, regval);
-
- if (regval & info->update_mask)
- info->is_enabled = true;
- else
- info->is_enabled = false;
-
- return info->is_enabled;
-}
-
static int ab8500_regulator_get_voltage_sel(struct regulator_dev *rdev)
{
int ret, val;
--
1.7.10.4

2013-04-07 15:13:46

by Axel Lin

[permalink] [raw]

Subject: [PATCH 2/4] regulator: ab8500-ext: Get rid of is_enabled from struct ab8500_ext_regulator_info

2013-04-07 15:15:31

by Axel Lin

[permalink] [raw]

Subject: [PATCH 3/4] regulator: ab8500-ext: Remove unnecessary checking for ab9540 and ab8540

This code was added by commit 0fe17e20a6
"regulator: ab8500-ext: Add support for AB9540 regulators"
and commit bd44e2cb "regulator: ab8500: Also check for AB8505 based platforms"

The original patch[1] is to set info->desc.ops = &ab9540_ext_regulator_ops.

However, ab9540_ext_regulator_ops is identical to ab8500_ext_regulator_ops[2].
Thus we can complete remove the unnecessary checking for ab9540 and ab8540.

[1] https://lkml.org/lkml/2013/3/28/333
[2] https://lkml.org/lkml/2013/4/1/178

Signed-off-by: Axel Lin <[email protected]>
---
drivers/regulator/ab8500-ext.c | 7 -------
1 file changed, 7 deletions(-)

diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
index 5e604a2..c7896af 100644
--- a/drivers/regulator/ab8500-ext.c
+++ b/drivers/regulator/ab8500-ext.c
@@ -389,15 +389,8 @@ int ab8500_ext_regulator_init(struct platform_device *pdev)
config.init_data = &pdata->ext_regulator[i];
config.driver_data = info;

- if ((is_ab9540(ab8500) || is_ab8540(ab8500)) &&
- ((info->desc.id == AB8500_EXT_SUPPLY1) ||
- (info->desc.id == AB8500_EXT_SUPPLY2) ||
- (info->desc.id == AB8500_EXT_SUPPLY3)))
- info->desc.ops = &ab8500_ext_regulator_ops;
-
/* register regulator with framework */
info->rdev = regulator_register(&info->desc, &config);
-
if (IS_ERR(info->rdev)) {
err = PTR_ERR(info->rdev);
dev_err(&pdev->dev, "failed to register regulator %s\n",
--
1.7.10.4

2013-04-07 15:17:57

by Axel Lin

[permalink] [raw]

Subject: [PATCH 4/4] regulator: ab8500-ext: Remove get_voltage to avoid duplicate implementation

The implementation of ab8500_ext_fixed_get_voltage is identical to
ab8500_ext_list_voltage. We can avoid the duplicate implementation by just
remove get_voltage. For fixed regulator, regulator core will call
list_voltage(rdev, 0) to get voltage if both get_voltage get_voltage_sel are
not implemented.

Signed-off-by: Axel Lin <[email protected]>
---
drivers/regulator/ab8500-ext.c | 16 ----------------
1 file changed, 16 deletions(-)

diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
index c7896af..9aee21c 100644
--- a/drivers/regulator/ab8500-ext.c
+++ b/drivers/regulator/ab8500-ext.c
@@ -237,21 +237,6 @@ static unsigned int ab8500_ext_regulator_get_mode(struct regulator_dev *rdev)
return ret;
}

-static int ab8500_ext_fixed_get_voltage(struct regulator_dev *rdev)
-{
- struct regulation_constraints *regu_constraints = rdev->constraints;
-
- if (regu_constraints == NULL) {
- dev_err(rdev_get_dev(rdev), "regulator constraints null pointer\n");
- return -EINVAL;
- }
- if (regu_constraints->min_uV && regu_constraints->max_uV) {
- if (regu_constraints->min_uV == regu_constraints->max_uV)
- return regu_constraints->min_uV;
- }
- return -EINVAL;
-}
-
static int ab8500_ext_list_voltage(struct regulator_dev *rdev,
unsigned selector)
{
@@ -275,7 +260,6 @@ static struct regulator_ops ab8500_ext_regulator_ops = {
.is_enabled = ab8500_ext_regulator_is_enabled,
.set_mode = ab8500_ext_regulator_set_mode,
.get_mode = ab8500_ext_regulator_get_mode,
- .get_voltage = ab8500_ext_fixed_get_voltage,
.list_voltage = ab8500_ext_list_voltage,
};

--
1.7.10.4

2013-04-08 07:52:24

by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH 1/4] regulator: ab8500: Get rid of is_enabled from struct ab8500_regulator_info

On Sun, 07 Apr 2013, Axel Lin wrote:

> The intention of this patch is to simplify the code.
>
> Maintain the is_enabled flag is not trivial, it not only needs to set/clear the
> flag in disable()/enable() but also needs to set the flag in is_enable() to get
> initial status. The only benefit of keeping is_enabled flag is just save a
> register read when set_mode(). Remove is_enabled flag makes the code simpler.
>
> This patch also moves ab8500_regulator_is_enabled() close to
> ab8500_regulator_[en|dis]able functions.
> This is required to avoid a forward declaration because now we call
> ab8500_regulator_is_enabled() in ab8500_regulator_set_mode().
> This change also makes the code better in readability by moving similar
> functions to one place.
>
> Signed-off-by: Axel Lin <[email protected]>

Code looks good to me:

Acked-by: Lee Jones <[email protected]>

> ---
> drivers/regulator/ab8500.c | 72 ++++++++++++++++++++------------------------
> 1 file changed, 32 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/regulator/ab8500.c b/drivers/regulator/ab8500.c
> index 517305e..9ebd131 100644
> --- a/drivers/regulator/ab8500.c
> +++ b/drivers/regulator/ab8500.c
> @@ -46,7 +46,6 @@ struct ab8500_shared_mode {
> * @desc: regulator description
> * @regulator_dev: regulator device
> * @shared_mode: used when mode is shared between two regulators
> - * @is_enabled: status of regulator (on/off)
> * @load_lp_uA: maximum load in idle (low power) mode
> * @update_bank: bank to control on/off
> * @update_reg: register to control on/off
> @@ -69,7 +68,6 @@ struct ab8500_regulator_info {
> struct regulator_desc desc;
> struct regulator_dev *regulator;
> struct ab8500_shared_mode *shared_mode;
> - bool is_enabled;
> int load_lp_uA;
> u8 update_bank;
> u8 update_reg;
> @@ -259,8 +257,6 @@ static int ab8500_regulator_enable(struct regulator_dev *rdev)
> return ret;
> }
>
> - info->is_enabled = true;
> -
> dev_vdbg(rdev_get_dev(rdev),
> "%s-enable (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n",
> info->desc.name, info->update_bank, info->update_reg,
> @@ -288,8 +284,6 @@ static int ab8500_regulator_disable(struct regulator_dev *rdev)
> return ret;
> }
>
> - info->is_enabled = false;
> -
> dev_vdbg(rdev_get_dev(rdev),
> "%s-disable (bank, reg, mask, value): 0x%x, 0x%x, 0x%x, 0x%x\n",
> info->desc.name, info->update_bank, info->update_reg,
> @@ -298,6 +292,37 @@ static int ab8500_regulator_disable(struct regulator_dev *rdev)
> return ret;
> }
>
> +static int ab8500_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> + int ret;
> + struct ab8500_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 = abx500_get_register_interruptible(info->dev,
> + info->update_bank, info->update_reg, &regval);
> + if (ret < 0) {
> + dev_err(rdev_get_dev(rdev),
> + "couldn't read 0x%x register\n", info->update_reg);
> + return ret;
> + }
> +
> + dev_vdbg(rdev_get_dev(rdev),
> + "%s-is_enabled (bank, reg, mask, value): 0x%x, 0x%x, 0x%x,"
> + " 0x%x\n",
> + info->desc.name, info->update_bank, info->update_reg,
> + info->update_mask, regval);
> +
> + if (regval & info->update_mask)
> + return 1;
> + else
> + return 0;
> +}
> +
> static unsigned int ab8500_regulator_get_optimum_mode(
> struct regulator_dev *rdev, int input_uV,
> int output_uV, int load_uA)
> @@ -398,7 +423,7 @@ static int ab8500_regulator_set_mode(struct regulator_dev *rdev,
> mask = info->update_mask;
> }
>
> - if (info->is_enabled || dmr) {
> + if (dmr || ab8500_regulator_is_enabled(rdev)) {
> ret = abx500_mask_and_set_register_interruptible(info->dev,
> bank, reg, mask, val);
> if (ret < 0)
> @@ -464,39 +489,6 @@ static unsigned int ab8500_regulator_get_mode(struct regulator_dev *rdev)
> return ret;
> }
>
> -static int ab8500_regulator_is_enabled(struct regulator_dev *rdev)
> -{
> - int ret;
> - struct ab8500_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 = abx500_get_register_interruptible(info->dev,
> - info->update_bank, info->update_reg, &regval);
> - if (ret < 0) {
> - dev_err(rdev_get_dev(rdev),
> - "couldn't read 0x%x register\n", info->update_reg);
> - return ret;
> - }
> -
> - dev_vdbg(rdev_get_dev(rdev),
> - "%s-is_enabled (bank, reg, mask, value): 0x%x, 0x%x, 0x%x,"
> - " 0x%x\n",
> - info->desc.name, info->update_bank, info->update_reg,
> - info->update_mask, regval);
> -
> - if (regval & info->update_mask)
> - info->is_enabled = true;
> - else
> - info->is_enabled = false;
> -
> - return info->is_enabled;
> -}
> -
> static int ab8500_regulator_get_voltage_sel(struct regulator_dev *rdev)
> {
> int ret, val;

--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

2013-04-08 07:53:34

by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH 4/4] regulator: ab8500-ext: Remove get_voltage to avoid duplicate implementation

On Sun, 07 Apr 2013, Axel Lin wrote:

> The implementation of ab8500_ext_fixed_get_voltage is identical to
> ab8500_ext_list_voltage. We can avoid the duplicate implementation by just
> remove get_voltage. For fixed regulator, regulator core will call
> list_voltage(rdev, 0) to get voltage if both get_voltage get_voltage_sel are
> not implemented.
>
> Signed-off-by: Axel Lin <[email protected]>

Nice simplification:

Acked-by: Lee Jones <[email protected]>

> ---
> drivers/regulator/ab8500-ext.c | 16 ----------------
> 1 file changed, 16 deletions(-)
>
> diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
> index c7896af..9aee21c 100644
> --- a/drivers/regulator/ab8500-ext.c
> +++ b/drivers/regulator/ab8500-ext.c
> @@ -237,21 +237,6 @@ static unsigned int ab8500_ext_regulator_get_mode(struct regulator_dev *rdev)
> return ret;
> }
>
> -static int ab8500_ext_fixed_get_voltage(struct regulator_dev *rdev)
> -{
> - struct regulation_constraints *regu_constraints = rdev->constraints;
> -
> - if (regu_constraints == NULL) {
> - dev_err(rdev_get_dev(rdev), "regulator constraints null pointer\n");
> - return -EINVAL;
> - }
> - if (regu_constraints->min_uV && regu_constraints->max_uV) {
> - if (regu_constraints->min_uV == regu_constraints->max_uV)
> - return regu_constraints->min_uV;
> - }
> - return -EINVAL;
> -}
> -
> static int ab8500_ext_list_voltage(struct regulator_dev *rdev,
> unsigned selector)
> {
> @@ -275,7 +260,6 @@ static struct regulator_ops ab8500_ext_regulator_ops = {
> .is_enabled = ab8500_ext_regulator_is_enabled,
> .set_mode = ab8500_ext_regulator_set_mode,
> .get_mode = ab8500_ext_regulator_get_mode,
> - .get_voltage = ab8500_ext_fixed_get_voltage,
> .list_voltage = ab8500_ext_list_voltage,
> };
>

--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

2013-04-08 07:54:58

by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH 2/4] regulator: ab8500-ext: Get rid of is_enabled from struct ab8500_ext_regulator_info

2013-04-08 10:24:52

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH 1/4] regulator: ab8500: Get rid of is_enabled from struct ab8500_regulator_info

On Sun, Apr 07, 2013 at 11:12:28PM +0800, Axel Lin wrote:

> The intention of this patch is to simplify the code.

Applied all, thanks.

Attachments:

(No filename) (136.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments