[PATCH v2 0/4] regulator: max77686/trats2: Disable some regulators in suspend

[PATCH v2 0/4] regulator: max77686/trats2: Disable some regulators in suspend

[Krzysztof Kozlowski]

Hi,


Changes since v1
================
1. Add patch 1/4 and 3/4.
2. Patch 2/4: Extend existing set_suspend_disable (for bucks) with
   LDO support. Implement set_suspend_enable. Suggested by Javier.
3. Patch 4/4: Add regulator suspend properties only to regulators
   actually supporting this. Other regulators, not implementing
   suspend enable/disable, will warn ("VCC_1.8V_IO: No configuration").
4. Patch 4/4: In suspend enable VHSIC_1.0V and VHSIC_1.8V regulators.
   Suggested by Chanwoo Choi.


Description
===========
This patchset makes use of Javier Martinez Cainllas changes [1].
It allows disabling some of the regulators during suspend to RAM.

Javier's patchset is necessary only to test it.

[1] [PATCH v3 0/2] ARM: EXYNOS: Call regulator suspend prepare/finish
    https://lkml.org/lkml/2014/10/20/545


Best regards,
Krzysztof


Krzysztof Kozlowski (4):
  regulator: max77686: Replace hard-coded opmode values with defines
  regulator: max77686: Implement suspend disable for some LDOs
  mfd/regulator: dt-bindings: max77686: Document regulators off in
    suspend
  ARM: dts: exynos4412-trats: Add suspend configuration for max77686
    regulators

 Documentation/devicetree/bindings/mfd/max77686.txt |  6 ++
 arch/arm/boot/dts/exynos4412-trats2.dts            | 72 +++++++++++++---------
 drivers/regulator/max77686.c                       | 58 +++++++++++++----
 3 files changed, 94 insertions(+), 42 deletions(-)

-- 
1.9.1

[PATCH v2 1/4] regulator: max77686: Replace hard-coded opmode values with defines

[Krzysztof Kozlowski]

Add defines for regulator operating modes which should be more readable,
especially if one does not have Maxim 77686 datasheet.

The patch does not introduce any functional change.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Suggested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/regulator/max77686.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index ef1af2debbd2..cffe0c69d57d 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -45,6 +45,16 @@
 #define MAX77686_DVS_MINUV	600000
 #define MAX77686_DVS_UVSTEP	12500
 
+/* On/off controlled by PWRREQ */
+#define MAX77686_OPMODE_OFF_PWRREQ	0x1
+/*
+ * For some regulators this means:
+ *  - forcing low power mode.
+ *  - low power mode controlled by PWRREQ.
+ */
+#define MAX77686_OPMODE_LOWPOWER	0x2
+#define MAX77686_OPMODE_NORMAL		0x3
+
 #define MAX77686_OPMODE_SHIFT	6
 #define MAX77686_OPMODE_BUCK234_SHIFT	4
 #define MAX77686_OPMODE_MASK	0x3
@@ -76,9 +86,10 @@ static int max77686_buck_set_suspend_disable(struct regulator_dev *rdev)
 	int ret, id = rdev_get_id(rdev);
 
 	if (id == MAX77686_BUCK1)
-		val = 0x1;
+		val = MAX77686_OPMODE_OFF_PWRREQ;
 	else
-		val = 0x1 << MAX77686_OPMODE_BUCK234_SHIFT;
+		val = MAX77686_OPMODE_OFF_PWRREQ
+			<< MAX77686_OPMODE_BUCK234_SHIFT;
 
 	ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
 				 rdev->desc->enable_mask, val);
@@ -103,10 +114,10 @@ static int max77686_set_suspend_mode(struct regulator_dev *rdev,
 
 	switch (mode) {
 	case REGULATOR_MODE_IDLE:			/* ON in LP Mode */
-		val = 0x2 << MAX77686_OPMODE_SHIFT;
+		val = MAX77686_OPMODE_LOWPOWER << MAX77686_OPMODE_SHIFT;
 		break;
 	case REGULATOR_MODE_NORMAL:			/* ON in Normal Mode */
-		val = 0x3 << MAX77686_OPMODE_SHIFT;
+		val = MAX77686_OPMODE_NORMAL << MAX77686_OPMODE_SHIFT;
 		break;
 	default:
 		pr_warn("%s: regulator_suspend_mode : 0x%x not supported\n",
@@ -133,13 +144,13 @@ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev,
 
 	switch (mode) {
 	case REGULATOR_MODE_STANDBY:			/* switch off */
-		val = 0x1 << MAX77686_OPMODE_SHIFT;
+		val = MAX77686_OPMODE_OFF_PWRREQ << MAX77686_OPMODE_SHIFT;
 		break;
 	case REGULATOR_MODE_IDLE:			/* ON in LP Mode */
-		val = 0x2 << MAX77686_OPMODE_SHIFT;
+		val = MAX77686_OPMODE_LOWPOWER << MAX77686_OPMODE_SHIFT;
 		break;
 	case REGULATOR_MODE_NORMAL:			/* ON in Normal Mode */
-		val = 0x3 << MAX77686_OPMODE_SHIFT;
+		val = MAX77686_OPMODE_NORMAL << MAX77686_OPMODE_SHIFT;
 		break;
 	default:
 		pr_warn("%s: regulator_suspend_mode : 0x%x not supported\n",
-- 
1.9.1

Re: [PATCH v2 1/4] regulator: max77686: Replace hard-coded opmode values with defines

[Javier Martinez Canillas]

Hello Krzysztof,

Thanks a lot for the re-spin.

On 10/21/2014 01:23 PM, Krzysztof Kozlowski wrote:
> Add defines for regulator operating modes which should be more readable,
> especially if one does not have Maxim 77686 datasheet.
> 
> The patch does not introduce any functional change.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Suggested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  drivers/regulator/max77686.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
> index ef1af2debbd2..cffe0c69d57d 100644
> --- a/drivers/regulator/max77686.c
> +++ b/drivers/regulator/max77686.c
> @@ -45,6 +45,16 @@
>  #define MAX77686_DVS_MINUV	600000
>  #define MAX77686_DVS_UVSTEP	12500
>  
> +/* On/off controlled by PWRREQ */
> +#define MAX77686_OPMODE_OFF_PWRREQ	0x1

Minor nit: maybe this should be called MAX77802_OFF_PWRREQ (without the OPMODE)?
Since afaiu from a conceptual pov the regulators only supports two modes: normal
and low power mode.

If a regulator is disabled or put in low power mode by PWRREQ on suspend, then
is not really an operating mode (the regulator output will still be normal until
suspend) but an enable pin to switch a regulator mode automatically by the SoC.

> +/*
> + * For some regulators this means:
> + *  - forcing low power mode.
> + *  - low power mode controlled by PWRREQ.
> + */
> +#define MAX77686_OPMODE_LOWPOWER	0x2

Can you also add a #define MAX77686_LP_PWRREQ 0x2 and use each one when
appropriate? That will better document when 0x2 means low power by PWRREQ
and when it means change regulator mode to low power.

Also, are you sure that forcing low power mode is 0x2 and not 0x1?

I'm asking because I see in the max77802 data-sheet that regulators that
supports its mode to be low power during runtime are using 0x1:

01b: Output ON in Low Power Mode

Maybe is different in the max77686 or I misunderstood your comment though...

> +#define MAX77686_OPMODE_NORMAL		0x3
> +
>  #define MAX77686_OPMODE_SHIFT	6
>  #define MAX77686_OPMODE_BUCK234_SHIFT	4
>  #define MAX77686_OPMODE_MASK	0x3
> @@ -76,9 +86,10 @@ static int max77686_buck_set_suspend_disable(struct regulator_dev *rdev)
>  	int ret, id = rdev_get_id(rdev);
>  
>  	if (id == MAX77686_BUCK1)
> -		val = 0x1;
> +		val = MAX77686_OPMODE_OFF_PWRREQ;
>  	else
> -		val = 0x1 << MAX77686_OPMODE_BUCK234_SHIFT;
> +		val = MAX77686_OPMODE_OFF_PWRREQ
> +			<< MAX77686_OPMODE_BUCK234_SHIFT;
>  
>  	ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
>  				 rdev->desc->enable_mask, val);
> @@ -103,10 +114,10 @@ static int max77686_set_suspend_mode(struct regulator_dev *rdev,
>  
>  	switch (mode) {
>  	case REGULATOR_MODE_IDLE:			/* ON in LP Mode */
> -		val = 0x2 << MAX77686_OPMODE_SHIFT;
> +		val = MAX77686_OPMODE_LOWPOWER << MAX77686_OPMODE_SHIFT;
>  		break;
>  	case REGULATOR_MODE_NORMAL:			/* ON in Normal Mode */
> -		val = 0x3 << MAX77686_OPMODE_SHIFT;
> +		val = MAX77686_OPMODE_NORMAL << MAX77686_OPMODE_SHIFT;
>  		break;
>  	default:
>  		pr_warn("%s: regulator_suspend_mode : 0x%x not supported\n",
> @@ -133,13 +144,13 @@ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev,
>  
>  	switch (mode) {
>  	case REGULATOR_MODE_STANDBY:			/* switch off */
> -		val = 0x1 << MAX77686_OPMODE_SHIFT;
> +		val = MAX77686_OPMODE_OFF_PWRREQ << MAX77686_OPMODE_SHIFT;
>  		break;
>  	case REGULATOR_MODE_IDLE:			/* ON in LP Mode */
> -		val = 0x2 << MAX77686_OPMODE_SHIFT;
> +		val = MAX77686_OPMODE_LOWPOWER << MAX77686_OPMODE_SHIFT;
>  		break;
>  	case REGULATOR_MODE_NORMAL:			/* ON in Normal Mode */
> -		val = 0x3 << MAX77686_OPMODE_SHIFT;
> +		val = MAX77686_OPMODE_NORMAL << MAX77686_OPMODE_SHIFT;
>  		break;
>  	default:
>  		pr_warn("%s: regulator_suspend_mode : 0x%x not supported\n",
> 

If you do those two changes please feel free to add my Reviewed-by.

Best regards,
Javier

Re: [PATCH v2 1/4] regulator: max77686: Replace hard-coded opmode values with defines

[Krzysztof Kozlowski]

On wto, 2014-10-21 at 13:57 +0200, Javier Martinez Canillas wrote:
> Hello Krzysztof,
> 
> Thanks a lot for the re-spin.
> 
> On 10/21/2014 01:23 PM, Krzysztof Kozlowski wrote:
> > Add defines for regulator operating modes which should be more readable,
> > especially if one does not have Maxim 77686 datasheet.
> > 
> > The patch does not introduce any functional change.
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > Suggested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> > ---
> >  drivers/regulator/max77686.c | 25 ++++++++++++++++++-------
> >  1 file changed, 18 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
> > index ef1af2debbd2..cffe0c69d57d 100644
> > --- a/drivers/regulator/max77686.c
> > +++ b/drivers/regulator/max77686.c
> > @@ -45,6 +45,16 @@
> >  #define MAX77686_DVS_MINUV	600000
> >  #define MAX77686_DVS_UVSTEP	12500
> >  
> > +/* On/off controlled by PWRREQ */
> > +#define MAX77686_OPMODE_OFF_PWRREQ	0x1
> 
> Minor nit: maybe this should be called MAX77802_OFF_PWRREQ (without the OPMODE)?
> Since afaiu from a conceptual pov the regulators only supports two modes: normal
> and low power mode.
> 
> If a regulator is disabled or put in low power mode by PWRREQ on suspend, then
> is not really an operating mode (the regulator output will still be normal until
> suspend) but an enable pin to switch a regulator mode automatically by the SoC.

No problem. This is just a define for value used in register. However
OPMODE is used in other places in that driver:
#define MAX77686_OPMODE_SHIFT   6
#define MAX77686_OPMODE_BUCK234_SHIFT   4
#define MAX77686_OPMODE_MASK    0x3

So this would be a little inconsistent...

> 
> > +/*
> > + * For some regulators this means:
> > + *  - forcing low power mode.
> > + *  - low power mode controlled by PWRREQ.
> > + */
> > +#define MAX77686_OPMODE_LOWPOWER	0x2
> 
> Can you also add a #define MAX77686_LP_PWRREQ 0x2 and use each one when
> appropriate? That will better document when 0x2 means low power by PWRREQ
> and when it means change regulator mode to low power.

Actually I made mistake in that comment. The 0x2 means force low power
mode only for buck1-4... but there is no set_suspend_mode for these
bucks. So this #define has only one semantic value.

> Also, are you sure that forcing low power mode is 0x2 and not 0x1?

You're right, for other LDOs, low power mode may be forced with 0x1. For
such regulators the max77686_set_suspend_mode() is used which does not
set value 0x1. This means that each define has only one semantic
meaning.

I'll re-spin and fix the documentation to reflect both device
capabilities and how the driver currently works.

Thanks for pointing this.

> 
> I'm asking because I see in the max77802 data-sheet that regulators that
> supports its mode to be low power during runtime are using 0x1:
> 
> 01b: Output ON in Low Power Mode
> 
> Maybe is different in the max77686 or I misunderstood your comment though...
> 
> > +#define MAX77686_OPMODE_NORMAL		0x3
> > +
> >  #define MAX77686_OPMODE_SHIFT	6
> >  #define MAX77686_OPMODE_BUCK234_SHIFT	4
> >  #define MAX77686_OPMODE_MASK	0x3
> > @@ -76,9 +86,10 @@ static int max77686_buck_set_suspend_disable(struct regulator_dev *rdev)
> >  	int ret, id = rdev_get_id(rdev);
> >  
> >  	if (id == MAX77686_BUCK1)
> > -		val = 0x1;
> > +		val = MAX77686_OPMODE_OFF_PWRREQ;
> >  	else
> > -		val = 0x1 << MAX77686_OPMODE_BUCK234_SHIFT;
> > +		val = MAX77686_OPMODE_OFF_PWRREQ
> > +			<< MAX77686_OPMODE_BUCK234_SHIFT;
> >  
> >  	ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
> >  				 rdev->desc->enable_mask, val);
> > @@ -103,10 +114,10 @@ static int max77686_set_suspend_mode(struct regulator_dev *rdev,
> >  
> >  	switch (mode) {
> >  	case REGULATOR_MODE_IDLE:			/* ON in LP Mode */
> > -		val = 0x2 << MAX77686_OPMODE_SHIFT;
> > +		val = MAX77686_OPMODE_LOWPOWER << MAX77686_OPMODE_SHIFT;
> >  		break;
> >  	case REGULATOR_MODE_NORMAL:			/* ON in Normal Mode */
> > -		val = 0x3 << MAX77686_OPMODE_SHIFT;
> > +		val = MAX77686_OPMODE_NORMAL << MAX77686_OPMODE_SHIFT;
> >  		break;
> >  	default:
> >  		pr_warn("%s: regulator_suspend_mode : 0x%x not supported\n",
> > @@ -133,13 +144,13 @@ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev,
> >  
> >  	switch (mode) {
> >  	case REGULATOR_MODE_STANDBY:			/* switch off */
> > -		val = 0x1 << MAX77686_OPMODE_SHIFT;
> > +		val = MAX77686_OPMODE_OFF_PWRREQ << MAX77686_OPMODE_SHIFT;
> >  		break;
> >  	case REGULATOR_MODE_IDLE:			/* ON in LP Mode */
> > -		val = 0x2 << MAX77686_OPMODE_SHIFT;
> > +		val = MAX77686_OPMODE_LOWPOWER << MAX77686_OPMODE_SHIFT;
> >  		break;
> >  	case REGULATOR_MODE_NORMAL:			/* ON in Normal Mode */
> > -		val = 0x3 << MAX77686_OPMODE_SHIFT;
> > +		val = MAX77686_OPMODE_NORMAL << MAX77686_OPMODE_SHIFT;
> >  		break;
> >  	default:
> >  		pr_warn("%s: regulator_suspend_mode : 0x%x not supported\n",
> > 
> 
> If you do those two changes please feel free to add my Reviewed-by.
> 
> Best regards,
> Javier

Re: [PATCH v2 1/4] regulator: max77686: Replace hard-coded opmode values with defines

[Javier Martinez Canillas]

Hello Krzysztof,

On 10/21/2014 02:22 PM, Krzysztof Kozlowski wrote:
> On wto, 2014-10-21 at 13:57 +0200, Javier Martinez Canillas wrote:
>> >  
>> > +/* On/off controlled by PWRREQ */
>> > +#define MAX77686_OPMODE_OFF_PWRREQ	0x1
>> 
>> Minor nit: maybe this should be called MAX77802_OFF_PWRREQ (without the OPMODE)?
>> Since afaiu from a conceptual pov the regulators only supports two modes: normal
>> and low power mode.
>> 
>> If a regulator is disabled or put in low power mode by PWRREQ on suspend, then
>> is not really an operating mode (the regulator output will still be normal until
>> suspend) but an enable pin to switch a regulator mode automatically by the SoC.
> 
> No problem. This is just a define for value used in register. However
> OPMODE is used in other places in that driver:
> #define MAX77686_OPMODE_SHIFT   6
> #define MAX77686_OPMODE_BUCK234_SHIFT   4
> #define MAX77686_OPMODE_MASK    0x3
>
> So this would be a little inconsistent...

Indeed, although I think there is a difference between talking about the OPMODE
field of the regulators control register (which the shift and mask refer to)
and the semantics of the value that is written (whether is an mode or an enable
pin).

But yeah, I agree the Maxim documentation is confusing and confused me too
before Mark explained to me that only two operating modes exist for this device.

> 
>> 
>> > +/*
>> > + * For some regulators this means:
>> > + *  - forcing low power mode.
>> > + *  - low power mode controlled by PWRREQ.
>> > + */
>> > +#define MAX77686_OPMODE_LOWPOWER	0x2
>> 
>> Can you also add a #define MAX77686_LP_PWRREQ 0x2 and use each one when
>> appropriate? That will better document when 0x2 means low power by PWRREQ
>> and when it means change regulator mode to low power.
> 
> Actually I made mistake in that comment. The 0x2 means force low power
> mode only for buck1-4... but there is no set_suspend_mode for these
> bucks. So this #define has only one semantic value.
> 
>> Also, are you sure that forcing low power mode is 0x2 and not 0x1?
> 
> You're right, for other LDOs, low power mode may be forced with 0x1. For
> such regulators the max77686_set_suspend_mode() is used which does not
> set value 0x1. This means that each define has only one semantic
> meaning.
> 
> I'll re-spin and fix the documentation to reflect both device
> capabilities and how the driver currently works.
> 
> Thanks for pointing this.
> 

You are welcome.

Best regards,
Javier 

[PATCH v2 2/4] regulator: max77686: Implement suspend disable for some LDOs

[Krzysztof Kozlowski]

Some LDOs of Maxim 77686 PMIC support disabling during system suspend
(LDO{2,6,7,8,10,11,12,14,15,16}). This was already implemented as part
of set_suspend_mode function. In that case the mode was one of:
 - disable,
 - normal mode,
 - low power mode.
However there are no bindings for setting the mode during suspend.

Add suspend disable for LDO regulators supporting this to the existing
max77686_buck_set_suspend_disable() function. This helps reducing
energy consumption during system sleep.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/regulator/max77686.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index cffe0c69d57d..05df72d584bf 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -79,17 +79,33 @@ struct max77686_data {
 };
 
 /* Some BUCKS supports Normal[ON/OFF] mode during suspend */
-static int max77686_buck_set_suspend_disable(struct regulator_dev *rdev)
+static int max77686_set_suspend_disable(struct regulator_dev *rdev)
 {
 	unsigned int val;
 	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
 	int ret, id = rdev_get_id(rdev);
 
-	if (id == MAX77686_BUCK1)
+	switch (id) {
+	case MAX77686_BUCK1:
 		val = MAX77686_OPMODE_OFF_PWRREQ;
-	else
+		break;
+	case MAX77686_BUCK2:
+	case MAX77686_BUCK3:
+	case MAX77686_BUCK4:
 		val = MAX77686_OPMODE_OFF_PWRREQ
 			<< MAX77686_OPMODE_BUCK234_SHIFT;
+		break;
+	case MAX77686_LDO2:
+	case MAX77686_LDO6 ... MAX77686_LDO8:
+	case MAX77686_LDO10 ... MAX77686_LDO12:
+	case MAX77686_LDO14 ... MAX77686_LDO16:
+		val = MAX77686_OPMODE_OFF_PWRREQ << MAX77686_OPMODE_SHIFT;
+		break;
+	default:
+		pr_warn("%s: regulator_suspend_disable not supported\n",
+			rdev->desc->name);
+		return -EINVAL;
+	}
 
 	ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
 				 rdev->desc->enable_mask, val);
@@ -171,6 +187,9 @@ static int max77686_enable(struct regulator_dev *rdev)
 {
 	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
 
+	if (max77686->opmode[rdev_get_id(rdev)] == MAX77686_OPMODE_OFF_PWRREQ)
+		max77686->opmode[rdev_get_id(rdev)] = MAX77686_OPMODE_NORMAL;
+
 	return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
 				  rdev->desc->enable_mask,
 				  max77686->opmode[rdev_get_id(rdev)]);
@@ -223,6 +242,8 @@ static struct regulator_ops max77686_ldo_ops = {
 	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
 	.set_voltage_time_sel	= regulator_set_voltage_time_sel,
 	.set_suspend_mode	= max77686_ldo_set_suspend_mode,
+	.set_suspend_disable	= max77686_set_suspend_disable,
+	.set_suspend_enable	= max77686_enable,
 };
 
 static struct regulator_ops max77686_buck1_ops = {
@@ -234,7 +255,8 @@ static struct regulator_ops max77686_buck1_ops = {
 	.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_suspend_disable	= max77686_buck_set_suspend_disable,
+	.set_suspend_disable	= max77686_set_suspend_disable,
+	.set_suspend_enable	= max77686_enable,
 };
 
 static struct regulator_ops max77686_buck_dvs_ops = {
@@ -247,7 +269,8 @@ static struct regulator_ops max77686_buck_dvs_ops = {
 	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
 	.set_voltage_time_sel	= regulator_set_voltage_time_sel,
 	.set_ramp_delay		= max77686_set_ramp_delay,
-	.set_suspend_disable	= max77686_buck_set_suspend_disable,
+	.set_suspend_disable	= max77686_set_suspend_disable,
+	.set_suspend_enable	= max77686_enable,
 };
 
 #define regulator_desc_ldo(num)		{				\
-- 
1.9.1

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

Re: [PATCH v2 2/4] regulator: max77686: Implement suspend disable for some LDOs

[Javier Martinez Canillas]

Hello Krzysztof,

On 10/21/2014 01:23 PM, Krzysztof Kozlowski wrote:
> Some LDOs of Maxim 77686 PMIC support disabling during system suspend
> (LDO{2,6,7,8,10,11,12,14,15,16}). This was already implemented as part
> of set_suspend_mode function. In that case the mode was one of:
>  - disable,
>  - normal mode,
>  - low power mode.
> However there are no bindings for setting the mode during suspend.
> 
> Add suspend disable for LDO regulators supporting this to the existing
> max77686_buck_set_suspend_disable() function. This helps reducing
> energy consumption during system sleep.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/regulator/max77686.c | 33 ++++++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
> index cffe0c69d57d..05df72d584bf 100644
> --- a/drivers/regulator/max77686.c
> +++ b/drivers/regulator/max77686.c
> @@ -79,17 +79,33 @@ struct max77686_data {
>  };
>  
>  /* Some BUCKS supports Normal[ON/OFF] mode during suspend */
> -static int max77686_buck_set_suspend_disable(struct regulator_dev *rdev)
> +static int max77686_set_suspend_disable(struct regulator_dev *rdev)
>  {
>  	unsigned int val;
>  	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
>  	int ret, id = rdev_get_id(rdev);
>  
> -	if (id == MAX77686_BUCK1)
> +	switch (id) {
> +	case MAX77686_BUCK1:
>  		val = MAX77686_OPMODE_OFF_PWRREQ;
> -	else
> +		break;
> +	case MAX77686_BUCK2:
> +	case MAX77686_BUCK3:
> +	case MAX77686_BUCK4:
>  		val = MAX77686_OPMODE_OFF_PWRREQ
>  			<< MAX77686_OPMODE_BUCK234_SHIFT;
> +		break;
> +	case MAX77686_LDO2:
> +	case MAX77686_LDO6 ... MAX77686_LDO8:
> +	case MAX77686_LDO10 ... MAX77686_LDO12:
> +	case MAX77686_LDO14 ... MAX77686_LDO16:
> +		val = MAX77686_OPMODE_OFF_PWRREQ << MAX77686_OPMODE_SHIFT;
> +		break;
> +	default:
> +		pr_warn("%s: regulator_suspend_disable not supported\n",
> +			rdev->desc->name);
> +		return -EINVAL;
> +	}
>  
>  	ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
>  				 rdev->desc->enable_mask, val);
> @@ -171,6 +187,9 @@ static int max77686_enable(struct regulator_dev *rdev)
>  {
>  	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
>  
> +	if (max77686->opmode[rdev_get_id(rdev)] == MAX77686_OPMODE_OFF_PWRREQ)
> +		max77686->opmode[rdev_get_id(rdev)] = MAX77686_OPMODE_NORMAL;
> +
>  	return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
>  				  rdev->desc->enable_mask,
>  				  max77686->opmode[rdev_get_id(rdev)]);
> @@ -223,6 +242,8 @@ static struct regulator_ops max77686_ldo_ops = {
>  	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
>  	.set_voltage_time_sel	= regulator_set_voltage_time_sel,
>  	.set_suspend_mode	= max77686_ldo_set_suspend_mode,
> +	.set_suspend_disable	= max77686_set_suspend_disable,
> +	.set_suspend_enable	= max77686_enable,
>  };
>  
>  static struct regulator_ops max77686_buck1_ops = {
> @@ -234,7 +255,8 @@ static struct regulator_ops max77686_buck1_ops = {
>  	.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_suspend_disable	= max77686_buck_set_suspend_disable,
> +	.set_suspend_disable	= max77686_set_suspend_disable,
> +	.set_suspend_enable	= max77686_enable,
>  };
>  
>  static struct regulator_ops max77686_buck_dvs_ops = {
> @@ -247,7 +269,8 @@ static struct regulator_ops max77686_buck_dvs_ops = {
>  	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
>  	.set_voltage_time_sel	= regulator_set_voltage_time_sel,
>  	.set_ramp_delay		= max77686_set_ramp_delay,
> -	.set_suspend_disable	= max77686_buck_set_suspend_disable,
> +	.set_suspend_disable	= max77686_set_suspend_disable,
> +	.set_suspend_enable	= max77686_enable,
>  };
>  
>  #define regulator_desc_ldo(num)		{				\
> 

Looks good to me.

Reviewed-by: Javier Martinez Canillas <javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>

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

[PATCH v2 3/4] mfd/regulator: dt-bindings: max77686: Document regulators off in suspend

[Krzysztof Kozlowski]

Add information which regulators can be disabled during system suspend.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Suggested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 Documentation/devicetree/bindings/mfd/max77686.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/max77686.txt b/Documentation/devicetree/bindings/mfd/max77686.txt
index 678f3cf0b8f0..75fdfaf41831 100644
--- a/Documentation/devicetree/bindings/mfd/max77686.txt
+++ b/Documentation/devicetree/bindings/mfd/max77686.txt
@@ -34,6 +34,12 @@ to get matched with their hardware counterparts as follow:
 	-BUCKn 	:	for BUCKs, where n can lie in range 1 to 9.
 			example: BUCK1, BUCK5, BUCK9.
 
+  Regulators which can be turned off during system suspend:
+	-LDOn	:	2, 6-8, 10-12, 14-16,
+	-BUCKn	:	1-4.
+  Use standard regulator bindings for it ('regulator-off-in-suspend').
+
+
 Example:
 
 	max77686@09 {
-- 
1.9.1

Re: [PATCH v2 3/4] mfd/regulator: dt-bindings: max77686: Document regulators off in suspend

[Javier Martinez Canillas]

Hello Krzysztof,

On 10/21/2014 01:23 PM, Krzysztof Kozlowski wrote:
> Add information which regulators can be disabled during system suspend.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Suggested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  Documentation/devicetree/bindings/mfd/max77686.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/max77686.txt b/Documentation/devicetree/bindings/mfd/max77686.txt
> index 678f3cf0b8f0..75fdfaf41831 100644
> --- a/Documentation/devicetree/bindings/mfd/max77686.txt
> +++ b/Documentation/devicetree/bindings/mfd/max77686.txt
> @@ -34,6 +34,12 @@ to get matched with their hardware counterparts as follow:
>  	-BUCKn 	:	for BUCKs, where n can lie in range 1 to 9.
>  			example: BUCK1, BUCK5, BUCK9.
>  
> +  Regulators which can be turned off during system suspend:
> +	-LDOn	:	2, 6-8, 10-12, 14-16,
> +	-BUCKn	:	1-4.
> +  Use standard regulator bindings for it ('regulator-off-in-suspend').
> +
> +
>  Example:
>  
>  	max77686@09 {
> 

Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

Best regards,
Javier

Re: [PATCH v2 3/4] mfd/regulator: dt-bindings: max77686: Document regulators off in suspend

[Lee Jones]

On Tue, 21 Oct 2014, Krzysztof Kozlowski wrote:

> Add information which regulators can be disabled during system suspend.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Suggested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  Documentation/devicetree/bindings/mfd/max77686.txt | 6 ++++++
>  1 file changed, 6 insertions(+)

I don't think this requires a DT Ack.

Applied with the extra '\n' removed.

> diff --git a/Documentation/devicetree/bindings/mfd/max77686.txt b/Documentation/devicetree/bindings/mfd/max77686.txt
> index 678f3cf0b8f0..75fdfaf41831 100644
> --- a/Documentation/devicetree/bindings/mfd/max77686.txt
> +++ b/Documentation/devicetree/bindings/mfd/max77686.txt
> @@ -34,6 +34,12 @@ to get matched with their hardware counterparts as follow:
>  	-BUCKn 	:	for BUCKs, where n can lie in range 1 to 9.
>  			example: BUCK1, BUCK5, BUCK9.
>  
> +  Regulators which can be turned off during system suspend:
> +	-LDOn	:	2, 6-8, 10-12, 14-16,
> +	-BUCKn	:	1-4.
> +  Use standard regulator bindings for it ('regulator-off-in-suspend').
> +
> +
>  Example:
>  
>  	max77686@09 {

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 4/4] ARM: dts: exynos4412-trats: Add suspend configuration for max77686 regulators

[Krzysztof Kozlowski]

Add suspend to RAM configuration for max77686 regulators. Some LDOs and
bucks are disabled. This reduces energy consumption during S2R,
approximately from 17 mA to 9 mA.

Additionally remove old and not supported bindings:
 - regulator-mem-off
 - regulator-mem-idle
 - regulator-mem-on
The max77686 driver does not parse them and they are not documented
anywere.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 arch/arm/boot/dts/exynos4412-trats2.dts | 72 +++++++++++++++++++--------------
 1 file changed, 42 insertions(+), 30 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4412-trats2.dts b/arch/arm/boot/dts/exynos4412-trats2.dts
index dd9ac66770f7..8067eb447829 100644
--- a/arch/arm/boot/dts/exynos4412-trats2.dts
+++ b/arch/arm/boot/dts/exynos4412-trats2.dts
@@ -225,7 +225,6 @@
 					regulator-min-microvolt = <1000000>;
 					regulator-max-microvolt = <1000000>;
 					regulator-always-on;
-					regulator-mem-on;
 				};
 
 				ldo2_reg: ldo2 {
@@ -234,7 +233,9 @@
 					regulator-min-microvolt = <1200000>;
 					regulator-max-microvolt = <1200000>;
 					regulator-always-on;
-					regulator-mem-on;
+					regulator-state-mem {
+						regulator-on-in-suspend;
+					};
 				};
 
 				ldo3_reg: ldo3 {
@@ -243,7 +244,6 @@
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
 					regulator-always-on;
-					regulator-mem-on;
 				};
 
 				ldo4_reg: ldo4 {
@@ -252,7 +252,6 @@
 					regulator-min-microvolt = <2800000>;
 					regulator-max-microvolt = <2800000>;
 					regulator-always-on;
-					regulator-mem-on;
 				};
 
 				ldo5_reg: ldo5 {
@@ -261,7 +260,6 @@
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
 					regulator-always-on;
-					regulator-mem-on;
 				};
 
 				ldo6_reg: ldo6 {
@@ -270,7 +268,9 @@
 					regulator-min-microvolt = <1000000>;
 					regulator-max-microvolt = <1000000>;
 					regulator-always-on;
-					regulator-mem-on;
+					regulator-state-mem {
+						regulator-on-in-suspend;
+					};
 				};
 
 				ldo7_reg: ldo7 {
@@ -279,7 +279,9 @@
 					regulator-min-microvolt = <1000000>;
 					regulator-max-microvolt = <1000000>;
 					regulator-always-on;
-					regulator-mem-on;
+					regulator-state-mem {
+						regulator-on-in-suspend;
+					};
 				};
 
 				ldo8_reg: ldo8 {
@@ -287,7 +289,9 @@
 					regulator-name = "VMIPI_1.0V";
 					regulator-min-microvolt = <1000000>;
 					regulator-max-microvolt = <1000000>;
-					regulator-mem-off;
+					regulator-state-mem {
+						regulator-off-in-suspend;
+					};
 				};
 
 				ldo9_reg: ldo9 {
@@ -295,7 +299,6 @@
 					regulator-name = "CAM_ISP_MIPI_1.2V";
 					regulator-min-microvolt = <1200000>;
 					regulator-max-microvolt = <1200000>;
-					regulator-mem-idle;
 				};
 
 				ldo10_reg: ldo10 {
@@ -303,7 +306,9 @@
 					regulator-name = "VMIPI_1.8V";
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
-					regulator-mem-off;
+					regulator-state-mem {
+						regulator-off-in-suspend;
+					};
 				};
 
 				ldo11_reg: ldo11 {
@@ -312,7 +317,9 @@
 					regulator-min-microvolt = <1950000>;
 					regulator-max-microvolt = <1950000>;
 					regulator-always-on;
-					regulator-mem-off;
+					regulator-state-mem {
+						regulator-off-in-suspend;
+					};
 				};
 
 				ldo12_reg: ldo12 {
@@ -320,7 +327,9 @@
 					regulator-name = "VUOTG_3.0V";
 					regulator-min-microvolt = <3000000>;
 					regulator-max-microvolt = <3000000>;
-					regulator-mem-off;
+					regulator-state-mem {
+						regulator-off-in-suspend;
+					};
 				};
 
 				ldo13_reg: ldo13 {
@@ -328,7 +337,6 @@
 					regulator-name = "NFC_AVDD_1.8V";
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
-					regulator-mem-idle;
 				};
 
 				ldo14_reg: ldo14 {
@@ -337,7 +345,9 @@
 					regulator-min-microvolt = <1950000>;
 					regulator-max-microvolt = <1950000>;
 					regulator-always-on;
-					regulator-mem-off;
+					regulator-state-mem {
+						regulator-off-in-suspend;
+					};
 				};
 
 				ldo15_reg: ldo15 {
@@ -345,7 +355,9 @@
 					regulator-name = "VHSIC_1.0V";
 					regulator-min-microvolt = <1000000>;
 					regulator-max-microvolt = <1000000>;
-					regulator-mem-off;
+					regulator-state-mem {
+						regulator-on-in-suspend;
+					};
 				};
 
 				ldo16_reg: ldo16 {
@@ -353,7 +365,9 @@
 					regulator-name = "VHSIC_1.8V";
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
-					regulator-mem-off;
+					regulator-state-mem {
+						regulator-on-in-suspend;
+					};
 				};
 
 				ldo17_reg: ldo17 {
@@ -361,7 +375,6 @@
 					regulator-name = "CAM_SENSOR_CORE_1.2V";
 					regulator-min-microvolt = <1200000>;
 					regulator-max-microvolt = <1200000>;
-					regulator-mem-idle;
 				};
 
 				ldo18_reg: ldo18 {
@@ -369,7 +382,6 @@
 					regulator-name = "CAM_ISP_SEN_IO_1.8V";
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
-					regulator-mem-idle;
 				};
 
 				ldo19_reg: ldo19 {
@@ -377,7 +389,6 @@
 					regulator-name = "VT_CAM_1.8V";
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
-					regulator-mem-idle;
 				};
 
 				ldo20_reg: ldo20 {
@@ -385,7 +396,6 @@
 					regulator-name = "VDDQ_PRE_1.8V";
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
-					regulator-mem-idle;
 				};
 
 				ldo21_reg: ldo21 {
@@ -393,7 +403,6 @@
 					regulator-name = "VTF_2.8V";
 					regulator-min-microvolt = <2800000>;
 					regulator-max-microvolt = <2800000>;
-					regulator-mem-idle;
 				};
 
 				ldo22_reg: ldo22 {
@@ -408,7 +417,6 @@
 					regulator-name = "TSP_AVDD_3.3V";
 					regulator-min-microvolt = <3300000>;
 					regulator-max-microvolt = <3300000>;
-					regulator-mem-idle;
 				};
 
 				ldo24_reg: ldo24 {
@@ -416,7 +424,6 @@
 					regulator-name = "TSP_VDD_1.8V";
 					regulator-min-microvolt = <1800000>;
 					regulator-max-microvolt = <1800000>;
-					regulator-mem-idle;
 				};
 
 				ldo25_reg: ldo25 {
@@ -424,7 +431,6 @@
 					regulator-name = "LCD_VCC_3.3V";
 					regulator-min-microvolt = <2800000>;
 					regulator-max-microvolt = <2800000>;
-					regulator-mem-idle;
 				};
 
 				ldo26_reg: ldo26 {
@@ -432,7 +438,6 @@
 					regulator-name = "MOTOR_VCC_3.0V";
 					regulator-min-microvolt = <3000000>;
 					regulator-max-microvolt = <3000000>;
-					regulator-mem-idle;
 				};
 
 				buck1_reg: buck1 {
@@ -442,7 +447,9 @@
 					regulator-max-microvolt = <1100000>;
 					regulator-always-on;
 					regulator-boot-on;
-					regulator-mem-off;
+					regulator-state-mem {
+						regulator-off-in-suspend;
+					};
 				};
 
 				buck2_reg: buck2 {
@@ -452,7 +459,9 @@
 					regulator-max-microvolt = <1500000>;
 					regulator-always-on;
 					regulator-boot-on;
-					regulator-mem-off;
+					regulator-state-mem {
+						regulator-on-in-suspend;
+					};
 				};
 
 				buck3_reg: buck3 {
@@ -462,7 +471,9 @@
 					regulator-max-microvolt = <1150000>;
 					regulator-always-on;
 					regulator-boot-on;
-					regulator-mem-off;
+					regulator-state-mem {
+						regulator-off-in-suspend;
+					};
 				};
 
 				buck4_reg: buck4 {
@@ -471,7 +482,9 @@
 					regulator-min-microvolt = <850000>;
 					regulator-max-microvolt = <1150000>;
 					regulator-boot-on;
-					regulator-mem-off;
+					regulator-state-mem {
+						regulator-off-in-suspend;
+					};
 				};
 
 				buck5_reg: buck5 {
@@ -510,7 +523,6 @@
 					regulator-name = "CAM_ISP_CORE_1.2V";
 					regulator-min-microvolt = <1000000>;
 					regulator-max-microvolt = <1200000>;
-					regulator-mem-off;
 				};
 			};
 		};
-- 
1.9.1

Re: [PATCH v2 4/4] ARM: dts: exynos4412-trats: Add suspend configuration for max77686 regulators

[Javier Martinez Canillas]

Hello Krzysztof,

On 10/21/2014 01:23 PM, Krzysztof Kozlowski wrote:
> Add suspend to RAM configuration for max77686 regulators. Some LDOs and
> bucks are disabled. This reduces energy consumption during S2R,
> approximately from 17 mA to 9 mA.
> 
> Additionally remove old and not supported bindings:
>  - regulator-mem-off
>  - regulator-mem-idle
>  - regulator-mem-on
> The max77686 driver does not parse them and they are not documented
> anywere.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  arch/arm/boot/dts/exynos4412-trats2.dts | 72 +++++++++++++++++++--------------
>  1 file changed, 42 insertions(+), 30 deletions(-)
> 

I'm not familiar with the board but the change looks good to me assuming each
regulator is doing the right thing.

I already gave you my Reviewed-by for the previous version but was not carried:

Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

Best regards,
Javier

Re: [PATCH v2 4/4] ARM: dts: exynos4412-trats: Add suspend configuration for max77686 regulators

[Krzysztof Kozlowski]

On wto, 2014-10-21 at 14:04 +0200, Javier Martinez Canillas wrote:
> Hello Krzysztof,
> 
> On 10/21/2014 01:23 PM, Krzysztof Kozlowski wrote:
> > Add suspend to RAM configuration for max77686 regulators. Some LDOs and
> > bucks are disabled. This reduces energy consumption during S2R,
> > approximately from 17 mA to 9 mA.
> > 
> > Additionally remove old and not supported bindings:
> >  - regulator-mem-off
> >  - regulator-mem-idle
> >  - regulator-mem-on
> > The max77686 driver does not parse them and they are not documented
> > anywere.
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > ---
> >  arch/arm/boot/dts/exynos4412-trats2.dts | 72 +++++++++++++++++++--------------
> >  1 file changed, 42 insertions(+), 30 deletions(-)
> > 
> 
> I'm not familiar with the board but the change looks good to me assuming each
> regulator is doing the right thing.
> 
> I already gave you my Reviewed-by for the previous version but was not carried:
> 
> Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

Thanks! I didn't add your review-by-tag because changes in the patch.

Best regards,
Krzysztof


> 
> Best regards,
> Javier