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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] regulator: wl2868c: preset LDOs enabled by wl2868c_probe()
  when SYS_EN is 0
- [Medium] regulator: wl2868c: reset released before enabling vsys in
  wl2868c_probe()
- [Medium] regulator: wl2868c: vin7-supply not enabled for I2C in
  wl2868c_probe()
--

commit fa96a3434b8aa9642e9973857f62fda162d22f23
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 seven LDO outputs of the Will
Semiconductor WL2868C I2C camera power management IC.

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

[ ... ]

> +static int wl2868c_enable(struct regulator_dev *rdev)
> +{
> +	struct wl2868c *chip = rdev_get_drvdata(rdev);
> +	unsigned int bits = rdev->desc->enable_mask;
> +	unsigned int ldos;
> +	int ret;
> +
> +	guard(mutex)(&chip->lock);
> +
> +	/*
> +	 * The enable register is written as a whole on purpose. SYS_EN gates
> +	 * every output of the chip, and the per-LDO bits may be preset by the
> +	 * firmware while SYS_EN is still clear: setting only SYS_EN and our own
> +	 * bit would then bring the preset LDOs up as well. Writing the register
> +	 * with the bits of the LDOs consumers have enabled, and only those,
> +	 * keeps the other outputs off. chip->enabled was seeded from the
> +	 * register in probe(), so an output the firmware left on is carried
> +	 * over instead of being cleared by the first enable.
> +	 */
> +	ldos = (chip->enabled | bits) & WL2868C_LDO_ENABLE_MASK;
> +	ret = regmap_write(chip->regmap, WL2868C_LDO_ENABLE,
> +			   WL2868C_SYS_ENABLE | ldos);
> +	if (ret)
> +		return ret;
> +
> +	chip->enabled = ldos;
> +
> +	return 0;
> +}

[ ... ]

> +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, value;
> +	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);
> +	}

[Severity: Medium]
Does wl2868c_probe() release the reset line and wait 1 ms before enabling
the vsys power supply below?

If vsys is initially off, the reset deassertion and 1 ms startup delay
happen while the chip is unpowered, and the subsequent device ID read runs
immediately after devm_regulator_get_enable() with no startup delay.
Because devres unwinds in reverse order, unbind or probe failure will also
disable vsys before wl2868c_reset() asserts reset.

> +	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");
> +
> +	/*
> +	 * The I2C interface is powered from the higher of VIN7 and VSYS, so the
> +	 * system supply has to be up before the device can answer at all. It
> +	 * stays enabled for as long as the driver is bound.
> +	 */
> +	ret = devm_regulator_get_enable(dev, "vsys");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable the system supply\n");
> +
> +	ret = regmap_read(config.regmap, WL2868C_DEVICE_ID, &id);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to read device ID\n");
> +	if (id != WL2868C_DEVICE_ID_VALUE)
> +		return dev_err_probe(dev, -ENODEV, "Unexpected device ID: %#x\n", id);

[Severity: Medium]
What happens on boards that power the control logic and I2C interface from
vin7-supply without vsys-supply?

The devicetree binding allows supplying either vin7-supply or vsys-supply,
but wl2868c_probe() only enables vsys and leaves vin7 attached solely as
the input supply of ldo7.  When vsys-supply is omitted,
devm_regulator_get_enable() falls back to a dummy regulator while
vin7-supply stays disabled during the initial register reads and whenever
ldo7 is off, leaving the chip unpowered on the I2C bus.

> +	/*
> +	 * Seed the tracked set with the outputs the firmware left enabled, so
> +	 * that the first enable() or disable() of any output preserves them.
> +	 * The regulator core reads the state back through is_enabled(), which
> +	 * does not look at this set.
> +	 */
> +	ret = regmap_read(config.regmap, WL2868C_LDO_ENABLE, &value);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to read the enable register\n");
> +
> +	chip->enabled = value & WL2868C_LDO_ENABLE_MASK;

[Severity: Medium]
Doesn't seeding chip->enabled without checking whether bit 7 (sys_en) is
set in value defeat the protection against preset LDO bits in
wl2868c_enable()?

When bit 7 is clear at probe time, any preset per-LDO bits in value are
copied into chip->enabled even though all outputs are off and
wl2868c_is_enabled() reports them as disabled.  The first wl2868c_enable()
call for any single output then writes chip->enabled | bits together with
bit 7, turning on all of the preset outputs and leaving them stuck on
even after consumers disable their regulators.

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