Re: [PATCH v2 2/3] regulator: pfuze100: add set_suspend_disable for LDO ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The callback `pfuze100_ldo_set_suspend_disable` does not explicitly clear the Low Power mode bit (`VGENxLPWR`), relying on the fragile assumption that the bootloader left it at its reset value of 0. If it is 1, the LDO enters low-power mode instead of powering off.
--

--- Patch [2]: [PATCH v2 2/3] regulator: pfuze100: add set_suspend_disable for LDO ops ---
commit b5ef15eff59cfdbe79ee9305f34ea04f79252ea1
Author: Joy Zou <joy.zou@oss.nxp.com>

regulator: pfuze100: add set_suspend_disable for LDO ops

This commit adds a set_suspend_disable callback to pfuze100_ldo_regulator_ops.
This supports the regulator-off-in-suspend DTS property for the VGEN LDO
regulators, allowing unused LDO regulators to be properly disabled during
system suspend.

> diff --git a/drivers/regulator/pfuze100-regulator.c b/drivers/regulator/pfuze100-regulator.c
> index 7d56c22b5e400..d3a0e25055248 100644
> --- a/drivers/regulator/pfuze100-regulator.c
> +++ b/drivers/regulator/pfuze100-regulator.c

[ ... ]

> @@ -158,6 +158,25 @@ static int pfuze100_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
>  	return ret;
>  }
>  
> +static int pfuze100_ldo_set_suspend_disable(struct regulator_dev *rdev)
> +{
> +	struct pfuze_chip *pfuze100 = rdev_get_drvdata(rdev);
> +	int id = rdev_get_id(rdev);
> +	struct pfuze_regulator *desc = &pfuze100->regulator_descs[id];
> +
> +	/*
> +	 * Set the standby bit so the LDO output is turned off when the PMIC
> +	 * receives a STANDBY event, using the per-regulator stby_reg/stby_mask
> +	 * that describe the standby control for each LDO.
> +	 *
> +	 * The stby_mask only covers the VGENxSTBY bit. The VGENxLPWR stays at
> +	 * its reset value of 0, so the LDO is switched off rather than put
> +	 * into low-power mode.
> +	 */
> +	return regmap_update_bits(pfuze100->regmap, desc->stby_reg,
> +				  desc->stby_mask, desc->stby_mask);

[Severity: Medium]
Will this sequence reliably turn off the regulator if the bootloader has
previously set the VGENxLPWR bit to 1?

Since this only updates the VGENxSTBY bit defined in stby_mask, it relies on
the VGENxLPWR bit remaining at its reset value of 0.

If the bootloader modified this bit, does the LDO enter low-power mode
instead of completely powering off during system suspend?

Could the VGENxLPWR bit be explicitly cleared here to guarantee the regulator
turns off, similar to how it is handled in pfuze_power_off_prepare()?

> +}
> +
>  static const struct regulator_ops pfuze100_ldo_regulator_ops = {
>  	.enable = regulator_enable_regmap,
>  	.disable = regulator_disable_regmap,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-b4-regulator-pf01-v2-0-a406c8737fdb@oss.nxp.com?part=2