linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/4] regulator: ab8500: Get rid of is_enabled from struct ab8500_regulator_info
@ 2013-04-07 15:12 Axel Lin
  2013-04-07 15:13 ` [PATCH 2/4] regulator: ab8500-ext: Get rid of is_enabled from struct ab8500_ext_regulator_info Axel Lin
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Axel Lin @ 2013-04-07 15:12 UTC (permalink / raw)
  To: Mark Brown
  Cc: Bengt Jonsson, Lee Jones, Yvan FILLION, Liam Girdwood, linux-kernel

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 <axel.lin@ingics.com>
---
 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




^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/4] regulator: ab8500-ext: Get rid of is_enabled from struct ab8500_ext_regulator_info
  2013-04-07 15:12 [PATCH 1/4] regulator: ab8500: Get rid of is_enabled from struct ab8500_regulator_info Axel Lin
@ 2013-04-07 15:13 ` Axel Lin
  2013-04-08  7:54   ` Lee Jones
  2013-04-07 15:15 ` [PATCH 3/4] regulator: ab8500-ext: Remove unnecessary checking for ab9540 and ab8540 Axel Lin
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 8+ messages in thread
From: Axel Lin @ 2013-04-07 15:13 UTC (permalink / raw)
  To: Mark Brown
  Cc: Bengt Jonsson, Lee Jones, Yvan FILLION, Liam Girdwood, linux-kernel

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.

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 drivers/regulator/ab8500-ext.c |   14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
index 57d43a1..5e604a2 100644
--- a/drivers/regulator/ab8500-ext.c
+++ b/drivers/regulator/ab8500-ext.c
@@ -29,7 +29,6 @@
  * @desc: regulator description
  * @rdev: regulator device
  * @cfg: regulator configuration (extension of regulator FW configuration)
- * @is_enabled: status of regulator (on/off)
  * @update_bank: bank to control on/off
  * @update_reg: register to control on/off
  * @update_mask: mask to enable/disable and set mode of regulator
@@ -46,7 +45,6 @@ struct ab8500_ext_regulator_info {
 	struct regulator_desc desc;
 	struct regulator_dev *rdev;
 	struct ab8500_ext_regulator_cfg *cfg;
-	bool is_enabled;
 	u8 update_bank;
 	u8 update_reg;
 	u8 update_mask;
@@ -78,8 +76,6 @@ static int enable(struct ab8500_ext_regulator_info *info, u8 *regval)
 		return ret;
 	}
 
-	info->is_enabled = true;
-
 	return ret;
 }
 
@@ -125,8 +121,6 @@ static int disable(struct ab8500_ext_regulator_info *info, u8 *regval)
 		return ret;
 	}
 
-	info->is_enabled = false;
-
 	return ret;
 }
 
@@ -177,11 +171,9 @@ static int ab8500_ext_regulator_is_enabled(struct regulator_dev *rdev)
 
 	if (((regval & info->update_mask) == info->update_val_lp) ||
 	    ((regval & info->update_mask) == info->update_val_hp))
-		info->is_enabled = true;
+		return 1;
 	else
-		info->is_enabled = false;
-
-	return info->is_enabled;
+		return 0;
 }
 
 static int ab8500_ext_regulator_set_mode(struct regulator_dev *rdev,
@@ -207,7 +199,7 @@ static int ab8500_ext_regulator_set_mode(struct regulator_dev *rdev,
 		return -EINVAL;
 	}
 
-	if (info->is_enabled) {
+	if (ab8500_ext_regulator_is_enabled(rdev)) {
 		u8 regval;
 
 		ret = enable(info, &regval);
-- 
1.7.10.4




^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/4] regulator: ab8500-ext: Remove unnecessary checking for ab9540 and ab8540
  2013-04-07 15:12 [PATCH 1/4] regulator: ab8500: Get rid of is_enabled from struct ab8500_regulator_info Axel Lin
  2013-04-07 15:13 ` [PATCH 2/4] regulator: ab8500-ext: Get rid of is_enabled from struct ab8500_ext_regulator_info Axel Lin
@ 2013-04-07 15:15 ` Axel Lin
  2013-04-07 15:17 ` [PATCH 4/4] regulator: ab8500-ext: Remove get_voltage to avoid duplicate implementation Axel Lin
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Axel Lin @ 2013-04-07 15:15 UTC (permalink / raw)
  To: Mark Brown
  Cc: Bengt Jonsson, Lee Jones, Yvan FILLION, Liam Girdwood, linux-kernel

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 <axel.lin@ingics.com>
---
 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




^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 4/4] regulator: ab8500-ext: Remove get_voltage to avoid duplicate implementation
  2013-04-07 15:12 [PATCH 1/4] regulator: ab8500: Get rid of is_enabled from struct ab8500_regulator_info Axel Lin
  2013-04-07 15:13 ` [PATCH 2/4] regulator: ab8500-ext: Get rid of is_enabled from struct ab8500_ext_regulator_info Axel Lin
  2013-04-07 15:15 ` [PATCH 3/4] regulator: ab8500-ext: Remove unnecessary checking for ab9540 and ab8540 Axel Lin
@ 2013-04-07 15:17 ` Axel Lin
  2013-04-08  7:53   ` Lee Jones
  2013-04-08  7:52 ` [PATCH 1/4] regulator: ab8500: Get rid of is_enabled from struct ab8500_regulator_info Lee Jones
  2013-04-08 10:24 ` Mark Brown
  4 siblings, 1 reply; 8+ messages in thread
From: Axel Lin @ 2013-04-07 15:17 UTC (permalink / raw)
  To: Mark Brown
  Cc: Bengt Jonsson, Lee Jones, Yvan FILLION, Liam Girdwood, linux-kernel

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 <axel.lin@ingics.com>
---
 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




^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/4] regulator: ab8500: Get rid of is_enabled from struct ab8500_regulator_info
  2013-04-07 15:12 [PATCH 1/4] regulator: ab8500: Get rid of is_enabled from struct ab8500_regulator_info Axel Lin
                   ` (2 preceding siblings ...)
  2013-04-07 15:17 ` [PATCH 4/4] regulator: ab8500-ext: Remove get_voltage to avoid duplicate implementation Axel Lin
@ 2013-04-08  7:52 ` Lee Jones
  2013-04-08 10:24 ` Mark Brown
  4 siblings, 0 replies; 8+ messages in thread
From: Lee Jones @ 2013-04-08  7:52 UTC (permalink / raw)
  To: Axel Lin
  Cc: Mark Brown, Bengt Jonsson, Yvan FILLION, Liam Girdwood, linux-kernel

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 <axel.lin@ingics.com>

Code looks good to me:

Acked-by: Lee Jones <lee.jones@linaro.org>

> ---
>  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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 4/4] regulator: ab8500-ext: Remove get_voltage to avoid duplicate implementation
  2013-04-07 15:17 ` [PATCH 4/4] regulator: ab8500-ext: Remove get_voltage to avoid duplicate implementation Axel Lin
@ 2013-04-08  7:53   ` Lee Jones
  0 siblings, 0 replies; 8+ messages in thread
From: Lee Jones @ 2013-04-08  7:53 UTC (permalink / raw)
  To: Axel Lin
  Cc: Mark Brown, Bengt Jonsson, Yvan FILLION, Liam Girdwood, linux-kernel

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 <axel.lin@ingics.com>

Nice simplification:

Acked-by: Lee Jones <lee.jones@linaro.org>

> ---
>  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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/4] regulator: ab8500-ext: Get rid of is_enabled from struct ab8500_ext_regulator_info
  2013-04-07 15:13 ` [PATCH 2/4] regulator: ab8500-ext: Get rid of is_enabled from struct ab8500_ext_regulator_info Axel Lin
@ 2013-04-08  7:54   ` Lee Jones
  0 siblings, 0 replies; 8+ messages in thread
From: Lee Jones @ 2013-04-08  7:54 UTC (permalink / raw)
  To: Axel Lin
  Cc: Mark Brown, Bengt Jonsson, Yvan FILLION, Liam Girdwood, linux-kernel

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.
> 
> Signed-off-by: Axel Lin <axel.lin@ingics.com>

Looks fine:

Acked-by: Lee Jones <lee.jones@linaro.org>

> ---
>  drivers/regulator/ab8500-ext.c |   14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/regulator/ab8500-ext.c b/drivers/regulator/ab8500-ext.c
> index 57d43a1..5e604a2 100644
> --- a/drivers/regulator/ab8500-ext.c
> +++ b/drivers/regulator/ab8500-ext.c
> @@ -29,7 +29,6 @@
>   * @desc: regulator description
>   * @rdev: regulator device
>   * @cfg: regulator configuration (extension of regulator FW configuration)
> - * @is_enabled: status of regulator (on/off)
>   * @update_bank: bank to control on/off
>   * @update_reg: register to control on/off
>   * @update_mask: mask to enable/disable and set mode of regulator
> @@ -46,7 +45,6 @@ struct ab8500_ext_regulator_info {
>  	struct regulator_desc desc;
>  	struct regulator_dev *rdev;
>  	struct ab8500_ext_regulator_cfg *cfg;
> -	bool is_enabled;
>  	u8 update_bank;
>  	u8 update_reg;
>  	u8 update_mask;
> @@ -78,8 +76,6 @@ static int enable(struct ab8500_ext_regulator_info *info, u8 *regval)
>  		return ret;
>  	}
>  
> -	info->is_enabled = true;
> -
>  	return ret;
>  }
>  
> @@ -125,8 +121,6 @@ static int disable(struct ab8500_ext_regulator_info *info, u8 *regval)
>  		return ret;
>  	}
>  
> -	info->is_enabled = false;
> -
>  	return ret;
>  }
>  
> @@ -177,11 +171,9 @@ static int ab8500_ext_regulator_is_enabled(struct regulator_dev *rdev)
>  
>  	if (((regval & info->update_mask) == info->update_val_lp) ||
>  	    ((regval & info->update_mask) == info->update_val_hp))
> -		info->is_enabled = true;
> +		return 1;
>  	else
> -		info->is_enabled = false;
> -
> -	return info->is_enabled;
> +		return 0;
>  }
>  
>  static int ab8500_ext_regulator_set_mode(struct regulator_dev *rdev,
> @@ -207,7 +199,7 @@ static int ab8500_ext_regulator_set_mode(struct regulator_dev *rdev,
>  		return -EINVAL;
>  	}
>  
> -	if (info->is_enabled) {
> +	if (ab8500_ext_regulator_is_enabled(rdev)) {
>  		u8 regval;
>  
>  		ret = enable(info, &regval);

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/4] regulator: ab8500: Get rid of is_enabled from struct ab8500_regulator_info
  2013-04-07 15:12 [PATCH 1/4] regulator: ab8500: Get rid of is_enabled from struct ab8500_regulator_info Axel Lin
                   ` (3 preceding siblings ...)
  2013-04-08  7:52 ` [PATCH 1/4] regulator: ab8500: Get rid of is_enabled from struct ab8500_regulator_info Lee Jones
@ 2013-04-08 10:24 ` Mark Brown
  4 siblings, 0 replies; 8+ messages in thread
From: Mark Brown @ 2013-04-08 10:24 UTC (permalink / raw)
  To: Axel Lin
  Cc: Bengt Jonsson, Lee Jones, Yvan FILLION, Liam Girdwood, linux-kernel

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

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.

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2013-04-08 10:24 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-04-07 15:12 [PATCH 1/4] regulator: ab8500: Get rid of is_enabled from struct ab8500_regulator_info Axel Lin
2013-04-07 15:13 ` [PATCH 2/4] regulator: ab8500-ext: Get rid of is_enabled from struct ab8500_ext_regulator_info Axel Lin
2013-04-08  7:54   ` Lee Jones
2013-04-07 15:15 ` [PATCH 3/4] regulator: ab8500-ext: Remove unnecessary checking for ab9540 and ab8540 Axel Lin
2013-04-07 15:17 ` [PATCH 4/4] regulator: ab8500-ext: Remove get_voltage to avoid duplicate implementation Axel Lin
2013-04-08  7:53   ` Lee Jones
2013-04-08  7:52 ` [PATCH 1/4] regulator: ab8500: Get rid of is_enabled from struct ab8500_regulator_info Lee Jones
2013-04-08 10:24 ` Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).