[PATCH] regulator: core bugfix: Use normal enable for always_on regulators

[PATCH] regulator: core bugfix: Use normal enable for always_on regulators

[Markus Pargmann]

always_on regulators are currently enabled at registration using the
enable function of struct regulator_ops.

The regulator framework does support gpios to enable/disable regulators.
gpios are not handled through struct regulator_ops functions as they are
a basic component of the framework. Instead they are handled within
_regulator_do_enable(). Unfortunately always-on regulator do not use
this function, to gpio-controlled always-on regulators which are not
enabled on boot. Furthermore they are disabled the whole time because
regulator_enable() does not call _regulator_do_enable() for always-on
regulator.

This patch seperates _regulator_do_enable_no_delay from the
_regulator_do_enable function to be able to call the enable function
without delays. It then fixes the always_on behaviour by using this
function.

Cc: stable@vger.kernel.org
Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/regulator/core.c | 45 +++++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 16a309e..8469244 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -953,6 +953,8 @@ static int machine_constraints_current(struct regulator_dev *rdev,
 	return 0;
 }
 
+static int _regulator_do_enable_no_delay(struct regulator_dev *rdev);
+
 /**
  * set_machine_constraints - sets regulator constraints
  * @rdev: regulator source
@@ -1013,9 +1015,8 @@ static int set_machine_constraints(struct regulator_dev *rdev,
 	/* If the constraints say the regulator should be on at this point
 	 * and we have control then make sure it is enabled.
 	 */
-	if ((rdev->constraints->always_on || rdev->constraints->boot_on) &&
-	    ops->enable) {
-		ret = ops->enable(rdev);
+	if (rdev->constraints->always_on || rdev->constraints->boot_on) {
+		ret = _regulator_do_enable_no_delay(rdev);
 		if (ret < 0) {
 			rdev_err(rdev, "failed to enable\n");
 			goto out;
@@ -1745,6 +1746,24 @@ static int regulator_ena_gpio_ctrl(struct regulator_dev *rdev, bool enable)
 	return 0;
 }
 
+static int _regulator_do_enable_no_delay(struct regulator_dev *rdev)
+{
+	int ret;
+
+	if (rdev->ena_pin) {
+		ret = regulator_ena_gpio_ctrl(rdev, true);
+		if (ret < 0)
+			return ret;
+		rdev->ena_gpio_state = 1;
+	} else if (rdev->desc->ops->enable) {
+		ret = rdev->desc->ops->enable(rdev);
+	} else {
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
 static int _regulator_do_enable(struct regulator_dev *rdev)
 {
 	int ret, delay;
@@ -1760,18 +1779,9 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 
 	trace_regulator_enable(rdev_get_name(rdev));
 
-	if (rdev->ena_pin) {
-		ret = regulator_ena_gpio_ctrl(rdev, true);
-		if (ret < 0)
-			return ret;
-		rdev->ena_gpio_state = 1;
-	} else if (rdev->desc->ops->enable) {
-		ret = rdev->desc->ops->enable(rdev);
-		if (ret < 0)
-			return ret;
-	} else {
-		return -EINVAL;
-	}
+	ret = _regulator_do_enable_no_delay(rdev);
+	if (ret)
+		return ret;
 
 	/* Allow the regulator to ramp; it would be useful to extend
 	 * this for bulk operations so that the regulators can ramp
@@ -3633,9 +3643,8 @@ int regulator_suspend_finish(void)
 		struct regulator_ops *ops = rdev->desc->ops;
 
 		mutex_lock(&rdev->mutex);
-		if ((rdev->use_count > 0  || rdev->constraints->always_on) &&
-				ops->enable) {
-			error = ops->enable(rdev);
+		if (rdev->use_count > 0  || rdev->constraints->always_on) {
+			error = _regulator_do_enable_no_delay(rdev);
 			if (error)
 				ret = error;
 		} else {
-- 
1.8.5.3

[PATCH] regulator: core bugfix: Use normal enable for always_on regulators

[Markus Pargmann]

always_on regulators are currently enabled at registration using the
enable function of struct regulator_ops.

The regulator framework does support gpios to enable/disable regulators.
gpios are not handled through struct regulator_ops functions as they are
a basic component of the framework. Instead they are handled within
_regulator_do_enable(). Unfortunately always-on regulator do not use
this function, to gpio-controlled always-on regulators which are not
enabled on boot. Furthermore they are disabled the whole time because
regulator_enable() does not call _regulator_do_enable() for always-on
regulator.

This patch seperates _regulator_do_enable_no_delay from the
_regulator_do_enable function to be able to call the enable function
without delays. It then fixes the always_on behaviour by using this
function.

Cc: stable at vger.kernel.org
Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/regulator/core.c | 45 +++++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 16a309e..8469244 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -953,6 +953,8 @@ static int machine_constraints_current(struct regulator_dev *rdev,
 	return 0;
 }
 
+static int _regulator_do_enable_no_delay(struct regulator_dev *rdev);
+
 /**
  * set_machine_constraints - sets regulator constraints
  * @rdev: regulator source
@@ -1013,9 +1015,8 @@ static int set_machine_constraints(struct regulator_dev *rdev,
 	/* If the constraints say the regulator should be on at this point
 	 * and we have control then make sure it is enabled.
 	 */
-	if ((rdev->constraints->always_on || rdev->constraints->boot_on) &&
-	    ops->enable) {
-		ret = ops->enable(rdev);
+	if (rdev->constraints->always_on || rdev->constraints->boot_on) {
+		ret = _regulator_do_enable_no_delay(rdev);
 		if (ret < 0) {
 			rdev_err(rdev, "failed to enable\n");
 			goto out;
@@ -1745,6 +1746,24 @@ static int regulator_ena_gpio_ctrl(struct regulator_dev *rdev, bool enable)
 	return 0;
 }
 
+static int _regulator_do_enable_no_delay(struct regulator_dev *rdev)
+{
+	int ret;
+
+	if (rdev->ena_pin) {
+		ret = regulator_ena_gpio_ctrl(rdev, true);
+		if (ret < 0)
+			return ret;
+		rdev->ena_gpio_state = 1;
+	} else if (rdev->desc->ops->enable) {
+		ret = rdev->desc->ops->enable(rdev);
+	} else {
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
 static int _regulator_do_enable(struct regulator_dev *rdev)
 {
 	int ret, delay;
@@ -1760,18 +1779,9 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 
 	trace_regulator_enable(rdev_get_name(rdev));
 
-	if (rdev->ena_pin) {
-		ret = regulator_ena_gpio_ctrl(rdev, true);
-		if (ret < 0)
-			return ret;
-		rdev->ena_gpio_state = 1;
-	} else if (rdev->desc->ops->enable) {
-		ret = rdev->desc->ops->enable(rdev);
-		if (ret < 0)
-			return ret;
-	} else {
-		return -EINVAL;
-	}
+	ret = _regulator_do_enable_no_delay(rdev);
+	if (ret)
+		return ret;
 
 	/* Allow the regulator to ramp; it would be useful to extend
 	 * this for bulk operations so that the regulators can ramp
@@ -3633,9 +3643,8 @@ int regulator_suspend_finish(void)
 		struct regulator_ops *ops = rdev->desc->ops;
 
 		mutex_lock(&rdev->mutex);
-		if ((rdev->use_count > 0  || rdev->constraints->always_on) &&
-				ops->enable) {
-			error = ops->enable(rdev);
+		if (rdev->use_count > 0  || rdev->constraints->always_on) {
+			error = _regulator_do_enable_no_delay(rdev);
 			if (error)
 				ret = error;
 		} else {
-- 
1.8.5.3

Re: [PATCH] regulator: core bugfix: Use normal enable for always_on regulators

[Mark Brown]

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

On Sun, Feb 16, 2014 at 08:00:56PM +0100, Markus Pargmann wrote:

Please use more standard subject lines, don't do things like "core
bugfix", just write a normal changelog.

> +static int _regulator_do_enable_no_delay(struct regulator_dev *rdev)
> +{
> +	int ret;
> +
> +	if (rdev->ena_pin) {
> +		ret = regulator_ena_gpio_ctrl(rdev, true);
> +		if (ret < 0)
> +			return ret;
> +		rdev->ena_gpio_state = 1;
> +	} else if (rdev->desc->ops->enable) {
> +		ret = rdev->desc->ops->enable(rdev);
> +	} else {
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}

I don't understand this.  Why is this called _no_delay() and why don't
we want to delay when applying constraints?  We don't want to ever be in
a position where we think a supply is enabled but it has in fact not
finished ramping, and of course enable() may in fact be blocking anyway.

The use of the common code to do enable is good fix but this just seems
odd.

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

[PATCH] regulator: core bugfix: Use normal enable for always_on regulators

[Mark Brown]

On Sun, Feb 16, 2014 at 08:00:56PM +0100, Markus Pargmann wrote:

Please use more standard subject lines, don't do things like "core
bugfix", just write a normal changelog.

> +static int _regulator_do_enable_no_delay(struct regulator_dev *rdev)
> +{
> +	int ret;
> +
> +	if (rdev->ena_pin) {
> +		ret = regulator_ena_gpio_ctrl(rdev, true);
> +		if (ret < 0)
> +			return ret;
> +		rdev->ena_gpio_state = 1;
> +	} else if (rdev->desc->ops->enable) {
> +		ret = rdev->desc->ops->enable(rdev);
> +	} else {
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}

I don't understand this.  Why is this called _no_delay() and why don't
we want to delay when applying constraints?  We don't want to ever be in
a position where we think a supply is enabled but it has in fact not
finished ramping, and of course enable() may in fact be blocking anyway.

The use of the common code to do enable is good fix but this just seems
odd.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140218/8af22c6e/attachment.sig>

Re: [PATCH] regulator: core bugfix: Use normal enable for always_on regulators

[Markus Pargmann]

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

On Tue, Feb 18, 2014 at 09:14:20AM +0900, Mark Brown wrote:
> On Sun, Feb 16, 2014 at 08:00:56PM +0100, Markus Pargmann wrote:
> 
> Please use more standard subject lines, don't do things like "core
> bugfix", just write a normal changelog.

Okay, will fix.

> 
> > +static int _regulator_do_enable_no_delay(struct regulator_dev *rdev)
> > +{
> > +	int ret;
> > +
> > +	if (rdev->ena_pin) {
> > +		ret = regulator_ena_gpio_ctrl(rdev, true);
> > +		if (ret < 0)
> > +			return ret;
> > +		rdev->ena_gpio_state = 1;
> > +	} else if (rdev->desc->ops->enable) {
> > +		ret = rdev->desc->ops->enable(rdev);
> > +	} else {
> > +		ret = -EINVAL;
> > +	}
> > +
> > +	return ret;
> > +}
> 
> I don't understand this.  Why is this called _no_delay() and why don't
> we want to delay when applying constraints?  We don't want to ever be in
> a position where we think a supply is enabled but it has in fact not
> finished ramping, and of course enable() may in fact be blocking anyway.

I tried not to modify the current behaviour of the core driver for
non-gpio regulators. Before this patch only ops->enable() was called
which also didn't have a delay. So I seperated the non-delay enable
function to have the same behaviour for normal regulators.

Also the constraints are applied when registering a new regulator. For
"boot-on" we should not delay because this regulator is already on by
definition. But I am not sure what to do with always-on regulators?

Thanks,

Markus


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[PATCH] regulator: core bugfix: Use normal enable for always_on regulators

[Markus Pargmann]

On Tue, Feb 18, 2014 at 09:14:20AM +0900, Mark Brown wrote:
> On Sun, Feb 16, 2014 at 08:00:56PM +0100, Markus Pargmann wrote:
> 
> Please use more standard subject lines, don't do things like "core
> bugfix", just write a normal changelog.

Okay, will fix.

> 
> > +static int _regulator_do_enable_no_delay(struct regulator_dev *rdev)
> > +{
> > +	int ret;
> > +
> > +	if (rdev->ena_pin) {
> > +		ret = regulator_ena_gpio_ctrl(rdev, true);
> > +		if (ret < 0)
> > +			return ret;
> > +		rdev->ena_gpio_state = 1;
> > +	} else if (rdev->desc->ops->enable) {
> > +		ret = rdev->desc->ops->enable(rdev);
> > +	} else {
> > +		ret = -EINVAL;
> > +	}
> > +
> > +	return ret;
> > +}
> 
> I don't understand this.  Why is this called _no_delay() and why don't
> we want to delay when applying constraints?  We don't want to ever be in
> a position where we think a supply is enabled but it has in fact not
> finished ramping, and of course enable() may in fact be blocking anyway.

I tried not to modify the current behaviour of the core driver for
non-gpio regulators. Before this patch only ops->enable() was called
which also didn't have a delay. So I seperated the non-delay enable
function to have the same behaviour for normal regulators.

Also the constraints are applied when registering a new regulator. For
"boot-on" we should not delay because this regulator is already on by
definition. But I am not sure what to do with always-on regulators?

Thanks,

Markus


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140218/a448bd84/attachment.sig>

Re: [PATCH] regulator: core bugfix: Use normal enable for always_on regulators

[Mark Brown]

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

On Tue, Feb 18, 2014 at 10:40:07PM +0100, Markus Pargmann wrote:
> On Tue, Feb 18, 2014 at 09:14:20AM +0900, Mark Brown wrote:

> > I don't understand this.  Why is this called _no_delay() and why don't
> > we want to delay when applying constraints?  We don't want to ever be in
> > a position where we think a supply is enabled but it has in fact not
> > finished ramping, and of course enable() may in fact be blocking anyway.

> I tried not to modify the current behaviour of the core driver for
> non-gpio regulators. Before this patch only ops->enable() was called
> which also didn't have a delay. So I seperated the non-delay enable
> function to have the same behaviour for normal regulators.

No, that's not good.  The fact that it wasn't applying delays is going
to be a bug - it should've been doing that.

> Also the constraints are applied when registering a new regulator. For
> "boot-on" we should not delay because this regulator is already on by
> definition. But I am not sure what to do with always-on regulators?

I'd just always apply a delay, it's simpler and more robust.

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

[PATCH] regulator: core bugfix: Use normal enable for always_on regulators

[Mark Brown]

On Tue, Feb 18, 2014 at 10:40:07PM +0100, Markus Pargmann wrote:
> On Tue, Feb 18, 2014 at 09:14:20AM +0900, Mark Brown wrote:

> > I don't understand this.  Why is this called _no_delay() and why don't
> > we want to delay when applying constraints?  We don't want to ever be in
> > a position where we think a supply is enabled but it has in fact not
> > finished ramping, and of course enable() may in fact be blocking anyway.

> I tried not to modify the current behaviour of the core driver for
> non-gpio regulators. Before this patch only ops->enable() was called
> which also didn't have a delay. So I seperated the non-delay enable
> function to have the same behaviour for normal regulators.

No, that's not good.  The fact that it wasn't applying delays is going
to be a bug - it should've been doing that.

> Also the constraints are applied when registering a new regulator. For
> "boot-on" we should not delay because this regulator is already on by
> definition. But I am not sure what to do with always-on regulators?

I'd just always apply a delay, it's simpler and more robust.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140219/d68d5fd4/attachment.sig>