[RFC PATCH 1/2] regulator: refactor valid_ops_mask checking code

[RFC PATCH 1/2] regulator: refactor valid_ops_mask checking code

[WEN Pingbo]

To make the code more compat and centralized, this patch add a
unified function - regulator_ops_is_valid. So we can add
some extra checking code easily later.

Signed-off-by: WEN Pingbo <pingbo.wen@linaro.org>
---
 drivers/regulator/core.c | 88 ++++++++++++++++--------------------------------
 1 file changed, 29 insertions(+), 59 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e0b7642..fe47d38 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -132,6 +132,19 @@ static bool have_full_constraints(void)
 	return has_full_constraints || of_have_populated_dt();
 }
 
+static bool regulator_ops_is_valid(struct regulator_dev *rdev, int ops)
+{
+	if (!rdev->constraints) {
+		rdev_err(rdev, "no constraints\n");
+		return false;
+	}
+
+	if (rdev->constraints->valid_ops_mask & ops)
+		return true;
+
+	return false;
+}
+
 static inline struct regulator_dev *rdev_get_supply(struct regulator_dev *rdev)
 {
 	if (rdev && rdev->supply)
@@ -198,28 +211,13 @@ static struct device_node *of_get_regulator(struct device *dev, const char *supp
 	return regnode;
 }
 
-static int _regulator_can_change_status(struct regulator_dev *rdev)
-{
-	if (!rdev->constraints)
-		return 0;
-
-	if (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_STATUS)
-		return 1;
-	else
-		return 0;
-}
-
 /* Platform voltage constraint check */
 static int regulator_check_voltage(struct regulator_dev *rdev,
 				   int *min_uV, int *max_uV)
 {
 	BUG_ON(*min_uV > *max_uV);
 
-	if (!rdev->constraints) {
-		rdev_err(rdev, "no constraints\n");
-		return -ENODEV;
-	}
-	if (!(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)) {
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE)) {
 		rdev_err(rdev, "voltage operation not allowed\n");
 		return -EPERM;
 	}
@@ -275,11 +273,7 @@ static int regulator_check_current_limit(struct regulator_dev *rdev,
 {
 	BUG_ON(*min_uA > *max_uA);
 
-	if (!rdev->constraints) {
-		rdev_err(rdev, "no constraints\n");
-		return -ENODEV;
-	}
-	if (!(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_CURRENT)) {
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_CURRENT)) {
 		rdev_err(rdev, "current operation not allowed\n");
 		return -EPERM;
 	}
@@ -312,11 +306,7 @@ static int regulator_mode_constrain(struct regulator_dev *rdev, int *mode)
 		return -EINVAL;
 	}
 
-	if (!rdev->constraints) {
-		rdev_err(rdev, "no constraints\n");
-		return -ENODEV;
-	}
-	if (!(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_MODE)) {
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_MODE)) {
 		rdev_err(rdev, "mode operation not allowed\n");
 		return -EPERM;
 	}
@@ -333,20 +323,6 @@ static int regulator_mode_constrain(struct regulator_dev *rdev, int *mode)
 	return -EINVAL;
 }
 
-/* dynamic regulator mode switching constraint check */
-static int regulator_check_drms(struct regulator_dev *rdev)
-{
-	if (!rdev->constraints) {
-		rdev_err(rdev, "no constraints\n");
-		return -ENODEV;
-	}
-	if (!(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS)) {
-		rdev_dbg(rdev, "drms operation not allowed\n");
-		return -EPERM;
-	}
-	return 0;
-}
-
 static ssize_t regulator_uV_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
@@ -692,8 +668,7 @@ static int drms_uA_update(struct regulator_dev *rdev)
 	 * first check to see if we can set modes at all, otherwise just
 	 * tell the consumer everything is OK.
 	 */
-	err = regulator_check_drms(rdev);
-	if (err < 0)
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_DRMS))
 		return 0;
 
 	if (!rdev->desc->ops->get_optimum_mode &&
@@ -893,7 +868,7 @@ static void print_constraints(struct regulator_dev *rdev)
 	rdev_dbg(rdev, "%s\n", buf);
 
 	if ((constraints->min_uV != constraints->max_uV) &&
-	    !(constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE))
+	    !regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE))
 		rdev_warn(rdev,
 			  "Voltage range but no REGULATOR_CHANGE_VOLTAGE\n");
 }
@@ -1334,7 +1309,7 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 	 * it is then we don't need to do nearly so much work for
 	 * enable/disable calls.
 	 */
-	if (!_regulator_can_change_status(rdev) &&
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS) &&
 	    _regulator_is_enabled(rdev))
 		regulator->always_on = true;
 
@@ -2111,15 +2086,15 @@ static int _regulator_enable(struct regulator_dev *rdev)
 	lockdep_assert_held_once(&rdev->mutex);
 
 	/* check voltage and requested load before enabling */
-	if (rdev->constraints &&
-	    (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS))
+	if (regulator_ops_is_valid(rdev, REGULATOR_CHANGE_DRMS))
 		drms_uA_update(rdev);
 
 	if (rdev->use_count == 0) {
 		/* The regulator may on if it's not switchable or left on */
 		ret = _regulator_is_enabled(rdev);
 		if (ret == -EINVAL || ret == 0) {
-			if (!_regulator_can_change_status(rdev))
+			if (!regulator_ops_is_valid(rdev,
+					REGULATOR_CHANGE_STATUS))
 				return -EPERM;
 
 			ret = _regulator_do_enable(rdev);
@@ -2221,7 +2196,7 @@ static int _regulator_disable(struct regulator_dev *rdev)
 	    (rdev->constraints && !rdev->constraints->always_on)) {
 
 		/* we are last user */
-		if (_regulator_can_change_status(rdev)) {
+		if (regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS)) {
 			ret = _notifier_call_chain(rdev,
 						   REGULATOR_EVENT_PRE_DISABLE,
 						   NULL);
@@ -2242,10 +2217,7 @@ static int _regulator_disable(struct regulator_dev *rdev)
 
 		rdev->use_count = 0;
 	} else if (rdev->use_count > 1) {
-
-		if (rdev->constraints &&
-			(rdev->constraints->valid_ops_mask &
-			REGULATOR_CHANGE_DRMS))
+		if (regulator_ops_is_valid(rdev, REGULATOR_CHANGE_DRMS))
 			drms_uA_update(rdev);
 
 		rdev->use_count--;
@@ -2489,8 +2461,7 @@ int regulator_can_change_voltage(struct regulator *regulator)
 {
 	struct regulator_dev	*rdev = regulator->rdev;
 
-	if (rdev->constraints &&
-	    (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)) {
+	if (regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE)) {
 		if (rdev->desc->n_voltages - rdev->desc->linear_min_sel > 1)
 			return 1;
 
@@ -2644,7 +2615,7 @@ int regulator_is_supported_voltage(struct regulator *regulator,
 	int i, voltages, ret;
 
 	/* If we can't change voltage check the current voltage */
-	if (!(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)) {
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE)) {
 		ret = regulator_get_voltage(regulator);
 		if (ret >= 0)
 			return min_uV <= ret && ret <= max_uV;
@@ -2850,7 +2821,7 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 	 * return successfully even though the regulator does not support
 	 * changing the voltage.
 	 */
-	if (!(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)) {
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE)) {
 		current_uV = _regulator_get_voltage(rdev);
 		if (min_uV <= current_uV && current_uV <= max_uV) {
 			regulator->min_uV = min_uV;
@@ -3365,8 +3336,7 @@ int regulator_allow_bypass(struct regulator *regulator, bool enable)
 	if (!rdev->desc->ops->set_bypass)
 		return 0;
 
-	if (rdev->constraints &&
-	    !(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_BYPASS))
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_BYPASS))
 		return 0;
 
 	mutex_lock(&rdev->mutex);
@@ -4386,7 +4356,7 @@ static int __init regulator_late_cleanup(struct device *dev, void *data)
 	if (c && c->always_on)
 		return 0;
 
-	if (c && !(c->valid_ops_mask & REGULATOR_CHANGE_STATUS))
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS))
 		return 0;
 
 	mutex_lock(&rdev->mutex);
-- 
1.9.1

[RFC PATCH 2/2] regulator: add boot protection flag

[WEN Pingbo]

In some platform, some critical shared regulator is initialized before
kernel loading. But in kernel booting, the driver probing order and
conflict operation from other regulator consumer, may set the regulator
in a undefined state, which will cause serious problem.

This patch try to add a boot_protection flag in regulator constraints.
So the regulator core will prevent the specified operation during kernel
booting.

The boot_protection flag only work before late_initicall. And as other
constraints liked, you can specify this flag in a board file, or in
dts file. By default, all operations of this regulator will be rejected
during kernel booting, if you add this flag in a regulator. But you
still have a chance to change this, by modifying boot_valid_ops_mask.

[ This patch depends on regulator_ops_is_valid patch. And some document
need to add, but I want to hear some voice first. ]

Signed-off-by: WEN Pingbo <pingbo.wen@linaro.org>
---
 drivers/regulator/core.c          | 24 +++++++++++++++++++++---
 drivers/regulator/of_regulator.c  | 29 +++++++++++++++++++++++++++++
 include/linux/regulator/machine.h |  2 ++
 3 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index fe47d38..5b9dc22 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -55,6 +55,7 @@ static LIST_HEAD(regulator_map_list);
 static LIST_HEAD(regulator_ena_gpio_list);
 static LIST_HEAD(regulator_supply_alias_list);
 static bool has_full_constraints;
+static bool regulator_has_booted;
 
 static struct dentry *debugfs_root;
 
@@ -139,7 +140,15 @@ static bool regulator_ops_is_valid(struct regulator_dev *rdev, int ops)
 		return false;
 	}
 
-	if (rdev->constraints->valid_ops_mask & ops)
+	/*
+	 * Ignore regulator boot-protection, after later_initcall.
+	 */
+	if (!regulator_has_booted && rdev->constraints->boot_protection) {
+		if (rdev->constraints->boot_valid_ops_mask & ops)
+			return true;
+		else
+			rdev_info(rdev, "rejected operation 0x%02x\n", ops);
+	} else if (rdev->constraints->valid_ops_mask & ops)
 		return true;
 
 	return false;
@@ -868,7 +877,7 @@ static void print_constraints(struct regulator_dev *rdev)
 	rdev_dbg(rdev, "%s\n", buf);
 
 	if ((constraints->min_uV != constraints->max_uV) &&
-	    !regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE))
+	    !(constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE))
 		rdev_warn(rdev,
 			  "Voltage range but no REGULATOR_CHANGE_VOLTAGE\n");
 }
@@ -1309,7 +1318,8 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 	 * it is then we don't need to do nearly so much work for
 	 * enable/disable calls.
 	 */
-	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS) &&
+	if (rdev->constraints &&
+	    !(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_STATUS) &&
 	    _regulator_is_enabled(rdev))
 		regulator->always_on = true;
 
@@ -4353,6 +4363,12 @@ static int __init regulator_late_cleanup(struct device *dev, void *data)
 	struct regulation_constraints *c = rdev->constraints;
 	int enabled, ret;
 
+	/*
+	 * The kernel boot is finished, let's unset boot_protection
+	 * Need a lock?
+	 */
+	c->boot_protection = 0;
+
 	if (c && c->always_on)
 		return 0;
 
@@ -4406,6 +4422,8 @@ static int __init regulator_init_complete(void)
 	if (of_have_populated_dt())
 		has_full_constraints = true;
 
+	regulator_has_booted = true;
+
 	/* If we have a full configuration then disable any regulators
 	 * we have permission to change the status for and which are
 	 * not in use or always_on.  This is effectively the default
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 6b0aa80..bfec59c 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -78,6 +78,35 @@ static void of_get_regulation_constraints(struct device_node *np,
 	if (of_property_read_bool(np, "regulator-allow-set-load"))
 		constraints->valid_ops_mask |= REGULATOR_CHANGE_DRMS;
 
+	constraints->boot_protection = of_property_read_bool(np,
+			"regulator-boot-protection");
+
+	if (constraints->boot_protection) {
+		if (of_property_read_bool(np, "boot-allow-set-voltage"))
+			constraints->boot_valid_ops_mask |=
+				REGULATOR_CHANGE_VOLTAGE;
+		if (of_property_read_bool(np, "boot-allow-set-current"))
+			constraints->boot_valid_ops_mask |=
+				REGULATOR_CHANGE_CURRENT;
+		if (of_property_read_bool(np, "boot-allow-set-mode"))
+			constraints->boot_valid_ops_mask |=
+				REGULATOR_CHANGE_MODE;
+		if (of_property_read_bool(np, "boot-allow-set-status"))
+			constraints->boot_valid_ops_mask |=
+				REGULATOR_CHANGE_STATUS;
+		if (of_property_read_bool(np, "boot-allow-set-load"))
+			constraints->boot_valid_ops_mask |=
+				REGULATOR_CHANGE_DRMS;
+		if (of_property_read_bool(np, "boot-allow-bypass"))
+			constraints->boot_valid_ops_mask |=
+				REGULATOR_CHANGE_BYPASS;
+
+		/*
+		 * boot_valid_ops_mask is a subset of valid_ops_mask
+		 */
+		constraints->boot_valid_ops_mask &= constraints->valid_ops_mask;
+	}
+
 	ret = of_property_read_u32(np, "regulator-ramp-delay", &pval);
 	if (!ret) {
 		if (pval)
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index 5d627c8..a4f5c0f 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -134,6 +134,7 @@ struct regulation_constraints {
 
 	/* valid operations for regulator on this machine */
 	unsigned int valid_ops_mask;
+	unsigned int boot_valid_ops_mask;
 
 	/* regulator input voltage - only if supply is another regulator */
 	int input_uV;
@@ -155,6 +156,7 @@ struct regulation_constraints {
 	/* constraint flags */
 	unsigned always_on:1;	/* regulator never off when system is on */
 	unsigned boot_on:1;	/* bootloader/firmware enabled regulator */
+	unsigned boot_protection:1;
 	unsigned apply_uV:1;	/* apply uV constraint if min == max */
 	unsigned ramp_disable:1; /* disable ramp delay */
 	unsigned soft_start:1;	/* ramp voltage slowly */
-- 
1.9.1

Re: [RFC PATCH 2/2] regulator: add boot protection flag

[Mark Brown]

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

On Sat, Apr 23, 2016 at 03:11:06PM +0800, WEN Pingbo wrote:

> This patch try to add a boot_protection flag in regulator constraints.
> So the regulator core will prevent the specified operation during kernel
> booting.

> The boot_protection flag only work before late_initicall. And as other
> constraints liked, you can specify this flag in a board file, or in
> dts file. By default, all operations of this regulator will be rejected
> during kernel booting, if you add this flag in a regulator. But you
> still have a chance to change this, by modifying boot_valid_ops_mask.

This is still a complete hack which is going to break as soon as things
are built modular, it's definitely *not* something that should ever
appear in DT since it depends so heavily on implementation details.  If
you need some driver to start early work on getting that sorted.

This is also going to interact badly with any other drivers that are
trying to configure things at runtime, if they've done enables and
disables (or especially an enable without a matching disable) their
refcounts are going to be wrong and if they've tried to do anything with
setting voltages we'll have completely ignored whatever they asked for
or told them that they can't change voltages.  If we were doing anything
like this it would need to be a lot more transparent to other
regulators sharing the supplies (which are presumably what's causing
problems here).

> [ This patch depends on regulator_ops_is_valid patch. And some document
> need to add, but I want to hear some voice first. ]

There is no need to say that patch 2 in a series depends on patch 1.

> @@ -868,7 +877,7 @@ static void print_constraints(struct regulator_dev *rdev)
>  	rdev_dbg(rdev, "%s\n", buf);
>  
>  	if ((constraints->min_uV != constraints->max_uV) &&
> -	    !regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE))
> +	    !(constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE))
>  		rdev_warn(rdev,
>  			  "Voltage range but no REGULATOR_CHANGE_VOLTAGE\n");
>  }

This appears to be unrelated?

> +	if (constraints->boot_protection) {
> +		if (of_property_read_bool(np, "boot-allow-set-voltage"))
> +			constraints->boot_valid_ops_mask |=
> +				REGULATOR_CHANGE_VOLTAGE;

We were factoring things out a minute ago...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Applied "regulator: refactor valid_ops_mask checking code" to the regulator tree

[Mark Brown]

The patch

   regulator: refactor valid_ops_mask checking code

has been applied to the regulator tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 8a34e979f684aa13e6c4bf23b394cca9dfabf4a9 Mon Sep 17 00:00:00 2001
From: WEN Pingbo <pingbo.wen@linaro.org>
Date: Sat, 23 Apr 2016 15:11:05 +0800
Subject: [PATCH] regulator: refactor valid_ops_mask checking code

To make the code more compat and centralized, this patch add a
unified function - regulator_ops_is_valid. So we can add
some extra checking code easily later.

Signed-off-by: WEN Pingbo <pingbo.wen@linaro.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/core.c | 88 ++++++++++++++++--------------------------------
 1 file changed, 29 insertions(+), 59 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 18dd7ee61455..bca9167d8c43 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -132,6 +132,19 @@ static bool have_full_constraints(void)
 	return has_full_constraints || of_have_populated_dt();
 }
 
+static bool regulator_ops_is_valid(struct regulator_dev *rdev, int ops)
+{
+	if (!rdev->constraints) {
+		rdev_err(rdev, "no constraints\n");
+		return false;
+	}
+
+	if (rdev->constraints->valid_ops_mask & ops)
+		return true;
+
+	return false;
+}
+
 static inline struct regulator_dev *rdev_get_supply(struct regulator_dev *rdev)
 {
 	if (rdev && rdev->supply)
@@ -198,28 +211,13 @@ static struct device_node *of_get_regulator(struct device *dev, const char *supp
 	return regnode;
 }
 
-static int _regulator_can_change_status(struct regulator_dev *rdev)
-{
-	if (!rdev->constraints)
-		return 0;
-
-	if (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_STATUS)
-		return 1;
-	else
-		return 0;
-}
-
 /* Platform voltage constraint check */
 static int regulator_check_voltage(struct regulator_dev *rdev,
 				   int *min_uV, int *max_uV)
 {
 	BUG_ON(*min_uV > *max_uV);
 
-	if (!rdev->constraints) {
-		rdev_err(rdev, "no constraints\n");
-		return -ENODEV;
-	}
-	if (!(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)) {
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE)) {
 		rdev_err(rdev, "voltage operation not allowed\n");
 		return -EPERM;
 	}
@@ -275,11 +273,7 @@ static int regulator_check_current_limit(struct regulator_dev *rdev,
 {
 	BUG_ON(*min_uA > *max_uA);
 
-	if (!rdev->constraints) {
-		rdev_err(rdev, "no constraints\n");
-		return -ENODEV;
-	}
-	if (!(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_CURRENT)) {
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_CURRENT)) {
 		rdev_err(rdev, "current operation not allowed\n");
 		return -EPERM;
 	}
@@ -312,11 +306,7 @@ static int regulator_mode_constrain(struct regulator_dev *rdev, int *mode)
 		return -EINVAL;
 	}
 
-	if (!rdev->constraints) {
-		rdev_err(rdev, "no constraints\n");
-		return -ENODEV;
-	}
-	if (!(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_MODE)) {
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_MODE)) {
 		rdev_err(rdev, "mode operation not allowed\n");
 		return -EPERM;
 	}
@@ -333,20 +323,6 @@ static int regulator_mode_constrain(struct regulator_dev *rdev, int *mode)
 	return -EINVAL;
 }
 
-/* dynamic regulator mode switching constraint check */
-static int regulator_check_drms(struct regulator_dev *rdev)
-{
-	if (!rdev->constraints) {
-		rdev_err(rdev, "no constraints\n");
-		return -ENODEV;
-	}
-	if (!(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS)) {
-		rdev_dbg(rdev, "drms operation not allowed\n");
-		return -EPERM;
-	}
-	return 0;
-}
-
 static ssize_t regulator_uV_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
@@ -692,8 +668,7 @@ static int drms_uA_update(struct regulator_dev *rdev)
 	 * first check to see if we can set modes at all, otherwise just
 	 * tell the consumer everything is OK.
 	 */
-	err = regulator_check_drms(rdev);
-	if (err < 0)
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_DRMS))
 		return 0;
 
 	if (!rdev->desc->ops->get_optimum_mode &&
@@ -893,7 +868,7 @@ static void print_constraints(struct regulator_dev *rdev)
 	rdev_dbg(rdev, "%s\n", buf);
 
 	if ((constraints->min_uV != constraints->max_uV) &&
-	    !(constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE))
+	    !regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE))
 		rdev_warn(rdev,
 			  "Voltage range but no REGULATOR_CHANGE_VOLTAGE\n");
 }
@@ -1354,7 +1329,7 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 	 * it is then we don't need to do nearly so much work for
 	 * enable/disable calls.
 	 */
-	if (!_regulator_can_change_status(rdev) &&
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS) &&
 	    _regulator_is_enabled(rdev))
 		regulator->always_on = true;
 
@@ -2131,15 +2106,15 @@ static int _regulator_enable(struct regulator_dev *rdev)
 	lockdep_assert_held_once(&rdev->mutex);
 
 	/* check voltage and requested load before enabling */
-	if (rdev->constraints &&
-	    (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_DRMS))
+	if (regulator_ops_is_valid(rdev, REGULATOR_CHANGE_DRMS))
 		drms_uA_update(rdev);
 
 	if (rdev->use_count == 0) {
 		/* The regulator may on if it's not switchable or left on */
 		ret = _regulator_is_enabled(rdev);
 		if (ret == -EINVAL || ret == 0) {
-			if (!_regulator_can_change_status(rdev))
+			if (!regulator_ops_is_valid(rdev,
+					REGULATOR_CHANGE_STATUS))
 				return -EPERM;
 
 			ret = _regulator_do_enable(rdev);
@@ -2241,7 +2216,7 @@ static int _regulator_disable(struct regulator_dev *rdev)
 	    (rdev->constraints && !rdev->constraints->always_on)) {
 
 		/* we are last user */
-		if (_regulator_can_change_status(rdev)) {
+		if (regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS)) {
 			ret = _notifier_call_chain(rdev,
 						   REGULATOR_EVENT_PRE_DISABLE,
 						   NULL);
@@ -2262,10 +2237,7 @@ static int _regulator_disable(struct regulator_dev *rdev)
 
 		rdev->use_count = 0;
 	} else if (rdev->use_count > 1) {
-
-		if (rdev->constraints &&
-			(rdev->constraints->valid_ops_mask &
-			REGULATOR_CHANGE_DRMS))
+		if (regulator_ops_is_valid(rdev, REGULATOR_CHANGE_DRMS))
 			drms_uA_update(rdev);
 
 		rdev->use_count--;
@@ -2509,8 +2481,7 @@ int regulator_can_change_voltage(struct regulator *regulator)
 {
 	struct regulator_dev	*rdev = regulator->rdev;
 
-	if (rdev->constraints &&
-	    (rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)) {
+	if (regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE)) {
 		if (rdev->desc->n_voltages - rdev->desc->linear_min_sel > 1)
 			return 1;
 
@@ -2664,7 +2635,7 @@ int regulator_is_supported_voltage(struct regulator *regulator,
 	int i, voltages, ret;
 
 	/* If we can't change voltage check the current voltage */
-	if (!(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)) {
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE)) {
 		ret = regulator_get_voltage(regulator);
 		if (ret >= 0)
 			return min_uV <= ret && ret <= max_uV;
@@ -2870,7 +2841,7 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 	 * return successfully even though the regulator does not support
 	 * changing the voltage.
 	 */
-	if (!(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE)) {
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE)) {
 		current_uV = _regulator_get_voltage(rdev);
 		if (min_uV <= current_uV && current_uV <= max_uV) {
 			regulator->min_uV = min_uV;
@@ -3385,8 +3356,7 @@ int regulator_allow_bypass(struct regulator *regulator, bool enable)
 	if (!rdev->desc->ops->set_bypass)
 		return 0;
 
-	if (rdev->constraints &&
-	    !(rdev->constraints->valid_ops_mask & REGULATOR_CHANGE_BYPASS))
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_BYPASS))
 		return 0;
 
 	mutex_lock(&rdev->mutex);
@@ -4406,7 +4376,7 @@ static int __init regulator_late_cleanup(struct device *dev, void *data)
 	if (c && c->always_on)
 		return 0;
 
-	if (c && !(c->valid_ops_mask & REGULATOR_CHANGE_STATUS))
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS))
 		return 0;
 
 	mutex_lock(&rdev->mutex);
-- 
2.8.0.rc3

Re: [RFC PATCH 2/2] regulator: add boot protection flag

[Pingbo Wen]

Hi, Mark

> 在 2016年4月25日，06:51，Mark Brown <broonie@kernel.org> 写道：
> 
> On Sat, Apr 23, 2016 at 03:11:06PM +0800, WEN Pingbo wrote:
> 
>> This patch try to add a boot_protection flag in regulator constraints.
>> So the regulator core will prevent the specified operation during kernel
>> booting.
> 
>> The boot_protection flag only work before late_initicall. And as other
>> constraints liked, you can specify this flag in a board file, or in
>> dts file. By default, all operations of this regulator will be rejected
>> during kernel booting, if you add this flag in a regulator. But you
>> still have a chance to change this, by modifying boot_valid_ops_mask.
> 
> This is still a complete hack which is going to break as soon as things
> are built modular, it's definitely *not* something that should ever
> appear in DT since it depends so heavily on implementation details.  If
> you need some driver to start early work on getting that sorted.
> 

I think this patch can handle the case you mentioned. I have add a
regulator_has_booted flag, and it will set in regulator_init_complete()
late_initcall hook. The regulator_ops_is_valid() will ignore boot
protection if this flag is set.

> This is also going to interact badly with any other drivers that are
> trying to configure things at runtime, if they've done enables and
> disables (or especially an enable without a matching disable) their
> refcounts are going to be wrong and if they've tried to do anything with
> setting voltages we'll have completely ignored whatever they asked for
> or told them that they can't change voltages.  If we were doing anything
> like this it would need to be a lot more transparent to other
> regulators sharing the supplies (which are presumably what's causing
> problems here).

Ok, I have to admit that the boot_protection didn’t cover this. If other
consumer try to configure during booting, it will get some error code.
And the consumers need to re-configure the regulator state after
late_initcall.

If we need to hold the state of other consumer, I prefer using a
dummy-consumer to hold this. And this is my next try.

> 
>> @@ -868,7 +877,7 @@ static void print_constraints(struct regulator_dev *rdev)
>> 	rdev_dbg(rdev, "%s\n", buf);
>> 
>> 	if ((constraints->min_uV != constraints->max_uV) &&
>> -	    !regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE))
>> +	    !(constraints->valid_ops_mask & REGULATOR_CHANGE_VOLTAGE))
>> 		rdev_warn(rdev,
>> 			  "Voltage range but no REGULATOR_CHANGE_VOLTAGE\n");
>> }
> 
> This appears to be unrelated?
> 
>> +	if (constraints->boot_protection) {
>> +		if (of_property_read_bool(np, "boot-allow-set-voltage"))
>> +			constraints->boot_valid_ops_mask |=
>> +				REGULATOR_CHANGE_VOLTAGE;
> 
> We were factoring things out a minute ago…

Actually, the two part are only want to check the regulator operation
capacity, if we include the boot_protect checking, it will get error.

Pingbo

Re: [RFC PATCH 2/2] regulator: add boot protection flag

[Mark Brown]

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

On Tue, Apr 26, 2016 at 07:46:07PM +0800, Pingbo Wen wrote:

> > This is still a complete hack which is going to break as soon as things
> > are built modular, it's definitely *not* something that should ever
> > appear in DT since it depends so heavily on implementation details.  If
> > you need some driver to start early work on getting that sorted.

> I think this patch can handle the case you mentioned. I have add a
> regulator_has_booted flag, and it will set in regulator_init_complete()
> late_initcall hook. The regulator_ops_is_valid() will ignore boot
> protection if this flag is set.

That doesn't help after we get to userspace so doesn't work as soon as
things are built as modules.  That's a key issue with making something
that's robust here.

> > This is also going to interact badly with any other drivers that are
> > trying to configure things at runtime, if they've done enables and
> > disables (or especially an enable without a matching disable) their

> Ok, I have to admit that the boot_protection didn’t cover this. If other
> consumer try to configure during booting, it will get some error code.
> And the consumers need to re-configure the regulator state after
> late_initcall.

The worst thing is where we silently accept but forget about things
because the code currently translates them into valid noops then later
start paying attention to the operations, error codes at least the other
driver can handle.

> If we need to hold the state of other consumer, I prefer using a
> dummy-consumer to hold this. And this is my next try.

Or possibly store the data on the consumer but don't act on it then go
round actually acting on it when we decide to do things - that's more
what I'd have expected and seems like it might be easier than creating a
separate object.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]