[PATCH v4 0/3] regulator: max77686/trats2: Disable some regulators in suspend

[PATCH v4 0/3] regulator: max77686/trats2: Disable some regulators in suspend

[Krzysztof Kozlowski]

Hi,


Changes since v3
================
1. Dropped patch 3/4 ("mfd/regulator: dt-bindings: max77686: Document
   regulators off in suspend"), applied by Lee Jones.
2. Patch 2/3: Fix missing shift when checking if current opmode is
   equal to off in suspend. The shift is various so I added a simple
   function max77686_set_opmode_enable(). Dropped Javier's review-by.
3. Patch 1/3: Add Javier's review-by.
4. Patch 3/3: Add Chanwoo's review-by.

Changes since v2
================
1. Patch 1/4: Fully describe different values used for controlling
   the regulators (low power modes, enable, disable). Remove "opmode"
   from new defines. Suggested by Javier.
2. Patch 2/4: minor nits after changing patch 1.
3. Add Javier's reviewed-by to 2, 3 and 4 patch.
4. Patch 3/4 (changes in documentation) should be picked up by Lee
   Jones but I'm resending it anyway.

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 (3):
  regulator: max77686: Replace hard-coded opmode values with defines
  regulator: max77686: Implement suspend disable for some LDOs
  ARM: dts: exynos4412-trats: Add suspend configuration for max77686
    regulators

 arch/arm/boot/dts/exynos4412-trats2.dts |  72 +++++++++++++---------
 drivers/regulator/max77686.c            | 105 +++++++++++++++++++++++++++-----
 2 files changed, 133 insertions(+), 44 deletions(-)

-- 
1.9.1

[PATCH v4 1/3] 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>
Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/regulator/max77686.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index ef1af2debbd2..3d0922051488 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -45,6 +45,25 @@
 #define MAX77686_DVS_MINUV	600000
 #define MAX77686_DVS_UVSTEP	12500
 
+/* Values used for configuring Buck[1234] */
+#define MAX77686_BUCK_OFF_PWRREQ	0x1
+#define MAX77686_BUCK_LOWPOWER		0x2
+#define MAX77686_BUCK_NORMAL		0x3
+
+/*
+ * Values used for configuring LDOn:
+ *  - LDO1, 3-5, 9, 13, 17-26: forcing low power mode
+ */
+#define MAX77686_LDO_LOWPOWER		0x1
+/*
+ * In the same time for LDOn:
+ *  - LDO2, 6-8, 10-12, 14-16: on/off controlled by PWRREQ
+ */
+#define MAX77686_LDO_OFF_PWRREQ		0x1
+/* Low power mode controlled by PWRREQ */
+#define MAX77686_LDO_LOWPOWER_PWRREQ	0x2
+#define MAX77686_LDO_NORMAL		0x3
+
 #define MAX77686_OPMODE_SHIFT	6
 #define MAX77686_OPMODE_BUCK234_SHIFT	4
 #define MAX77686_OPMODE_MASK	0x3
@@ -76,9 +95,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_BUCK_OFF_PWRREQ;
 	else
-		val = 0x1 << MAX77686_OPMODE_BUCK234_SHIFT;
+		val = MAX77686_BUCK_OFF_PWRREQ
+			<< MAX77686_OPMODE_BUCK234_SHIFT;
 
 	ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
 				 rdev->desc->enable_mask, val);
@@ -103,10 +123,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_LDO_LOWPOWER_PWRREQ << MAX77686_OPMODE_SHIFT;
 		break;
 	case REGULATOR_MODE_NORMAL:			/* ON in Normal Mode */
-		val = 0x3 << MAX77686_OPMODE_SHIFT;
+		val = MAX77686_LDO_NORMAL << MAX77686_OPMODE_SHIFT;
 		break;
 	default:
 		pr_warn("%s: regulator_suspend_mode : 0x%x not supported\n",
@@ -133,13 +153,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_LDO_OFF_PWRREQ << MAX77686_OPMODE_SHIFT;
 		break;
 	case REGULATOR_MODE_IDLE:			/* ON in LP Mode */
-		val = 0x2 << MAX77686_OPMODE_SHIFT;
+		val = MAX77686_LDO_LOWPOWER_PWRREQ << MAX77686_OPMODE_SHIFT;
 		break;
 	case REGULATOR_MODE_NORMAL:			/* ON in Normal Mode */
-		val = 0x3 << MAX77686_OPMODE_SHIFT;
+		val = MAX77686_LDO_NORMAL << MAX77686_OPMODE_SHIFT;
 		break;
 	default:
 		pr_warn("%s: regulator_suspend_mode : 0x%x not supported\n",
-- 
1.9.1

[PATCH v4 2/3] 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@samsung.com>
---
 drivers/regulator/max77686.c | 75 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 66 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index 3d0922051488..4b35c19c9286 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -87,18 +87,31 @@ struct max77686_data {
 	unsigned int opmode[MAX77686_REGULATORS];
 };
 
-/* Some BUCKS supports Normal[ON/OFF] mode during suspend */
-static int max77686_buck_set_suspend_disable(struct regulator_dev *rdev)
+/* Some BUCKs and LDOs supports Normal[ON/OFF] mode during suspend */
+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_BUCK_OFF_PWRREQ;
-	else
-		val = MAX77686_BUCK_OFF_PWRREQ
-			<< MAX77686_OPMODE_BUCK234_SHIFT;
+		break;
+	case MAX77686_BUCK2 ...	MAX77686_BUCK4:
+		val = MAX77686_BUCK_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_LDO_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);
@@ -176,13 +189,53 @@ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev,
 	return 0;
 }
 
+/*
+ * For regulators supporting disabled in suspend it checks if opmode
+ * is 'off_pwrreq' mode (disabled in suspend) and changes it to 'enable'.
+ * For other regulators does nothing.
+ */
+static void max77686_set_opmode_enable(struct max77686_data *max77686, int id)
+{
+	unsigned int opmode_shift;
+
+	/*
+	 * Same enable function is used for LDO and bucks. Assuming
+	 * the same values are used for enable registers.
+	 */
+	BUILD_BUG_ON(MAX77686_LDO_OFF_PWRREQ != MAX77686_BUCK_OFF_PWRREQ);
+	BUILD_BUG_ON(MAX77686_LDO_NORMAL != MAX77686_BUCK_NORMAL);
+
+	switch (id) {
+	case MAX77686_BUCK1:
+		opmode_shift = 0;
+		break;
+	case MAX77686_BUCK2 ...	MAX77686_BUCK4:
+		opmode_shift = MAX77686_OPMODE_BUCK234_SHIFT;
+		break;
+	case MAX77686_LDO2:
+	case MAX77686_LDO6 ... MAX77686_LDO8:
+	case MAX77686_LDO10 ... MAX77686_LDO12:
+	case MAX77686_LDO14 ... MAX77686_LDO16:
+		opmode_shift = MAX77686_OPMODE_SHIFT;
+		break;
+	default:
+		return;
+	}
+
+	if (max77686->opmode[id] == (MAX77686_LDO_OFF_PWRREQ << opmode_shift))
+		max77686->opmode[id] = MAX77686_LDO_NORMAL << opmode_shift;
+}
+
 static int max77686_enable(struct regulator_dev *rdev)
 {
 	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
+	int id = rdev_get_id(rdev);
+
+	max77686_set_opmode_enable(max77686, id);
 
 	return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
 				  rdev->desc->enable_mask,
-				  max77686->opmode[rdev_get_id(rdev)]);
+				  max77686->opmode[id]);
 }
 
 static int max77686_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
@@ -232,6 +285,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 = {
@@ -243,7 +298,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 = {
@@ -256,7 +312,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

Re: [PATCH v4 2/3] regulator: max77686: Implement suspend disable for some LDOs

[Javier Martinez Canillas]

Hello Krzysztof,

On 10/27/2014 10:44 AM, 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@samsung.com>
> ---
>  drivers/regulator/max77686.c | 75 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 66 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
> index 3d0922051488..4b35c19c9286 100644
> --- a/drivers/regulator/max77686.c
> +++ b/drivers/regulator/max77686.c
> @@ -87,18 +87,31 @@ struct max77686_data {
>  	unsigned int opmode[MAX77686_REGULATORS];
>  };
>  
> -/* Some BUCKS supports Normal[ON/OFF] mode during suspend */
> -static int max77686_buck_set_suspend_disable(struct regulator_dev *rdev)
> +/* Some BUCKs and LDOs supports Normal[ON/OFF] mode during suspend */
> +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_BUCK_OFF_PWRREQ;
> -	else
> -		val = MAX77686_BUCK_OFF_PWRREQ
> -			<< MAX77686_OPMODE_BUCK234_SHIFT;
> +		break;
> +	case MAX77686_BUCK2 ...	MAX77686_BUCK4:
> +		val = MAX77686_BUCK_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_LDO_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);
> @@ -176,13 +189,53 @@ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev,
>  	return 0;
>  }
>  
> +/*
> + * For regulators supporting disabled in suspend it checks if opmode
> + * is 'off_pwrreq' mode (disabled in suspend) and changes it to 'enable'.
> + * For other regulators does nothing.
> + */
> +static void max77686_set_opmode_enable(struct max77686_data *max77686, int id)
> +{
> +	unsigned int opmode_shift;
> +
> +	/*
> +	 * Same enable function is used for LDO and bucks. Assuming
> +	 * the same values are used for enable registers.
> +	 */
> +	BUILD_BUG_ON(MAX77686_LDO_OFF_PWRREQ != MAX77686_BUCK_OFF_PWRREQ);
> +	BUILD_BUG_ON(MAX77686_LDO_NORMAL != MAX77686_BUCK_NORMAL);
> +
> +	switch (id) {
> +	case MAX77686_BUCK1:
> +		opmode_shift = 0;
> +		break;
> +	case MAX77686_BUCK2 ...	MAX77686_BUCK4:
> +		opmode_shift = MAX77686_OPMODE_BUCK234_SHIFT;
> +		break;
> +	case MAX77686_LDO2:
> +	case MAX77686_LDO6 ... MAX77686_LDO8:
> +	case MAX77686_LDO10 ... MAX77686_LDO12:
> +	case MAX77686_LDO14 ... MAX77686_LDO16:
> +		opmode_shift = MAX77686_OPMODE_SHIFT;
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	if (max77686->opmode[id] == (MAX77686_LDO_OFF_PWRREQ << opmode_shift))
> +		max77686->opmode[id] = MAX77686_LDO_NORMAL << opmode_shift;
> +}
> +
>  static int max77686_enable(struct regulator_dev *rdev)
>  {
>  	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
> +	int id = rdev_get_id(rdev);
> +
> +	max77686_set_opmode_enable(max77686, id);
>  
>  	return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
>  				  rdev->desc->enable_mask,
> -				  max77686->opmode[rdev_get_id(rdev)]);
> +				  max77686->opmode[id]);
>  }

the max77802 driver handles this slightly differently. It stores in opmode[id]
just the value without the shift and there is a max77802_get_opmode_shift() to
obtain the shift that is used before updating the register, e.g:

int shift = max77802_get_opmode_shift(id);
return regmap_update_bits(..., max77802->opmode[id] << shift);

I think the max77802 driver approach is better since it avoids duplicating the
switch statement to get the shift for each regulator. Just look how the max77802
enable and disable functions are easier to read. Looking at the max77686 driver,
I see that there are assumptions that opmode[id] has the value shifted so your
changes are consistent with the rest of the driver but I would consider changing
this.

Best regards,
Javier

Re: [PATCH v4 2/3] regulator: max77686: Implement suspend disable for some LDOs

[Krzysztof Kozlowski]

On pon, 2014-10-27 at 11:08 +0100, Javier Martinez Canillas wrote:
> Hello Krzysztof,
> 
> On 10/27/2014 10:44 AM, 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@samsung.com>
> > ---
> >  drivers/regulator/max77686.c | 75 ++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 66 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
> > index 3d0922051488..4b35c19c9286 100644
> > --- a/drivers/regulator/max77686.c
> > +++ b/drivers/regulator/max77686.c
> > @@ -87,18 +87,31 @@ struct max77686_data {
> >  	unsigned int opmode[MAX77686_REGULATORS];
> >  };
> >  
> > -/* Some BUCKS supports Normal[ON/OFF] mode during suspend */
> > -static int max77686_buck_set_suspend_disable(struct regulator_dev *rdev)
> > +/* Some BUCKs and LDOs supports Normal[ON/OFF] mode during suspend */
> > +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_BUCK_OFF_PWRREQ;
> > -	else
> > -		val = MAX77686_BUCK_OFF_PWRREQ
> > -			<< MAX77686_OPMODE_BUCK234_SHIFT;
> > +		break;
> > +	case MAX77686_BUCK2 ...	MAX77686_BUCK4:
> > +		val = MAX77686_BUCK_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_LDO_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);
> > @@ -176,13 +189,53 @@ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev,
> >  	return 0;
> >  }
> >  
> > +/*
> > + * For regulators supporting disabled in suspend it checks if opmode
> > + * is 'off_pwrreq' mode (disabled in suspend) and changes it to 'enable'.
> > + * For other regulators does nothing.
> > + */
> > +static void max77686_set_opmode_enable(struct max77686_data *max77686, int id)
> > +{
> > +	unsigned int opmode_shift;
> > +
> > +	/*
> > +	 * Same enable function is used for LDO and bucks. Assuming
> > +	 * the same values are used for enable registers.
> > +	 */
> > +	BUILD_BUG_ON(MAX77686_LDO_OFF_PWRREQ != MAX77686_BUCK_OFF_PWRREQ);
> > +	BUILD_BUG_ON(MAX77686_LDO_NORMAL != MAX77686_BUCK_NORMAL);
> > +
> > +	switch (id) {
> > +	case MAX77686_BUCK1:
> > +		opmode_shift = 0;
> > +		break;
> > +	case MAX77686_BUCK2 ...	MAX77686_BUCK4:
> > +		opmode_shift = MAX77686_OPMODE_BUCK234_SHIFT;
> > +		break;
> > +	case MAX77686_LDO2:
> > +	case MAX77686_LDO6 ... MAX77686_LDO8:
> > +	case MAX77686_LDO10 ... MAX77686_LDO12:
> > +	case MAX77686_LDO14 ... MAX77686_LDO16:
> > +		opmode_shift = MAX77686_OPMODE_SHIFT;
> > +		break;
> > +	default:
> > +		return;
> > +	}
> > +
> > +	if (max77686->opmode[id] == (MAX77686_LDO_OFF_PWRREQ << opmode_shift))
> > +		max77686->opmode[id] = MAX77686_LDO_NORMAL << opmode_shift;
> > +}
> > +
> >  static int max77686_enable(struct regulator_dev *rdev)
> >  {
> >  	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
> > +	int id = rdev_get_id(rdev);
> > +
> > +	max77686_set_opmode_enable(max77686, id);
> >  
> >  	return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
> >  				  rdev->desc->enable_mask,
> > -				  max77686->opmode[rdev_get_id(rdev)]);
> > +				  max77686->opmode[id]);
> >  }
> 
> the max77802 driver handles this slightly differently. It stores in opmode[id]
> just the value without the shift and there is a max77802_get_opmode_shift() to
> obtain the shift that is used before updating the register, e.g:
> 
> int shift = max77802_get_opmode_shift(id);
> return regmap_update_bits(..., max77802->opmode[id] << shift);
> 
> I think the max77802 driver approach is better since it avoids duplicating the
> switch statement to get the shift for each regulator. Just look how the max77802
> enable and disable functions are easier to read. Looking at the max77686 driver,
> I see that there are assumptions that opmode[id] has the value shifted so your
> changes are consistent with the rest of the driver but I would consider changing
> this.

Storing opmode in non-shifted form is intuitive also to me. That's way I
slipped the bug in previous version.

I'll change this to non-shifted opmode. The patch will grow bigger but
the code should be more readable.

Thanks!
Krzysztof


> Best regards,
> Javier
> 

Re: [PATCH v4 2/3] regulator: max77686: Implement suspend disable for some LDOs

[Javier Martinez Canillas]

On 10/27/2014 11:27 AM, Krzysztof Kozlowski wrote:
> 
> Storing opmode in non-shifted form is intuitive also to me. That's way I
> slipped the bug in previous version.
> 

Great.

> I'll change this to non-shifted opmode. The patch will grow bigger but
> the code should be more readable.
> 

Maybe split in two patches? A preparatory that adds max77686_get_opmode_shift()
and changes the current driver and a second one that implements suspend disable
for LDOs?

> Thanks!

You are welcome.

Best regards,
Javier

Re: [PATCH v4 2/3] regulator: max77686: Implement suspend disable for some LDOs

[Krzysztof Kozlowski]

On pon, 2014-10-27 at 11:32 +0100, Javier Martinez Canillas wrote:
> On 10/27/2014 11:27 AM, Krzysztof Kozlowski wrote:
> > 
> > Storing opmode in non-shifted form is intuitive also to me. That's way I
> > slipped the bug in previous version.
> > 
> 
> Great.
> 
> > I'll change this to non-shifted opmode. The patch will grow bigger but
> > the code should be more readable.
> > 
> 
> Maybe split in two patches? A preparatory that adds max77686_get_opmode_shift()
> and changes the current driver and a second one that implements suspend disable
> for LDOs?

Yes, that was the plan :).

> 
> > Thanks!
> 
> You are welcome.
> 
> Best regards,
> Javier

[PATCH v4 3/3] 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>
Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reviewed-by: Chanwoo Choi <cw00.choi@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