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

[sashiko-bot@kernel.org]

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