[PATCH 5/5] ARM: dts: Add initial regulator mode on exynos Peach boards

[PATCH 5/5] ARM: dts: Add initial regulator mode on exynos Peach boards

[Javier Martinez Canillas]

The regulator core now has support to choose a default initial
operating mode for regulators from DT. Set the initial opmode
for the max77802 PMIC regulators with the same modes that are
used in the downstream ChromeOS kernel, in order to allow the
system to lower power at suspend time.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 arch/arm/boot/dts/exynos5420-peach-pit.dts | 26 ++++++++++++++++++++++++++
 arch/arm/boot/dts/exynos5800-peach-pi.dts  | 26 ++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts
index 9a050e1..f7eb70d 100644
--- a/arch/arm/boot/dts/exynos5420-peach-pit.dts
+++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts
@@ -13,6 +13,7 @@
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/clock/maxim,max77802.h>
+#include <dt-bindings/regulator/regulator.h>
 #include "exynos5420.dtsi"
 
 / {
@@ -192,6 +193,7 @@
 				regulator-always-on;
 				regulator-boot-on;
 				regulator-ramp-delay = <12500>;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			buck2_reg: BUCK2 {
@@ -201,6 +203,7 @@
 				regulator-always-on;
 				regulator-boot-on;
 				regulator-ramp-delay = <12500>;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			buck3_reg: BUCK3 {
@@ -210,6 +213,7 @@
 				regulator-always-on;
 				regulator-boot-on;
 				regulator-ramp-delay = <12500>;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			buck4_reg: BUCK4 {
@@ -219,6 +223,7 @@
 				regulator-always-on;
 				regulator-boot-on;
 				regulator-ramp-delay = <12500>;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			buck5_reg: BUCK5 {
@@ -227,6 +232,7 @@
 				regulator-max-microvolt = <1200000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			buck6_reg: BUCK6 {
@@ -236,6 +242,7 @@
 				regulator-always-on;
 				regulator-boot-on;
 				regulator-ramp-delay = <12500>;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			buck7_reg: BUCK7 {
@@ -244,6 +251,7 @@
 				regulator-max-microvolt = <1350000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-initial-mode = <REGULATOR_MODE_NORMAL>;
 			};
 
 			buck8_reg: BUCK8 {
@@ -252,6 +260,7 @@
 				regulator-max-microvolt = <2850000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			buck9_reg: BUCK9 {
@@ -260,6 +269,7 @@
 				regulator-max-microvolt = <2000000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-initial-mode = <REGULATOR_MODE_NORMAL>;
 			};
 
 			buck10_reg: BUCK10 {
@@ -268,6 +278,7 @@
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-initial-mode = <REGULATOR_MODE_NORMAL>;
 			};
 
 			ldo1_reg: LDO1 {
@@ -275,6 +286,7 @@
 				regulator-min-microvolt = <1000000>;
 				regulator-max-microvolt = <1000000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_IDLE>;
 			};
 
 			ldo2_reg: LDO2 {
@@ -288,6 +300,7 @@
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_IDLE>;
 			};
 
 			vqmmc_sdcard: ldo4_reg: LDO4 {
@@ -295,6 +308,7 @@
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <2800000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo5_reg: LDO5 {
@@ -302,6 +316,7 @@
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo6_reg: LDO6 {
@@ -309,6 +324,7 @@
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo7_reg: LDO7 {
@@ -322,6 +338,7 @@
 				regulator-min-microvolt = <1000000>;
 				regulator-max-microvolt = <1000000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo9_reg: LDO9 {
@@ -329,6 +346,7 @@
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_IDLE>;
 			};
 
 			ldo10_reg: LDO10 {
@@ -336,6 +354,7 @@
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo11_reg: LDO11 {
@@ -343,6 +362,7 @@
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_IDLE>;
 			};
 
 			ldo12_reg: LDO12 {
@@ -350,6 +370,7 @@
 				regulator-min-microvolt = <3000000>;
 				regulator-max-microvolt = <3000000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo13_reg: LDO13 {
@@ -357,6 +378,7 @@
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_IDLE>;
 			};
 
 			ldo14_reg: LDO14 {
@@ -364,6 +386,7 @@
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo15_reg: LDO15 {
@@ -371,6 +394,7 @@
 				regulator-min-microvolt = <1000000>;
 				regulator-max-microvolt = <1000000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo17_reg: LDO17 {
@@ -378,6 +402,7 @@
 				regulator-min-microvolt = <900000>;
 				regulator-max-microvolt = <1400000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo18_reg: LDO18 {
@@ -451,6 +476,7 @@
 				regulator-min-microvolt = <1000000>;
 				regulator-max-microvolt = <1000000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo32_reg: LDO32 {
diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts
index e8fdda8..3da8084 100644
--- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
+++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
@@ -13,6 +13,7 @@
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/clock/maxim,max77802.h>
+#include <dt-bindings/regulator/regulator.h>
 #include "exynos5800.dtsi"
 
 / {
@@ -191,6 +192,7 @@
 				regulator-always-on;
 				regulator-boot-on;
 				regulator-ramp-delay = <12500>;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			buck2_reg: BUCK2 {
@@ -200,6 +202,7 @@
 				regulator-always-on;
 				regulator-boot-on;
 				regulator-ramp-delay = <12500>;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			buck3_reg: BUCK3 {
@@ -209,6 +212,7 @@
 				regulator-always-on;
 				regulator-boot-on;
 				regulator-ramp-delay = <12500>;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			buck4_reg: BUCK4 {
@@ -218,6 +222,7 @@
 				regulator-always-on;
 				regulator-boot-on;
 				regulator-ramp-delay = <12500>;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			buck5_reg: BUCK5 {
@@ -226,6 +231,7 @@
 				regulator-max-microvolt = <1200000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			buck6_reg: BUCK6 {
@@ -235,6 +241,7 @@
 				regulator-always-on;
 				regulator-boot-on;
 				regulator-ramp-delay = <12500>;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			buck7_reg: BUCK7 {
@@ -243,6 +250,7 @@
 				regulator-max-microvolt = <1350000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-initial-mode = <REGULATOR_MODE_NORMAL>;
 			};
 
 			buck8_reg: BUCK8 {
@@ -251,6 +259,7 @@
 				regulator-max-microvolt = <2850000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			buck9_reg: BUCK9 {
@@ -259,6 +268,7 @@
 				regulator-max-microvolt = <2000000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-initial-mode = <REGULATOR_MODE_NORMAL>;
 			};
 
 			buck10_reg: BUCK10 {
@@ -267,6 +277,7 @@
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-initial-mode = <REGULATOR_MODE_NORMAL>;
 			};
 
 			ldo1_reg: LDO1 {
@@ -274,6 +285,7 @@
 				regulator-min-microvolt = <1000000>;
 				regulator-max-microvolt = <1000000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_IDLE>;
 			};
 
 			ldo2_reg: LDO2 {
@@ -287,6 +299,7 @@
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_IDLE>;
 			};
 
 			vqmmc_sdcard: ldo4_reg: LDO4 {
@@ -294,6 +307,7 @@
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <2800000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo5_reg: LDO5 {
@@ -301,6 +315,7 @@
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo6_reg: LDO6 {
@@ -308,6 +323,7 @@
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo7_reg: LDO7 {
@@ -321,6 +337,7 @@
 				regulator-min-microvolt = <1000000>;
 				regulator-max-microvolt = <1000000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo9_reg: LDO9 {
@@ -328,6 +345,7 @@
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_IDLE>;
 			};
 
 			ldo10_reg: LDO10 {
@@ -335,6 +353,7 @@
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo11_reg: LDO11 {
@@ -342,6 +361,7 @@
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_IDLE>;
 			};
 
 			ldo12_reg: LDO12 {
@@ -349,6 +369,7 @@
 				regulator-min-microvolt = <3000000>;
 				regulator-max-microvolt = <3000000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo13_reg: LDO13 {
@@ -356,6 +377,7 @@
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_IDLE>;
 			};
 
 			ldo14_reg: LDO14 {
@@ -363,6 +385,7 @@
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo15_reg: LDO15 {
@@ -370,6 +393,7 @@
 				regulator-min-microvolt = <1000000>;
 				regulator-max-microvolt = <1000000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo17_reg: LDO17 {
@@ -377,6 +401,7 @@
 				regulator-min-microvolt = <900000>;
 				regulator-max-microvolt = <1400000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo18_reg: LDO18 {
@@ -450,6 +475,7 @@
 				regulator-min-microvolt = <1000000>;
 				regulator-max-microvolt = <1000000>;
 				regulator-always-on;
+				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
 			};
 
 			ldo32_reg: LDO32 {
-- 
2.1.0

Re: [PATCH 5/5] ARM: dts: Add initial regulator mode on exynos Peach boards

[Mark Brown]

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

On Wed, Oct 08, 2014 at 03:44:07PM +0200, Javier Martinez Canillas wrote:

> The regulator core now has support to choose a default initial
> operating mode for regulators from DT. Set the initial opmode
> for the max77802 PMIC regulators with the same modes that are
> used in the downstream ChromeOS kernel, in order to allow the
> system to lower power at suspend time.

The stated goal of this change doesn't appear to correspond to what it
actually does.  It's saying that we want to be able to set the regulator
into low power modes on suspend which is a sensible and useful thing to
want to do (especially for regulators which don't have physical suspend
modes which we currently make any effort to handle) but what the change
actually does is cause us to set the default state of supplies on boot.

The device tree should describe what it's trying to achieve, otherwise
even if things happen to work right now it's going to be vulnerable to
being broken by future kernel changes which take what it's saying at
face value.

>  			buck2_reg: BUCK2 {
> @@ -201,6 +203,7 @@
>  				regulator-always-on;
>  				regulator-boot-on;
>  				regulator-ramp-delay = <12500>;
> +				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
>  			};

This appears to set the supply which is labelled as VDD_ARM into standby
mode by default (from a first glance the change appears to set all
supplies into standby mode) and of course we currently have no way of
changing the mode at runtime in DT systems.  Are you *really* sure this
is a good idea of which an electrical engineer would approve, CPU cores
are typically one of the most demanding workloads available for a
regulator?

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

Re: [PATCH 5/5] ARM: dts: Add initial regulator mode on exynos Peach boards

[Javier Martinez Canillas]

On 10/08/2014 04:56 PM, Mark Brown wrote:
> On Wed, Oct 08, 2014 at 03:44:07PM +0200, Javier Martinez Canillas wrote:
> 
>> The regulator core now has support to choose a default initial
>> operating mode for regulators from DT. Set the initial opmode
>> for the max77802 PMIC regulators with the same modes that are
>> used in the downstream ChromeOS kernel, in order to allow the
>> system to lower power at suspend time.
> 
> The stated goal of this change doesn't appear to correspond to what it
> actually does.  It's saying that we want to be able to set the regulator
> into low power modes on suspend which is a sensible and useful thing to
> want to do (especially for regulators which don't have physical suspend
> modes which we currently make any effort to handle) but what the change
> actually does is cause us to set the default state of supplies on boot.
> 

Well, setting a regulator into low power mode on suspend is something
that Chanwoo's series tried to address. At least on v3 since he removed
regulator-mode property for the regulator-state-{standby,mem,disk} on v4.

What this series tried to solve is when you have to set a opmode on boot
to change how the physical suspend is managed by the PMIC.

In the Maxim77802 PMIC is even trickier since not all the modes have the
semantics. That is the value 0x1 could have different meanings depending
of the regulator.

> The device tree should describe what it's trying to achieve, otherwise
> even if things happen to work right now it's going to be vulnerable to
> being broken by future kernel changes which take what it's saying at
> face value.
> 
>>  			buck2_reg: BUCK2 {
>> @@ -201,6 +203,7 @@
>>  				regulator-always-on;
>>  				regulator-boot-on;
>>  				regulator-ramp-delay = <12500>;
>> +				regulator-initial-mode = <REGULATOR_MODE_STANDBY>;
>>  			};
> 
> This appears to set the supply which is labelled as VDD_ARM into standby
> mode by default (from a first glance the change appears to set all
> supplies into standby mode) and of course we currently have no way of
> changing the mode at runtime in DT systems.  Are you *really* sure this
> is a good idea of which an electrical engineer would approve, CPU cores
> are typically one of the most demanding workloads available for a
> regulator?
> 

Well, the standby mode for this regulator is actually:

Output ON/OFF Controlled by PWRREQ
	PWRREQ=HIGH (1): Output ON in Normal Mode
	PWRREQ=LOW (0): Output OFF

this means that when the Soc enters into suspend mode a pin is toggled
and that pin is connected to the PWRREQ line in the PMIC to signal that
the SoC has entered in sleep mode so the regulator has to be OFF.

When the SoC leaves sleep mode and is resumed again, the pin is toggled
so the PMIC puts the regulator in ON again.

There is other mode that instead of turning off the regulator, keeps it
enabled but in low power mode.

Best regards,
Javier

Re: [PATCH 5/5] ARM: dts: Add initial regulator mode on exynos Peach boards

[Javier Martinez Canillas]

Hello Mark,

On 10/08/2014 09:51 PM, Mark Brown wrote:
> On Wed, Oct 08, 2014 at 06:25:00PM +0200, Javier Martinez Canillas
>> What this series tried to solve is when you have to set a opmode on
>> boot to change how the physical suspend is managed by the PMIC.
> 
> Think about the consequences of what's being said here.  The goal is to
> set the mode in system suspend which will presumably be a minimal power
> consumption mode where minimal load is expected.  What's being said is
> that in order to implement this we want to set the default mode used
> while the system is running to this mode.  This will mean that we'll be
> in this low power mode while running full speed.  That in turn not only
> means that the DT says something other than what we're trying to do and
> hence looks like it's buggy but most likely also means that the system
> won't run stably under load since the regulators are in power saving
> mode.
>

No, while system is running the regulator won't be in low power mode even
after changing the mode. The mode only affects what the PMIC will do with
the regulator when the system enters to and exit from sleep mode and the
AP signals the PMIC about it.

Maybe you already got it from my previous email but I'll summarize how the
hardware works just to be sure we are on the same page:

* The PMIC allows the AP to power up and down voltage rails via a PWRREQ
  pin to signal when it enters and exit from sleep mode.

* Each regulator control register has a 2-bit field called OPMODE in the
  data-sheet that allows to set 4 different operating modes. For most
  regulators, the modes are the following:

  0x0: Output OFF (regardless of PWRREQ)
  0x1: Output ON/OFF controlled by PWRREQ
       PWRREQ = HIGH (1): Output ON in Normal Mode
       PWRREQ = LOW (0): Output OFF
  0x2: Output ON with Low Power Mode controlled by PWRREQ
       PWRREQ = HIGH (1): Output ON in Low Power Mode
       PWRREQ = LOW (0): Output OFF
  0x3: Output ON in Normal Mode (regardless of PWRREQ)

* Not all regulators have the same modes, for example 0x1 in some LDOs
  means "Output ON in Low Power Mode regardless of PWRREQ" and so on.

Since PWRREQ is HIGH when the AP is in normal mode, there are only two
modes that most regulators can be on runtime: ON and OFF.
 
> As has been said previously please try to think about things in terms of
> abstractions and what the actual goal is, don't try to shoehorn things
> into places they happen to solve the immediate problem without regard to
> the bigger picture or comprehensibility.
>

I could had proposed a DT property that would be specific to the max77802
regulator driver but instead I really tried to not only care about my
particular use case and come up with a solution that could be generic and
useful for others.

As Krzysztof said in other thread, this feature is common to many Maxim
PMICs and AFAIK the Rockchip RK808 PMIC has a similar feature to choose
between two modes: Output ON vs Output ON/OFF controlled by a SLEEP pin.

But maybe trying to make it generic was my mistake and a device-specific
DT binding is the proper solution here...
 
>> > This appears to set the supply which is labelled as VDD_ARM into standby
>> > mode by default (from a first glance the change appears to set all
>> > supplies into standby mode) and of course we currently have no way of
>> > changing the mode at runtime in DT systems.  Are you *really* sure this
>> > is a good idea of which an electrical engineer would approve, CPU cores
>> > are typically one of the most demanding workloads available for a
>> > regulator?
> 
>> Well, the standby mode for this regulator is actually:
> 
>> Output ON/OFF Controlled by PWRREQ
>> 	PWRREQ=HIGH (1): Output ON in Normal Mode
>> 	PWRREQ=LOW (0): Output OFF
> 
> This isn't a mode at all.  This is an enable control and hence it
> should not be implemented as a mode.  It should be adequately documented
> but for the avoidance of doubt a regulation mode should control the
> quality and efficiency of regulation, if something can cause the
> regulator to be disabled (except perhaps as a consequence of handling a
> failure in regulation) then it shouldn't be a mode.
> 

I see, I thought that an operating mode could be anything that alter the
regulator behavior either during runtime or when the system is suspended.
But under your definition, it is true that most max77802 regulators have
only two modes: ON and OFF (and some of them have a third Low Power mode).

I think though that a generic way to configure this enable control feature
is needed. Maybe adding a new pair of .{get,set}_suspend_control function
pointers to struct regulator_ops and an .initial_suspend_ctrl field to the
struct regulation_constraints?

That way the core could parse a generic DT property and call the function
handlers but each driver can document in their own DT bindings what their
control values are and how those affects the regulators during suspend?

Best regards,
Javier

Re: [PATCH 5/5] ARM: dts: Add initial regulator mode on exynos Peach boards

[Mark Brown]

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

On Thu, Oct 09, 2014 at 04:27:37PM +0200, Javier Martinez Canillas wrote:

> I see, I thought that an operating mode could be anything that alter the
> regulator behavior either during runtime or when the system is suspended.
> But under your definition, it is true that most max77802 regulators have

It's not just me, it's the code and all the users and documentation...

> only two modes: ON and OFF (and some of them have a third Low Power mode).

...but let's be clear, only "on" (normal) and low power are modes here.
Like I keep saying please think things through - if modes also include
enable control why would they be treated separately in the API?

> I think though that a generic way to configure this enable control feature
> is needed. Maybe adding a new pair of .{get,set}_suspend_control function
> pointers to struct regulator_ops and an .initial_suspend_ctrl field to the
> struct regulation_constraints?

> That way the core could parse a generic DT property and call the function
> handlers but each driver can document in their own DT bindings what their
> control values are and how those affects the regulators during suspend?

That maps poorly onto a lot of devices which have control schemes which
are more complex than this, for example placing regulators into groups
which are then controlled en masse or with internal sequencing options.
There's also the general taste thing with an API that basically just
consists of passing a random value through - there's a lack of
generality there, it wouldn't be possible to write a generic user of the
API which is a bit of a warning sign.

If you just care about the specific "this pin controls enable in sleep
state" I'd suggest making an interface that very specifically does that.

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

Re: [PATCH 5/5] ARM: dts: Add initial regulator mode on exynos Peach boards

[Javier Martinez Canillas]

Hello Mark,

Thanks for your feedback.

On 10/09/2014 07:48 PM, Mark Brown wrote:
> On Thu, Oct 09, 2014 at 04:27:37PM +0200, Javier Martinez Canillas wrote:
> 
>> I see, I thought that an operating mode could be anything that alter the
>> regulator behavior either during runtime or when the system is suspended.
>> But under your definition, it is true that most max77802 regulators have
> 
> It's not just me, it's the code and all the users and documentation...
> 

Sorry, I didn't mean that you are not correct but more that I was wrong
on my assumptions.

>> only two modes: ON and OFF (and some of them have a third Low Power mode).
> 
> ...but let's be clear, only "on" (normal) and low power are modes here.
> Like I keep saying please think things through - if modes also include
> enable control why would they be treated separately in the API?
> 

Right, I got confused again by the terminology in the Maxim data-sheet that
list output OFF as an opmode but I understand that OFF is not a mode and
that the regulator API treats it separately.

>> I think though that a generic way to configure this enable control feature
>> is needed. Maybe adding a new pair of .{get,set}_suspend_control function
>> pointers to struct regulator_ops and an .initial_suspend_ctrl field to the
>> struct regulation_constraints?
> 
>> That way the core could parse a generic DT property and call the function
>> handlers but each driver can document in their own DT bindings what their
>> control values are and how those affects the regulators during suspend?
> 
> That maps poorly onto a lot of devices which have control schemes which
> are more complex than this, for example placing regulators into groups
> which are then controlled en masse or with internal sequencing options.
> There's also the general taste thing with an API that basically just
> consists of passing a random value through - there's a lack of
> generality there, it wouldn't be possible to write a generic user of the
> API which is a bit of a warning sign.
> 
> If you just care about the specific "this pin controls enable in sleep
> state" I'd suggest making an interface that very specifically does that.
> 

Yes, I'll not try to make it generic and will do something that is specific
to this device.

Best regards,
Javier

Re: [PATCH 5/5] ARM: dts: Add initial regulator mode on exynos Peach boards

[Mark Brown]

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

On Thu, Oct 09, 2014 at 11:40:17PM +0200, Javier Martinez Canillas wrote:
> On 10/09/2014 07:48 PM, Mark Brown wrote:
> > On Thu, Oct 09, 2014 at 04:27:37PM +0200, Javier Martinez Canillas wrote:

> >> only two modes: ON and OFF (and some of them have a third Low Power mode).

> > ...but let's be clear, only "on" (normal) and low power are modes here.
> > Like I keep saying please think things through - if modes also include
> > enable control why would they be treated separately in the API?

> Right, I got confused again by the terminology in the Maxim data-sheet that
> list output OFF as an opmode but I understand that OFF is not a mode and
> that the regulator API treats it separately.

OK, please do try to be careful with things like this - when there's
problems around confusion about concepts it's very important to make
sure that communication is clear.

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