Re: [PATCH v5 2/4] regulator: willsemi,wl2868c: add the WL2868C camera PMIC driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] regulator: wl2868c: wl2868c_enable() activates other LDOs with preset bits
--

commit 90fe2a3492ce0c2ca1434cba3d7a67c4de55955e
Author: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>

regulator: willsemi,wl2868c: add the WL2868C camera PMIC driver

This commit adds a regulator driver for the Will Semiconductor WL2868C
I2C camera PMIC supporting its seven LDO outputs, shared SYS_EN master
enable bit, and optional reset GPIO.

> diff --git a/drivers/regulator/wl2868c-regulator.c b/drivers/regulator/wl2868c-regulator.c
> new file mode 100644
> index 0000000000000..9c4f4240b1de1
> --- /dev/null
> +++ b/drivers/regulator/wl2868c-regulator.c
> @@ -0,0 +1,155 @@

[ ... ]

> +#define WL2868C_DEVICE_ID		0x00
> +#define WL2868C_DEVICE_ID_VALUE		0x82
> +#define WL2868C_LDO_ENABLE		0x0e
> +#define WL2868C_SYS_ENABLE		BIT(7)
> +
> +static int wl2868c_enable(struct regulator_dev *rdev)
> +{
> +	/*
> +	 * SYS_EN is the master enable of the chip: no output comes up unless
> +	 * it is set together with the per-LDO bit.
> +	 */
> +	return regmap_update_bits(rdev->regmap, WL2868C_LDO_ENABLE,
> +				  WL2868C_SYS_ENABLE | rdev->desc->enable_mask,
> +				  WL2868C_SYS_ENABLE | rdev->desc->enable_mask);
> +}
> +
> +static int wl2868c_is_enabled(struct regulator_dev *rdev)
> +{
> +	unsigned int mask = WL2868C_SYS_ENABLE | rdev->desc->enable_mask;
> +	unsigned int value;
> +	int ret;
> +
> +	ret = regmap_read(rdev->regmap, WL2868C_LDO_ENABLE, &value);
> +	if (ret)
> +		return ret;
> +
> +	return (value & mask) == mask;
> +}

[Severity: Medium]
If WL2868C_SYS_ENABLE is initially clear while one or more per-LDO bits in
WL2868C_LDO_ENABLE are set (for example, from pre-probe state when the
optional reset GPIO is absent), won't enabling a single LDO in
wl2868c_enable() unintentionally turn on those other LDOs as well?

While WL2868C_SYS_ENABLE is 0, wl2868c_is_enabled() returns 0 for every LDO
regardless of its per-LDO bit, so regulator_late_cleanup() sees those LDOs
as already disabled and does not call ops->disable() to clear their bits in
WL2868C_LDO_ENABLE.

When a consumer later enables one regulator via
_regulator_enable() -> _regulator_do_enable() -> wl2868c_enable(),
regmap_update_bits() sets WL2868C_SYS_ENABLE | rdev->desc->enable_mask while
preserving the other bits in WL2868C_LDO_ENABLE. Any other LDO with its bit
already set then becomes active in hardware and starts reporting enabled via
wl2868c_is_enabled() despite having rdev->use_count == 0, which also causes
a later _regulator_enable() on that LDO to see _regulator_is_enabled(rdev) > 0
and skip _regulator_do_enable() along with its 150 us enable_time delay.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102060458.461372-1-xiaqinghuan@mail.ustc.edu.cn?part=2