* [PATCH v4 0/3] regulator: max77686/trats2: Disable some regulators in suspend
@ 2014-10-27 9:44 Krzysztof Kozlowski
2014-10-27 9:44 ` [PATCH v4 1/3] regulator: max77686: Replace hard-coded opmode values with defines Krzysztof Kozlowski
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Krzysztof Kozlowski @ 2014-10-27 9:44 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 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4 1/3] regulator: max77686: Replace hard-coded opmode values with defines
2014-10-27 9:44 [PATCH v4 0/3] regulator: max77686/trats2: Disable some regulators in suspend Krzysztof Kozlowski
@ 2014-10-27 9:44 ` Krzysztof Kozlowski
2014-10-27 9:44 ` [PATCH v4 2/3] regulator: max77686: Implement suspend disable for some LDOs Krzysztof Kozlowski
2014-10-27 9:44 ` [PATCH v4 3/3] ARM: dts: exynos4412-trats: Add suspend configuration for max77686 regulators Krzysztof Kozlowski
2 siblings, 0 replies; 8+ messages in thread
From: Krzysztof Kozlowski @ 2014-10-27 9:44 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>
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v4 2/3] regulator: max77686: Implement suspend disable for some LDOs
2014-10-27 9:44 [PATCH v4 0/3] regulator: max77686/trats2: Disable some regulators in suspend Krzysztof Kozlowski
2014-10-27 9:44 ` [PATCH v4 1/3] regulator: max77686: Replace hard-coded opmode values with defines Krzysztof Kozlowski
@ 2014-10-27 9:44 ` Krzysztof Kozlowski
2014-10-27 10:08 ` Javier Martinez Canillas
2014-10-27 9:44 ` [PATCH v4 3/3] ARM: dts: exynos4412-trats: Add suspend configuration for max77686 regulators Krzysztof Kozlowski
2 siblings, 1 reply; 8+ messages in thread
From: Krzysztof Kozlowski @ 2014-10-27 9:44 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, linux-kernel, Ben Dooks, Kukjin Kim,
Russell King, linux-arm-kernel, linux-samsung-soc, devicetree
Cc: Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz, Chanwoo Choi,
Kyungmin Park, Javier Martinez Canillas, Marek Szyprowski
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v4 3/3] ARM: dts: exynos4412-trats: Add suspend configuration for max77686 regulators
2014-10-27 9:44 [PATCH v4 0/3] regulator: max77686/trats2: Disable some regulators in suspend Krzysztof Kozlowski
2014-10-27 9:44 ` [PATCH v4 1/3] regulator: max77686: Replace hard-coded opmode values with defines Krzysztof Kozlowski
2014-10-27 9:44 ` [PATCH v4 2/3] regulator: max77686: Implement suspend disable for some LDOs Krzysztof Kozlowski
@ 2014-10-27 9:44 ` Krzysztof Kozlowski
2 siblings, 0 replies; 8+ messages in thread
From: Krzysztof Kozlowski @ 2014-10-27 9:44 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>
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v4 2/3] regulator: max77686: Implement suspend disable for some LDOs
2014-10-27 9:44 ` [PATCH v4 2/3] regulator: max77686: Implement suspend disable for some LDOs Krzysztof Kozlowski
@ 2014-10-27 10:08 ` Javier Martinez Canillas
2014-10-27 10:27 ` Krzysztof Kozlowski
0 siblings, 1 reply; 8+ messages in thread
From: Javier Martinez Canillas @ 2014-10-27 10:08 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/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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 2/3] regulator: max77686: Implement suspend disable for some LDOs
2014-10-27 10:08 ` Javier Martinez Canillas
@ 2014-10-27 10:27 ` Krzysztof Kozlowski
2014-10-27 10:32 ` Javier Martinez Canillas
0 siblings, 1 reply; 8+ messages in thread
From: Krzysztof Kozlowski @ 2014-10-27 10:27 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 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 2/3] regulator: max77686: Implement suspend disable for some LDOs
2014-10-27 10:27 ` Krzysztof Kozlowski
@ 2014-10-27 10:32 ` Javier Martinez Canillas
2014-10-27 10:35 ` Krzysztof Kozlowski
0 siblings, 1 reply; 8+ messages in thread
From: Javier Martinez Canillas @ 2014-10-27 10:32 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
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 2/3] regulator: max77686: Implement suspend disable for some LDOs
2014-10-27 10:32 ` Javier Martinez Canillas
@ 2014-10-27 10:35 ` Krzysztof Kozlowski
0 siblings, 0 replies; 8+ messages in thread
From: Krzysztof Kozlowski @ 2014-10-27 10:35 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 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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-10-27 10:35 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-27 9:44 [PATCH v4 0/3] regulator: max77686/trats2: Disable some regulators in suspend Krzysztof Kozlowski
2014-10-27 9:44 ` [PATCH v4 1/3] regulator: max77686: Replace hard-coded opmode values with defines Krzysztof Kozlowski
2014-10-27 9:44 ` [PATCH v4 2/3] regulator: max77686: Implement suspend disable for some LDOs Krzysztof Kozlowski
2014-10-27 10:08 ` Javier Martinez Canillas
2014-10-27 10:27 ` Krzysztof Kozlowski
2014-10-27 10:32 ` Javier Martinez Canillas
2014-10-27 10:35 ` Krzysztof Kozlowski
2014-10-27 9:44 ` [PATCH v4 3/3] ARM: dts: exynos4412-trats: Add suspend configuration for max77686 regulators 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).