devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] Add regulator suspend and resume support
@ 2017-12-21  6:25 Chunyan Zhang
  2017-12-21  6:25 ` [PATCH 3/5] drivers: regulator: leave one item to record whether regulator is enabled Chunyan Zhang
       [not found] ` <1513837506-26543-1-git-send-email-zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
  0 siblings, 2 replies; 11+ messages in thread
From: Chunyan Zhang @ 2017-12-21  6:25 UTC (permalink / raw)
  To: Mark Brown, Rob Herring
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Chunyan Zhang

Some systems need to set regulators to specific states when they enter low
power modes, especially around CPUs.

Currently the regulator driver, for suspend and resume features, provides
two functions which are exported for being called directly by any modules
or subsystems when they thought the regulator should be entering into
suspend states.

This patchset adds hooks to PM suspend core and provides suspend/resume
callback functions to regulator device, for those who can be switched
off or set low voltage in suspend states only need to implement the
callback functions in the driver, and set the right configurations for
suspend states via device tree and the APIs which regulator core
driver provides.

Those drivers who use the old interfaces - i.e. regulator_suspend_prepare()
and regulator_suspend_finish() should stop using that, since we leave these
two functions empty and plan to remove them one day in the future.

Any comments would be greatly appreciated.

Thanks,
Chunyan

Chunyan Zhang (5):
  bindings: regulator: added support for suspend states
  regulator: make regulator voltage be an array to support more states
  drivers: regulator: leave one item to record whether regulator is
    enabled
  drivers: regulator: empty the old suspend functions
  regulator: add PM suspend and resume hooks

 .../devicetree/bindings/regulator/regulator.txt    |  11 +-
 drivers/regulator/core.c                           | 342 ++++++++++++++-------
 drivers/regulator/internal.h                       |  18 +-
 drivers/regulator/of_regulator.c                   |  18 +-
 include/linux/regulator/driver.h                   |   2 +
 include/linux/regulator/machine.h                  |  31 +-
 6 files changed, 299 insertions(+), 123 deletions(-)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/5] bindings: regulator: added support for suspend states
       [not found] ` <1513837506-26543-1-git-send-email-zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
@ 2017-12-21  6:25   ` Chunyan Zhang
       [not found]     ` <1513837506-26543-2-git-send-email-zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
  2017-12-21  6:25   ` [PATCH 2/5] regulator: make regulator voltage be an array to support more states Chunyan Zhang
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Chunyan Zhang @ 2017-12-21  6:25 UTC (permalink / raw)
  To: Mark Brown, Rob Herring
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Chunyan Zhang

Documented a few new added properties which are used for supporting
regulator suspend states.

Signed-off-by: Chunyan Zhang <zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 Documentation/devicetree/bindings/regulator/regulator.txt | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
index 378f6dc..618a322 100644
--- a/Documentation/devicetree/bindings/regulator/regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/regulator.txt
@@ -42,8 +42,15 @@ Optional properties:
 - regulator-state-[mem/disk] node has following common properties:
 	- regulator-on-in-suspend: regulator should be on in suspend state.
 	- regulator-off-in-suspend: regulator should be off in suspend state.
-	- regulator-suspend-microvolt: regulator should be set to this voltage
-	  in suspend.
+	- regulator-suspend-min-microvolt: minimum voltage may be set in
+	  suspend state.
+	- regulator-suspend-max-microvolt: maximum voltage may be set in
+	  suspend state.
+	- regulator-suspend-microvolt: the default voltage which regulator
+	  should be set in suspend, this can be adjusted among
+	  <regulator-suspend-min-microvolt, regulator-suspend-max-microvolt>
+	- regulator-changeable-in-suspend: whether the default voltage and
+	  the regulator on/off in suspend can be changed in runtime.
 	- regulator-mode: operating mode in the given suspend state.
 	  The set of possible operating modes depends on the capabilities of
 	  every hardware so the valid modes are documented on each regulator
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/5] regulator: make regulator voltage be an array to support more states
       [not found] ` <1513837506-26543-1-git-send-email-zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
  2017-12-21  6:25   ` [PATCH 1/5] bindings: regulator: added support for suspend states Chunyan Zhang
@ 2017-12-21  6:25   ` Chunyan Zhang
  2017-12-21  6:25   ` [PATCH 4/5] drivers: regulator: empty the old suspend functions Chunyan Zhang
  2017-12-21  6:25   ` [PATCH 5/5] regulator: add PM suspend and resume hooks Chunyan Zhang
  3 siblings, 0 replies; 11+ messages in thread
From: Chunyan Zhang @ 2017-12-21  6:25 UTC (permalink / raw)
  To: Mark Brown, Rob Herring
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Chunyan Zhang

Some regulator consumers would like to make the regulator device
keeping a voltage range output when the system entering into
suspend states.

Making regulator voltage be an array can allow consumers to set voltage
for normal state as well as for suspend states through the same code.

Signed-off-by: Chunyan Zhang <zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/regulator/core.c     | 63 ++++++++++++++++++++++++--------------------
 drivers/regulator/internal.h | 18 +++++++++++--
 2 files changed, 51 insertions(+), 30 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index b64b791..97bc9f7 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -240,22 +240,25 @@ static int regulator_check_voltage(struct regulator_dev *rdev,
  * regulator consumers
  */
 static int regulator_check_consumers(struct regulator_dev *rdev,
-				     int *min_uV, int *max_uV)
+				     int *min_uV, int *max_uV,
+				     suspend_state_t state)
 {
 	struct regulator *regulator;
+	struct regulator_voltage *voltage;
 
 	list_for_each_entry(regulator, &rdev->consumer_list, list) {
+		voltage = &regulator->voltage[state];
 		/*
 		 * Assume consumers that didn't say anything are OK
 		 * with anything in the constraint range.
 		 */
-		if (!regulator->min_uV && !regulator->max_uV)
+		if (!voltage->min_uV && !voltage->max_uV)
 			continue;
 
-		if (*max_uV > regulator->max_uV)
-			*max_uV = regulator->max_uV;
-		if (*min_uV < regulator->min_uV)
-			*min_uV = regulator->min_uV;
+		if (*max_uV > voltage->max_uV)
+			*max_uV = voltage->max_uV;
+		if (*min_uV < voltage->min_uV)
+			*min_uV = voltage->min_uV;
 	}
 
 	if (*min_uV > *max_uV) {
@@ -1356,9 +1359,9 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 		debugfs_create_u32("uA_load", 0444, regulator->debugfs,
 				   &regulator->uA_load);
 		debugfs_create_u32("min_uV", 0444, regulator->debugfs,
-				   &regulator->min_uV);
+				   &regulator->voltage[PM_SUSPEND_ON].min_uV);
 		debugfs_create_u32("max_uV", 0444, regulator->debugfs,
-				   &regulator->max_uV);
+				   &regulator->voltage[PM_SUSPEND_ON].max_uV);
 		debugfs_create_file("constraint_flags", 0444,
 				    regulator->debugfs, regulator,
 				    &constraint_flags_fops);
@@ -2898,9 +2901,11 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 }
 
 static int regulator_set_voltage_unlocked(struct regulator *regulator,
-					  int min_uV, int max_uV)
+					  int min_uV, int max_uV,
+					  suspend_state_t state)
 {
 	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_voltage *voltage = &regulator->voltage[state];
 	int ret = 0;
 	int old_min_uV, old_max_uV;
 	int current_uV;
@@ -2911,7 +2916,7 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 	 * should be a noop (some cpufreq implementations use the same
 	 * voltage for multiple frequencies, for example).
 	 */
-	if (regulator->min_uV == min_uV && regulator->max_uV == max_uV)
+	if (voltage->min_uV == min_uV && voltage->max_uV == max_uV)
 		goto out;
 
 	/* If we're trying to set a range that overlaps the current voltage,
@@ -2921,8 +2926,8 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 	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;
-			regulator->max_uV = max_uV;
+			voltage->min_uV = min_uV;
+			voltage->max_uV = max_uV;
 			goto out;
 		}
 	}
@@ -2940,12 +2945,12 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 		goto out;
 
 	/* restore original values in case of error */
-	old_min_uV = regulator->min_uV;
-	old_max_uV = regulator->max_uV;
-	regulator->min_uV = min_uV;
-	regulator->max_uV = max_uV;
+	old_min_uV = voltage->min_uV;
+	old_max_uV = voltage->max_uV;
+	voltage->min_uV = min_uV;
+	voltage->max_uV = max_uV;
 
-	ret = regulator_check_consumers(rdev, &min_uV, &max_uV);
+	ret = regulator_check_consumers(rdev, &min_uV, &max_uV, state);
 	if (ret < 0)
 		goto out2;
 
@@ -2982,7 +2987,7 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 
 	if (supply_change_uV > 0) {
 		ret = regulator_set_voltage_unlocked(rdev->supply,
-				best_supply_uV, INT_MAX);
+				best_supply_uV, INT_MAX, state);
 		if (ret) {
 			dev_err(&rdev->dev, "Failed to increase supply voltage: %d\n",
 					ret);
@@ -2996,7 +3001,7 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 
 	if (supply_change_uV < 0) {
 		ret = regulator_set_voltage_unlocked(rdev->supply,
-				best_supply_uV, INT_MAX);
+				best_supply_uV, INT_MAX, state);
 		if (ret)
 			dev_warn(&rdev->dev, "Failed to decrease supply voltage: %d\n",
 					ret);
@@ -3007,8 +3012,8 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 out:
 	return ret;
 out2:
-	regulator->min_uV = old_min_uV;
-	regulator->max_uV = old_max_uV;
+	voltage->min_uV = old_min_uV;
+	voltage->max_uV = old_max_uV;
 
 	return ret;
 }
@@ -3037,7 +3042,8 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
 
 	regulator_lock_supply(regulator->rdev);
 
-	ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV);
+	ret = regulator_set_voltage_unlocked(regulator, min_uV, max_uV,
+					     PM_SUSPEND_ON);
 
 	regulator_unlock_supply(regulator->rdev);
 
@@ -3138,6 +3144,7 @@ EXPORT_SYMBOL_GPL(regulator_set_voltage_time_sel);
 int regulator_sync_voltage(struct regulator *regulator)
 {
 	struct regulator_dev *rdev = regulator->rdev;
+	struct regulator_voltage *voltage = &regulator->voltage[PM_SUSPEND_ON];
 	int ret, min_uV, max_uV;
 
 	mutex_lock(&rdev->mutex);
@@ -3149,20 +3156,20 @@ int regulator_sync_voltage(struct regulator *regulator)
 	}
 
 	/* This is only going to work if we've had a voltage configured. */
-	if (!regulator->min_uV && !regulator->max_uV) {
+	if (!voltage->min_uV && !voltage->max_uV) {
 		ret = -EINVAL;
 		goto out;
 	}
 
-	min_uV = regulator->min_uV;
-	max_uV = regulator->max_uV;
+	min_uV = voltage->min_uV;
+	max_uV = voltage->max_uV;
 
 	/* This should be a paranoia check... */
 	ret = regulator_check_voltage(rdev, &min_uV, &max_uV);
 	if (ret < 0)
 		goto out;
 
-	ret = regulator_check_consumers(rdev, &min_uV, &max_uV);
+	ret = regulator_check_consumers(rdev, &min_uV, &max_uV, 0);
 	if (ret < 0)
 		goto out;
 
@@ -4424,8 +4431,8 @@ static void regulator_summary_show_subtree(struct seq_file *s,
 		switch (rdev->desc->type) {
 		case REGULATOR_VOLTAGE:
 			seq_printf(s, "%37dmV %5dmV",
-				   consumer->min_uV / 1000,
-				   consumer->max_uV / 1000);
+				   consumer->voltage[PM_SUSPEND_ON].min_uV / 1000,
+				   consumer->voltage[PM_SUSPEND_ON].max_uV / 1000);
 			break;
 		case REGULATOR_CURRENT:
 			break;
diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index 66a8ea0..aba8e414 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -16,10 +16,25 @@
 #ifndef __REGULATOR_INTERNAL_H
 #define __REGULATOR_INTERNAL_H
 
+#include <linux/suspend.h>
+
+#define REGULATOR_STATES_NUM	(PM_SUSPEND_MAX + 1)
+
+struct regulator_voltage {
+	int min_uV;
+	int max_uV;
+};
+
 /*
  * struct regulator
  *
  * One for each consumer device.
+ * @voltage - a voltage array for each state of runtime, i.e.:
+ *            PM_SUSPEND_ON
+ *            PM_SUSPEND_TO_IDLE
+ *            PM_SUSPEND_STANDBY
+ *            PM_SUSPEND_MEM
+ *            PM_SUSPEND_MAX
  */
 struct regulator {
 	struct device *dev;
@@ -27,8 +42,7 @@ struct regulator {
 	unsigned int always_on:1;
 	unsigned int bypass:1;
 	int uA_load;
-	int min_uV;
-	int max_uV;
+	struct regulator_voltage voltage[REGULATOR_STATES_NUM];
 	const char *supply_name;
 	struct device_attribute dev_attr;
 	struct regulator_dev *rdev;
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/5] drivers: regulator: leave one item to record whether regulator is enabled
  2017-12-21  6:25 [PATCH 0/5] Add regulator suspend and resume support Chunyan Zhang
@ 2017-12-21  6:25 ` Chunyan Zhang
       [not found] ` <1513837506-26543-1-git-send-email-zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
  1 sibling, 0 replies; 11+ messages in thread
From: Chunyan Zhang @ 2017-12-21  6:25 UTC (permalink / raw)
  To: Mark Brown, Rob Herring; +Cc: devicetree, linux-kernel, Chunyan Zhang

The items "disabled" and "enabled" are a little redundant, since only one
of them would be set to record if the regulator device should keep on
or be switched to off in suspend states.

So in this patch, the "disabled" was removed, only leave the "enabled":
  - enabled == 1 for regulator-on-in-suspend
  - enabled == 0 for regulator-off-in-suspend
  - enabled == -1 means do nothing when entering suspend mode.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 drivers/regulator/core.c          | 14 ++++++--------
 drivers/regulator/of_regulator.c  |  6 ++++--
 include/linux/regulator/machine.h | 12 +++++++++---
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 97bc9f7..5ea80e9 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -742,21 +742,19 @@ static int suspend_set_state(struct regulator_dev *rdev,
 	 * only warn if the driver implements set_suspend_voltage or
 	 * set_suspend_mode callback.
 	 */
-	if (!rstate->enabled && !rstate->disabled) {
+	if (rstate->enabled != ENABLE_IN_SUSPEND &&
+	    rstate->enabled != DISABLE_IN_SUSPEND) {
 		if (rdev->desc->ops->set_suspend_voltage ||
 		    rdev->desc->ops->set_suspend_mode)
 			rdev_warn(rdev, "No configuration\n");
 		return 0;
 	}
 
-	if (rstate->enabled && rstate->disabled) {
-		rdev_err(rdev, "invalid configuration\n");
-		return -EINVAL;
-	}
-
-	if (rstate->enabled && rdev->desc->ops->set_suspend_enable)
+	if (rstate->enabled == ENABLE_IN_SUSPEND &&
+		rdev->desc->ops->set_suspend_enable)
 		ret = rdev->desc->ops->set_suspend_enable(rdev);
-	else if (rstate->disabled && rdev->desc->ops->set_suspend_disable)
+	else if (rstate->enabled == DISABLE_IN_SUSPEND &&
+		rdev->desc->ops->set_suspend_disable)
 		ret = rdev->desc->ops->set_suspend_disable(rdev);
 	else /* OK if set_suspend_enable or set_suspend_disable is NULL */
 		ret = 0;
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 14637a0..41dad42 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -177,10 +177,12 @@ static void of_get_regulation_constraints(struct device_node *np,
 
 		if (of_property_read_bool(suspend_np,
 					"regulator-on-in-suspend"))
-			suspend_state->enabled = true;
+			suspend_state->enabled = ENABLE_IN_SUSPEND;
 		else if (of_property_read_bool(suspend_np,
 					"regulator-off-in-suspend"))
-			suspend_state->disabled = true;
+			suspend_state->enabled = DISABLE_IN_SUSPEND;
+		else
+			suspend_state->enabled = DO_NOTHING_IN_SUSPEND;
 
 		if (!of_property_read_u32(suspend_np,
 					"regulator-suspend-microvolt", &pval))
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index 9cd4fef..e50519f 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -42,6 +42,11 @@ struct regulator;
 #define REGULATOR_CHANGE_DRMS		0x10
 #define REGULATOR_CHANGE_BYPASS		0x20
 
+/* operations in suspend mode */
+#define DO_NOTHING_IN_SUSPEND	(-1)
+#define DISABLE_IN_SUSPEND	0
+#define ENABLE_IN_SUSPEND	1
+
 /* Regulator active discharge flags */
 enum regulator_active_discharge {
 	REGULATOR_ACTIVE_DISCHARGE_DEFAULT,
@@ -58,14 +63,15 @@ enum regulator_active_discharge {
  *
  * @uV: Operating voltage during suspend.
  * @mode: Operating mode during suspend.
- * @enabled: Enabled during suspend.
- * @disabled: Disabled during suspend.
+ * @enabled: operations during suspend.
+ *	     - DO_NOTHING_IN_SUSPEND
+ *	     - DISABLE_IN_SUSPEND
+ *	     - ENABLE_IN_SUSPEND
  */
 struct regulator_state {
 	int uV;	/* suspend voltage */
 	unsigned int mode; /* suspend regulator operating mode */
 	int enabled; /* is regulator enabled in this suspend state */
-	int disabled; /* is the regulator disabled in this suspend state */
 };
 
 /**
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 4/5] drivers: regulator: empty the old suspend functions
       [not found] ` <1513837506-26543-1-git-send-email-zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
  2017-12-21  6:25   ` [PATCH 1/5] bindings: regulator: added support for suspend states Chunyan Zhang
  2017-12-21  6:25   ` [PATCH 2/5] regulator: make regulator voltage be an array to support more states Chunyan Zhang
@ 2017-12-21  6:25   ` Chunyan Zhang
       [not found]     ` <1513837506-26543-5-git-send-email-zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
  2017-12-21  6:25   ` [PATCH 5/5] regulator: add PM suspend and resume hooks Chunyan Zhang
  3 siblings, 1 reply; 11+ messages in thread
From: Chunyan Zhang @ 2017-12-21  6:25 UTC (permalink / raw)
  To: Mark Brown, Rob Herring
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Chunyan Zhang

Regualtor suspend/resume functions should only be called by PM suspend
core via registering dev_pm_ops, and regulator devices should implement
the callback functions.  Thus, any regulator consumer shouldn't call
the regulator suspend/resume functions directly.

In order to avoid compile errors, two empty functions with the same name
still be left for the time being.

Signed-off-by: Chunyan Zhang <zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/regulator/core.c          | 74 ---------------------------------------
 include/linux/regulator/machine.h |  5 ++-
 2 files changed, 2 insertions(+), 77 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 5ea80e9..080c233 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4179,80 +4179,6 @@ void regulator_unregister(struct regulator_dev *rdev)
 }
 EXPORT_SYMBOL_GPL(regulator_unregister);
 
-static int _regulator_suspend_prepare(struct device *dev, void *data)
-{
-	struct regulator_dev *rdev = dev_to_rdev(dev);
-	const suspend_state_t *state = data;
-	int ret;
-
-	mutex_lock(&rdev->mutex);
-	ret = suspend_prepare(rdev, *state);
-	mutex_unlock(&rdev->mutex);
-
-	return ret;
-}
-
-/**
- * regulator_suspend_prepare - prepare regulators for system wide suspend
- * @state: system suspend state
- *
- * Configure each regulator with it's suspend operating parameters for state.
- * This will usually be called by machine suspend code prior to supending.
- */
-int regulator_suspend_prepare(suspend_state_t state)
-{
-	/* ON is handled by regulator active state */
-	if (state == PM_SUSPEND_ON)
-		return -EINVAL;
-
-	return class_for_each_device(&regulator_class, NULL, &state,
-				     _regulator_suspend_prepare);
-}
-EXPORT_SYMBOL_GPL(regulator_suspend_prepare);
-
-static int _regulator_suspend_finish(struct device *dev, void *data)
-{
-	struct regulator_dev *rdev = dev_to_rdev(dev);
-	int ret;
-
-	mutex_lock(&rdev->mutex);
-	if (rdev->use_count > 0  || rdev->constraints->always_on) {
-		if (!_regulator_is_enabled(rdev)) {
-			ret = _regulator_do_enable(rdev);
-			if (ret)
-				dev_err(dev,
-					"Failed to resume regulator %d\n",
-					ret);
-		}
-	} else {
-		if (!have_full_constraints())
-			goto unlock;
-		if (!_regulator_is_enabled(rdev))
-			goto unlock;
-
-		ret = _regulator_do_disable(rdev);
-		if (ret)
-			dev_err(dev, "Failed to suspend regulator %d\n", ret);
-	}
-unlock:
-	mutex_unlock(&rdev->mutex);
-
-	/* Keep processing regulators in spite of any errors */
-	return 0;
-}
-
-/**
- * regulator_suspend_finish - resume regulators from system wide suspend
- *
- * Turn on regulators that might be turned off by regulator_suspend_prepare
- * and that should be turned on according to the regulators properties.
- */
-int regulator_suspend_finish(void)
-{
-	return class_for_each_device(&regulator_class, NULL, NULL,
-				     _regulator_suspend_finish);
-}
-EXPORT_SYMBOL_GPL(regulator_suspend_finish);
 
 /**
  * regulator_has_full_constraints - the system has fully specified constraints
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index e50519f..b4ddb56 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -231,12 +231,12 @@ struct regulator_init_data {
 
 #ifdef CONFIG_REGULATOR
 void regulator_has_full_constraints(void);
-int regulator_suspend_prepare(suspend_state_t state);
-int regulator_suspend_finish(void);
 #else
 static inline void regulator_has_full_constraints(void)
 {
 }
+#endif
+
 static inline int regulator_suspend_prepare(suspend_state_t state)
 {
 	return 0;
@@ -245,6 +245,5 @@ static inline int regulator_suspend_finish(void)
 {
 	return 0;
 }
-#endif
 
 #endif
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 5/5] regulator: add PM suspend and resume hooks
       [not found] ` <1513837506-26543-1-git-send-email-zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
                     ` (2 preceding siblings ...)
  2017-12-21  6:25   ` [PATCH 4/5] drivers: regulator: empty the old suspend functions Chunyan Zhang
@ 2017-12-21  6:25   ` Chunyan Zhang
  3 siblings, 0 replies; 11+ messages in thread
From: Chunyan Zhang @ 2017-12-21  6:25 UTC (permalink / raw)
  To: Mark Brown, Rob Herring
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Chunyan Zhang

In this patch, consumers are allowed to set suspend voltage, and this
actually just set the "uV" in constraint::regulator_state, when the
regulator_suspend_late() was called by PM core through callback when
the system is entering into suspend, the regulator device would act
suspend activity then.

And it assumes that if any consumer set suspend voltage, the regulator
device should be enabled in the suspend state.  And if the suspend
voltage of a regulator device for all consumers was set zero, the
regulator device would be off in the suspend state.

This patch also provides a new function hook to regulator devices for
resuming from suspend states.

Signed-off-by: Chunyan Zhang <zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/regulator/core.c          | 251 +++++++++++++++++++++++++++++++++-----
 drivers/regulator/of_regulator.c  |  12 ++
 include/linux/regulator/driver.h  |   2 +
 include/linux/regulator/machine.h |  16 ++-
 4 files changed, 249 insertions(+), 32 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 080c233..3f4d3aa 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -236,6 +236,12 @@ static int regulator_check_voltage(struct regulator_dev *rdev,
 	return 0;
 }
 
+/* return 0 if the state is valid */
+static int regulator_check_states(suspend_state_t state)
+{
+	return (state > PM_SUSPEND_MAX || state == PM_SUSPEND_TO_IDLE);
+}
+
 /* Make sure we select a voltage that suits the needs of all
  * regulator consumers
  */
@@ -327,6 +333,24 @@ static int regulator_mode_constrain(struct regulator_dev *rdev,
 	return -EINVAL;
 }
 
+static inline struct regulator_state *
+regulator_get_suspend_state(struct regulator_dev *rdev, suspend_state_t state)
+{
+	if (rdev->constraints == NULL)
+		return NULL;
+
+	switch (state) {
+	case PM_SUSPEND_STANDBY:
+		return &rdev->constraints->state_standby;
+	case PM_SUSPEND_MEM:
+		return &rdev->constraints->state_mem;
+	case PM_SUSPEND_MAX:
+		return &rdev->constraints->state_disk;
+	default:
+		return NULL;
+	}
+}
+
 static ssize_t regulator_uV_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
@@ -734,9 +758,14 @@ static int drms_uA_update(struct regulator_dev *rdev)
 }
 
 static int suspend_set_state(struct regulator_dev *rdev,
-	struct regulator_state *rstate)
+				    suspend_state_t state)
 {
 	int ret = 0;
+	struct regulator_state *rstate;
+
+	rstate = regulator_get_suspend_state(rdev, state);
+	if (rstate == NULL)
+		return -EINVAL;
 
 	/* If we have no suspend mode configration don't set anything;
 	 * only warn if the driver implements set_suspend_voltage or
@@ -779,28 +808,8 @@ static int suspend_set_state(struct regulator_dev *rdev,
 			return ret;
 		}
 	}
-	return ret;
-}
 
-/* locks held by caller */
-static int suspend_prepare(struct regulator_dev *rdev, suspend_state_t state)
-{
-	if (!rdev->constraints)
-		return -EINVAL;
-
-	switch (state) {
-	case PM_SUSPEND_STANDBY:
-		return suspend_set_state(rdev,
-			&rdev->constraints->state_standby);
-	case PM_SUSPEND_MEM:
-		return suspend_set_state(rdev,
-			&rdev->constraints->state_mem);
-	case PM_SUSPEND_MAX:
-		return suspend_set_state(rdev,
-			&rdev->constraints->state_disk);
-	default:
-		return -EINVAL;
-	}
+	return ret;
 }
 
 static void print_constraints(struct regulator_dev *rdev)
@@ -1069,7 +1078,7 @@ static int set_machine_constraints(struct regulator_dev *rdev,
 
 	/* do we need to setup our suspend state */
 	if (rdev->constraints->initial_state) {
-		ret = suspend_prepare(rdev, rdev->constraints->initial_state);
+		ret = suspend_set_state(rdev, rdev->constraints->initial_state);
 		if (ret < 0) {
 			rdev_err(rdev, "failed to set suspend state\n");
 			return ret;
@@ -2898,6 +2907,35 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 	return ret;
 }
 
+static int _regulator_do_set_suspend_voltage(struct regulator_dev *rdev,
+				  int min_uV, int max_uV, suspend_state_t state)
+{
+	struct regulator_state *rstate;
+	int uV, sel;
+
+	rstate = regulator_get_suspend_state(rdev, state);
+	if (rstate == NULL)
+		return -EINVAL;
+
+	if (!rstate->changeable)
+		return -EINVAL;
+
+	if (min_uV < rstate->min_uV)
+		min_uV = rstate->min_uV;
+	if (max_uV > rstate->max_uV)
+		max_uV = rstate->max_uV;
+
+	sel = regulator_map_voltage(rdev, min_uV, max_uV);
+	if (sel < 0)
+		return sel;
+
+	uV = rdev->desc->ops->list_voltage(rdev, sel);
+	if (uV >= min_uV && uV <= max_uV)
+		rstate->uV = uV;
+
+	return 0;
+}
+
 static int regulator_set_voltage_unlocked(struct regulator *regulator,
 					  int min_uV, int max_uV,
 					  suspend_state_t state)
@@ -2993,7 +3031,11 @@ static int regulator_set_voltage_unlocked(struct regulator *regulator,
 		}
 	}
 
-	ret = _regulator_do_set_voltage(rdev, min_uV, max_uV);
+	if (state == PM_SUSPEND_ON)
+		ret = _regulator_do_set_voltage(rdev, min_uV, max_uV);
+	else
+		ret = _regulator_do_set_suspend_voltage(rdev, min_uV,
+							max_uV, state);
 	if (ret < 0)
 		goto out2;
 
@@ -3049,6 +3091,92 @@ int regulator_set_voltage(struct regulator *regulator, int min_uV, int max_uV)
 }
 EXPORT_SYMBOL_GPL(regulator_set_voltage);
 
+static inline int regulator_suspend_toggle(struct regulator_dev *rdev,
+					   suspend_state_t state, bool en)
+{
+	struct regulator_state *rstate;
+
+	rstate = regulator_get_suspend_state(rdev, state);
+	if (rstate == NULL)
+		return -EINVAL;
+
+	if (!rstate->changeable)
+		return -EINVAL;
+
+	rstate->enabled = en;
+
+	return 0;
+}
+
+static int regulator_suspend_enable(struct regulator_dev *rdev,
+				    suspend_state_t state)
+{
+	return regulator_suspend_toggle(rdev, state, true);
+}
+
+static int regulator_suspend_disable(struct regulator_dev *rdev,
+				     suspend_state_t state)
+{
+	struct regulator *regulator;
+	struct regulator_voltage *voltage;
+
+	/*
+	 * if any consumer wants this regulator device keeping on in
+	 * suspend states, don't set it as disabled.
+	 */
+	list_for_each_entry(regulator, &rdev->consumer_list, list) {
+		voltage = &regulator->voltage[state];
+		if (voltage->min_uV || voltage->max_uV)
+			return 0;
+	}
+
+	return regulator_suspend_toggle(rdev, state, false);
+}
+
+static int _regulator_set_suspend_voltage(struct regulator *regulator,
+					  int min_uV, int max_uV,
+					  suspend_state_t state)
+{
+	int ret;
+	struct regulator_dev *rdev = regulator->rdev;
+
+	/*
+	 * We assume users want to switch off the regulator device for
+	 * suspend state when setting min_uV and max_uV all with zero.
+	 */
+	if (min_uV == 0 && max_uV == 0) {
+		regulator->voltage[state].min_uV = 0;
+		regulator->voltage[state].max_uV = 0;
+		return regulator_suspend_disable(rdev, state);
+	}
+
+	ret = regulator_suspend_enable(rdev, state);
+	if (ret < 0)
+		return ret;
+
+	return regulator_set_voltage_unlocked(regulator, min_uV, max_uV, state);
+}
+
+int regulator_set_suspend_voltage(struct regulator *regulator, int min_uV,
+				  int max_uV, suspend_state_t state)
+{
+	int ret = 0;
+
+	/* PM_SUSPEND_ON is handled by regulator_set_voltage() */
+	if (regulator_check_states(state) || state == PM_SUSPEND_ON)
+		return -EINVAL;
+
+	regulator_lock_supply(regulator->rdev);
+
+	ret = _regulator_set_suspend_voltage(regulator, min_uV,
+					     max_uV, state);
+
+	regulator_unlock_supply(regulator->rdev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(regulator_set_suspend_voltage);
+
 /**
  * regulator_set_voltage_time - get raise/fall time
  * @regulator: regulator source
@@ -3923,12 +4051,6 @@ static void regulator_dev_release(struct device *dev)
 	kfree(rdev);
 }
 
-static struct class regulator_class = {
-	.name = "regulator",
-	.dev_release = regulator_dev_release,
-	.dev_groups = regulator_dev_groups,
-};
-
 static void rdev_init_debugfs(struct regulator_dev *rdev)
 {
 	struct device *parent = rdev->dev.parent;
@@ -4179,7 +4301,76 @@ void regulator_unregister(struct regulator_dev *rdev)
 }
 EXPORT_SYMBOL_GPL(regulator_unregister);
 
+static int _regulator_suspend_late(struct device *dev, void *data)
+{
+	struct regulator_dev *rdev = dev_to_rdev(dev);
+	suspend_state_t *state = data;
+	int ret;
 
+	mutex_lock(&rdev->mutex);
+	ret = suspend_set_state(rdev, *state);
+	mutex_unlock(&rdev->mutex);
+
+	return ret;
+}
+
+/**
+ * regulator_suspend_late - prepare regulators for system wide suspend
+ * @state: system suspend state
+ *
+ * Configure each regulator with it's suspend operating parameters for state.
+ */
+static int regulator_suspend_late(struct device *dev)
+{
+	suspend_state_t state = pm_suspend_target_state;
+
+	return class_for_each_device(&regulator_class, NULL, &state,
+				     _regulator_suspend_late);
+}
+static int _regulator_resume_early(struct device *dev, void *data)
+{
+	int ret = 0;
+	struct regulator_dev *rdev = dev_to_rdev(dev);
+	suspend_state_t *state = data;
+	struct regulator_state *rstate;
+
+	rstate = regulator_get_suspend_state(rdev, *state);
+	if (rstate == NULL)
+		return -EINVAL;
+
+	mutex_lock(&rdev->mutex);
+
+	if (rdev->desc->ops->resume_early &&
+	    (rstate->enabled == ENABLE_IN_SUSPEND ||
+	     rstate->enabled == DISABLE_IN_SUSPEND))
+		ret = rdev->desc->ops->resume_early(rdev);
+
+	mutex_unlock(&rdev->mutex);
+
+	return ret;
+}
+
+static int regulator_resume_early(struct device *dev)
+{
+	suspend_state_t state = pm_suspend_target_state;
+
+	return class_for_each_device(&regulator_class, NULL, &state,
+				     _regulator_resume_early);
+}
+
+static const struct dev_pm_ops __maybe_unused regulator_pm_ops = {
+	.suspend_late	= regulator_suspend_late,
+	.resume_early	= regulator_resume_early,
+};
+
+static struct class regulator_class = {
+	.name = "regulator",
+	.dev_release = regulator_dev_release,
+	.dev_groups = regulator_dev_groups,
+#ifdef CONFIG_PM_SLEEP
+	.pm = &regulator_pm_ops,
+#endif
+};
 /**
  * regulator_has_full_constraints - the system has fully specified constraints
  *
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 41dad42..7dc8fd2 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -188,6 +188,18 @@ static void of_get_regulation_constraints(struct device_node *np,
 					"regulator-suspend-microvolt", &pval))
 			suspend_state->uV = pval;
 
+		if (!of_property_read_u32(np, "regulator-suspend-min-microvolt",
+					  &pval))
+			suspend_state->min_uV = pval;
+
+		if (!of_property_read_u32(np, "regulator-suspend-max-microvolt",
+					  &pval))
+			suspend_state->max_uV = pval;
+
+		if (of_property_read_bool(suspend_np,
+					"regulator-changeable-in-suspend"))
+			suspend_state->changeable = true;
+
 		if (i == PM_SUSPEND_MEM)
 			constraints->initial_state = PM_SUSPEND_MEM;
 
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 94417b4..4c00486 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -214,6 +214,8 @@ struct regulator_ops {
 	/* set regulator suspend operating mode (defined in consumer.h) */
 	int (*set_suspend_mode) (struct regulator_dev *, unsigned int mode);
 
+	int (*resume_early)(struct regulator_dev *rdev);
+
 	int (*set_pull_down) (struct regulator_dev *);
 };
 
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index b4ddb56..f60ec1f 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -42,7 +42,12 @@ struct regulator;
 #define REGULATOR_CHANGE_DRMS		0x10
 #define REGULATOR_CHANGE_BYPASS		0x20
 
-/* operations in suspend mode */
+/*
+ * operations in suspend mode
+ * DO_NOTHING_IN_SUSPEND - the default value
+ * DISABLE_IN_SUSPEND	- turn off regulator in suspend states
+ * ENABLE_IN_SUSPEND	- keep regulator on in suspend states
+ */
 #define DO_NOTHING_IN_SUSPEND	(-1)
 #define DISABLE_IN_SUSPEND	0
 #define ENABLE_IN_SUSPEND	1
@@ -61,17 +66,24 @@ enum regulator_active_discharge {
  * state.  One of enabled or disabled must be set for the
  * configuration to be applied.
  *
- * @uV: Operating voltage during suspend.
+ * @uV: Default operating voltage during suspend, it can be adjusted
+ *	among <min_uV, max_uV>
+ * @min_uV: Minimum suspend voltage may be set.
+ * @max_uV: Maximum suspend voltage may be set.
  * @mode: Operating mode during suspend.
  * @enabled: operations during suspend.
  *	     - DO_NOTHING_IN_SUSPEND
  *	     - DISABLE_IN_SUSPEND
  *	     - ENABLE_IN_SUSPEND
+ * @changeable: Is this state can be changed.
  */
 struct regulator_state {
 	int uV;	/* suspend voltage */
+	int min_uV;
+	int max_uV;
 	unsigned int mode; /* suspend regulator operating mode */
 	int enabled; /* is regulator enabled in this suspend state */
+	bool changeable;
 };
 
 /**
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/5] bindings: regulator: added support for suspend states
       [not found]     ` <1513837506-26543-2-git-send-email-zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
@ 2017-12-21 23:26       ` Rob Herring
  2017-12-22  6:05         ` Chunyan Zhang
  0 siblings, 1 reply; 11+ messages in thread
From: Rob Herring @ 2017-12-21 23:26 UTC (permalink / raw)
  To: Chunyan Zhang
  Cc: Mark Brown, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Chunyan Zhang

On Thu, Dec 21, 2017 at 02:25:02PM +0800, Chunyan Zhang wrote:
> Documented a few new added properties which are used for supporting
> regulator suspend states.

Your commit message should answer why you need this. What problem do you 
have that this solves?

> 
> Signed-off-by: Chunyan Zhang <zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/regulator/regulator.txt | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
> index 378f6dc..618a322 100644
> --- a/Documentation/devicetree/bindings/regulator/regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/regulator.txt
> @@ -42,8 +42,15 @@ Optional properties:
>  - regulator-state-[mem/disk] node has following common properties:
>  	- regulator-on-in-suspend: regulator should be on in suspend state.
>  	- regulator-off-in-suspend: regulator should be off in suspend state.
> -	- regulator-suspend-microvolt: regulator should be set to this voltage
> -	  in suspend.
> +	- regulator-suspend-min-microvolt: minimum voltage may be set in
> +	  suspend state.
> +	- regulator-suspend-max-microvolt: maximum voltage may be set in
> +	  suspend state.
> +	- regulator-suspend-microvolt: the default voltage which regulator
> +	  should be set in suspend, this can be adjusted among
> +	  <regulator-suspend-min-microvolt, regulator-suspend-max-microvolt>

Perhaps this should stay a single property with: <target> <min> <max>

Though why would you ever not try to set to the minimum voltage within 
the range of <min> to <max>?

> +	- regulator-changeable-in-suspend: whether the default voltage and
> +	  the regulator on/off in suspend can be changed in runtime.
>  	- regulator-mode: operating mode in the given suspend state.
>  	  The set of possible operating modes depends on the capabilities of
>  	  every hardware so the valid modes are documented on each regulator
> -- 
> 2.7.4
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/5] bindings: regulator: added support for suspend states
  2017-12-21 23:26       ` Rob Herring
@ 2017-12-22  6:05         ` Chunyan Zhang
  2017-12-22 15:49           ` Mark Brown
  0 siblings, 1 reply; 11+ messages in thread
From: Chunyan Zhang @ 2017-12-22  6:05 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Brown, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Chunyan Zhang

Hi Rob,

On 22 December 2017 at 07:26, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Dec 21, 2017 at 02:25:02PM +0800, Chunyan Zhang wrote:
>> Documented a few new added properties which are used for supporting
>> regulator suspend states.
>
> Your commit message should answer why you need this. What problem do you
> have that this solves?
>
>>
>> Signed-off-by: Chunyan Zhang <zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/regulator/regulator.txt | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
>> index 378f6dc..618a322 100644
>> --- a/Documentation/devicetree/bindings/regulator/regulator.txt
>> +++ b/Documentation/devicetree/bindings/regulator/regulator.txt
>> @@ -42,8 +42,15 @@ Optional properties:
>>  - regulator-state-[mem/disk] node has following common properties:
>>       - regulator-on-in-suspend: regulator should be on in suspend state.
>>       - regulator-off-in-suspend: regulator should be off in suspend state.
>> -     - regulator-suspend-microvolt: regulator should be set to this voltage
>> -       in suspend.
>> +     - regulator-suspend-min-microvolt: minimum voltage may be set in
>> +       suspend state.
>> +     - regulator-suspend-max-microvolt: maximum voltage may be set in
>> +       suspend state.
>> +     - regulator-suspend-microvolt: the default voltage which regulator
>> +       should be set in suspend, this can be adjusted among
>> +       <regulator-suspend-min-microvolt, regulator-suspend-max-microvolt>
>
> Perhaps this should stay a single property with: <target> <min> <max>

Do you mean that change the property name from "regulator-suspend-microvolt" to
"regulator-suspend-target-microvolt"?

"regulator-suspend-microvolt" is the one that some SoC is using. My
intention was just to keep that configuration still working.

> Though why would you ever not try to set to the minimum voltage within
> the range of <min> to <max>?

IIUC, you mean just removing "regulator-suspend-microvolt", and use
"regulator-suspend-min-microvolt" as the default value for suspend
voltage?
I think that can work, but would it be better to not remove that right
now, since some one is using that?

Thanks for your review!

Chunyan

>
>> +     - regulator-changeable-in-suspend: whether the default voltage and
>> +       the regulator on/off in suspend can be changed in runtime.
>>       - regulator-mode: operating mode in the given suspend state.
>>         The set of possible operating modes depends on the capabilities of
>>         every hardware so the valid modes are documented on each regulator
>> --
>> 2.7.4
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/5] bindings: regulator: added support for suspend states
  2017-12-22  6:05         ` Chunyan Zhang
@ 2017-12-22 15:49           ` Mark Brown
  0 siblings, 0 replies; 11+ messages in thread
From: Mark Brown @ 2017-12-22 15:49 UTC (permalink / raw)
  To: Chunyan Zhang; +Cc: Rob Herring, devicetree, linux-kernel, Chunyan Zhang

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

On Fri, Dec 22, 2017 at 02:05:21PM +0800, Chunyan Zhang wrote:
> On 22 December 2017 at 07:26, Rob Herring <robh@kernel.org> wrote:
> > On Thu, Dec 21, 2017 at 02:25:02PM +0800, Chunyan Zhang wrote:

> >> +     - regulator-suspend-microvolt: the default voltage which regulator
> >> +       should be set in suspend, this can be adjusted among
> >> +       <regulator-suspend-min-microvolt, regulator-suspend-max-microvolt>

> > Perhaps this should stay a single property with: <target> <min> <max>

> Do you mean that change the property name from "regulator-suspend-microvolt" to
> "regulator-suspend-target-microvolt"?

> "regulator-suspend-microvolt" is the one that some SoC is using. My
> intention was just to keep that configuration still working.

Yeah, the regulator-suspend-microvolt is an existing property.  Thinking
about it we should probably say that it's equivalent to setting both
-min and -max to the same value and possibly mark it as deprecated too.
It is documented in regulator.txt already.

> > Though why would you ever not try to set to the minimum voltage within
> > the range of <min> to <max>?

> IIUC, you mean just removing "regulator-suspend-microvolt", and use
> "regulator-suspend-min-microvolt" as the default value for suspend
> voltage?
> I think that can work, but would it be better to not remove that right
> now, since some one is using that?

Indeed.  I think my suggestion above would cover things.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Applied "regulator: empty the old suspend functions" to the regulator tree
       [not found]     ` <1513837506-26543-5-git-send-email-zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
@ 2018-01-25 12:15       ` Mark Brown
  2018-01-26 15:26       ` Mark Brown
  1 sibling, 0 replies; 11+ messages in thread
From: Mark Brown @ 2018-01-25 12:15 UTC (permalink / raw)
  To: Chunyan Zhang; +Cc: Mark Brown

The patch

   regulator: empty the old suspend functions

has been applied to the regulator tree at

   https://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 6103427372a81f7018525ca84c0647f36532a94e Mon Sep 17 00:00:00 2001
From: Chunyan Zhang <zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Date: Tue, 23 Jan 2018 19:59:42 +0800
Subject: [PATCH] regulator: empty the old suspend functions

Regualtor suspend/resume functions should only be called by PM suspend
core via registering dev_pm_ops, and regulator devices should implement
the callback functions.  Thus, any regulator consumer shouldn't call
the regulator suspend/resume functions directly.

In order to avoid compile errors, two empty functions with the same name
still be left for the time being.

Signed-off-by: Chunyan Zhang <zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/regulator/core.c          | 74 ---------------------------------------
 include/linux/regulator/machine.h |  5 ++-
 2 files changed, 2 insertions(+), 77 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 5ea80e94eb69..080c2334edc5 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4179,80 +4179,6 @@ void regulator_unregister(struct regulator_dev *rdev)
 }
 EXPORT_SYMBOL_GPL(regulator_unregister);
 
-static int _regulator_suspend_prepare(struct device *dev, void *data)
-{
-	struct regulator_dev *rdev = dev_to_rdev(dev);
-	const suspend_state_t *state = data;
-	int ret;
-
-	mutex_lock(&rdev->mutex);
-	ret = suspend_prepare(rdev, *state);
-	mutex_unlock(&rdev->mutex);
-
-	return ret;
-}
-
-/**
- * regulator_suspend_prepare - prepare regulators for system wide suspend
- * @state: system suspend state
- *
- * Configure each regulator with it's suspend operating parameters for state.
- * This will usually be called by machine suspend code prior to supending.
- */
-int regulator_suspend_prepare(suspend_state_t state)
-{
-	/* ON is handled by regulator active state */
-	if (state == PM_SUSPEND_ON)
-		return -EINVAL;
-
-	return class_for_each_device(&regulator_class, NULL, &state,
-				     _regulator_suspend_prepare);
-}
-EXPORT_SYMBOL_GPL(regulator_suspend_prepare);
-
-static int _regulator_suspend_finish(struct device *dev, void *data)
-{
-	struct regulator_dev *rdev = dev_to_rdev(dev);
-	int ret;
-
-	mutex_lock(&rdev->mutex);
-	if (rdev->use_count > 0  || rdev->constraints->always_on) {
-		if (!_regulator_is_enabled(rdev)) {
-			ret = _regulator_do_enable(rdev);
-			if (ret)
-				dev_err(dev,
-					"Failed to resume regulator %d\n",
-					ret);
-		}
-	} else {
-		if (!have_full_constraints())
-			goto unlock;
-		if (!_regulator_is_enabled(rdev))
-			goto unlock;
-
-		ret = _regulator_do_disable(rdev);
-		if (ret)
-			dev_err(dev, "Failed to suspend regulator %d\n", ret);
-	}
-unlock:
-	mutex_unlock(&rdev->mutex);
-
-	/* Keep processing regulators in spite of any errors */
-	return 0;
-}
-
-/**
- * regulator_suspend_finish - resume regulators from system wide suspend
- *
- * Turn on regulators that might be turned off by regulator_suspend_prepare
- * and that should be turned on according to the regulators properties.
- */
-int regulator_suspend_finish(void)
-{
-	return class_for_each_device(&regulator_class, NULL, NULL,
-				     _regulator_suspend_finish);
-}
-EXPORT_SYMBOL_GPL(regulator_suspend_finish);
 
 /**
  * regulator_has_full_constraints - the system has fully specified constraints
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index e50519f13d4d..b4ddb56e7c19 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -231,12 +231,12 @@ struct regulator_init_data {
 
 #ifdef CONFIG_REGULATOR
 void regulator_has_full_constraints(void);
-int regulator_suspend_prepare(suspend_state_t state);
-int regulator_suspend_finish(void);
 #else
 static inline void regulator_has_full_constraints(void)
 {
 }
+#endif
+
 static inline int regulator_suspend_prepare(suspend_state_t state)
 {
 	return 0;
@@ -245,6 +245,5 @@ static inline int regulator_suspend_finish(void)
 {
 	return 0;
 }
-#endif
 
 #endif
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Applied "regulator: empty the old suspend functions" to the regulator tree
       [not found]     ` <1513837506-26543-5-git-send-email-zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
  2018-01-25 12:15       ` Applied "regulator: empty the old suspend functions" to the regulator tree Mark Brown
@ 2018-01-26 15:26       ` Mark Brown
  1 sibling, 0 replies; 11+ messages in thread
From: Mark Brown @ 2018-01-26 15:26 UTC (permalink / raw)
  To: Chunyan Zhang; +Cc: Mark Brown

The patch

   regulator: empty the old suspend functions

has been applied to the regulator tree at

   https://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 aa27bbc6c6c60227c096d515f55ffe6cdfef7d2b Mon Sep 17 00:00:00 2001
From: Chunyan Zhang <zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Date: Fri, 26 Jan 2018 21:08:46 +0800
Subject: [PATCH] regulator: empty the old suspend functions

Regualtor suspend/resume functions should only be called by PM suspend
core via registering dev_pm_ops, and regulator devices should implement
the callback functions.  Thus, any regulator consumer shouldn't call
the regulator suspend/resume functions directly.

In order to avoid compile errors, two empty functions with the same name
still be left for the time being.

Signed-off-by: Chunyan Zhang <zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/regulator/core.c          | 74 ---------------------------------------
 include/linux/regulator/machine.h |  5 ++-
 2 files changed, 2 insertions(+), 77 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 5ea80e94eb69..080c2334edc5 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4179,80 +4179,6 @@ void regulator_unregister(struct regulator_dev *rdev)
 }
 EXPORT_SYMBOL_GPL(regulator_unregister);
 
-static int _regulator_suspend_prepare(struct device *dev, void *data)
-{
-	struct regulator_dev *rdev = dev_to_rdev(dev);
-	const suspend_state_t *state = data;
-	int ret;
-
-	mutex_lock(&rdev->mutex);
-	ret = suspend_prepare(rdev, *state);
-	mutex_unlock(&rdev->mutex);
-
-	return ret;
-}
-
-/**
- * regulator_suspend_prepare - prepare regulators for system wide suspend
- * @state: system suspend state
- *
- * Configure each regulator with it's suspend operating parameters for state.
- * This will usually be called by machine suspend code prior to supending.
- */
-int regulator_suspend_prepare(suspend_state_t state)
-{
-	/* ON is handled by regulator active state */
-	if (state == PM_SUSPEND_ON)
-		return -EINVAL;
-
-	return class_for_each_device(&regulator_class, NULL, &state,
-				     _regulator_suspend_prepare);
-}
-EXPORT_SYMBOL_GPL(regulator_suspend_prepare);
-
-static int _regulator_suspend_finish(struct device *dev, void *data)
-{
-	struct regulator_dev *rdev = dev_to_rdev(dev);
-	int ret;
-
-	mutex_lock(&rdev->mutex);
-	if (rdev->use_count > 0  || rdev->constraints->always_on) {
-		if (!_regulator_is_enabled(rdev)) {
-			ret = _regulator_do_enable(rdev);
-			if (ret)
-				dev_err(dev,
-					"Failed to resume regulator %d\n",
-					ret);
-		}
-	} else {
-		if (!have_full_constraints())
-			goto unlock;
-		if (!_regulator_is_enabled(rdev))
-			goto unlock;
-
-		ret = _regulator_do_disable(rdev);
-		if (ret)
-			dev_err(dev, "Failed to suspend regulator %d\n", ret);
-	}
-unlock:
-	mutex_unlock(&rdev->mutex);
-
-	/* Keep processing regulators in spite of any errors */
-	return 0;
-}
-
-/**
- * regulator_suspend_finish - resume regulators from system wide suspend
- *
- * Turn on regulators that might be turned off by regulator_suspend_prepare
- * and that should be turned on according to the regulators properties.
- */
-int regulator_suspend_finish(void)
-{
-	return class_for_each_device(&regulator_class, NULL, NULL,
-				     _regulator_suspend_finish);
-}
-EXPORT_SYMBOL_GPL(regulator_suspend_finish);
 
 /**
  * regulator_has_full_constraints - the system has fully specified constraints
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index ce89c5548c89..c4a56df8931b 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -236,12 +236,12 @@ struct regulator_init_data {
 
 #ifdef CONFIG_REGULATOR
 void regulator_has_full_constraints(void);
-int regulator_suspend_prepare(suspend_state_t state);
-int regulator_suspend_finish(void);
 #else
 static inline void regulator_has_full_constraints(void)
 {
 }
+#endif
+
 static inline int regulator_suspend_prepare(suspend_state_t state)
 {
 	return 0;
@@ -250,6 +250,5 @@ static inline int regulator_suspend_finish(void)
 {
 	return 0;
 }
-#endif
 
 #endif
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-01-26 15:26 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-21  6:25 [PATCH 0/5] Add regulator suspend and resume support Chunyan Zhang
2017-12-21  6:25 ` [PATCH 3/5] drivers: regulator: leave one item to record whether regulator is enabled Chunyan Zhang
     [not found] ` <1513837506-26543-1-git-send-email-zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2017-12-21  6:25   ` [PATCH 1/5] bindings: regulator: added support for suspend states Chunyan Zhang
     [not found]     ` <1513837506-26543-2-git-send-email-zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2017-12-21 23:26       ` Rob Herring
2017-12-22  6:05         ` Chunyan Zhang
2017-12-22 15:49           ` Mark Brown
2017-12-21  6:25   ` [PATCH 2/5] regulator: make regulator voltage be an array to support more states Chunyan Zhang
2017-12-21  6:25   ` [PATCH 4/5] drivers: regulator: empty the old suspend functions Chunyan Zhang
     [not found]     ` <1513837506-26543-5-git-send-email-zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2018-01-25 12:15       ` Applied "regulator: empty the old suspend functions" to the regulator tree Mark Brown
2018-01-26 15:26       ` Mark Brown
2017-12-21  6:25   ` [PATCH 5/5] regulator: add PM suspend and resume hooks Chunyan Zhang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).