[RFC PATCH 0/3] Add MAX77686 Operating mode support

[RFC PATCH 0/3] Add MAX77686 Operating mode support

[Abhilash Kesavan]

Currently, we cannot specify the regulator suspend state via device
tree. Rather than adding suspend state/mode/enable/disable properties
add an optional MAX77686 operating mode property which can be used to
set the regulator mode initially.

We are currently bypassing the set_suspend_disable and set_suspend_mode
call-backs. Please review and suggest improvements.

This is based on
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git : for-next
merged with
git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git : for-next


Abhilash Kesavan (3):
  ARM: DTS: CROS5250: Add regulator operating mode bindings
  regulator: max77686: Add support for various operating modes
  dt: Document: Add optional MAX77686 operating mode bindings

 Documentation/devicetree/bindings/mfd/max77686.txt |    6 +++
 arch/arm/boot/dts/cros5250-common.dtsi             |   36 ++++++++++++++++++++
 drivers/regulator/max77686.c                       |   11 +++++-
 include/linux/mfd/max77686.h                       |    1 +
 4 files changed, 53 insertions(+), 1 deletions(-)

-- 
1.7.8.6

[RFC PATCH 1/3] ARM: DTS: CROS5250: Add regulator operating mode bindings

[Abhilash Kesavan]

Some of the LDOs and BUCKs on the MAX77686 PMIC can be put into a
low power or standby state. Add bindings to control the operating
mode. This results in significant power savings during suspend.

Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
---
 arch/arm/boot/dts/cros5250-common.dtsi |   36 ++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/cros5250-common.dtsi b/arch/arm/boot/dts/cros5250-common.dtsi
index 98ff65b..10d1812 100644
--- a/arch/arm/boot/dts/cros5250-common.dtsi
+++ b/arch/arm/boot/dts/cros5250-common.dtsi
@@ -35,6 +35,8 @@
 					regulator-min-microvolt = <1000000>;
 					regulator-max-microvolt = <1000000>;
 					regulator-always-on;
+					/* Output ON/LPM controlled by PWRREQ */
+					max77686-opmode = <0x40>;
 				};
 
 				ldo2_reg: LDO2 {
@@ -42,6 +44,8 @@
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
 					regulator-always-on;
+					/* Output ON/OFF controlled by PWRREQ */
+					max77686-opmode = <0x20>;
 				};
 
 				ldo3_reg: LDO3 {
@@ -49,6 +53,8 @@
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
 					regulator-always-on;
+					/* Output ON/LPM controlled by PWRREQ */
+					max77686-opmode = <0x80>;
 				};
 
 				ldo7_reg: LDO7 {
@@ -56,6 +62,8 @@
 					regulator-min-microvolt = <1100000>;
 					regulator-max-microvolt = <1100000>;
 					regulator-always-on;
+					/* Output ON/OFF controlled by PWRREQ */
+					max77686-opmode = <0x40>;
 				};
 
 				ldo8_reg: LDO8 {
@@ -63,6 +71,8 @@
 					regulator-min-microvolt = <1000000>;
 					regulator-max-microvolt = <1000000>;
 					regulator-always-on;
+					/* Output ON/OFF controlled by PWRREQ */
+					max77686-opmode = <0x40>;
 				};
 
 				ldo10_reg: LDO10 {
@@ -70,6 +80,8 @@
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
 					regulator-always-on;
+					/* Output ON/OFF controlled by PWRREQ */
+					max77686-opmode = <0x40>;
 				};
 
 				ldo12_reg: LDO12 {
@@ -77,6 +89,8 @@
 					regulator-min-microvolt = <3000000>;
 					regulator-max-microvolt = <3000000>;
 					regulator-always-on;
+					/* Output ON/OFF controlled by PWRREQ */
+					max77686-opmode = <0x40>;
 				};
 
 				ldo14_reg: LDO14 {
@@ -84,6 +98,8 @@
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
 					regulator-always-on;
+					/* Output ON/OFF controlled by PWRREQ */
+					max77686-opmode = <0x40>;
 				};
 
 				ldo15_reg: LDO15 {
@@ -91,6 +107,8 @@
 					regulator-min-microvolt = <1000000>;
 					regulator-max-microvolt = <1000000>;
 					regulator-always-on;
+					/* Output ON/OFF controlled by PWRREQ */
+					max77686-opmode = <0x40>;
 				};
 
 				ldo16_reg: LDO16 {
@@ -98,6 +116,8 @@
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
 					regulator-always-on;
+					/* Output ON/OFF controlled by PWRREQ */
+					max77686-opmode = <0x40>;
 				};
 
 				buck1_reg: BUCK1 {
@@ -106,6 +126,8 @@
 					regulator-max-microvolt = <1300000>;
 					regulator-always-on;
 					regulator-boot-on;
+					/* Output ON/OFF controlled by PWRREQ */
+					max77686-opmode = <0x1>;
 				};
 
 				buck2_reg: BUCK2 {
@@ -114,6 +136,8 @@
 					regulator-max-microvolt = <1350000>;
 					regulator-always-on;
 					regulator-boot-on;
+					/* Output ON/OFF controlled by PWRREQ */
+					max77686-opmode = <0x10>;
 				};
 
 				buck3_reg: BUCK3 {
@@ -122,6 +146,8 @@
 					regulator-max-microvolt = <1200000>;
 					regulator-always-on;
 					regulator-boot-on;
+					/* Output ON/OFF controlled by PWRREQ */
+					max77686-opmode = <0x10>;
 				};
 
 				buck4_reg: BUCK4 {
@@ -130,6 +156,8 @@
 					regulator-max-microvolt = <1300000>;
 					regulator-always-on;
 					regulator-boot-on;
+					/* Output ON/OFF controlled by PWRREQ */
+					max77686-opmode = <0x10>;
 				};
 
 				buck5_reg: BUCK5 {
@@ -138,6 +166,8 @@
 					regulator-max-microvolt = <1800000>;
 					regulator-always-on;
 					regulator-boot-on;
+					/* Output ON */
+					max77686-opmode = <0x3>;
 				};
 
 				buck6_reg: BUCK6 {
@@ -145,6 +175,8 @@
 					regulator-min-microvolt = <1350000>;
 					regulator-max-microvolt = <1350000>;
 					regulator-always-on;
+					/* Output ON */
+					max77686-opmode = <0x3>;
 				};
 
 				buck7_reg: BUCK7 {
@@ -152,6 +184,8 @@
 					regulator-min-microvolt = <2000000>;
 					regulator-max-microvolt = <2000000>;
 					regulator-always-on;
+					/* Output ON */
+					max77686-opmode = <0x3>;
 				};
 
 				buck8_reg: BUCK8 {
@@ -159,6 +193,8 @@
 					regulator-min-microvolt = <2850000>;
 					regulator-max-microvolt = <2850000>;
 					regulator-always-on;
+					/* Output OFF of ENB8 is low */
+					max77686-opmode = <0x1>;
 				};
 			};
 		};
-- 
1.7.8.6

[RFC PATCH 2/3] regulator: max77686: Add support for various operating modes

[Abhilash Kesavan]

Currently, we cannot specify the regulator suspend state via device
tree. Add an optional operating mode property which can be used to
set initially the regulator mode.
We are currently bypassing the set_suspend_disable and set_suspend_mode
call-backs.

Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
---
 drivers/regulator/max77686.c |   11 ++++++++++-
 include/linux/mfd/max77686.h |    1 +
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index b85040c..7f16bc7 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -410,6 +410,12 @@ static int max77686_pmic_dt_parse_pdata(struct max77686_dev *iodev,
 		of_regulator_match(iodev->dev, regulators_np, &rmatch, 1);
 		rdata[i].initdata = rmatch.init_data;
 		rdata[i].of_node = rmatch.of_node;
+		if (of_property_read_u32(rdata[i].of_node, "max77686-opmode",
+			&rdata[i].opmode)) {
+			dev_warn(iodev->dev, "no op_mode property property at %s\n",
+			rmatch.name);
+			rdata[i].opmode = regulators[i].enable_mask;
+		}
 	}
 
 	pdata->regulators = rdata;
@@ -465,7 +471,10 @@ static int max77686_pmic_probe(struct platform_device *pdev)
 		config.init_data = pdata->regulators[i].initdata;
 		config.of_node = pdata->regulators[i].of_node;
 
-		max77686->opmode[i] = regulators[i].enable_mask;
+		if (config.of_node)
+			max77686->opmode[i] = pdata->regulators[i].opmode;
+		else
+			max77686->opmode[i] = regulators[i].enable_mask;
 		max77686->rdev[i] = regulator_register(&regulators[i], &config);
 		if (IS_ERR(max77686->rdev[i])) {
 			ret = PTR_ERR(max77686->rdev[i]);
diff --git a/include/linux/mfd/max77686.h b/include/linux/mfd/max77686.h
index 46c0f32..e8bd83b 100644
--- a/include/linux/mfd/max77686.h
+++ b/include/linux/mfd/max77686.h
@@ -73,6 +73,7 @@ enum max77686_regulators {
 
 struct max77686_regulator_data {
 	int id;
+	int opmode;
 	struct regulator_init_data *initdata;
 	struct device_node *of_node;
 };
-- 
1.7.8.6

[RFC PATCH 3/3] dt: Document: Add optional MAX77686 operating mode bindings

[Abhilash Kesavan]

Add documenatation for various operating mode capabilities of
the MAX77686 PMIC.

Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
---
 Documentation/devicetree/bindings/mfd/max77686.txt |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/max77686.txt b/Documentation/devicetree/bindings/mfd/max77686.txt
index c6a3469..f2867a9 100644
--- a/Documentation/devicetree/bindings/mfd/max77686.txt
+++ b/Documentation/devicetree/bindings/mfd/max77686.txt
@@ -13,6 +13,12 @@ Required properties:
 - interrupts : This i2c device has an IRQ line connected to the main SoC.
 - interrupt-parent : The parent interrupt controller.
 
+optional properties:
+- max77686-opmode : The regulators of max77686 have various operating mode
+  capabilities such as low power mode, standby mode (controlled by PWRREQ
+  signal) etc. Check the regulator CTRL register for the bits setting these
+  modes.
+
 Optional node:
 - voltage-regulators : The regulators of max77686 have to be instantiated
   under subnode named "voltage-regulators" using the following format.
-- 
1.7.8.6

Re: [RFC PATCH 3/3] dt: Document: Add optional MAX77686 operating mode bindings

[Doug Anderson]

Abhilash,

Thanks for posting up these patches.  Just going to do my commenting
directly on the documentation patch since they are high-level
comments.

On Sun, Dec 9, 2012 at 10:26 PM, Abhilash Kesavan <a.kesavan@samsung.com> wrote:
> Add documenatation for various operating mode capabilities of
> the MAX77686 PMIC.
>
> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> ---
>  Documentation/devicetree/bindings/mfd/max77686.txt |    6 ++++++

This should probably be squashed with PATCH 2/3.  It seems to be best
to introduce documentation in the same patch as first usage.


>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mfd/max77686.txt b/Documentation/devicetree/bindings/mfd/max77686.txt
> index c6a3469..f2867a9 100644
> --- a/Documentation/devicetree/bindings/mfd/max77686.txt
> +++ b/Documentation/devicetree/bindings/mfd/max77686.txt
> @@ -13,6 +13,12 @@ Required properties:
>  - interrupts : This i2c device has an IRQ line connected to the main SoC.
>  - interrupt-parent : The parent interrupt controller.
>
> +optional properties:
> +- max77686-opmode : The regulators of max77686 have various operating mode
> +  capabilities such as low power mode, standby mode (controlled by PWRREQ
> +  signal) etc. Check the regulator CTRL register for the bits setting these
> +  modes.

I have a feeling we're going to want something a little more generic
than just whacking a register value straight into the device tree.

I would propose the following for LDOs to match the generic regulator
define, but someone with more device tree experience may have a better
idea.  The idea is to map to the generic set_suspend_mode() /
set_mode():
  regulator-suspend-mode = <REGULATOR_MODE_XXX constant>
  regulator-mode = <REGULATOR_MODE_XXX constant>

The max77686 appears to support NORMAL and IDLE mode for most LDOs, so
the only reasonable values for these two would be 2 and 4.  It appears
that it would also be nonsensical (impossible to map to a register
value) to have REGULATOR_MODE_NORMAL for suspend and
REGULATOR_MODE_IDLE for the normal running mode.


For bucks 1-3, you'd just have a boolean property mapping to
set_suspend_disable().  Perhaps:
  regulator-suspend-en;

That wouldn't allow access to the "Forced low power mode" of bucks
1-3, but I don't think there's any code to support that now so nobody
must be using it.


Given that all of the above maps nicely into regulator constants and
regulator functions, I'd imagine that support could be added straight
into the generic regulator code and you won't have to touch max77686
at all (unless you wanted to add support for set_mode(), which doesn't
seem to be there).


All of the above would set _defaults_ for regulators.  If device-tree
people think it's wise, we could add "-default" into the names.

> +
>  Optional node:
>  - voltage-regulators : The regulators of max77686 have to be instantiated
>    under subnode named "voltage-regulators" using the following format.
> --
> 1.7.8.6
>

-Doug

Re: [RFC PATCH 3/3] dt: Document: Add optional MAX77686 operating mode bindings

[Mark Brown]

On Mon, Dec 10, 2012 at 10:30:42AM -0800, Doug Anderson wrote:

> The max77686 appears to support NORMAL and IDLE mode for most LDOs, so
> the only reasonable values for these two would be 2 and 4.  It appears
> that it would also be nonsensical (impossible to map to a register
> value) to have REGULATOR_MODE_NORMAL for suspend and
> REGULATOR_MODE_IDLE for the normal running mode.

The concept of normal and idle modes is Linux-specific, it's not a
generic hardware property.  You'd want to have something like the
expected current drain, the regulator API does already have support for
mapping into an optimal mode already.

Re: [RFC PATCH 2/3] regulator: max77686: Add support for various operating modes

[Mark Brown]

On Mon, Dec 10, 2012 at 11:56:23AM +0530, Abhilash Kesavan wrote:

> +		if (of_property_read_u32(rdata[i].of_node, "max77686-opmode",
> +			&rdata[i].opmode)) {
> +			dev_warn(iodev->dev, "no op_mode property property at %s\n",
> +			rmatch.name);
> +			rdata[i].opmode = regulators[i].enable_mask;
> +		}

Binding documenation is mandatory for any new OF properties, please add
this.

Re: [RFC PATCH 2/3] regulator: max77686: Add support for various operating modes

[Abhilash Kesavan]

Mark Brown <broonie <at> opensource.wolfsonmicro.com> writes:

[...]
> Binding documenation is mandatory for any new OF properties, please add
> this.
Patch 3/3 of this series adds documentation for the max77686-opmode property.
> 

Re: [RFC PATCH 2/3] regulator: max77686: Add support for various operating modes

[Abhilash Kesavan]

Adding missed Cc's

On Mon, Dec 10, 2012 at 1:49 PM, Abhilash Kesavan
<kesavan.abhilash@gmail.com> wrote:
> Mark Brown <broonie <at> opensource.wolfsonmicro.com> writes:
>
> [...]
>> Binding documenation is mandatory for any new OF properties, please add
>> this.
> Patch 3/3 of this series adds documentation for the max77686-opmode property.
>>
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 2/3] regulator: max77686: Add support for various operating modes

[Mark Brown]

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

On Mon, Dec 10, 2012 at 02:06:49PM +0530, Abhilash Kesavan wrote:
> On Mon, Dec 10, 2012 at 1:49 PM, Abhilash Kesavan
> <kesavan.abhilash@gmail.com> wrote:

> > Mark Brown <broonie <at> opensource.wolfsonmicro.com> writes:

> >> Binding documenation is mandatory for any new OF properties, please add
> >> this.

> > Patch 3/3 of this series adds documentation for the max77686-opmode property.

There is no point in splitting changes like this up, it just makes
review harder - in this case it caused me to not read your patch due
to the missing documentation.  Put the whole change together unless
things are getting too big to review.

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

Re: [RFC PATCH 2/3] regulator: max77686: Add support for various operating modes

[Abhilash Kesavan]

On Tue, Dec 11, 2012 at 6:27 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Dec 10, 2012 at 02:06:49PM +0530, Abhilash Kesavan wrote:
>> On Mon, Dec 10, 2012 at 1:49 PM, Abhilash Kesavan
>> <kesavan.abhilash@gmail.com> wrote:
>
>> > Mark Brown <broonie <at> opensource.wolfsonmicro.com> writes:
>
>> >> Binding documenation is mandatory for any new OF properties, please add
>> >> this.
>
>> > Patch 3/3 of this series adds documentation for the max77686-opmode property.
>
> There is no point in splitting changes like this up, it just makes
> review harder - in this case it caused me to not read your patch due
> to the missing documentation.  Put the whole change together unless
> things are getting too big to review.
OK.

Can you please have a look at Doug's proposal for handling the
operating mode generically in patch 3/3 of this series.
If you are OK with it then I'll start work on implementing it.

Thanks,
Abhilash