* [PATCH v2 0/4] regulator: max77686/trats2: Disable some regulators in suspend
@ 2014-10-21 11:23 Krzysztof Kozlowski
2014-10-21 11:23 ` [PATCH v2 1/4] regulator: max77686: Replace hard-coded opmode values with defines Krzysztof Kozlowski
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Krzysztof Kozlowski @ 2014-10-21 11:23 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, linux-kernel, Ben Dooks, Kukjin Kim,
Russell King, linux-arm-kernel, linux-samsung-soc, devicetree
Cc: Kyungmin Park, Marek Szyprowski, Bartlomiej Zolnierkiewicz,
Javier Martinez Canillas, Chanwoo Choi, 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/4] regulator: max77686: Replace hard-coded opmode values with defines
2014-10-21 11:23 [PATCH v2 0/4] regulator: max77686/trats2: Disable some regulators in suspend Krzysztof Kozlowski
@ 2014-10-21 11:23 ` Krzysztof Kozlowski
2014-10-21 11:57 ` Javier Martinez Canillas
[not found] ` <1413890597-31037-1-git-send-email-k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Krzysztof Kozlowski @ 2014-10-21 11:23 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, linux-kernel, Ben Dooks, Kukjin Kim,
Russell King, linux-arm-kernel, linux-samsung-soc, devicetree
Cc: Kyungmin Park, Marek Szyprowski, Bartlomiej Zolnierkiewicz,
Javier Martinez Canillas, Chanwoo Choi, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/4] regulator: max77686: Implement suspend disable for some LDOs
[not found] ` <1413890597-31037-1-git-send-email-k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
@ 2014-10-21 11:23 ` Krzysztof Kozlowski
[not found] ` <1413890597-31037-3-git-send-email-k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
0 siblings, 1 reply; 13+ messages in thread
From: Krzysztof Kozlowski @ 2014-10-21 11:23 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
Ben Dooks, Kukjin Kim, Russell King,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: Kyungmin Park, Marek Szyprowski, Bartlomiej Zolnierkiewicz,
Javier Martinez Canillas, Chanwoo Choi, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 3/4] mfd/regulator: dt-bindings: max77686: Document regulators off in suspend
2014-10-21 11:23 [PATCH v2 0/4] regulator: max77686/trats2: Disable some regulators in suspend Krzysztof Kozlowski
2014-10-21 11:23 ` [PATCH v2 1/4] regulator: max77686: Replace hard-coded opmode values with defines Krzysztof Kozlowski
[not found] ` <1413890597-31037-1-git-send-email-k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
@ 2014-10-21 11:23 ` Krzysztof Kozlowski
2014-10-21 12:01 ` Javier Martinez Canillas
2014-10-21 12:06 ` Lee Jones
2014-10-21 11:23 ` [PATCH v2 4/4] ARM: dts: exynos4412-trats: Add suspend configuration for max77686 regulators Krzysztof Kozlowski
3 siblings, 2 replies; 13+ messages in thread
From: Krzysztof Kozlowski @ 2014-10-21 11:23 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, linux-kernel, Ben Dooks, Kukjin Kim,
Russell King, linux-arm-kernel, linux-samsung-soc, devicetree
Cc: Kyungmin Park, Marek Szyprowski, Bartlomiej Zolnierkiewicz,
Javier Martinez Canillas, Chanwoo Choi, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 4/4] ARM: dts: exynos4412-trats: Add suspend configuration for max77686 regulators
2014-10-21 11:23 [PATCH v2 0/4] regulator: max77686/trats2: Disable some regulators in suspend Krzysztof Kozlowski
` (2 preceding siblings ...)
2014-10-21 11:23 ` [PATCH v2 3/4] mfd/regulator: dt-bindings: max77686: Document regulators off in suspend Krzysztof Kozlowski
@ 2014-10-21 11:23 ` Krzysztof Kozlowski
2014-10-21 12:04 ` Javier Martinez Canillas
3 siblings, 1 reply; 13+ messages in thread
From: Krzysztof Kozlowski @ 2014-10-21 11:23 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, linux-kernel, Ben Dooks, Kukjin Kim,
Russell King, linux-arm-kernel, linux-samsung-soc, devicetree
Cc: Kyungmin Park, Marek Szyprowski, Bartlomiej Zolnierkiewicz,
Javier Martinez Canillas, Chanwoo Choi, 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/4] regulator: max77686: Replace hard-coded opmode values with defines
2014-10-21 11:23 ` [PATCH v2 1/4] regulator: max77686: Replace hard-coded opmode values with defines Krzysztof Kozlowski
@ 2014-10-21 11:57 ` Javier Martinez Canillas
2014-10-21 12:22 ` Krzysztof Kozlowski
0 siblings, 1 reply; 13+ messages in thread
From: Javier Martinez Canillas @ 2014-10-21 11:57 UTC (permalink / raw)
To: Krzysztof Kozlowski, Liam Girdwood, Mark Brown, linux-kernel,
Ben Dooks, Kukjin Kim, Russell King, linux-arm-kernel,
linux-samsung-soc, devicetree
Cc: Kyungmin Park, Marek Szyprowski, Bartlomiej Zolnierkiewicz,
Chanwoo Choi
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/4] regulator: max77686: Implement suspend disable for some LDOs
[not found] ` <1413890597-31037-3-git-send-email-k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
@ 2014-10-21 12:00 ` Javier Martinez Canillas
0 siblings, 0 replies; 13+ messages in thread
From: Javier Martinez Canillas @ 2014-10-21 12:00 UTC (permalink / raw)
To: Krzysztof Kozlowski, Liam Girdwood, Mark Brown,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Ben Dooks, Kukjin Kim,
Russell King, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: Kyungmin Park, Marek Szyprowski, Bartlomiej Zolnierkiewicz,
Chanwoo Choi
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/4] mfd/regulator: dt-bindings: max77686: Document regulators off in suspend
2014-10-21 11:23 ` [PATCH v2 3/4] mfd/regulator: dt-bindings: max77686: Document regulators off in suspend Krzysztof Kozlowski
@ 2014-10-21 12:01 ` Javier Martinez Canillas
2014-10-21 12:06 ` Lee Jones
1 sibling, 0 replies; 13+ messages in thread
From: Javier Martinez Canillas @ 2014-10-21 12:01 UTC (permalink / raw)
To: Krzysztof Kozlowski, Liam Girdwood, Mark Brown, linux-kernel,
Ben Dooks, Kukjin Kim, Russell King, linux-arm-kernel,
linux-samsung-soc, devicetree
Cc: Kyungmin Park, Marek Szyprowski, Bartlomiej Zolnierkiewicz,
Chanwoo Choi
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 4/4] ARM: dts: exynos4412-trats: Add suspend configuration for max77686 regulators
2014-10-21 11:23 ` [PATCH v2 4/4] ARM: dts: exynos4412-trats: Add suspend configuration for max77686 regulators Krzysztof Kozlowski
@ 2014-10-21 12:04 ` Javier Martinez Canillas
2014-10-21 12:24 ` Krzysztof Kozlowski
0 siblings, 1 reply; 13+ messages in thread
From: Javier Martinez Canillas @ 2014-10-21 12:04 UTC (permalink / raw)
To: Krzysztof Kozlowski, Liam Girdwood, Mark Brown, linux-kernel,
Ben Dooks, Kukjin Kim, Russell King, linux-arm-kernel,
linux-samsung-soc, devicetree
Cc: Kyungmin Park, Marek Szyprowski, Bartlomiej Zolnierkiewicz,
Chanwoo Choi
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/4] mfd/regulator: dt-bindings: max77686: Document regulators off in suspend
2014-10-21 11:23 ` [PATCH v2 3/4] mfd/regulator: dt-bindings: max77686: Document regulators off in suspend Krzysztof Kozlowski
2014-10-21 12:01 ` Javier Martinez Canillas
@ 2014-10-21 12:06 ` Lee Jones
1 sibling, 0 replies; 13+ messages in thread
From: Lee Jones @ 2014-10-21 12:06 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Liam Girdwood, Mark Brown, linux-kernel, Ben Dooks, Kukjin Kim,
Russell King, linux-arm-kernel, linux-samsung-soc, devicetree,
Bartlomiej Zolnierkiewicz, Chanwoo Choi, Kyungmin Park,
Javier Martinez Canillas, Marek Szyprowski
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/4] regulator: max77686: Replace hard-coded opmode values with defines
2014-10-21 11:57 ` Javier Martinez Canillas
@ 2014-10-21 12:22 ` Krzysztof Kozlowski
2014-10-21 12:29 ` Javier Martinez Canillas
0 siblings, 1 reply; 13+ messages in thread
From: Krzysztof Kozlowski @ 2014-10-21 12:22 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Liam Girdwood, Mark Brown, linux-kernel, Ben Dooks, Kukjin Kim,
Russell King, linux-arm-kernel, linux-samsung-soc, devicetree,
Kyungmin Park, Marek Szyprowski, Bartlomiej Zolnierkiewicz,
Chanwoo Choi
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 4/4] ARM: dts: exynos4412-trats: Add suspend configuration for max77686 regulators
2014-10-21 12:04 ` Javier Martinez Canillas
@ 2014-10-21 12:24 ` Krzysztof Kozlowski
0 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Kozlowski @ 2014-10-21 12:24 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Liam Girdwood, Mark Brown, linux-kernel, Ben Dooks, Kukjin Kim,
Russell King, linux-arm-kernel, linux-samsung-soc, devicetree,
Kyungmin Park, Marek Szyprowski, Bartlomiej Zolnierkiewicz,
Chanwoo Choi
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/4] regulator: max77686: Replace hard-coded opmode values with defines
2014-10-21 12:22 ` Krzysztof Kozlowski
@ 2014-10-21 12:29 ` Javier Martinez Canillas
0 siblings, 0 replies; 13+ messages in thread
From: Javier Martinez Canillas @ 2014-10-21 12:29 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Liam Girdwood, Mark Brown, linux-kernel, Ben Dooks, Kukjin Kim,
Russell King, linux-arm-kernel, linux-samsung-soc, devicetree,
Kyungmin Park, Marek Szyprowski, Bartlomiej Zolnierkiewicz,
Chanwoo Choi
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-10-21 12:29 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-21 11:23 [PATCH v2 0/4] regulator: max77686/trats2: Disable some regulators in suspend Krzysztof Kozlowski
2014-10-21 11:23 ` [PATCH v2 1/4] regulator: max77686: Replace hard-coded opmode values with defines Krzysztof Kozlowski
2014-10-21 11:57 ` Javier Martinez Canillas
2014-10-21 12:22 ` Krzysztof Kozlowski
2014-10-21 12:29 ` Javier Martinez Canillas
[not found] ` <1413890597-31037-1-git-send-email-k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2014-10-21 11:23 ` [PATCH v2 2/4] regulator: max77686: Implement suspend disable for some LDOs Krzysztof Kozlowski
[not found] ` <1413890597-31037-3-git-send-email-k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
2014-10-21 12:00 ` Javier Martinez Canillas
2014-10-21 11:23 ` [PATCH v2 3/4] mfd/regulator: dt-bindings: max77686: Document regulators off in suspend Krzysztof Kozlowski
2014-10-21 12:01 ` Javier Martinez Canillas
2014-10-21 12:06 ` Lee Jones
2014-10-21 11:23 ` [PATCH v2 4/4] ARM: dts: exynos4412-trats: Add suspend configuration for max77686 regulators Krzysztof Kozlowski
2014-10-21 12:04 ` Javier Martinez Canillas
2014-10-21 12:24 ` Krzysztof Kozlowski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).