[PATCH] Regulator: add suspend-finish API for regulator core.

[PATCH] Regulator: add suspend-finish API for regulator core.

[MyungJoo Ham]

The regulator core had suspend-prepare that turns off the regulators
when entering a system-wide suspend. However, it did not have
suspend-finish that recovers the change made by suspend-prepare and
depends on machine pm code or the regulator device or driver
doing so.

This patch adds regulator_suspend_finish that pairs with the
previously-existed regulator_suspend_prepare. The function
regulator_suspend_finish turns on the regulators that have always_on set
or positive use_count so that we can recover the regulator states at
resume.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/regulator/core.c          |   24 ++++++++++++++++++++++++
 include/linux/regulator/machine.h |    1 +
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 9fa2095..b1e71fd 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2653,6 +2653,30 @@ out:
 EXPORT_SYMBOL_GPL(regulator_suspend_prepare);
 
 /**
+ * regulator_suspend_prepare - resume regulators from system wide suspend
+ *
+ * Turn on regulators that might be turned off by regulator_suspend_prepare.
+ */
+int regulator_suspend_finish(void)
+{
+	struct regulator_dev *rdev;
+	int ret = 0, error;
+
+	mutex_lock(&regulator_list_mutex);
+	list_for_each_entry(rdev, &regulator_list, list) {
+		if ((rdev->use_count > 0  || rdev->constraints->always_on) &&
+				rdev->desc->ops->enable) {
+			error = rdev->desc->ops->enable(rdev);
+			if (error)
+				ret = error;
+		}
+	}
+	mutex_unlock(&regulator_list_mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(regulator_suspend_finish);
+
+/**
  * regulator_has_full_constraints - the system has fully specified constraints
  *
  * Calling this function will cause the regulator API to disable all
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index 761c745..c4c4fc4 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -186,6 +186,7 @@ struct regulator_init_data {
 };
 
 int regulator_suspend_prepare(suspend_state_t state);
+int regulator_suspend_finish(void);
 
 #ifdef CONFIG_REGULATOR
 void regulator_has_full_constraints(void);
-- 
1.7.1

Re: [PATCH] Regulator: add suspend-finish API for regulator core.

[Mark Brown]

On Tue, Mar 08, 2011 at 11:03:58AM +0900, MyungJoo Ham wrote:
> The regulator core had suspend-prepare that turns off the regulators
> when entering a system-wide suspend. However, it did not have
> suspend-finish that recovers the change made by suspend-prepare and
> depends on machine pm code or the regulator device or driver
> doing so.

This is a good idea, thanks for working on it.

Your commit message is sligtly inaccurate as this isn't what
suspend_prepare() is for, suspend_prepare() is for matching the suspend
mode configuration of regulators that support that with the suspend mode
Linux is using (RAM, disk and so on).  There is no need for this to
recover the pre-suspend state as hardware implementing suspend mode
configuration should be able to do so autonomously.

Of course not all hardware supports a distinct suspend mode and for
hardware that doesn't we should be doing pretty much this - it's a
bit of a hole in our regulator support at the minute.  It should really
be coupled with a soft suspend mode implementation which can put the
regulators into an appropriate state for suspend on the way down.

> +               if ((rdev->use_count > 0  || rdev->constraints->always_on) &&
> +                               rdev->desc->ops->enable) {
> +                       error = rdev->desc->ops->enable(rdev);
> +                       if (error)
> +                               ret = error;

We should probably also be turning off regulators that shouldn't be on -
a regulator may default to being enabled when we don't want it.
Thinking about it we can probably share most if not all of the code with
regulator_init_complete()...

Ideally we'd also restore voltages but that can always be added later.

> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(regulator_suspend_finish);

Hrm, I'd really expect the core to be arranging for this to happen
rather than exporting the function?  Though the sequencing so it gets
called at the right time might be a bit tricky and I've not actually
looked at the isues here.

Re: [PATCH] Regulator: add suspend-finish API for regulator core.

[MyungJoo Ham]

On Tue, Mar 8, 2011 at 10:38 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Mar 08, 2011 at 11:03:58AM +0900, MyungJoo Ham wrote:
>> The regulator core had suspend-prepare that turns off the regulators
>> when entering a system-wide suspend. However, it did not have
>> suspend-finish that recovers the change made by suspend-prepare and
>> depends on machine pm code or the regulator device or driver
>> doing so.
>
> This is a good idea, thanks for working on it.

Welcome.

>
> Your commit message is sligtly inaccurate as this isn't what
> suspend_prepare() is for, suspend_prepare() is for matching the suspend
> mode configuration of regulators that support that with the suspend mode
> Linux is using (RAM, disk and so on).  There is no need for this to
> recover the pre-suspend state as hardware implementing suspend mode
> configuration should be able to do so autonomously.
>
> Of course not all hardware supports a distinct suspend mode and for
> hardware that doesn't we should be doing pretty much this - it's a
> bit of a hole in our regulator support at the minute.  It should really
> be coupled with a soft suspend mode implementation which can put the
> regulators into an appropriate state for suspend on the way down.

Yes. It'd be more accurate to describe that the patch is to set states
of the regulators according to their state-related regulator values
(use_count, always_on) at resume.

>
>> +               if ((rdev->use_count > 0  || rdev->constraints->always_on) &&
>> +                               rdev->desc->ops->enable) {
>> +                       error = rdev->desc->ops->enable(rdev);
>> +                       if (error)
>> +                               ret = error;
>
> We should probably also be turning off regulators that shouldn't be on -
> a regulator may default to being enabled when we don't want it.
> Thinking about it we can probably share most if not all of the code with
> regulator_init_complete()...
>
> Ideally we'd also restore voltages but that can always be added later.

Agreed. I'll add rdev->desc->ops->disable(rdev) for the else statement
of that if statement.

>
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(regulator_suspend_finish);
>
> Hrm, I'd really expect the core to be arranging for this to happen
> rather than exporting the function?  Though the sequencing so it gets
> called at the right time might be a bit tricky and I've not actually
> looked at the isues here.
>

Currently, it depends on machine's pm code calling
regulator_suspend_prepare. And, that's why I made
regulator_suspend_finish to be called by someone else. This appears to
be odd to me as well; however, probably, like
regulator_init_complete() is optional, some systems cannot allow
regulators to be controlled by regulator_suspend_prepare by default or
the executed position (after suspend_ops->prepare, right before
suspend_ops->enter, or ...) may vary. If we are to let the core
arrange regulator_suspend_prepare/finish, do you think putting it
right after "suspend_ops->prepare" and right before
"suspend_ops->enter"?


Version-2 Patch is incoming. (with disable feature at suspend_finish)

-- 
MyungJoo Ham (함명주), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858

[PATCH v2] Regulator: add suspend-finish API for regulator core.

[MyungJoo Ham]

The regulator core had suspend-prepare that turns off the regulators
when entering a system-wide suspend. However, it did not have
suspend-finish that pairs with suspend-prepare and the regulator core
has assumed that the regulator devices and their drivers support
autonomous recover at resume.

This patch adds regulator_suspend_finish that pairs with the
previously-existed regulator_suspend_prepare. The function
regulator_suspend_finish turns on the regulators that have always_on set
or positive use_count so that we can reset the regulator states
appropriately at resume.

In regulator_suspend_finish, if has_full_constraints, it disables
unnecessary regulators.

Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/regulator/core.c          |   40 +++++++++++++++++++++++++++++++++++++
 include/linux/regulator/machine.h |    1 +
 2 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 9fa2095..4d34cc0 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2653,6 +2653,46 @@ out:
 EXPORT_SYMBOL_GPL(regulator_suspend_prepare);
 
 /**
+ * regulator_suspend_prepare - resume regulators from system wide suspend
+ *
+ * Turn on regulators that might be turned off by regulator_suspend_prepare.
+ */
+int regulator_suspend_finish(void)
+{
+	struct regulator_dev *rdev;
+	int ret = 0, error;
+
+	mutex_lock(&regulator_list_mutex);
+	list_for_each_entry(rdev, &regulator_list, list) {
+		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 (error)
+				ret = error;
+		} else {
+			if (!has_full_constraints)
+				goto unlock;
+			if (!ops->disable)
+				goto unlock;
+			if (ops->is_enabled && !ops->is_enabled(rdev))
+				goto unlock;
+
+			error = ops->disable(rdev);
+			if (error)
+				ret = error;
+		}
+unlock:
+		mutex_unlock(&rdev->mutex);
+	}
+	mutex_unlock(&regulator_list_mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(regulator_suspend_finish);
+
+/**
  * regulator_has_full_constraints - the system has fully specified constraints
  *
  * Calling this function will cause the regulator API to disable all
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index 761c745..c4c4fc4 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -186,6 +186,7 @@ struct regulator_init_data {
 };
 
 int regulator_suspend_prepare(suspend_state_t state);
+int regulator_suspend_finish(void);
 
 #ifdef CONFIG_REGULATOR
 void regulator_has_full_constraints(void);
-- 
1.7.1

Re: [PATCH v2] Regulator: add suspend-finish API for regulator core.

[Igor Grinberg]

Hi,


just a minor comment issue...


On 03/09/11 04:37, MyungJoo Ham wrote:

> The regulator core had suspend-prepare that turns off the regulators
> when entering a system-wide suspend. However, it did not have
> suspend-finish that pairs with suspend-prepare and the regulator core
> has assumed that the regulator devices and their drivers support
> autonomous recover at resume.
>
> This patch adds regulator_suspend_finish that pairs with the
> previously-existed regulator_suspend_prepare. The function
> regulator_suspend_finish turns on the regulators that have always_on set
> or positive use_count so that we can reset the regulator states
> appropriately at resume.
>
> In regulator_suspend_finish, if has_full_constraints, it disables
> unnecessary regulators.
>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/regulator/core.c          |   40 +++++++++++++++++++++++++++++++++++++
>  include/linux/regulator/machine.h |    1 +
>  2 files changed, 41 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 9fa2095..4d34cc0 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2653,6 +2653,46 @@ out:
>  EXPORT_SYMBOL_GPL(regulator_suspend_prepare);
>  
>  /**
> + * regulator_suspend_prepare - resume regulators from system wide suspend

s/prepare/finish/

> + *
> + * Turn on regulators that might be turned off by regulator_suspend_prepare.
> + */
> +int regulator_suspend_finish(void)
> +{
> +	struct regulator_dev *rdev;
> +	int ret = 0, error;
> +
> +	mutex_lock(&regulator_list_mutex);
> +	list_for_each_entry(rdev, &regulator_list, list) {
> +		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 (error)
> +				ret = error;
> +		} else {
> +			if (!has_full_constraints)
> +				goto unlock;
> +			if (!ops->disable)
> +				goto unlock;
> +			if (ops->is_enabled && !ops->is_enabled(rdev))
> +				goto unlock;
> +
> +			error = ops->disable(rdev);
> +			if (error)
> +				ret = error;
> +		}
> +unlock:
> +		mutex_unlock(&rdev->mutex);
> +	}
> +	mutex_unlock(&regulator_list_mutex);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(regulator_suspend_finish);
> +
> +/**
>   * regulator_has_full_constraints - the system has fully specified constraints
>   *
>   * Calling this function will cause the regulator API to disable all
> diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
> index 761c745..c4c4fc4 100644
> --- a/include/linux/regulator/machine.h
> +++ b/include/linux/regulator/machine.h
> @@ -186,6 +186,7 @@ struct regulator_init_data {
>  };
>  
>  int regulator_suspend_prepare(suspend_state_t state);
> +int regulator_suspend_finish(void);
>  
>  #ifdef CONFIG_REGULATOR
>  void regulator_has_full_constraints(void);

-- 
Regards,
Igor.