[PATCH 0/5] regulator: Add support for initial operating modes

[PATCH 0/5] regulator: Add support for initial operating modes

[Javier Martinez Canillas]

Hello Mark,

This series add support to setup an initial operating mode for regulators.

There were previous attempts to solve the same issue by adding DT bindings
that were driver-specific like Abhilash Kesavan's "Add MAX77686 Operating
mode support" [0] or Krzysztof Kozlowski's "regulator: s2mps11: Add opmode
for S2MPS14 regulators" [1] series.

But there are many IC with regulators that can be configured with different
operating modes so this is a problem that has to be solved generically and
in fact the regulator core already has a .initial_mode field in the struct
regulation_constraints that is used to call the regulator driver .set_mode
function handler with an initial mode, when regulator constraints are set.

So with the old days of platform data, board files could fill this but with
Device Trees is not possible. The same issue happens with suspend states
since there isn't currently a way to fill this information on DT booting.
That's why there were also different attempts to solve the suspend states
issue like [2] and [3]. Chanwoo's series [4] seems to be close to finally
fix the suspend states issue by filling this data from DT but it does not
support the initial regulator operating mode so this series do that on top
of Chanwoo's work.

Possible use cases for default operating mode support in DT are:

a) Boards wants to set different operating modes for regulators in order to
   lower power at suspend time. For example some SoCs have a dedicated pin
   that is pulled high during normal operation and is pulled down when the
   system enters in sleep mode. A PMIC may support automatically disabling
   a regulator, put it in low power mode or changing the voltage when is
   signalled by the SoC that has entered in sleep mode. Each regulator could
   have different constraints so being able to choose the mode is useful.

b) Regulator drivers that read the operating mode from a HW register may be
   reading OFF if the regulator was disabled and the PMIC is not reset on
   warm reboot. In this case the .enable function may set OFF as the default
   mode thus the regulator failing to be enabled. This can be worked around
   by setting the mode to NORMAL if OFF is read from a hardware register but
   is more robust to explicitly configure that from the DT.

These patches do not really depend on Chanwoo's series but are based on top
since both series are complementary to solve the same general issue and to
avoid merge conflicts. But I can resend to just base on top of the regulator
for-next branch if that is easier for you.

The series is composed of the following patches:

Javier Martinez Canillas (5):
  regulator: of: Add regulator-initial-mode parse support
  regulator: dt-bindings: Add DT include for constants
  regulator: dt-bindings: Add regulator-initial-mode support
  regulator: max77802: Add regulator operating mode set support
  ARM: dts: Add initial regulator mode on exynos Peach boards

 .../devicetree/bindings/regulator/regulator.txt    |  8 ++++++
 arch/arm/boot/dts/exynos5420-peach-pit.dts         | 26 ++++++++++++++++++
 arch/arm/boot/dts/exynos5800-peach-pi.dts          | 26 ++++++++++++++++++
 drivers/regulator/max77802.c                       | 32 ++++++++++++++++++++++
 drivers/regulator/of_regulator.c                   |  3 ++
 include/dt-bindings/regulator/regulator.h          | 16 +++++++++++
 6 files changed, 111 insertions(+)
 create mode 100644 include/dt-bindings/regulator/regulator.h

Patch #1 adds support to set the .initial_mode from DT, patch #2 adds a
include header to avoid DTS using magic numbers for the regulator modes,
patch #3 describes the new "regulator-initial-mode" property in the DT
binding doc, patch #4 adds supports to the max77802 for setting modes
and finally patch #5 adds the initial regulator modes for the regulators
in the max77802 PMIC of the Exynos Peach Chromebooks.

Thanks a lot and best regards,
Javier

[0]: https://lkml.org/lkml/2012/12/10/18
[1]: https://lkml.org/lkml/2014/2/13/82
[2]: https://lkml.org/lkml/2012/12/10/434
[3]: https://lkml.org/lkml/2013/7/25/592
[4]: https://lkml.org/lkml/2014/7/9/16
--
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

[PATCH 1/5] regulator: of: Add regulator-initial-mode parse support

[Javier Martinez Canillas]

The regulator core allows boards to define an initial operating mode to
be used as a default for regulators. With board files and platform data,
it is possible to fill a struct regulation_constraints .initial_mode to
set an initial mode for each regulator.

But currently there isn't a way to do the same with DeviceTrees. Argubly
the operating modes are Linux-specific so that information should not be
in the DT which should be used to only describe hardware. But regulators
having different operating modes is also a hardware property since many
PMICs have support to set different modes for their regulators.

So, the generic REGULATOR_MODE_{FAST,NORMAL,IDLE,STANDBY} modes can be
used to describe different level of power efficiency required for each
regulator and drivers can map those levels to the real modes supported
by the hardware as stated on their data-sheet.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/regulator/of_regulator.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 18236be..a076b7f 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -93,6 +93,9 @@ static void of_get_regulation_constraints(struct device_node *np,
 		};
 	}
 
+	if (!of_property_read_u32(np, "regulator-initial-mode", &pval))
+		constraints->initial_mode = pval;
+
 	for (i = 0; i < ARRAY_SIZE(regulator_states); i++) {
 		switch (i) {
 		case PM_SUSPEND_MEM:
-- 
2.1.0

[PATCH 2/5] regulator: dt-bindings: Add DT include for constants

[Javier Martinez Canillas]

Device Tree source files can set a regulator operating mode to be
used as an initial default. Instead of using magic numbers, a header
file can be included that provides macro definitions for the opmodes.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 include/dt-bindings/regulator/regulator.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 include/dt-bindings/regulator/regulator.h

diff --git a/include/dt-bindings/regulator/regulator.h b/include/dt-bindings/regulator/regulator.h
new file mode 100644
index 0000000..5051093
--- /dev/null
+++ b/include/dt-bindings/regulator/regulator.h
@@ -0,0 +1,16 @@
+/*
+ * This header provides constants for regulator bindings.
+ */
+
+#ifndef _DT_BINDINGS_REGULATOR_REGULATOR_H
+#define _DT_BINDINGS_REGULATOR_REGULATOR_H
+
+/*
+ * Regulator operating modes.
+ */
+#define REGULATOR_MODE_FAST			0x1
+#define REGULATOR_MODE_NORMAL			0x2
+#define REGULATOR_MODE_IDLE			0x4
+#define REGULATOR_MODE_STANDBY			0x8
+
+#endif
-- 
2.1.0

[PATCH 3/5] regulator: dt-bindings: Add regulator-initial-mode support

[Javier Martinez Canillas]

Regulators can run on different operating modes (opmodes). This allows
systems to choose the most efficient opmode for each regulator. The
regulator core defines a set of generic modes so each system can define
the opmode in these generic terms and drivers are responsible to map the
generic modes to the ones supported by each hardware according to their
data-sheet.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 Documentation/devicetree/bindings/regulator/regulator.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
index ccba90b..a9d6767 100644
--- a/Documentation/devicetree/bindings/regulator/regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/regulator.txt
@@ -23,6 +23,14 @@ Optional properties:
   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-initial-mode: initial regulator operating mode. One of following:
+	<1>: REGULATOR_MODE_FAST    - Regulator can handle fast changes.
+	<2>: REGULATOR_MODE_NORMAL  - Normal regulator power supply mode.
+	<4>: REGULATOR_MODE_IDLE    - Regulator runs in a more efficient mode.
+	<8>: REGULATOR_MODE_STANDBY - Regulator runs in the most efficient mode.
+  modes are defined in the dt-bindings/regulator/regulator.h header and can be
+  used in device tree sources files. If no mode is defined, then the OS will not
+  manage the operating mode and the HW default values will be used instead.
 - 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.
-- 
2.1.0

[PATCH 4/5] regulator: max77802: Add regulator operating mode set support

[Javier Martinez Canillas]

Add a function handler for the struct regulator_ops .set_mode so an
operating mode (opmode) can be set for regulators.

Regulators opmode are defined using the generic REGULATOR_MODE_*
modes so the driver maps these generic modes to the device-specific
operating modes as stated in the hardware data-sheet.

Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/regulator/max77802.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/regulator/max77802.c b/drivers/regulator/max77802.c
index d89792b..04ed5cf 100644
--- a/drivers/regulator/max77802.c
+++ b/drivers/regulator/max77802.c
@@ -83,6 +83,34 @@ static int max77802_get_opmode_shift(int id)
 	return -EINVAL;
 }
 
+static int max77802_set_mode(struct regulator_dev *rdev, unsigned int mode)
+{
+	struct max77802_regulator_prv *max77802 = rdev_get_drvdata(rdev);
+	unsigned int val;
+	int id = rdev_get_id(rdev);
+	int shift = max77802_get_opmode_shift(id);
+
+	switch (mode) {
+	case REGULATOR_MODE_NORMAL:
+		val = MAX77802_OPMODE_NORMAL;
+		break;
+	case REGULATOR_MODE_IDLE:
+		val = MAX77802_OPMODE_LP;
+		break;
+	case REGULATOR_MODE_STANDBY:
+		val = MAX77802_OPMODE_STANDBY;
+		break;
+	default:
+		dev_warn(&rdev->dev, "%s: regulator mode: 0x%x not supported\n",
+			 rdev->desc->name, mode);
+		return -EINVAL;
+	}
+
+	max77802->opmode[id] = val;
+	return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+				  rdev->desc->enable_mask, val << shift);
+}
+
 /*
  * Some BUCKS supports Normal[ON/OFF] mode during suspend
  *
@@ -247,6 +275,7 @@ static struct regulator_ops max77802_ldo_ops_logic1 = {
 	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
 	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
 	.set_voltage_time_sel	= regulator_set_voltage_time_sel,
+	.set_mode		= max77802_set_mode,
 	.set_suspend_mode	= max77802_ldo_set_suspend_mode_logic1,
 };
 
@@ -262,6 +291,7 @@ static struct regulator_ops max77802_ldo_ops_logic2 = {
 	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
 	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
 	.set_voltage_time_sel	= regulator_set_voltage_time_sel,
+	.set_mode		= max77802_set_mode,
 	.set_suspend_mode	= max77802_ldo_set_suspend_mode_logic2,
 };
 
@@ -274,6 +304,7 @@ static struct regulator_ops max77802_buck_16_dvs_ops = {
 	.disable		= regulator_disable_regmap,
 	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
 	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.set_mode		= max77802_set_mode,
 	.set_voltage_time_sel	= regulator_set_voltage_time_sel,
 	.set_ramp_delay		= max77802_set_ramp_delay_4bit,
 	.set_suspend_disable	= max77802_buck_set_suspend_disable,
@@ -288,6 +319,7 @@ static struct regulator_ops max77802_buck_dvs_ops = {
 	.disable		= regulator_disable_regmap,
 	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
 	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.set_mode		= max77802_set_mode,
 	.set_voltage_time_sel	= regulator_set_voltage_time_sel,
 	.set_ramp_delay		= max77802_set_ramp_delay_2bit,
 	.set_suspend_disable	= max77802_buck_set_suspend_disable,
-- 
2.1.0

[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 1/5] regulator: of: Add regulator-initial-mode parse support

[Mark Brown]

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

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

> But currently there isn't a way to do the same with DeviceTrees. Argubly
> the operating modes are Linux-specific so that information should not be
> in the DT which should be used to only describe hardware. But regulators
> having different operating modes is also a hardware property since many
> PMICs have support to set different modes for their regulators.

That doesn't mean that the definition of those modes is something we can
sensibly provide in generic code, especially in a completely
undocumented fashion (perhaps you've done that later in the patch series
but bisection also applies to reviewability).

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

Re: [PATCH 3/5] regulator: dt-bindings: Add regulator-initial-mode support

[Mark Brown]

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

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

> +- regulator-initial-mode: initial regulator operating mode. One of following:
> +	<1>: REGULATOR_MODE_FAST    - Regulator can handle fast changes.
> +	<2>: REGULATOR_MODE_NORMAL  - Normal regulator power supply mode.
> +	<4>: REGULATOR_MODE_IDLE    - Regulator runs in a more efficient mode.
> +	<8>: REGULATOR_MODE_STANDBY - Regulator runs in the most efficient mode.
> +  modes are defined in the dt-bindings/regulator/regulator.h header and can be
> +  used in device tree sources files. If no mode is defined, then the OS will not
> +  manage the operating mode and the HW default values will be used instead.

...and as previously and repeatedly discussed this still gives the user
no way of telling if or how these modes might correspond to what the
chip is capable of doing.  This really needs addressing rather than
ignoring, for example by not trying to define the modes in generic
bindings.

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

Re: [PATCH 4/5] regulator: max77802: Add regulator operating mode set support

[Mark Brown]

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

On Wed, Oct 08, 2014 at 03:44:06PM +0200, Javier Martinez Canillas wrote:
> Add a function handler for the struct regulator_ops .set_mode so an
> operating mode (opmode) can be set for regulators.
> 
> Regulators opmode are defined using the generic REGULATOR_MODE_*
> modes so the driver maps these generic modes to the device-specific
> operating modes as stated in the hardware data-sheet.

Why is this implementing a set operation but not a get operation?

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

Re: [PATCH 1/5] regulator: of: Add regulator-initial-mode parse support

[Javier Martinez Canillas]

Hello Mark,

Thanks for the feedback.

On 10/08/2014 04:25 PM, Mark Brown wrote:
> On Wed, Oct 08, 2014 at 03:44:03PM +0200, Javier Martinez Canillas wrote:
> 
>> But currently there isn't a way to do the same with DeviceTrees. Argubly
>> the operating modes are Linux-specific so that information should not be
>> in the DT which should be used to only describe hardware. But regulators
>> having different operating modes is also a hardware property since many
>> PMICs have support to set different modes for their regulators.
> 
> That doesn't mean that the definition of those modes is something we can
> sensibly provide in generic code, especially in a completely
> undocumented fashion (perhaps you've done that later in the patch series
> but bisection also applies to reviewability).
> 

Yes, patch #3 updates the regulator DT binding doc and documents what each
regulator mode is supposed to be. Basically is just a short description of
what is already documented in linux/regulator/consumer.h [0].

If what is enough for you I can reorganize the patch-set so that patch is
the first one.

As a general question, now that the convention is for DT binding docs to go
in a separate patch, should the DT documentation be added before or after
that code using these bindings is added?

That is something that is not explained in
Documentation/devicetree/bindings/submitting-patches.txt.

Best regards,
Javier

[0]: http://lxr.free-electrons.com/source/include/linux/regulator/consumer.h#L40

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 1/5] regulator: of: Add regulator-initial-mode parse support

[Mark Brown]

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

On Wed, Oct 08, 2014 at 04:38:53PM +0200, Javier Martinez Canillas wrote:
> On 10/08/2014 04:25 PM, Mark Brown wrote:

> > That doesn't mean that the definition of those modes is something we can
> > sensibly provide in generic code, especially in a completely
> > undocumented fashion (perhaps you've done that later in the patch series
> > but bisection also applies to reviewability).

> As a general question, now that the convention is for DT binding docs to go
> in a separate patch, should the DT documentation be added before or after
> that code using these bindings is added?

It fairly obviously needs to go before so that reviewers can tell if the
code is actually implementing the binding.

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

Re: [PATCH 3/5] regulator: dt-bindings: Add regulator-initial-mode support

[Javier Martinez Canillas]

On 10/08/2014 04:34 PM, Mark Brown wrote:
> On Wed, Oct 08, 2014 at 03:44:05PM +0200, Javier Martinez Canillas wrote:
> 
>> +- regulator-initial-mode: initial regulator operating mode. One of following:
>> +	<1>: REGULATOR_MODE_FAST    - Regulator can handle fast changes.
>> +	<2>: REGULATOR_MODE_NORMAL  - Normal regulator power supply mode.
>> +	<4>: REGULATOR_MODE_IDLE    - Regulator runs in a more efficient mode.
>> +	<8>: REGULATOR_MODE_STANDBY - Regulator runs in the most efficient mode.
>> +  modes are defined in the dt-bindings/regulator/regulator.h header and can be
>> +  used in device tree sources files. If no mode is defined, then the OS will not
>> +  manage the operating mode and the HW default values will be used instead.
> 
> ...and as previously and repeatedly discussed this still gives the user
> no way of telling if or how these modes might correspond to what the
> chip is capable of doing.  This really needs addressing rather than
> ignoring, for example by not trying to define the modes in generic
> bindings.
> 

Drivers could add custom per-device DT properties. That is how it was
solved in the ChromeOS kernel. There is a regulator-op-mode DT property
whose value is just a magic number with the value that has to be written
in the OPMODE field of the control register of each regulator and that
is made on the driver probe.

Another option is to not document the standard modes in the generic
regulator binding but instead on each device DT binding doc. That way
each device binding can define what are the semantics of the standard
modes and how correspond to the real modes in the chip.

That way the regulator-initial-mode could still be generic and parsed
by the core instead of each driver implementing their own custom parsing.

Best regards,
Javier

Re: [PATCH 4/5] regulator: max77802: Add regulator operating mode set support

[Javier Martinez Canillas]

On 10/08/2014 04:36 PM, Mark Brown wrote:
> On Wed, Oct 08, 2014 at 03:44:06PM +0200, Javier Martinez Canillas wrote:
>> Add a function handler for the struct regulator_ops .set_mode so an
>> operating mode (opmode) can be set for regulators.
>> 
>> Regulators opmode are defined using the generic REGULATOR_MODE_*
>> modes so the driver maps these generic modes to the device-specific
>> operating modes as stated in the hardware data-sheet.
> 
> Why is this implementing a set operation but not a get operation?
> 

Right, I'll add it on the next version. Thanks for pointing out this.

Best regards,
Javier

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 1/5] regulator: of: Add regulator-initial-mode parse support

[Javier Martinez Canillas]

On 10/08/2014 05:12 PM, Mark Brown wrote:
> On Wed, Oct 08, 2014 at 04:38:53PM +0200, Javier Martinez Canillas wrote:
>> On 10/08/2014 04:25 PM, Mark Brown wrote:
> 
>> > That doesn't mean that the definition of those modes is something we can
>> > sensibly provide in generic code, especially in a completely
>> > undocumented fashion (perhaps you've done that later in the patch series
>> > but bisection also applies to reviewability).
> 
>> As a general question, now that the convention is for DT binding docs to go
>> in a separate patch, should the DT documentation be added before or after
>> that code using these bindings is added?
> 
> It fairly obviously needs to go before so that reviewers can tell if the
> code is actually implementing the binding.
> 

Well, is not fairly obvious to me. One can also say the opposite, why the
kernel is documenting a DT binding that is not (currently) implemented?

That's why what makes the most sense for me is what the old convention did,
add the DT binding docs in the same patch that implements the binding.

Anyways, thanks for letting me know what is the convention today.

Re: [PATCH 3/5] regulator: dt-bindings: Add regulator-initial-mode support

[Krzysztof Kozlowski]

On śro, 2014-10-08 at 15:44 +0200, Javier Martinez Canillas wrote:
> Regulators can run on different operating modes (opmodes). This allows
> systems to choose the most efficient opmode for each regulator. The
> regulator core defines a set of generic modes so each system can define
> the opmode in these generic terms and drivers are responsible to map the
> generic modes to the ones supported by each hardware according to their
> data-sheet.
> 
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  Documentation/devicetree/bindings/regulator/regulator.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt
> index ccba90b..a9d6767 100644
> --- a/Documentation/devicetree/bindings/regulator/regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/regulator.txt
> @@ -23,6 +23,14 @@ Optional properties:
>    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-initial-mode: initial regulator operating mode. One of following:
> +	<1>: REGULATOR_MODE_FAST    - Regulator can handle fast changes.
> +	<2>: REGULATOR_MODE_NORMAL  - Normal regulator power supply mode.
> +	<4>: REGULATOR_MODE_IDLE    - Regulator runs in a more efficient mode.
> +	<8>: REGULATOR_MODE_STANDBY - Regulator runs in the most efficient mode.
> +  modes are defined in the dt-bindings/regulator/regulator.h header and can be
> +  used in device tree sources files. If no mode is defined, then the OS will not
> +  manage the operating mode and the HW default values will be used instead.
>  - 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.

I agree with the need and the idea of generic bindings for operating
modes for regulators. At least for Exynos-based boards the PMICs have
quite similar opmodes.

However the regulator mode from consumer.h (and in above doc) does not
match well with these opmodes. Example is yours patch 4/5:
 - idle ("more efficient mode") maps to "low power mode in suspend",
 - standby ("the most efficient mode") maps to "OFF in suspend".

Actually we are not enable "efficient modes" but we configure how the
regulator will behave when AP says - I'm suspending.


Another issue: is "initial_state" not doing all this already?

Best regards,
Krzysztof

Re: [PATCH 1/5] regulator: of: Add regulator-initial-mode parse support

[Mark Rutland]

On Wed, Oct 08, 2014 at 05:29:26PM +0100, Javier Martinez Canillas wrote:
> On 10/08/2014 05:12 PM, Mark Brown wrote:
> > On Wed, Oct 08, 2014 at 04:38:53PM +0200, Javier Martinez Canillas wrote:
> >> On 10/08/2014 04:25 PM, Mark Brown wrote:
> > 
> >> > That doesn't mean that the definition of those modes is something we can
> >> > sensibly provide in generic code, especially in a completely
> >> > undocumented fashion (perhaps you've done that later in the patch series
> >> > but bisection also applies to reviewability).
> > 
> >> As a general question, now that the convention is for DT binding docs to go
> >> in a separate patch, should the DT documentation be added before or after
> >> that code using these bindings is added?
> > 
> > It fairly obviously needs to go before so that reviewers can tell if the
> > code is actually implementing the binding.
> > 
> 
> Well, is not fairly obvious to me. One can also say the opposite, why the
> kernel is documenting a DT binding that is not (currently) implemented?

Checkpatch will complain regarding undocumented bindings, so from a
pragmatic point of view the binding must come first.

Personally, when I read a patch series I do an initial pass in-order,
and having the binding first makes things clearer. I might have some
questions regarding the binding that the driver answers later, and it makes it
easier to spot undocumented properties or conventions used by the
driver. Doing so the other way around usually leaves me with more
questions at the end.

> That's why what makes the most sense for me is what the old convention did,
> add the DT binding docs in the same patch that implements the binding.

Having a separate patch for the binding is very helpful for those of us
doing review. For one thing it helps us to find the binding document,
which can be important when a driver is thousands of lines long. For
another it means that we can be clear that our Acked-by, Reviewed-by,
etc apply to the binding and not necessarily the rest of the code.

For small patches, this is obviously less of a concern.

Thanks,
Mark.

Re: [PATCH 3/5] regulator: dt-bindings: Add regulator-initial-mode support

[Javier Martinez Canillas]

Hello Krzysztof,

Thanks a lot for your feedback.

On 10/09/2014 10:45 AM, Krzysztof Kozlowski wrote:
> On śro, 2014-10-08 at 15:44 +0200, Javier Martinez Canillas wrote:
>> --- a/Documentation/devicetree/bindings/regulator/regulator.txt
>> +++ b/Documentation/devicetree/bindings/regulator/regulator.txt
>> @@ -23,6 +23,14 @@ Optional properties:
>>    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-initial-mode: initial regulator operating mode. One of following:
>> +	<1>: REGULATOR_MODE_FAST    - Regulator can handle fast changes.
>> +	<2>: REGULATOR_MODE_NORMAL  - Normal regulator power supply mode.
>> +	<4>: REGULATOR_MODE_IDLE    - Regulator runs in a more efficient mode.
>> +	<8>: REGULATOR_MODE_STANDBY - Regulator runs in the most efficient mode.
>> +  modes are defined in the dt-bindings/regulator/regulator.h header and can be
>> +  used in device tree sources files. If no mode is defined, then the OS will not
>> +  manage the operating mode and the HW default values will be used instead.
>>  - 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.
> 
> I agree with the need and the idea of generic bindings for operating
> modes for regulators. At least for Exynos-based boards the PMICs have
> quite similar opmodes.
> 
> However the regulator mode from consumer.h (and in above doc) does not
> match well with these opmodes. Example is yours patch 4/5:
>  - idle ("more efficient mode") maps to "low power mode in suspend",
>  - standby ("the most efficient mode") maps to "OFF in suspend".
> 
> Actually we are not enable "efficient modes" but we configure how the
> regulator will behave when AP says - I'm suspending.
>

Agree, Mark also pointed out that there is a difference between changing
how the regulator will behave on runtime vs changing how the regulator
will behave during system suspend. AFAIU from his explanation, the modes
defined in consumer.h only applies to the former and conceptually there
should be a difference between those two cases even when the Maxim PMIC
seems to mix it both in the data-sheet and by using the same field.

I answered Mark on patch #5 with a suggestion on how to handle this in
a generic way to avoid each driver having their own custom DT binding
and duplicating the parsing code.

Could you please answer there with your thoughts?
 
> 
> Another issue: is "initial_state" not doing all this already?
> 

One of the options I indeed evaluated was if using initial_state could be
possible. But currently the .set_suspend_mode function handler is only
called for the STANDBY, MEM and MAX suspend modes. While is true that this
could be extended to also support PM_SUSPEND_ON, I don't think that it
makes sense conceptually. Since is not the same to set a mode to be used
at runtime than setting a mode to be used during suspend.

AFAIU that is the reason why there are separate .set_suspend_mode and
.set_mode operations and that's why there are both an .initial_state
and an .initial_mode.

> Best regards,
> Krzysztof
>

Best regards,
Javier

Re: [PATCH 1/5] regulator: of: Add regulator-initial-mode parse support

[Javier Martinez Canillas]

Hello Mark,

On 10/09/2014 12:27 PM, Mark Rutland wrote:
>> 
>> Well, is not fairly obvious to me. One can also say the opposite, why the
>> kernel is documenting a DT binding that is not (currently) implemented?
> 
> Checkpatch will complain regarding undocumented bindings, so from a
> pragmatic point of view the binding must come first.
>
> Personally, when I read a patch series I do an initial pass in-order,
> and having the binding first makes things clearer. I might have some
> questions regarding the binding that the driver answers later, and it makes it
> easier to spot undocumented properties or conventions used by the
> driver. Doing so the other way around usually leaves me with more
> questions at the end.
>

Thanks a lot for the explanation, it certainly makes sense then to have
the DT binding before. I'll propose a patch to add that information to
Documentation/devicetree/bindings/submitting-patches.txt so people
(like me) who didn't find it obvious can know what the convention is.
 
>> That's why what makes the most sense for me is what the old convention did,
>> add the DT binding docs in the same patch that implements the binding.
> 
> Having a separate patch for the binding is very helpful for those of us
> doing review. For one thing it helps us to find the binding document,
> which can be important when a driver is thousands of lines long. For
> another it means that we can be clear that our Acked-by, Reviewed-by,
> etc apply to the binding and not necessarily the rest of the code.
> 

Agreed.

> For small patches, this is obviously less of a concern.
> 
> Thanks,
> Mark.
> 

Best regards,
Javier

Re: [PATCH 3/5] regulator: dt-bindings: Add regulator-initial-mode support

[Mark Brown]

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

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

> Agree, Mark also pointed out that there is a difference between changing
> how the regulator will behave on runtime vs changing how the regulator
> will behave during system suspend. AFAIU from his explanation, the modes
> defined in consumer.h only applies to the former and conceptually there
> should be a difference between those two cases even when the Maxim PMIC
> seems to mix it both in the data-sheet and by using the same field.

No, that's not accurate at all - you're still not getting the concepts
of modes or suspend handling in the regulator API.  I really think you
need to take a step back and try to understand what's currently there
before trying to make changes here.  We've got a set of operations we
can use to change the regulator configuration, if you look at the
existing driver interface you'll see that these are matched with
equivalent operations for setting the behaviour when in suspend
(including a set_suspend_mode() operation).  

Like I keep saying abstractions are really important to making sure the
code is maintainable.

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

Re: [PATCH 3/5] regulator: dt-bindings: Add regulator-initial-mode support

[Javier Martinez Canillas]

On 10/09/2014 09:01 PM, Mark Brown wrote:
> On Thu, Oct 09, 2014 at 05:04:35PM +0200, Javier Martinez Canillas wrote:
> 
>> Agree, Mark also pointed out that there is a difference between changing
>> how the regulator will behave on runtime vs changing how the regulator
>> will behave during system suspend. AFAIU from his explanation, the modes
>> defined in consumer.h only applies to the former and conceptually there
>> should be a difference between those two cases even when the Maxim PMIC
>> seems to mix it both in the data-sheet and by using the same field.
> 
> No, that's not accurate at all - you're still not getting the concepts
> of modes or suspend handling in the regulator API.  I really think you
> need to take a step back and try to understand what's currently there
> before trying to make changes here.  We've got a set of operations we
> can use to change the regulator configuration, if you look at the
> existing driver interface you'll see that these are matched with
> equivalent operations for setting the behaviour when in suspend
> (including a set_suspend_mode() operation).  
> 

I tried to say that there is a difference between the need to change
within the kernel a regulator configuration (e.g: change opmode from
normal to low power) when the system is going to enter in suspend state
vs setting a register at probe time that will force the PMIC to change
the regulator to low power mode on suspend without kernel intervention.

> Like I keep saying abstractions are really important to making sure the
> code is maintainable.
> 

Agree, I'll try to come up with a more sensible solution by using the
existing abstractions from the regulator framework.

Best regards,
Javier
--
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

Re: [PATCH 1/5] regulator: of: Add regulator-initial-mode parse support

[Mark Rutland]

On Thu, Oct 09, 2014 at 04:19:47PM +0100, Javier Martinez Canillas wrote:
> Hello Mark,
> 
> On 10/09/2014 12:27 PM, Mark Rutland wrote:
> >> 
> >> Well, is not fairly obvious to me. One can also say the opposite, why the
> >> kernel is documenting a DT binding that is not (currently) implemented?
> > 
> > Checkpatch will complain regarding undocumented bindings, so from a
> > pragmatic point of view the binding must come first.
> >
> > Personally, when I read a patch series I do an initial pass in-order,
> > and having the binding first makes things clearer. I might have some
> > questions regarding the binding that the driver answers later, and it makes it
> > easier to spot undocumented properties or conventions used by the
> > driver. Doing so the other way around usually leaves me with more
> > questions at the end.
> >
> 
> Thanks a lot for the explanation, it certainly makes sense then to have
> the DT binding before. I'll propose a patch to add that information to
> Documentation/devicetree/bindings/submitting-patches.txt so people
> (like me) who didn't find it obvious can know what the convention is.

That sounds like a good idea; yes please.

Thanks,
Mark.