[PATCHv4 0/2] regulator: of: Add support for pasing regulator suspend state

[PATCHv4 0/2] regulator: of: Add support for pasing regulator suspend state

[Chanwoo Choi]

The regulators would set different state/mode according to the kind of suspend
state. So regulation_constraints structure has already regulator suspend state filed.
This patch parse regulator suspend state from devicetree file.

For example:

	ldoX_reg: LDOx {
		regulator-name = "VAP_XXX_1.2V";
		regulator-min-microvolt = <1200000>;
		regulator-max-microvolt = <1200000>;
		regulator-always-on;

		regulator-initial-state = <3>;	/* PM_SUSPEND_MEM */
		regulator-state-mem {
			regulator-off-in-suspend;
		};

		regulator-state-disk {
			regulator-volt = <1200000>;
			regulator-on-in-suspend;
		};
	};

Changes from v3:
- Don't support 'regulator-state-standby' mode
- Remove 'regulator-mode' property

Changes from v2:
- Fix over 80 lines by using checkpatch script
- Rebase this patchset on latest for-next branch of regulator.git

Changes from v1:
- Check whether regulator-initial-state and regulator-mode is correct or not
- Add more detailed description about regulator-initial-state, regulator-mode
  and regulator-state-[standby/mem/disk] for devicetree bindings
- Modify example of regulator suspend state in bindings documentation

Chanwoo Choi (2):
  regulator: of: Add support for parsing regulator_state for suspend state
  dt-bindings: regulator: Add regulator suspend state for PM state

 .../devicetree/bindings/regulator/regulator.txt    | 22 ++++++++
 drivers/regulator/of_regulator.c                   | 65 +++++++++++++++++++++-
 2 files changed, 85 insertions(+), 2 deletions(-)

-- 
1.8.0

[PATCHv4 1/2] regulator: of: Add support for parsing regulator_state for suspend state

[Chanwoo Choi]

The regulation_constraints structure includes specific field to support
suspend state for global PMIC SUSPEND/HIBERNATE mode. This patch add support
for parsing regulator_state for suspend state.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/regulator/of_regulator.c | 65 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 63 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index ee5e67b..5fe5748 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -16,12 +16,19 @@
 #include <linux/regulator/machine.h>
 #include <linux/regulator/of_regulator.h>
 
+const char *const regulator_states[PM_SUSPEND_MAX + 1] = {
+	[PM_SUSPEND_MEM]	= "regulator-state-mem",
+	[PM_SUSPEND_MAX]	= "regulator-state-disk",
+};
+
 static void of_get_regulation_constraints(struct device_node *np,
 					struct regulator_init_data **init_data)
 {
-	const __be32 *min_uV, *max_uV;
+	const __be32 *min_uV, *max_uV, *suspend_uV;
 	struct regulation_constraints *constraints = &(*init_data)->constraints;
-	int ret;
+	struct regulator_state *suspend_state;
+	struct device_node *suspend_np;
+	int ret, i;
 	u32 pval;
 
 	constraints->name = of_get_property(np, "regulator-name", NULL);
@@ -70,6 +77,60 @@ static void of_get_regulation_constraints(struct device_node *np,
 	ret = of_property_read_u32(np, "regulator-enable-ramp-delay", &pval);
 	if (!ret)
 		constraints->enable_time = pval;
+
+	ret = of_property_read_u32(np, "regulator-initial-state", &pval);
+	if (!ret) {
+		switch (pval) {
+		case PM_SUSPEND_MEM:
+		case PM_SUSPEND_MAX:
+			constraints->initial_state = pval;
+			break;
+		default:
+			break;
+		};
+	}
+
+	for (i = 0; i < ARRAY_SIZE(regulator_states); i++) {
+		switch (i) {
+		case PM_SUSPEND_MEM:
+			suspend_state = &constraints->state_mem;
+			break;
+		case PM_SUSPEND_MAX:
+			suspend_state = &constraints->state_disk;
+			break;
+		case PM_SUSPEND_ON:
+		case PM_SUSPEND_FREEZE:
+		case PM_SUSPEND_STANDBY:
+		default:
+			continue;
+		};
+
+		suspend_np = of_get_child_by_name(np, regulator_states[i]);
+		if (!suspend_np || !suspend_state)
+			continue;
+
+		suspend_uV = of_get_property(suspend_np, "regulator-volt",
+						NULL);
+		if (suspend_uV) {
+			suspend_state->uV = be32_to_cpu(*suspend_uV);
+
+			if (suspend_state->uV < constraints->min_uV)
+				suspend_state->uV = constraints->min_uV;
+			if (suspend_state->uV > constraints->max_uV)
+				suspend_state->uV = constraints->max_uV;
+		}
+
+		if (of_property_read_bool(suspend_np,
+					"regulator-on-in-suspend"))
+			suspend_state->enabled = true;
+
+		if (of_property_read_bool(suspend_np,
+					"regulator-off-in-suspend"))
+			suspend_state->disabled = true;
+
+		suspend_state = NULL;
+		suspend_np = NULL;
+	}
 }
 
 /**
-- 
1.8.0

Re: [PATCHv4 1/2] regulator: of: Add support for parsing regulator_state for suspend state

[Mark Brown]

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

On Mon, Aug 18, 2014 at 03:27:02PM +0900, Chanwoo Choi wrote:

> +		suspend_uV = of_get_property(suspend_np, "regulator-volt",
> +						NULL);
> +		if (suspend_uV) {
> +			suspend_state->uV = be32_to_cpu(*suspend_uV);
> +
> +			if (suspend_state->uV < constraints->min_uV)
> +				suspend_state->uV = constraints->min_uV;
> +			if (suspend_state->uV > constraints->max_uV)
> +				suspend_state->uV = constraints->max_uV;
> +		}

A few things here.  One is that this will fail if we don't have a
voltage range specified at runtime.  It is possible that the user needs
to override the voltage in suspend mode but not at runtime.  Perhaps we
want to make them specify the runtime voltage anyway but if we do then
the binding needs to say that this is mandatory even if it's just
restating the hardware default.

It's also not clear to me that the suspend voltage needs to be in the
range that we can vary at runtime, especially if we're not giving
permission for runtime variation.  From that point of view we should
probably just not check, but if we are going to check we should print a
warning that we're doing it so people know that what they set in the DT
isn't being followed.

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

Re: [PATCHv4 1/2] regulator: of: Add support for parsing regulator_state for suspend state

[Chanwoo Choi]

Dear Mark,

On 09/05/2014 07:49 AM, Mark Brown wrote:
> On Mon, Aug 18, 2014 at 03:27:02PM +0900, Chanwoo Choi wrote:
> 
>> +		suspend_uV = of_get_property(suspend_np, "regulator-volt",
>> +						NULL);
>> +		if (suspend_uV) {
>> +			suspend_state->uV = be32_to_cpu(*suspend_uV);
>> +
>> +			if (suspend_state->uV < constraints->min_uV)
>> +				suspend_state->uV = constraints->min_uV;
>> +			if (suspend_state->uV > constraints->max_uV)
>> +				suspend_state->uV = constraints->max_uV;
>> +		}
> 
> A few things here.  One is that this will fail if we don't have a
> voltage range specified at runtime.  It is possible that the user needs
> to override the voltage in suspend mode but not at runtime.  Perhaps we
> want to make them specify the runtime voltage anyway but if we do then
> the binding needs to say that this is mandatory even if it's just
> restating the hardware default.
> 
> It's also not clear to me that the suspend voltage needs to be in the
> range that we can vary at runtime, especially if we're not giving
> permission for runtime variation.  From that point of view we should
> probably just not check, but if we are going to check we should print a
> warning that we're doing it so people know that what they set in the DT
> isn't being followed.
> 

Firstly, I'm so sorry about late reply because of vacation.

I'll drop 'regulator-volt' property because of remaining with not clear state.

Next patchset (v5) will just include 'regulator-{on,off}-in-suspend'
without 'regulator-volt' property.

Best Regards,
Chanwoo Choi

[PATCHv4 2/2] dt-bindings: regulator: Add regulator suspend state for PM state

[Chanwoo Choi]

This patch add regulator suspend state to constraint in dt file. The regulation_
constraints structure already has regulator suspend state field as following.
The regulator suspend state control the state of regulator according to
PM (Power Management) state.
- struct regulator_state state_disk
- struct regulator_state state_mem

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 .../devicetree/bindings/regulator/regulator.txt    | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
index 8607433..ccba90b 100644
--- a/Documentation/devicetree/bindings/regulator/regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/regulator.txt
@@ -19,6 +19,23 @@ Optional properties:
   design requires. This property describes the total system ramp time
   required due to the combination of internal ramping of the regulator itself,
   and board design issues such as trace capacitance and load on the supply.
+- regulator-initial-state: initial state for suspend state, cnd set initial
+  state among following defined suspend states:
+  <3>: PM_SUSPEND_MEM - Setup regulator according to regulator-state-mem
+  <4>: PM_SUSPEND_MAX - Setup regulator according to regulator-state-disk
+- regulator-state-mem sub-root node for Suspend-to-RAM mode
+  : suspend to memory, the device goes to sleep, but all data stored in memory,
+  only some external interrupt can wake the device.
+- regulator-state-disk sub-root node for Suspend-to-disk mode
+  : suspend to disk, this state operates similarly to Suspend-to-RAM,
+  but includes a final step of writing memory contents to disk.
+- regulator-state-[mem/disk] node has following common properties:
+	- regulator-volt: voltage consumers may set in suspend state.
+	- regulator-on-in-suspend: regulator should be on in suspend state.
+	- regulator-off-in-suspend: regulator should be off in suspend state.
+	If node don't include regulator-[on/off]-in-suspend, can't change
+	regulator state in suspend mode and only should sustain the regulator
+	state of normal state.
 
 Deprecated properties:
 - regulator-compatible: If a regulator chip contains multiple
@@ -34,6 +51,11 @@ Example:
 		regulator-max-microvolt = <2500000>;
 		regulator-always-on;
 		vin-supply = <&vin>;
+
+		regulator-state-mem {
+			regulator-volt = <1000000>;
+			regulator-on-in-suspend;
+		};
 	};
 
 Regulator Consumers:
-- 
1.8.0

Re: [PATCHv4 2/2] dt-bindings: regulator: Add regulator suspend state for PM state

[Mark Brown]

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

On Mon, Aug 18, 2014 at 03:27:03PM +0900, Chanwoo Choi wrote:

> +- regulator-initial-state: initial state for suspend state, cnd set initial
> +  state among following defined suspend states:
> +  <3>: PM_SUSPEND_MEM - Setup regulator according to regulator-state-mem
> +  <4>: PM_SUSPEND_MAX - Setup regulator according to regulator-state-disk

Please add defines for these in include/dt-bindings.  Probably worth
pointing out that if absent the state is the hardware default (same for
most of these.

> +- regulator-state-[mem/disk] node has following common properties:
> +	- regulator-volt: voltage consumers may set in suspend state.

We use -uV rather than -volt in other bits of the bindings.

> +	- regulator-on-in-suspend: regulator should be on in suspend state.
> +	- regulator-off-in-suspend: regulator should be off in suspend state.
> +	If node don't include regulator-[on/off]-in-suspend, can't change
> +	regulator state in suspend mode and only should sustain the regulator
> +	state of normal state.

Probably better to say that the OS will not manage the state here -
otherwise we have to explicitly override any hardware default that's
there which drivers and the core don't do right now.

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