[PATCH 0/3] Regulator updates for 2.6.34

[PATCH 0/3] Regulator updates for 2.6.34

[Mark Brown]

The following changes since commit 204f78ec75e6888bbf1a7b5a3ed0cbfff9bade27:
  Tobias Klauser (1):
        regulator/lp3971: Storage class should be before const qualifier

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator-2.6.git for-lrg

A couple of new APIs, plus a use of one of them.

Mark Brown (3):
      regulator: Add notifier event on regulator disable
      regulator: Allow regulators to specify the time taken to ramp on enable
      regulator: Implement enable_time() for WM835x ISINKs

 drivers/regulator/core.c             |   48 ++++++++++++++++++++++++++++-----
 drivers/regulator/wm8350-regulator.c |   46 ++++++++++++++++++++++++++++++++
 include/linux/regulator/consumer.h   |    4 ++-
 include/linux/regulator/driver.h     |    6 ++++
 4 files changed, 95 insertions(+), 9 deletions(-)

[PATCH 1/3] regulator: Add notifier event on regulator disable

[Mark Brown]

The intended use case is for drivers which disable regulators to save
power but need to do some work to restore the hardware state when
restarting.  If the supplies are not actually disabled due to board
limits or sharing with other active devices this notifier allows the
driver to avoid unneeded reinitialisation, particularly when used with
runtime PM.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/regulator/core.c           |    7 +++++--
 include/linux/regulator/consumer.h |    4 +++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 686ef27..cce76a8 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1341,6 +1341,9 @@ static int _regulator_disable(struct regulator_dev *rdev)
 				       __func__, rdev_get_name(rdev));
 				return ret;
 			}
+
+			_notifier_call_chain(rdev, REGULATOR_EVENT_DISABLE,
+					     NULL);
 		}
 
 		/* decrease our supplies ref count and disable if required */
@@ -1399,8 +1402,8 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
 			return ret;
 		}
 		/* notify other consumers that power has been forced off */
-		_notifier_call_chain(rdev, REGULATOR_EVENT_FORCE_DISABLE,
-			NULL);
+		_notifier_call_chain(rdev, REGULATOR_EVENT_FORCE_DISABLE |
+			REGULATOR_EVENT_DISABLE, NULL);
 	}
 
 	/* decrease our supplies ref count and disable if required */
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index 030d922..28c9fd0 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -89,8 +89,9 @@
  * REGULATION_OUT Regulator output is out of regulation.
  * FAIL           Regulator output has failed.
  * OVER_TEMP      Regulator over temp.
- * FORCE_DISABLE  Regulator shut down by software.
+ * FORCE_DISABLE  Regulator forcibly shut down by software.
  * VOLTAGE_CHANGE Regulator voltage changed.
+ * DISABLE        Regulator was disabled.
  *
  * NOTE: These events can be OR'ed together when passed into handler.
  */
@@ -102,6 +103,7 @@
 #define REGULATOR_EVENT_OVER_TEMP		0x10
 #define REGULATOR_EVENT_FORCE_DISABLE		0x20
 #define REGULATOR_EVENT_VOLTAGE_CHANGE		0x40
+#define REGULATOR_EVENT_DISABLE 		0x80
 
 struct regulator;
 
-- 
1.6.5.7

Re: [PATCH 1/3] regulator: Add notifier event on regulator disable

[Jeffrey Carlyle]

On Mon, Jan 4, 2010 at 12:17 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> The intended use case is for drivers which disable regulators to save
> power but need to do some work to restore the hardware state when
> restarting.  If the supplies are not actually disabled due to board
> limits or sharing with other active devices this notifier allows the
> driver to avoid unneeded reinitialisation, particularly when used with
> runtime PM.
>
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  drivers/regulator/core.c           |    7 +++++--
>  include/linux/regulator/consumer.h |    4 +++-
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 686ef27..cce76a8 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -1341,6 +1341,9 @@ static int _regulator_disable(struct regulator_dev *rdev)
>                                       __func__, rdev_get_name(rdev));
>                                return ret;
>                        }
> +
> +                       _notifier_call_chain(rdev, REGULATOR_EVENT_DISABLE,
> +                                            NULL);
>                }
>
>                /* decrease our supplies ref count and disable if required */
> @@ -1399,8 +1402,8 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
>                        return ret;
>                }
>                /* notify other consumers that power has been forced off */
> -               _notifier_call_chain(rdev, REGULATOR_EVENT_FORCE_DISABLE,
> -                       NULL);
> +               _notifier_call_chain(rdev, REGULATOR_EVENT_FORCE_DISABLE |
> +                       REGULATOR_EVENT_DISABLE, NULL);
>        }
>
>        /* decrease our supplies ref count and disable if required */
> diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
> index 030d922..28c9fd0 100644
> --- a/include/linux/regulator/consumer.h
> +++ b/include/linux/regulator/consumer.h
> @@ -89,8 +89,9 @@
>  * REGULATION_OUT Regulator output is out of regulation.
>  * FAIL           Regulator output has failed.
>  * OVER_TEMP      Regulator over temp.
> - * FORCE_DISABLE  Regulator shut down by software.
> + * FORCE_DISABLE  Regulator forcibly shut down by software.
>  * VOLTAGE_CHANGE Regulator voltage changed.
> + * DISABLE        Regulator was disabled.
>  *
>  * NOTE: These events can be OR'ed together when passed into handler.
>  */
> @@ -102,6 +103,7 @@
>  #define REGULATOR_EVENT_OVER_TEMP              0x10
>  #define REGULATOR_EVENT_FORCE_DISABLE          0x20
>  #define REGULATOR_EVENT_VOLTAGE_CHANGE         0x40
> +#define REGULATOR_EVENT_DISABLE                0x80
>
>  struct regulator;
>
> --
> 1.6.5.7

Mark et al, sorry to drag up something from nine months ago, but I'm
having a problem with this patch. I have a regulator A that sets
regulator B as its supply. When I call set_supply to add B as the
supply for A, regulator A gets added to the supply_list for regulator
B.

When I call regulator_disable(A), I end up with a call chain like this:

regulator_disable(A)
- mutex_lock(A)
- _regulator_disable(A)
-- _regulator_disable(B)
--- _notifier_call_chain(B)
---- mutex_lock(A)

Which results in dead lock since we trying to acquire the mutex lock
for regulator A which we already hold.

When I call regulator_disable(A), regulator_disable grabs the
mutex_lock for A. Then while inside _regulator_disable,
regulator_disable(B) is called since B is the supply for A. With this
patch, _regulator_disable(B) ends up calling _notifier_call_chain(B).
A is in the supply_list for B, so inside the notifier loop we try to
grab the mutext_lock for A again.

Am I doing something wrong in how I am setting up B as the supply for
A or is this bug in the regulator core code?

Thanks,
 - Jeff

Re: [PATCH 1/3] regulator: Add notifier event on regulator disable

[Mark Brown]

On Fri, Oct 08, 2010 at 08:27:10AM -0500, Jeffrey Carlyle wrote:

> Am I doing something wrong in how I am setting up B as the supply for
> A or is this bug in the regulator core code?

Nothing in mainline chains regulators so this will just have bitrotted.

[PATCH] regulator: avoid deadlock when disabling regulator with supply

[Jeffrey Carlyle]

I have a regulator A that sets regulator B as its supply. When I call
set_supply to add B as the supply for A, regulator A gets added to the
supply_list for regulator B.

When I call regulator_disable(A), I end up with a call chain like this:

regulator_disable(A)
- mutex_lock(A)
- _regulator_disable(A)
-- _regulator_disable(B)
--- _notifier_call_chain(B)
---- mutex_lock(A)

Which results in dead lock since we are trying to acquire the mutex lock
for regulator A which we already hold.

This patch addresses this issue by moving the call to disable regulator
B outside of the lock aquired inside the initial call to
regulator_disable.

This change also addresses the issue of not acquiring the mutex for
regulator B before calling _regulator_disable(B).

Change-Id: I769669452b9e1b59b252298d589738aabb03529c
Signed-off-by: Jeffrey Carlyle <jeff.carlyle@motorola.com>
---
 drivers/regulator/core.c |   38 +++++++++++++++++++++++++++++---------
 1 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index cc8b337..e40e656 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -63,7 +63,8 @@ struct regulator {
 };
 
 static int _regulator_is_enabled(struct regulator_dev *rdev);
-static int _regulator_disable(struct regulator_dev *rdev);
+static int _regulator_disable(struct regulator_dev *rdev,
+		struct regulator_dev **supply_rdev_ptr);
 static int _regulator_get_voltage(struct regulator_dev *rdev);
 static int _regulator_get_current_limit(struct regulator_dev *rdev);
 static unsigned int _regulator_get_mode(struct regulator_dev *rdev);
@@ -1343,12 +1344,14 @@ int regulator_enable(struct regulator *regulator)
 	mutex_lock(&rdev->mutex);
 	ret = _regulator_enable(rdev);
 	mutex_unlock(&rdev->mutex);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_enable);
 
 /* locks held by regulator_disable() */
-static int _regulator_disable(struct regulator_dev *rdev)
+static int _regulator_disable(struct regulator_dev *rdev,
+		struct regulator_dev **supply_rdev_ptr)
 {
 	int ret = 0;
 
@@ -1376,8 +1379,7 @@ static int _regulator_disable(struct regulator_dev *rdev)
 		}
 
 		/* decrease our supplies ref count and disable if required */
-		if (rdev->supply)
-			_regulator_disable(rdev->supply);
+		*supply_rdev_ptr = rdev->supply;
 
 		rdev->use_count = 0;
 	} else if (rdev->use_count > 1) {
@@ -1407,17 +1409,29 @@ static int _regulator_disable(struct regulator_dev *rdev)
 int regulator_disable(struct regulator *regulator)
 {
 	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *supply_rdev = NULL;
 	int ret = 0;
 
 	mutex_lock(&rdev->mutex);
-	ret = _regulator_disable(rdev);
+	ret = _regulator_disable(rdev, &supply_rdev);
 	mutex_unlock(&rdev->mutex);
+
+	/* decrease our supplies ref count and disable if required */
+	while (supply_rdev != NULL) {
+		rdev = supply_rdev;
+
+		mutex_lock(&rdev->mutex);
+		_regulator_disable(rdev, &supply_rdev);
+		mutex_unlock(&rdev->mutex);
+	}
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_disable);
 
 /* locks held by regulator_force_disable() */
-static int _regulator_force_disable(struct regulator_dev *rdev)
+static int _regulator_force_disable(struct regulator_dev *rdev,
+		struct regulator_dev **supply_rdev_ptr)
 {
 	int ret = 0;
 
@@ -1436,8 +1450,7 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
 	}
 
 	/* decrease our supplies ref count and disable if required */
-	if (rdev->supply)
-		_regulator_disable(rdev->supply);
+	*supply_rdev_ptr = rdev->supply;
 
 	rdev->use_count = 0;
 	return ret;
@@ -1454,12 +1467,19 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
  */
 int regulator_force_disable(struct regulator *regulator)
 {
+	struct regulator_dev *supply_rdev = NULL;
 	int ret;
 
 	mutex_lock(&regulator->rdev->mutex);
 	regulator->uA_load = 0;
-	ret = _regulator_force_disable(regulator->rdev);
+	ret = _regulator_force_disable(regulator->rdev, &supply_rdev);
 	mutex_unlock(&regulator->rdev->mutex);
+
+	if (supply_rdev)
+		regulator_disable(
+				get_device_regulator(rdev_get_dev(supply_rdev))
+				);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_force_disable);
-- 
1.6.3.3

Re: [PATCH] regulator: avoid deadlock when disabling regulator with supply

[Mark Brown]

On Fri, Oct 08, 2010 at 01:43:54PM -0500, Jeffrey Carlyle wrote:

> I have a regulator A that sets regulator B as its supply. When I call
> set_supply to add B as the supply for A, regulator A gets added to the
> supply_list for regulator B.

Looks good, thanks!

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

but some stylistic things below:

> Change-Id: I769669452b9e1b59b252298d589738aabb03529c

Hrm?

>  static int _regulator_get_voltage(struct regulator_dev *rdev);
>  static int _regulator_get_current_limit(struct regulator_dev *rdev);
>  static unsigned int _regulator_get_mode(struct regulator_dev *rdev);
> @@ -1343,12 +1344,14 @@ int regulator_enable(struct regulator *regulator)
>  	mutex_lock(&rdev->mutex);
>  	ret = _regulator_enable(rdev);
>  	mutex_unlock(&rdev->mutex);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(regulator_enable);

Random indentation change here.

> +
> +	if (supply_rdev)
> +		regulator_disable(
> +				get_device_regulator(rdev_get_dev(supply_rdev))
> +				);
> +

Eew, indentation!  In cases like this it's better to just ignore the
excesive line length.

[PATCH v2] regulator: avoid deadlock when disabling regulator with supply

[y]

From: Jeffrey Carlyle <jeff.carlyle@motorola.com>

I have a regulator A that sets regulator B as its supply. When I call
set_supply to add B as the supply for A, regulator A gets added to the
supply_list for regulator B.

When I call regulator_disable(A), I end up with a call chain like this:

regulator_disable(A)
> mutex_lock(A)
> _regulator_disable(A)
>> _regulator_disable(B)
>>> _notifier_call_chain(B)
>>>> mutex_lock(A)

Which results in dead lock since we are trying to acquire the mutex lock
for regulator A which we already hold.

This patch addresses this issue by moving the call to disable regulator
B outside of the lock aquired inside the initial call to
regulator_disable.

This change also addresses the issue of not acquiring the mutex for
regulator B before calling _regulator_disable(B).

Signed-off-by: Jeffrey Carlyle <jeff.carlyle@motorola.com>
---
 drivers/regulator/core.c |   35 ++++++++++++++++++++++++++---------
 1 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index cc8b337..2d215b5 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -63,7 +63,8 @@ struct regulator {
 };
 
 static int _regulator_is_enabled(struct regulator_dev *rdev);
-static int _regulator_disable(struct regulator_dev *rdev);
+static int _regulator_disable(struct regulator_dev *rdev,
+		struct regulator_dev **supply_rdev_ptr);
 static int _regulator_get_voltage(struct regulator_dev *rdev);
 static int _regulator_get_current_limit(struct regulator_dev *rdev);
 static unsigned int _regulator_get_mode(struct regulator_dev *rdev);
@@ -1348,7 +1349,8 @@ int regulator_enable(struct regulator *regulator)
 EXPORT_SYMBOL_GPL(regulator_enable);
 
 /* locks held by regulator_disable() */
-static int _regulator_disable(struct regulator_dev *rdev)
+static int _regulator_disable(struct regulator_dev *rdev,
+		struct regulator_dev **supply_rdev_ptr)
 {
 	int ret = 0;
 
@@ -1376,8 +1378,7 @@ static int _regulator_disable(struct regulator_dev *rdev)
 		}
 
 		/* decrease our supplies ref count and disable if required */
-		if (rdev->supply)
-			_regulator_disable(rdev->supply);
+		*supply_rdev_ptr = rdev->supply;
 
 		rdev->use_count = 0;
 	} else if (rdev->use_count > 1) {
@@ -1407,17 +1408,29 @@ static int _regulator_disable(struct regulator_dev *rdev)
 int regulator_disable(struct regulator *regulator)
 {
 	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *supply_rdev = NULL;
 	int ret = 0;
 
 	mutex_lock(&rdev->mutex);
-	ret = _regulator_disable(rdev);
+	ret = _regulator_disable(rdev, &supply_rdev);
 	mutex_unlock(&rdev->mutex);
+
+	/* decrease our supplies ref count and disable if required */
+	while (supply_rdev != NULL) {
+		rdev = supply_rdev;
+
+		mutex_lock(&rdev->mutex);
+		_regulator_disable(rdev, &supply_rdev);
+		mutex_unlock(&rdev->mutex);
+	}
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_disable);
 
 /* locks held by regulator_force_disable() */
-static int _regulator_force_disable(struct regulator_dev *rdev)
+static int _regulator_force_disable(struct regulator_dev *rdev,
+		struct regulator_dev **supply_rdev_ptr)
 {
 	int ret = 0;
 
@@ -1436,8 +1449,7 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
 	}
 
 	/* decrease our supplies ref count and disable if required */
-	if (rdev->supply)
-		_regulator_disable(rdev->supply);
+	*supply_rdev_ptr = rdev->supply;
 
 	rdev->use_count = 0;
 	return ret;
@@ -1454,12 +1466,17 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
  */
 int regulator_force_disable(struct regulator *regulator)
 {
+	struct regulator_dev *supply_rdev = NULL;
 	int ret;
 
 	mutex_lock(&regulator->rdev->mutex);
 	regulator->uA_load = 0;
-	ret = _regulator_force_disable(regulator->rdev);
+	ret = _regulator_force_disable(regulator->rdev, &supply_rdev);
 	mutex_unlock(&regulator->rdev->mutex);
+
+	if (supply_rdev)
+		regulator_disable(get_device_regulator(rdev_get_dev(supply_rdev)));
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_force_disable);
-- 
1.6.3.3

[PATCH v2] regulator: avoid deadlock when disabling regulator with supply

[Jeffrey Carlyle]

I have a regulator A that sets regulator B as its supply. When I call
set_supply to add B as the supply for A, regulator A gets added to the
supply_list for regulator B.

When I call regulator_disable(A), I end up with a call chain like this:

regulator_disable(A)
> mutex_lock(A)
> _regulator_disable(A)
>> _regulator_disable(B)
>>> _notifier_call_chain(B)
>>>> mutex_lock(A)

Which results in dead lock since we are trying to acquire the mutex lock
for regulator A which we already hold.

This patch addresses this issue by moving the call to disable regulator
B outside of the lock aquired inside the initial call to
regulator_disable.

This change also addresses the issue of not acquiring the mutex for
regulator B before calling _regulator_disable(B).

Signed-off-by: Jeffrey Carlyle <jeff.carlyle@motorola.com>
---
 drivers/regulator/core.c |   35 ++++++++++++++++++++++++++---------
 1 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index cc8b337..2d215b5 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -63,7 +63,8 @@ struct regulator {
 };
 
 static int _regulator_is_enabled(struct regulator_dev *rdev);
-static int _regulator_disable(struct regulator_dev *rdev);
+static int _regulator_disable(struct regulator_dev *rdev,
+		struct regulator_dev **supply_rdev_ptr);
 static int _regulator_get_voltage(struct regulator_dev *rdev);
 static int _regulator_get_current_limit(struct regulator_dev *rdev);
 static unsigned int _regulator_get_mode(struct regulator_dev *rdev);
@@ -1348,7 +1349,8 @@ int regulator_enable(struct regulator *regulator)
 EXPORT_SYMBOL_GPL(regulator_enable);
 
 /* locks held by regulator_disable() */
-static int _regulator_disable(struct regulator_dev *rdev)
+static int _regulator_disable(struct regulator_dev *rdev,
+		struct regulator_dev **supply_rdev_ptr)
 {
 	int ret = 0;
 
@@ -1376,8 +1378,7 @@ static int _regulator_disable(struct regulator_dev *rdev)
 		}
 
 		/* decrease our supplies ref count and disable if required */
-		if (rdev->supply)
-			_regulator_disable(rdev->supply);
+		*supply_rdev_ptr = rdev->supply;
 
 		rdev->use_count = 0;
 	} else if (rdev->use_count > 1) {
@@ -1407,17 +1408,29 @@ static int _regulator_disable(struct regulator_dev *rdev)
 int regulator_disable(struct regulator *regulator)
 {
 	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_dev *supply_rdev = NULL;
 	int ret = 0;
 
 	mutex_lock(&rdev->mutex);
-	ret = _regulator_disable(rdev);
+	ret = _regulator_disable(rdev, &supply_rdev);
 	mutex_unlock(&rdev->mutex);
+
+	/* decrease our supplies ref count and disable if required */
+	while (supply_rdev != NULL) {
+		rdev = supply_rdev;
+
+		mutex_lock(&rdev->mutex);
+		_regulator_disable(rdev, &supply_rdev);
+		mutex_unlock(&rdev->mutex);
+	}
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_disable);
 
 /* locks held by regulator_force_disable() */
-static int _regulator_force_disable(struct regulator_dev *rdev)
+static int _regulator_force_disable(struct regulator_dev *rdev,
+		struct regulator_dev **supply_rdev_ptr)
 {
 	int ret = 0;
 
@@ -1436,8 +1449,7 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
 	}
 
 	/* decrease our supplies ref count and disable if required */
-	if (rdev->supply)
-		_regulator_disable(rdev->supply);
+	*supply_rdev_ptr = rdev->supply;
 
 	rdev->use_count = 0;
 	return ret;
@@ -1454,12 +1466,17 @@ static int _regulator_force_disable(struct regulator_dev *rdev)
  */
 int regulator_force_disable(struct regulator *regulator)
 {
+	struct regulator_dev *supply_rdev = NULL;
 	int ret;
 
 	mutex_lock(&regulator->rdev->mutex);
 	regulator->uA_load = 0;
-	ret = _regulator_force_disable(regulator->rdev);
+	ret = _regulator_force_disable(regulator->rdev, &supply_rdev);
 	mutex_unlock(&regulator->rdev->mutex);
+
+	if (supply_rdev)
+		regulator_disable(get_device_regulator(rdev_get_dev(supply_rdev)));
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_force_disable);
-- 
1.6.3.3

Re: [PATCH v2] regulator: avoid deadlock when disabling regulator with supply

[Mark Brown]

On Fri, Oct 08, 2010 at 02:49:19PM -0500, Jeffrey Carlyle wrote:
> I have a regulator A that sets regulator B as its supply. When I call
> set_supply to add B as the supply for A, regulator A gets added to the
> supply_list for regulator B.
> 
> When I call regulator_disable(A), I end up with a call chain like this:

Looks good!

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Re: [PATCH v2] regulator: avoid deadlock when disabling regulator with supply

[Liam Girdwood]

On Fri, 2010-10-08 at 14:10 -0700, Mark Brown wrote:
> On Fri, Oct 08, 2010 at 02:49:19PM -0500, Jeffrey Carlyle wrote:
> > I have a regulator A that sets regulator B as its supply. When I call
> > set_supply to add B as the supply for A, regulator A gets added to the
> > supply_list for regulator B.
> > 
> > When I call regulator_disable(A), I end up with a call chain like this:
> 
> Looks good!
> 
> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Applied.

Thanks

Liam
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

[PATCH 2/3] regulator: Allow regulators to specify the time taken to ramp on enable

[Mark Brown]

Regulators may sometimes take longer to enable than the control operation
used to do so, either because the regulator has ramp rate control used to
limit inrush current or because the control operation is very fast (GPIO
being the most common example of this).  In order to ensure that consumers
do not rely on the regulator before it is enabled provide an enable_time()
operation and have the core delay for that time before returning to the
caller.

This is implemented as a function since the ramp rate may be specified in
voltage per unit time and therefore the time depend on the configuration.
In future it would be desirable to allow the bulk operations to run the
delays for multiple enables in parallel but this is not currently supported.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/regulator/core.c         |   41 ++++++++++++++++++++++++++++++++-----
 include/linux/regulator/driver.h |    6 +++++
 2 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index cce76a8..5a3509b 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -19,6 +19,7 @@
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <linux/suspend.h>
+#include <linux/delay.h>
 #include <linux/regulator/consumer.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
@@ -1084,6 +1085,13 @@ overflow_err:
 	return NULL;
 }
 
+static int _regulator_get_enable_time(struct regulator_dev *rdev)
+{
+	if (!rdev->desc->ops->enable_time)
+		return 0;
+	return rdev->desc->ops->enable_time(rdev);
+}
+
 /* Internal regulator request function */
 static struct regulator *_regulator_get(struct device *dev, const char *id,
 					int exclusive)
@@ -1251,7 +1259,7 @@ static int _regulator_can_change_status(struct regulator_dev *rdev)
 /* locks held by regulator_enable() */
 static int _regulator_enable(struct regulator_dev *rdev)
 {
-	int ret;
+	int ret, delay;
 
 	/* do we need to enable the supply regulator first */
 	if (rdev->supply) {
@@ -1275,13 +1283,34 @@ static int _regulator_enable(struct regulator_dev *rdev)
 			if (!_regulator_can_change_status(rdev))
 				return -EPERM;
 
-			if (rdev->desc->ops->enable) {
-				ret = rdev->desc->ops->enable(rdev);
-				if (ret < 0)
-					return ret;
-			} else {
+			if (!rdev->desc->ops->enable)
 				return -EINVAL;
+
+			/* Query before enabling in case configuration
+			 * dependant.  */
+			ret = _regulator_get_enable_time(rdev);
+			if (ret >= 0) {
+				delay = ret;
+			} else {
+				printk(KERN_WARNING
+					"%s: enable_time() failed for %s: %d\n",
+					__func__, rdev_get_name(rdev),
+					ret);
+				delay = 0;
 			}
+
+			/* Allow the regulator to ramp; it would be useful
+			 * to extend this for bulk operations so that the
+			 * regulators can ramp together.  */
+			ret = rdev->desc->ops->enable(rdev);
+			if (ret < 0)
+				return ret;
+
+			if (delay >= 1000)
+				mdelay(delay / 1000);
+			else if (delay)
+				udelay(delay);
+
 		} else if (ret < 0) {
 			printk(KERN_ERR "%s: is_enabled() failed for %s: %d\n",
 			       __func__, rdev_get_name(rdev), ret);
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 31f2055..592cd7c 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -58,6 +58,9 @@ enum regulator_status {
  * @get_optimum_mode: Get the most efficient operating mode for the regulator
  *                    when running with the specified parameters.
  *
+ * @enable_time: Time taken for the regulator voltage output voltage to
+ *               stabalise after being enabled, in microseconds.
+ *
  * @set_suspend_voltage: Set the voltage for the regulator when the system
  *                       is suspended.
  * @set_suspend_enable: Mark the regulator as enabled when the system is
@@ -93,6 +96,9 @@ struct regulator_ops {
 	int (*set_mode) (struct regulator_dev *, unsigned int mode);
 	unsigned int (*get_mode) (struct regulator_dev *);
 
+	/* Time taken to enable the regulator */
+	int (*enable_time) (struct regulator_dev *);
+
 	/* report regulator status ... most other accessors report
 	 * control inputs, this reports results of combining inputs
 	 * from Linux (and other sources) with the actual load.
-- 
1.6.5.7

[PATCH 3/3] regulator: Implement enable_time() for WM835x ISINKs

[Mark Brown]

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/regulator/wm8350-regulator.c |   46 ++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/wm8350-regulator.c b/drivers/regulator/wm8350-regulator.c
index 1bbff09..927e735 100644
--- a/drivers/regulator/wm8350-regulator.c
+++ b/drivers/regulator/wm8350-regulator.c
@@ -290,6 +290,51 @@ static int wm8350_isink_is_enabled(struct regulator_dev *rdev)
 	return -EINVAL;
 }
 
+static int wm8350_isink_enable_time(struct regulator_dev *rdev)
+{
+	struct wm8350 *wm8350 = rdev_get_drvdata(rdev);
+	int isink = rdev_get_id(rdev);
+	int reg;
+
+	switch (isink) {
+	case WM8350_ISINK_A:
+		reg = wm8350_reg_read(wm8350, WM8350_CSA_FLASH_CONTROL);
+		break;
+	case WM8350_ISINK_B:
+		reg = wm8350_reg_read(wm8350, WM8350_CSB_FLASH_CONTROL);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (reg & WM8350_CS1_FLASH_MODE) {
+		switch (reg & WM8350_CS1_ON_RAMP_MASK) {
+		case 0:
+			return 0;
+		case 1:
+			return 1950;
+		case 2:
+			return 3910;
+		case 3:
+			return 7800;
+		}
+	} else {
+		switch (reg & WM8350_CS1_ON_RAMP_MASK) {
+		case 0:
+			return 0;
+		case 1:
+			return 250000;
+		case 2:
+			return 500000;
+		case 3:
+			return 1000000;
+		}
+	}
+
+	return -EINVAL;
+}
+
+
 int wm8350_isink_set_flash(struct wm8350 *wm8350, int isink, u16 mode,
 			   u16 trigger, u16 duration, u16 on_ramp, u16 off_ramp,
 			   u16 drive)
@@ -1221,6 +1266,7 @@ static struct regulator_ops wm8350_isink_ops = {
 	.enable = wm8350_isink_enable,
 	.disable = wm8350_isink_disable,
 	.is_enabled = wm8350_isink_is_enabled,
+	.enable_time = wm8350_isink_enable_time,
 };
 
 static struct regulator_desc wm8350_reg[NUM_WM8350_REGULATORS] = {
-- 
1.6.5.7

Re: [PATCH 0/3] Regulator updates for 2.6.34

[Liam Girdwood]

On Mon, 2010-01-04 at 18:16 +0000, Mark Brown wrote:
> The following changes since commit 204f78ec75e6888bbf1a7b5a3ed0cbfff9bade27:
>   Tobias Klauser (1):
>         regulator/lp3971: Storage class should be before const qualifier
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator-2.6.git for-lrg
> 
> A couple of new APIs, plus a use of one of them.
> 
> Mark Brown (3):
>       regulator: Add notifier event on regulator disable
>       regulator: Allow regulators to specify the time taken to ramp on enable
>       regulator: Implement enable_time() for WM835x ISINKs
> 
>  drivers/regulator/core.c             |   48 ++++++++++++++++++++++++++++-----
>  drivers/regulator/wm8350-regulator.c |   46 ++++++++++++++++++++++++++++++++
>  include/linux/regulator/consumer.h   |    4 ++-
>  include/linux/regulator/driver.h     |    6 ++++
>  4 files changed, 95 insertions(+), 9 deletions(-)
> --

Pulled.

Thanks

Liam