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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] regulator: wl2868c: uninitialized chip->enabled in wl2868c_probe()
- [Medium] regulator: wl2868c: missing vsys-supply enable in wl2868c_probe()
- [Medium] regulator: wl2868c: premature state update in wl2868c_disable()
--

commit 43e6bd60d4f6c3f0ce8478df036a5927be90da6e
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 power management IC supporting its seven LDO outputs.

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

[ ... ]

> +static int wl2868c_disable(struct regulator_dev *rdev)
> +{
> +	struct wl2868c *chip = rdev_get_drvdata(rdev);
> +	unsigned int bits = rdev->desc->enable_mask;
> +	unsigned int ldos;
> +
> +	guard(mutex)(&chip->lock);
> +
> +	chip->enabled &= ~bits;
> +
> +	/*
> +	 * SYS_EN stays set while at least one output is enabled; the per-LDO
> +	 * bits of the other outputs stay clear so that setting it cannot
> +	 * switch anything else on.
> +	 */
> +	ldos = chip->enabled & WL2868C_LDO_ENABLE_MASK;
> +
> +	return regmap_write(chip->regmap, WL2868C_LDO_ENABLE,
> +			    ldos ? WL2868C_SYS_ENABLE | ldos : 0);
> +}

[Severity: Medium]
If regmap_write() fails in wl2868c_disable(), does chip->enabled get out
of sync with both the hardware and the regulator core?

In wl2868c_enable(), chip->enabled |= bits is only updated after
regmap_write() succeeds. Here in wl2868c_disable(), chip->enabled &= ~bits
is cleared before calling regmap_write().

When ops->disable() returns a negative error code in _regulator_disable():

drivers/regulator/core.c:_regulator_disable() {
    ...
    ret = _regulator_do_disable(rdev);
    if (ret < 0) {
        rdev_err(rdev, "failed to disable: %pe\n", ERR_PTR(ret));
        _notifier_call_chain(rdev,
                REGULATOR_EVENT_ABORT_DISABLE,
                NULL);
        return ret;
    }
    ...
    rdev->use_count = 0;
}

the regulator core aborts the disable and keeps rdev->use_count at 1 while
the LDO remains enabled in hardware, but chip->enabled has already lost
that LDO's bit. A subsequent wl2868c_enable() or wl2868c_disable() on
another LDO will then overwrite WL2868C_LDO_ENABLE with that bit cleared,
shutting off the still-enabled output.

Should chip->enabled only be updated after regmap_write() succeeds,
matching wl2868c_enable()?

> +
> +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;
> +}

[ ... ]

> +static const struct regulator_desc wl2868c_regulators[] = {
> +	WL2868C_LDO(1, "vin12", 496000, 128, 0x03, 0x7f, BIT(0)),
> +	WL2868C_LDO(2, "vin12", 496000, 128, 0x04, 0x7f, BIT(1)),
> +	WL2868C_LDO(3, "vin34", 1504000, 256, 0x05, 0xff, BIT(2)),
> +	WL2868C_LDO(4, "vin34", 1504000, 256, 0x06, 0xff, BIT(3)),
> +	WL2868C_LDO(5, "vin5", 1504000, 256, 0x07, 0xff, BIT(4)),
> +	WL2868C_LDO(6, "vin6", 1504000, 256, 0x08, 0xff, BIT(5)),
> +	WL2868C_LDO(7, "vin7", 1504000, 256, 0x09, 0xff, BIT(6)),
> +};

[ ... ]

> +static int wl2868c_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct regulator_config config = { .dev = dev };
> +	struct regulator_dev *rdev;
> +	struct gpio_desc *reset;
> +	struct wl2868c *chip;
> +	unsigned int id;
> +	int ret, i;
> +
> +	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	mutex_init(&chip->lock);
> +	config.driver_data = chip;
> +
> +	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(reset))
> +		return dev_err_probe(dev, PTR_ERR(reset), "Failed to get reset GPIO\n");
> +
> +	if (reset) {
> +		ret = devm_add_action_or_reset(dev, wl2868c_reset, reset);
> +		if (ret)
> +			return ret;
> +		fsleep(1000);
> +	}
> +
> +	chip->regmap = devm_regmap_init_i2c(client, &wl2868c_regmap_config);
> +	config.regmap = chip->regmap;
> +	if (IS_ERR(config.regmap))
> +		return dev_err_probe(dev, PTR_ERR(config.regmap),
> +				     "Failed to create regmap\n");
> +
> +	ret = regmap_read(config.regmap, WL2868C_DEVICE_ID, &id);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to read device ID\n");

[Severity: Medium]
Does wl2868c_probe() need to request and enable vsys-supply (or the
control logic supply) before accessing the chip over I2C?

Documentation/devicetree/bindings/regulator/willsemi,wl2868c.yaml defines
vsys-supply and notes that the internal control logic is powered from the
higher of VIN7 and VSYS, requiring at least one of vin7-supply or
vsys-supply.

However, vsys-supply is never requested or enabled by the driver, and
vin7-supply is only registered as the supply_name for LDO7. On systems
where the control logic is powered by a switchable vsys-supply (or
vin7-supply when LDO7 is not enabled), the chip may not be powered when
regmap_read() reads WL2868C_DEVICE_ID in wl2868c_probe(), or
regulator_late_cleanup() may disable an unclaimed vsys-supply while LDO1
through LDO6 are in use.

> +	if (id != WL2868C_DEVICE_ID_VALUE)
> +		return dev_err_probe(dev, -ENODEV, "Unexpected device ID: %#x\n", id);
> +
> +	for (i = 0; i < ARRAY_SIZE(wl2868c_regulators); i++) {
> +		rdev = devm_regulator_register(dev, &wl2868c_regulators[i], &config);
> +		if (IS_ERR(rdev))
> +			return dev_err_probe(dev, PTR_ERR(rdev),
> +					     "Failed to register LDO%d\n", i + 1);
> +	}

[Severity: Medium]
What happens if WL2868C_SYS_ENABLE and one or more LDO bits are already
set in WL2868C_LDO_ENABLE when wl2868c_probe() runs (for example, when the
optional reset GPIO is not present or was already deasserted by the
bootloader)?

In wl2868c_probe(), chip->enabled starts at 0, whereas
wl2868c_is_enabled() reads WL2868C_LDO_ENABLE directly from the hardware
and returns 1 for any LDO that is already active alongside
WL2868C_SYS_ENABLE.

When a consumer calls regulator_enable() on a boot-enabled LDO,
_regulator_enable() checks _regulator_is_enabled(rdev):

drivers/regulator/core.c:_regulator_enable() {
    ...
    if (rdev->use_count == 0) {
        /* The regulator may on if it's not switchable or left on */
        ret = _regulator_is_enabled(rdev);
        if (ret == -EINVAL || ret == 0) {
            if (!regulator_ops_is_valid(rdev,
                    REGULATOR_CHANGE_STATUS)) {
                ret = -EPERM;
                goto err_consumer_disable;
            }

            ret = _regulator_do_enable(rdev);
            if (ret < 0)
                goto err_consumer_disable;

            _notifier_call_chain(rdev, REGULATOR_EVENT_ENABLE,
                         NULL);
        } else if (ret < 0) {
            ...
        }
    }

    rdev->use_count++;
    ...
}

Because wl2868c_is_enabled() returns 1, _regulator_enable() skips calling
_regulator_do_enable() -> wl2868c_enable() and increments rdev->use_count
to 1 while that LDO's bit remains 0 in chip->enabled.

When any other LDO is subsequently enabled in wl2868c_enable() or disabled
in wl2868c_disable() (including regulator_late_cleanup() turning off an
unused LDO), the driver writes WL2868C_LDO_ENABLE from chip->enabled and
clears the boot-enabled LDO's bit while its consumer is actively using it.
Similarly, if multiple LDOs are already enabled at boot and marked
regulator-always-on or regulator-boot-on, registering the first one calls
wl2868c_enable() with chip->enabled == 0 and powers off the others.

Should wl2868c_probe() initialize chip->enabled from WL2868C_LDO_ENABLE
when WL2868C_SYS_ENABLE is already set?

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