[v9,4/5] regulators: axp20x: Change dcdc-workmode DT property to boolean flag

Commit Message

Chen-Yu Tsai Jan. 22, 2015, 3:39 p.m. UTC

The "x-powers,dcdc-workmode" property only has 2 values. A request was
made to change this to a boolean flag named "x-powers,dcdc-workmode-pwm".
Given that this particular binding hasn't been used anywhere, it should
be safe to change.

Previously the absence of the property meant just keeping the PMIC in
whatever state was set by the bootloader. After this change, the absence
means using the "auto" (PWM/PFM hybrid) mode, which is also the hardware's
default.

This patch also makes axp20x_set_dcdc_workmode() ignore attempts to set
the workmode for non-DCDC regulators.

Signed-off-by: Chen-Yu Tsai <[e-mail]>
---
 drivers/regulator/axp20x-regulator.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

Comments

Mark Brown Jan. 27, 2015, 12:27 p.m. UTC | #1

On Thu, Jan 22, 2015 at 11:39:06PM +0800, Chen-Yu Tsai wrote:
> The "x-powers,dcdc-workmode" property only has 2 values. A request was
> made to change this to a boolean flag named "x-powers,dcdc-workmode-pwm".
> Given that this particular binding hasn't been used anywhere, it should
> be safe to change.
> 
> Previously the absence of the property meant just keeping the PMIC in
> whatever state was set by the bootloader. After this change, the absence
> means using the "auto" (PWM/PFM hybrid) mode, which is also the hardware's
> default.
> 
> This patch also makes axp20x_set_dcdc_workmode() ignore attempts to set
> the workmode for non-DCDC regulators.

Again I don't understand what this is supposed to accomplish, this seems
to just make the binding less capable by forcing Linux to make a
decision on the configuration rather than allowing it to use the
hardware defaults untouched.  This runs contrary to our normal approach
to regulaors which is to try to avoid touching the hardware unless
explicit configuration is provided to say it's OK which in turn is there
because getting regulators wrong can be such a bad thing for the system.

Patch

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index 990ac453abc8..0b099e5ece3f 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -202,19 +202,21 @@  static int axp20x_regulator_parse_dt(struct platform_device *pdev)
 	return 0;
 }
 
-static int axp20x_set_dcdc_workmode(struct regulator_dev *rdev, int id, u32 workmode)
+static int axp20x_set_dcdc_workmode(struct regulator_dev *rdev, int id, bool workmode)
 {
 	unsigned int mask = AXP20X_WORKMODE_DCDC2_MASK;
+	u32 val;
 
+	/* ignore setting workmode for non-dcdc regulators */
 	if ((id != AXP20X_DCDC2) && (id != AXP20X_DCDC3))
-		return -EINVAL;
+		return 0;
 
 	if (id == AXP20X_DCDC3)
 		mask = AXP20X_WORKMODE_DCDC3_MASK;
 
-	workmode <<= ffs(mask) - 1;
+	val = workmode ? mask : 0;
 
-	return regmap_update_bits(rdev->regmap, AXP20X_DCDC_MODE, mask, workmode);
+	return regmap_update_bits(rdev->regmap, AXP20X_DCDC_MODE, mask, val);
 }
 
 static int axp20x_regulator_probe(struct platform_device *pdev)
@@ -225,8 +227,8 @@  static int axp20x_regulator_probe(struct platform_device *pdev)
 		.dev = pdev->dev.parent,
 		.regmap = axp20x->regmap,
 	};
-	int ret, i;
-	u32 workmode;
+	int i;
+	bool workmode;
 
 	/* This only sets the dcdc freq. Ignore any errors */
 	axp20x_regulator_parse_dt(pdev);
@@ -241,14 +243,11 @@  static int axp20x_regulator_probe(struct platform_device *pdev)
 			return PTR_ERR(rdev);
 		}
 
-		ret = of_property_read_u32(rdev->dev.of_node,
-					   "x-powers,dcdc-workmode",
-					   &workmode);
-		if (!ret) {
-			if (axp20x_set_dcdc_workmode(rdev, i, workmode))
-				dev_err(&pdev->dev, "Failed to set workmode on %s\n",
-					axp20x_regulators[i].name);
-		}
+		workmode = of_property_read_bool(rdev->dev.of_node,
+						 "x-powers,dcdc-workmode-pwm");
+		if (axp20x_set_dcdc_workmode(rdev, i, workmode))
+			dev_err(&pdev->dev, "Failed to set workmode on %s\n",
+				axp20x_regulators[i].name);
 	}
 
 	return 0;