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

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

[Krzysztof Kozlowski]

Hi,


Changes since v4
================
1. New patch: 2/4. Store the current value of opmode in
   non-shifted form. Along with changes in defines used for opmodes
   this simplifies everything a lot. Suggested by Javier.
2. Drop Javier's review-by from patch 1 because I changed the
   defines to cover both LDO and buck at the same time.
3. Patch 3/4: adding suspend disable is now simpler - just re-use
   the existing function.

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 (4):
  regulator: max77686: Replace hard-coded opmode values with defines
  regulator: max77686: Store opmode non-shifted
  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/gpu/arm/mali400/ump/arch        |  1 +
 drivers/regulator/max77686.c            | 81 +++++++++++++++++++++++++--------
 3 files changed, 105 insertions(+), 49 deletions(-)
 create mode 120000 drivers/gpu/arm/mali400/ump/arch

-- 
1.9.1

[PATCH v5 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 | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index ef1af2debbd2..c625a8a7940d 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -45,6 +45,23 @@
 #define MAX77686_DVS_MINUV	600000
 #define MAX77686_DVS_UVSTEP	12500
 
+/*
+ * Values used for configuring LDOs and bucks.
+ * Forcing low power mode: LDO1, 3-5, 9, 13, 17-26
+ */
+#define MAX77686_LDO_LOWPOWER		0x1
+/*
+ * On/off controlled by PWRREQ:
+ *  - LDO2, 6-8, 10-12, 14-16
+ *  - buck[1234]
+ */
+#define MAX77686_OFF_PWRREQ		0x1
+/* Low power mode controlled by PWRREQ: All LDOs */
+#define MAX77686_LDO_LOWPOWER_PWRREQ	0x2
+/* Forcing low power mode: buck[234] */
+#define MAX77686_BUCK_LOWPOWER		0x2
+#define MAX77686_NORMAL			0x3
+
 #define MAX77686_OPMODE_SHIFT	6
 #define MAX77686_OPMODE_BUCK234_SHIFT	4
 #define MAX77686_OPMODE_MASK	0x3
@@ -76,9 +93,9 @@ 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_OFF_PWRREQ;
 	else
-		val = 0x1 << MAX77686_OPMODE_BUCK234_SHIFT;
+		val = MAX77686_OFF_PWRREQ << MAX77686_OPMODE_BUCK234_SHIFT;
 
 	ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
 				 rdev->desc->enable_mask, val);
@@ -103,10 +120,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_NORMAL << MAX77686_OPMODE_SHIFT;
 		break;
 	default:
 		pr_warn("%s: regulator_suspend_mode : 0x%x not supported\n",
@@ -133,13 +150,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_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_NORMAL << MAX77686_OPMODE_SHIFT;
 		break;
 	default:
 		pr_warn("%s: regulator_suspend_mode : 0x%x not supported\n",
-- 
1.9.1

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

[Mark Brown]

On Mon, Oct 27, 2014 at 01:11:47PM +0100, Krzysztof Kozlowski wrote:
> Add defines for regulator operating modes which should be more readable,
> especially if one does not have Maxim 77686 datasheet.

Applied, thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141028/3e1f3e5f/attachment.sig>

[PATCH v5 2/4] regulator: max77686: Store opmode non-shifted

[Krzysztof Kozlowski]

Introduce simple helper for calculating the shift for OPMODE field in
registers. This allows storing the current value of opmode in
non-shifted form and simplifies a little set_suspend_disable and enable
functions. Additionally this will allow adding support LDOs to the
existing set_suspend_disable function.

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

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index c625a8a7940d..2ebc4257425b 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -85,20 +85,32 @@ struct max77686_data {
 	unsigned int opmode[MAX77686_REGULATORS];
 };
 
+static unsigned int max77686_get_opmode_shift(int id)
+{
+	switch (id) {
+	case MAX77686_BUCK1:
+	case MAX77686_BUCK5 ... MAX77686_BUCK9:
+		return 0;
+	case MAX77686_BUCK2 ... MAX77686_BUCK4:
+		return MAX77686_OPMODE_BUCK234_SHIFT;
+	default:
+		/* all LDOs */
+		return MAX77686_OPMODE_SHIFT;
+	}
+}
+
 /* Some BUCKS supports Normal[ON/OFF] mode during suspend */
 static int max77686_buck_set_suspend_disable(struct regulator_dev *rdev)
 {
-	unsigned int val;
+	unsigned int val, shift;
 	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
 	int ret, id = rdev_get_id(rdev);
 
-	if (id == MAX77686_BUCK1)
-		val = MAX77686_OFF_PWRREQ;
-	else
-		val = MAX77686_OFF_PWRREQ << MAX77686_OPMODE_BUCK234_SHIFT;
+	shift = max77686_get_opmode_shift(id);
+	val = MAX77686_OFF_PWRREQ;
 
 	ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
-				 rdev->desc->enable_mask, val);
+				 rdev->desc->enable_mask, val << shift);
 	if (ret)
 		return ret;
 
@@ -120,10 +132,10 @@ static int max77686_set_suspend_mode(struct regulator_dev *rdev,
 
 	switch (mode) {
 	case REGULATOR_MODE_IDLE:			/* ON in LP Mode */
-		val = MAX77686_LDO_LOWPOWER_PWRREQ << MAX77686_OPMODE_SHIFT;
+		val = MAX77686_LDO_LOWPOWER_PWRREQ;
 		break;
 	case REGULATOR_MODE_NORMAL:			/* ON in Normal Mode */
-		val = MAX77686_NORMAL << MAX77686_OPMODE_SHIFT;
+		val = MAX77686_NORMAL;
 		break;
 	default:
 		pr_warn("%s: regulator_suspend_mode : 0x%x not supported\n",
@@ -132,7 +144,8 @@ static int max77686_set_suspend_mode(struct regulator_dev *rdev,
 	}
 
 	ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
-				  rdev->desc->enable_mask, val);
+				  rdev->desc->enable_mask,
+				  val << MAX77686_OPMODE_SHIFT);
 	if (ret)
 		return ret;
 
@@ -150,13 +163,13 @@ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev,
 
 	switch (mode) {
 	case REGULATOR_MODE_STANDBY:			/* switch off */
-		val = MAX77686_OFF_PWRREQ << MAX77686_OPMODE_SHIFT;
+		val = MAX77686_OFF_PWRREQ;
 		break;
 	case REGULATOR_MODE_IDLE:			/* ON in LP Mode */
-		val = MAX77686_LDO_LOWPOWER_PWRREQ << MAX77686_OPMODE_SHIFT;
+		val = MAX77686_LDO_LOWPOWER_PWRREQ;
 		break;
 	case REGULATOR_MODE_NORMAL:			/* ON in Normal Mode */
-		val = MAX77686_NORMAL << MAX77686_OPMODE_SHIFT;
+		val = MAX77686_NORMAL;
 		break;
 	default:
 		pr_warn("%s: regulator_suspend_mode : 0x%x not supported\n",
@@ -165,7 +178,8 @@ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev,
 	}
 
 	ret = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
-				 rdev->desc->enable_mask, val);
+				 rdev->desc->enable_mask,
+				 val << MAX77686_OPMODE_SHIFT);
 	if (ret)
 		return ret;
 
@@ -176,10 +190,14 @@ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev,
 static int max77686_enable(struct regulator_dev *rdev)
 {
 	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
+	unsigned int shift;
+	int id = rdev_get_id(rdev);
+
+	shift = max77686_get_opmode_shift(id);
 
 	return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
 				  rdev->desc->enable_mask,
-				  max77686->opmode[rdev_get_id(rdev)]);
+				  max77686->opmode[id] << shift);
 }
 
 static int max77686_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
@@ -495,7 +513,8 @@ 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;
+		max77686->opmode[i] = regulators[i].enable_mask >>
+						max77686_get_opmode_shift(i);
 		rdev = devm_regulator_register(&pdev->dev,
 						&regulators[i], &config);
 		if (IS_ERR(rdev)) {
-- 
1.9.1

[PATCH v5 2/4] regulator: max77686: Store opmode non-shifted

[Javier Martinez Canillas]

Hello Krzysztof,

On 10/27/2014 01:11 PM, Krzysztof Kozlowski wrote:
> Introduce simple helper for calculating the shift for OPMODE field in
> registers. This allows storing the current value of opmode in
> non-shifted form and simplifies a little set_suspend_disable and enable
> functions. Additionally this will allow adding support LDOs to the
> existing set_suspend_disable function.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Suggested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  drivers/regulator/max77686.c | 49 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c

The patch looks good to me.

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

>  
>  static int max77686_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
> @@ -495,7 +513,8 @@ 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;
> +		max77686->opmode[i] = regulators[i].enable_mask >>
> +						max77686_get_opmode_shift(i);

I don't think it has to be done in your patch but as a follow-up it would be
good to change this to:

		max77686->opmode[i] = MAX77686_NORMAL;

since that is really what this code is trying to do AFAIU. This just happens
to work because the value of MAX77686_OPMODE_MASK (0x3) is also "Output ON in
Normal Mode" but the code is not correct IMHO.

Or even better, the regulator mode should be read from the hw registers instead
of setting always to normal. That is what the max77802 driver does for example.

Best regards,
Javier

[PATCH v5 2/4] regulator: max77686: Store opmode non-shifted

[Krzysztof Kozlowski]

On pon, 2014-10-27 at 14:37 +0100, Javier Martinez Canillas wrote:
> Hello Krzysztof,
> 
> On 10/27/2014 01:11 PM, Krzysztof Kozlowski wrote:
> > Introduce simple helper for calculating the shift for OPMODE field in
> > registers. This allows storing the current value of opmode in
> > non-shifted form and simplifies a little set_suspend_disable and enable
> > functions. Additionally this will allow adding support LDOs to the
> > existing set_suspend_disable function.
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > Suggested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> > ---
> >  drivers/regulator/max77686.c | 49 ++++++++++++++++++++++++++++++--------------
> >  1 file changed, 34 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
> 
> The patch looks good to me.
> 
> Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> 
> >  
> >  static int max77686_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
> > @@ -495,7 +513,8 @@ 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;
> > +		max77686->opmode[i] = regulators[i].enable_mask >>
> > +						max77686_get_opmode_shift(i);
> 
> I don't think it has to be done in your patch but as a follow-up it would be
> good to change this to:
> 
> 		max77686->opmode[i] = MAX77686_NORMAL;
> 
> since that is really what this code is trying to do AFAIU. This just happens
> to work because the value of MAX77686_OPMODE_MASK (0x3) is also "Output ON in
> Normal Mode" but the code is not correct IMHO.

It is technically correct (and regulator core use it this way if
enable_val is not specified) but I agree that MAX77686_NORMAL would be
more readable. I have some next patches almost ready, so I'll change it
there.

> Or even better, the regulator mode should be read from the hw registers instead
> of setting always to normal. That is what the max77802 driver does for example.

This would put trust on the bootloader... I don't know if I could trust
him :)

[PATCH v5 2/4] regulator: max77686: Store opmode non-shifted

[Mark Brown]

On Mon, Oct 27, 2014 at 01:11:48PM +0100, Krzysztof Kozlowski wrote:
> Introduce simple helper for calculating the shift for OPMODE field in
> registers. This allows storing the current value of opmode in
> non-shifted form and simplifies a little set_suspend_disable and enable
> functions. Additionally this will allow adding support LDOs to the
> existing set_suspend_disable function.

Applied, thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141028/7ae748ae/attachment.sig>

[PATCH v5 3/4] regulator: max77686: Add 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. Re-use 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 | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
index 2ebc4257425b..c6bf6f79bd2a 100644
--- a/drivers/regulator/max77686.c
+++ b/drivers/regulator/max77686.c
@@ -99,8 +99,8 @@ static unsigned int max77686_get_opmode_shift(int id)
 	}
 }
 
-/* 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, shift;
 	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
@@ -195,6 +195,9 @@ static int max77686_enable(struct regulator_dev *rdev)
 
 	shift = max77686_get_opmode_shift(id);
 
+	if (max77686->opmode[id] == MAX77686_OFF_PWRREQ)
+		max77686->opmode[id] = MAX77686_NORMAL;
+
 	return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
 				  rdev->desc->enable_mask,
 				  max77686->opmode[id] << shift);
@@ -247,6 +250,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 = {
@@ -258,7 +263,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 = {
@@ -271,7 +277,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

[PATCH v5 3/4] regulator: max77686: Add suspend disable for some LDOs

[Javier Martinez Canillas]

Hello Krzysztof,

On 10/27/2014 01:11 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. Re-use 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 | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c
> index 2ebc4257425b..c6bf6f79bd2a 100644
> --- a/drivers/regulator/max77686.c
> +++ b/drivers/regulator/max77686.c

Looks good to me.

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

Best regards,
Javier

[PATCH v5 3/4] regulator: max77686: Add suspend disable for some LDOs

[Mark Brown]

On Mon, Oct 27, 2014 at 01:11:49PM +0100, Krzysztof Kozlowski wrote:

> @@ -247,6 +250,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,

This looks wrong, you're using the regular enable operation as suspend
enable.  How does that work without disrupting the current runtime
state?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141028/0fc88ae1/attachment.sig>

[PATCH v5 3/4] regulator: max77686: Add suspend disable for some LDOs

[Krzysztof Kozlowski]

On wto, 2014-10-28 at 22:31 +0000, Mark Brown wrote:
> On Mon, Oct 27, 2014 at 01:11:49PM +0100, Krzysztof Kozlowski wrote:
> 
> > @@ -247,6 +250,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,
> 
> This looks wrong, you're using the regular enable operation as suspend
> enable.  How does that work without disrupting the current runtime
> state?

Currently it shouldn't disrupt state of regulator because during runtime
it may only be only: on (0x3) or off (0x0). Suspend enable in max77686
writes 0x3 to the register which means - always on.

If regulator was disabled before suspend then it has to be enabled
during suspend_enable() call which is exactly what max77686_enable does.
If it was enabled then nothing happens.

Best regards,
Krzysztof

[PATCH v5 3/4] regulator: max77686: Add suspend disable for some LDOs

[Mark Brown]

On Wed, Oct 29, 2014 at 10:20:13AM +0100, Krzysztof Kozlowski wrote:
> On wto, 2014-10-28 at 22:31 +0000, Mark Brown wrote:

> > This looks wrong, you're using the regular enable operation as suspend
> > enable.  How does that work without disrupting the current runtime
> > state?

> Currently it shouldn't disrupt state of regulator because during runtime
> it may only be only: on (0x3) or off (0x0). Suspend enable in max77686
> writes 0x3 to the register which means - always on.

> If regulator was disabled before suspend then it has to be enabled
> during suspend_enable() call which is exactly what max77686_enable does.
> If it was enabled then nothing happens.

No, this isn't suspend enable control - this is normal, standard enable
control and the device has no suspend enable control.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141029/7504a43a/attachment.sig>

[PATCH v5 3/4] regulator: max77686: Add suspend disable for some LDOs

[Krzysztof Kozlowski]

On ?ro, 2014-10-29 at 10:01 +0000, Mark Brown wrote:
> On Wed, Oct 29, 2014 at 10:20:13AM +0100, Krzysztof Kozlowski wrote:
> > On wto, 2014-10-28 at 22:31 +0000, Mark Brown wrote:
> 
> > > This looks wrong, you're using the regular enable operation as suspend
> > > enable.  How does that work without disrupting the current runtime
> > > state?
> 
> > Currently it shouldn't disrupt state of regulator because during runtime
> > it may only be only: on (0x3) or off (0x0). Suspend enable in max77686
> > writes 0x3 to the register which means - always on.
> 
> > If regulator was disabled before suspend then it has to be enabled
> > during suspend_enable() call which is exactly what max77686_enable does.
> > If it was enabled then nothing happens.
> 
> No, this isn't suspend enable control - this is normal, standard enable
> control and the device has no suspend enable control.

You mean that for such regulator the driver shouldn't implement
suspend_enable()?

Best regards,
Krzysztof

[PATCH v5 3/4] regulator: max77686: Add suspend disable for some LDOs

[Mark Brown]

On Wed, Oct 29, 2014 at 11:18:54AM +0100, Krzysztof Kozlowski wrote:
> On ?ro, 2014-10-29 at 10:01 +0000, Mark Brown wrote:

> > No, this isn't suspend enable control - this is normal, standard enable
> > control and the device has no suspend enable control.

> You mean that for such regulator the driver shouldn't implement
> suspend_enable()?

Yes, if there is no separate control of suspend mode in hardware then of
course the driver shouldn't implement operations for things it doesn't
have.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141029/4adfbd3e/attachment.sig>

[PATCH v5 3/4] regulator: max77686: Add suspend disable for some LDOs

[Krzysztof Kozlowski]

On ?ro, 2014-10-29 at 10:31 +0000, Mark Brown wrote:
> On Wed, Oct 29, 2014 at 11:18:54AM +0100, Krzysztof Kozlowski wrote:
> > On ?ro, 2014-10-29 at 10:01 +0000, Mark Brown wrote:
> 
> > > No, this isn't suspend enable control - this is normal, standard enable
> > > control and the device has no suspend enable control.
> 
> > You mean that for such regulator the driver shouldn't implement
> > suspend_enable()?
> 
> Yes, if there is no separate control of suspend mode in hardware then of
> course the driver shouldn't implement operations for things it doesn't
> have.

Oh, thanks! I'll send fixed patch.

This means that probably the max77802 ("mirrored" driver) should be
fixed...

Best regards,
Krzysztof

[PATCH v5 3/4] regulator: max77686: Add suspend disable for some LDOs

[Javier Martinez Canillas]

Hello Krzysztof,

On 10/29/2014 11:44 AM, Krzysztof Kozlowski wrote:
> On ?ro, 2014-10-29 at 10:31 +0000, Mark Brown wrote:
>> On Wed, Oct 29, 2014 at 11:18:54AM +0100, Krzysztof Kozlowski wrote:
>> > On ?ro, 2014-10-29 at 10:01 +0000, Mark Brown wrote:
>> 
>> > > No, this isn't suspend enable control - this is normal, standard enable
>> > > control and the device has no suspend enable control.
>> 
>> > You mean that for such regulator the driver shouldn't implement
>> > suspend_enable()?
>> 
>> Yes, if there is no separate control of suspend mode in hardware then of
>> course the driver shouldn't implement operations for things it doesn't
>> have.
> 
> Oh, thanks! I'll send fixed patch.
> 
> This means that probably the max77802 ("mirrored" driver) should be
> fixed...
> 

Indeed, I had the same confusion that you had. Just to avoid duplicating work,
do you want me to send a fix or are you going to include one on your series?

> Best regards,
> Krzysztof
> 
> 

Best regards,
Javier

[PATCH v5 3/4] regulator: max77686: Add suspend disable for some LDOs

[Krzysztof Kozlowski]

On ?ro, 2014-10-29 at 11:51 +0100, Javier Martinez Canillas wrote:
> Hello Krzysztof,
> 
> On 10/29/2014 11:44 AM, Krzysztof Kozlowski wrote:
> > On ?ro, 2014-10-29 at 10:31 +0000, Mark Brown wrote:
> >> On Wed, Oct 29, 2014 at 11:18:54AM +0100, Krzysztof Kozlowski wrote:
> >> > On ?ro, 2014-10-29 at 10:01 +0000, Mark Brown wrote:
> >> 
> >> > > No, this isn't suspend enable control - this is normal, standard enable
> >> > > control and the device has no suspend enable control.
> >> 
> >> > You mean that for such regulator the driver shouldn't implement
> >> > suspend_enable()?
> >> 
> >> Yes, if there is no separate control of suspend mode in hardware then of
> >> course the driver shouldn't implement operations for things it doesn't
> >> have.
> > 
> > Oh, thanks! I'll send fixed patch.
> > 
> > This means that probably the max77802 ("mirrored" driver) should be
> > fixed...
> > 
> 
> Indeed, I had the same confusion that you had. Just to avoid duplicating work,
> do you want me to send a fix or are you going to include one on your series?

I'll send a patch for max77802 also.

Best regards,
Krzysztof

[PATCH v5 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>
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