[PATCH] regulator: pca9450: Support regulator-off-in-suspend

[PATCH] regulator: pca9450: Support regulator-off-in-suspend

[Fabio Estevam]

From: Fabio Estevam <festevam@nabladev.com>

The PCA9450 uses each regulator's ENMODE field to control whether the
regulator remains enabled when the PMIC transitions from RUN to STANDBY
mode.

The driver does not currently implement set_suspend_disable(), so a
regulator configured with regulator-off-in-suspend remains enabled
during system suspend.

Implement set_suspend_disable() for the buck regulators and LDO3-LDO5
by programming ENMODE to 10b. This keeps the regulator enabled in RUN
mode, turns it off when PMIC_STBY_REQ is asserted, and enables it again
when the PMIC returns to RUN mode.

Keep LDO1 and LDO2 on regulator operations without
set_suspend_disable(), because these regulators supply the SNVS domain
and must remain enabled in STANDBY mode.

Measured on a custom i.MX8MP board, turning off NVCC_SD2 (LDO5) during
system suspend reduced power consumption by approximately 64 mW.

Signed-off-by: Fabio Estevam <festevam@nabladev.com>
---
 drivers/regulator/pca9450-regulator.c | 43 +++++++++++++++++++++++----
 include/linux/regulator/pca9450.h     |  3 ++
 2 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/pca9450-regulator.c b/drivers/regulator/pca9450-regulator.c
index c41db70fa052..13082585140f 100644
--- a/drivers/regulator/pca9450-regulator.c
+++ b/drivers/regulator/pca9450-regulator.c
@@ -77,6 +77,24 @@ static const unsigned int pca9450_dvs_buck_ramp_table[] = {
 	25000, 12500, 6250, 3125
 };
 
+/*
+ * ENMODE = 10b keeps the regulator enabled in RUN mode and disables it
+ * while PMIC_STBY_REQ is asserted.
+ */
+static int pca9450_buck_set_suspend_disable(struct regulator_dev *rdev)
+{
+	return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+				  rdev->desc->enable_mask,
+				  BUCK_ENMODE_ONREQ_STBYREQ);
+}
+
+static int pca9450_ldo_set_suspend_disable(struct regulator_dev *rdev)
+{
+	return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+				  rdev->desc->enable_mask,
+				  LDO_ENMODE_ONREQ_STBYREQ);
+}
+
 static const struct regulator_ops pca9450_dvs_buck_regulator_ops = {
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -88,6 +106,7 @@ static const struct regulator_ops pca9450_dvs_buck_regulator_ops = {
 	.set_ramp_delay	= regulator_set_ramp_delay_regmap,
 	.set_mode = pca9450_buck_set_mode,
 	.get_mode = pca9450_buck_get_mode,
+	.set_suspend_disable = pca9450_buck_set_suspend_disable,
 };
 
 static const struct regulator_ops pca9450_buck_regulator_ops = {
@@ -100,6 +119,7 @@ static const struct regulator_ops pca9450_buck_regulator_ops = {
 	.set_voltage_time_sel = regulator_set_voltage_time_sel,
 	.set_mode = pca9450_buck_set_mode,
 	.get_mode = pca9450_buck_get_mode,
+	.set_suspend_disable = pca9450_buck_set_suspend_disable,
 };
 
 static const struct regulator_ops pca9450_ldo_regulator_ops = {
@@ -111,6 +131,16 @@ static const struct regulator_ops pca9450_ldo_regulator_ops = {
 	.get_voltage_sel = regulator_get_voltage_sel_regmap,
 };
 
+static const struct regulator_ops pca9450_ldo_suspend_regulator_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.is_enabled = regulator_is_enabled_regmap,
+	.list_voltage = regulator_list_voltage_linear_range,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.set_suspend_disable = pca9450_ldo_set_suspend_disable,
+};
+
 static unsigned int pca9450_ldo5_get_reg_voltage_sel(struct regulator_dev *rdev)
 {
 	struct pca9450 *pca9450 = rdev_get_drvdata(rdev);
@@ -164,6 +194,7 @@ static const struct regulator_ops pca9450_ldo5_regulator_ops = {
 	.list_voltage = regulator_list_voltage_linear_range,
 	.set_voltage_sel = pca9450_ldo5_set_voltage_sel_regmap,
 	.get_voltage_sel = pca9450_ldo5_get_voltage_sel_regmap,
+	.set_suspend_disable = pca9450_ldo_set_suspend_disable,
 };
 
 /*
@@ -575,7 +606,7 @@ static struct pca9450_regulator_desc pca9450a_regulators[] = {
 			.of_match = of_match_ptr("LDO3"),
 			.regulators_node = of_match_ptr("regulators"),
 			.id = PCA9450_LDO3,
-			.ops = &pca9450_ldo_regulator_ops,
+			.ops = &pca9450_ldo_suspend_regulator_ops,
 			.type = REGULATOR_VOLTAGE,
 			.n_voltages = PCA9450_LDO3_VOLTAGE_NUM,
 			.linear_ranges = pca9450_ldo34_volts,
@@ -594,7 +625,7 @@ static struct pca9450_regulator_desc pca9450a_regulators[] = {
 			.of_match = of_match_ptr("LDO4"),
 			.regulators_node = of_match_ptr("regulators"),
 			.id = PCA9450_LDO4,
-			.ops = &pca9450_ldo_regulator_ops,
+			.ops = &pca9450_ldo_suspend_regulator_ops,
 			.type = REGULATOR_VOLTAGE,
 			.n_voltages = PCA9450_LDO4_VOLTAGE_NUM,
 			.linear_ranges = pca9450_ldo34_volts,
@@ -820,7 +851,7 @@ static struct pca9450_regulator_desc pca9450bc_regulators[] = {
 			.of_match = of_match_ptr("LDO3"),
 			.regulators_node = of_match_ptr("regulators"),
 			.id = PCA9450_LDO3,
-			.ops = &pca9450_ldo_regulator_ops,
+			.ops = &pca9450_ldo_suspend_regulator_ops,
 			.type = REGULATOR_VOLTAGE,
 			.n_voltages = PCA9450_LDO3_VOLTAGE_NUM,
 			.linear_ranges = pca9450_ldo34_volts,
@@ -839,7 +870,7 @@ static struct pca9450_regulator_desc pca9450bc_regulators[] = {
 			.of_match = of_match_ptr("LDO4"),
 			.regulators_node = of_match_ptr("regulators"),
 			.id = PCA9450_LDO4,
-			.ops = &pca9450_ldo_regulator_ops,
+			.ops = &pca9450_ldo_suspend_regulator_ops,
 			.type = REGULATOR_VOLTAGE,
 			.n_voltages = PCA9450_LDO4_VOLTAGE_NUM,
 			.linear_ranges = pca9450_ldo34_volts,
@@ -1040,7 +1071,7 @@ static struct pca9450_regulator_desc pca9451a_regulators[] = {
 			.of_match = of_match_ptr("LDO3"),
 			.regulators_node = of_match_ptr("regulators"),
 			.id = PCA9450_LDO3,
-			.ops = &pca9450_ldo_regulator_ops,
+			.ops = &pca9450_ldo_suspend_regulator_ops,
 			.type = REGULATOR_VOLTAGE,
 			.n_voltages = PCA9450_LDO3_VOLTAGE_NUM,
 			.linear_ranges = pca9450_ldo34_volts,
@@ -1059,7 +1090,7 @@ static struct pca9450_regulator_desc pca9451a_regulators[] = {
 			.of_match = of_match_ptr("LDO4"),
 			.regulators_node = of_match_ptr("regulators"),
 			.id = PCA9450_LDO4,
-			.ops = &pca9450_ldo_regulator_ops,
+			.ops = &pca9450_ldo_suspend_regulator_ops,
 			.type = REGULATOR_VOLTAGE,
 			.n_voltages = PCA9450_LDO4_VOLTAGE_NUM,
 			.linear_ranges = pca9450_ldo34_volts,
diff --git a/include/linux/regulator/pca9450.h b/include/linux/regulator/pca9450.h
index 0df8b3c48082..8925e9c9376b 100644
--- a/include/linux/regulator/pca9450.h
+++ b/include/linux/regulator/pca9450.h
@@ -213,6 +213,9 @@ enum {
 #define LDO5H_EN_MASK			0xC0
 #define LDO5HOUT_MASK			0x0F
 
+/* LDO ENMODE value: ON in RUN, OFF while PMIC_STBY_REQ is asserted */
+#define LDO_ENMODE_ONREQ_STBYREQ	0x80
+
 /* PCA9450_REG_IRQ bits */
 #define IRQ_PWRON			0x80
 #define IRQ_WDOGB			0x40
-- 
2.43.0

Re: [PATCH] regulator: pca9450: Support regulator-off-in-suspend

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] set_suspend_disable unconditionally overwrites the regulator's RUN state, forcing disabled regulators to turn ON upon resume.
- [High] Setting a buck regulator's suspend-disable state breaks the regulator_is_enabled() API contract, causing state desynchronization.

Pre-existing issues:
- [High] LDO5 suspend and runtime enable operations are hardcoded to LDO5CTRL_L, silently failing when the SD_VSEL pin is HIGH.
--

--- Patch [1]: [PATCH] regulator: pca9450: Support regulator-off-in-suspend ---
commit 906a14c015dd9c2836b5ec2820d49824597625f2
Author: Fabio Estevam <festevam@nabladev.com>
Subject: regulator: pca9450: Support regulator-off-in-suspend

The PCA9450 uses each regulator's ENMODE field to control whether the
regulator remains enabled when the PMIC transitions from RUN to STANDBY
mode.

Implement set_suspend_disable() for the buck regulators and LDO3-LDO5
by programming ENMODE to 10b to keep the regulator enabled in RUN
mode and off when PMIC_STBY_REQ is asserted.

> diff --git a/drivers/regulator/pca9450-regulator.c b/drivers/regulator/pca9450-regulator.c
> index c41db70fa0529..13082585140f4 100644
> --- a/drivers/regulator/pca9450-regulator.c
> +++ b/drivers/regulator/pca9450-regulator.c
> @@ -77,6 +77,24 @@ static const unsigned int pca9450_dvs_buck_ramp_table[] = {
>  	25000, 12500, 6250, 3125
>  };
>  
> +/*
> + * ENMODE = 10b keeps the regulator enabled in RUN mode and disables it
> + * while PMIC_STBY_REQ is asserted.
> + */
> +static int pca9450_buck_set_suspend_disable(struct regulator_dev *rdev)
> +{
> +	return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
> +				  rdev->desc->enable_mask,
> +				  BUCK_ENMODE_ONREQ_STBYREQ);
> +}

[Severity: High]
Does this unconditionally overwrite the regulator's RUN state? If a regulator
is currently disabled at runtime but its device tree node has the
regulator-off-in-suspend constraint, the regulator core's regulator_suspend()
will still call this function. Writing BUCK_ENMODE_ONREQ_STBYREQ (0x02)
encodes 'ON in RUN mode, OFF in STANDBY mode', which would bypass the kernel's
state machine and force the regulator to turn ON when the system resumes to
RUN mode.

Could this also cause a state desynchronization with regulator_is_enabled()?
The driver uses regulator_is_enabled_regmap(), which performs a strict
equality check against rdev->desc->enable_val. Since most buck regulators
define enable_val as BUCK_ENMODE_ONREQ (0x01), updating the bits to 0x02
here will cause the core to incorrectly report an active regulator as
disabled.

> +
> +static int pca9450_ldo_set_suspend_disable(struct regulator_dev *rdev)
> +{
> +	return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
> +				  rdev->desc->enable_mask,
> +				  LDO_ENMODE_ONREQ_STBYREQ);
> +}

[Severity: High]
Similar to the buck regulators, will this unconditionally overwrite the shared
ENMODE bits for LDOs? If the regulator was disabled in RUN mode, this writes
LDO_ENMODE_ONREQ_STBYREQ (0x80) and forces it ON in RUN mode.

[ ... ]

> @@ -164,6 +194,7 @@ static const struct regulator_ops pca9450_ldo5_regulator_ops = {
>  	.list_voltage = regulator_list_voltage_linear_range,
>  	.set_voltage_sel = pca9450_ldo5_set_voltage_sel_regmap,
>  	.get_voltage_sel = pca9450_ldo5_get_voltage_sel_regmap,
> +	.set_suspend_disable = pca9450_ldo_set_suspend_disable,
>  };

[Severity: High]
This is a pre-existing issue, but will pca9450_ldo_set_suspend_disable()
silently fail to apply to LDO5 if the external SD_VSEL pin is driven HIGH?

Looking at the pca9450a_regulators[] definition, LDO5 statically hardcodes
enable_reg to PCA9450_REG_LDO5CTRL_L:

    .vsel_reg = PCA9450_REG_LDO5CTRL_H,
    .vsel_mask = LDO5HOUT_MASK,
    .enable_reg = PCA9450_REG_LDO5CTRL_L,
    .enable_mask = LDO5H_EN_MASK,

When SD_VSEL is HIGH (e.g., during 1.8V UHS SD card operation), the PMIC
hardware uses the ENMODE bits from LDO5CTRL_H. Writing to LDO5CTRL_L here
and in the standard regulator_enable_regmap() operation will be completely
ignored by the hardware.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724201322.73937-1-festevam@gmail.com?part=1