Re: [PATCH 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: check SYS_EN before seeding enabled in
  wl2868c_probe()
- [Medium] regulator: wl2868c: enable vsys before deasserting reset in
  wl2868c_probe()
- [Medium] regulator: wl2868c: handle vin7-supply for control logic in
  wl2868c_probe()
--

commit 3a2a7e94fa21fa44fb2fb53973938985aaea5820
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
seven-channel I2C camera PMIC.

> 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

[ ... ]

> +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]
Should the supply be enabled before releasing reset and waiting 1 ms?

devm_regulator_get_enable(dev, "vsys") is called after this block, right
before reading the device id register.  If vsys was off before probe,
reset is deasserted and the 1 ms fsleep(1000) delay elapses while the chip
is still unpowered, and the first register read runs immediately after
vsys turns on with no startup delay.

Registering wl2868c_reset() before devm_regulator_get_enable() also means
devres teardown disables vsys before asserting reset on unbind.

> +
> +	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");

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

The devicetree binding allows supplying either vin7-supply or vsys-supply:

Documentation/devicetree/bindings/regulator/willsemi,wl2868c.yaml:
    anyOf:
      - required:
          - vin7-supply
      - required:
          - vsys-supply

When vsys-supply is omitted, devm_regulator_get_enable(dev, "vsys") falls
back to a dummy regulator with a warning and leaves vin7-supply disabled
during the register reads below.  Because vin7 is only attached as the
supply for ldo7, the chip also loses control power whenever ldo7 is off.

> +
> +	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);
> +
> +	/*
> +	 * 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]
Does seeding chip->enabled without checking WL2868C_SYS_ENABLE turn on
preset outputs when the first regulator is enabled?

As described in wl2868c_enable(), firmware may preset per-output bits in
WL2868C_LDO_ENABLE while WL2868C_SYS_ENABLE is still clear, and
wl2868c_is_enabled() reports those outputs as disabled.  Copying those
bits into chip->enabled when WL2868C_SYS_ENABLE is clear causes the first
wl2868c_enable() call on any output to write WL2868C_SYS_ENABLE | ldos and
power on all preset outputs as well.  Seeding chip->enabled only when
value & WL2868C_SYS_ENABLE is set avoids this.

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