Re: [PATCH v5 2/2] regulator: add SGM3804 Dual Output driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Enable GPIOs are requested with `GPIOD_ASIS` but never explicitly configured as outputs, preventing the regulator from powering up.
- [High] The driver performs I2C communication immediately after asserting the enable GPIO, bypassing the required IC power-on delay.
- [Medium] Incorrect regmap state handling on `regcache_sync()` failure leads to silent I2C write failures for active rails.
- [Medium] Hardware is powered down before setting the regmap to cache-only mode in `sgm3804_disable()`, creating a race condition.
- [Low] The `ctx->lock` mutex is initialized without a corresponding `mutex_destroy()`, bypassing mutex debugging.
--

commit cd53576c2dc0f1b61e28aacf0d0ccbf347793fe0
Author: KancyJoe <kancy2333@outlook.com>

regulator: add SGM3804 Dual Output driver

Add support for the SG Micro SGM3804 Single Inductor Dual Output
Buck/Boost Converter. It uses a regmap cache because registers are only
writable when the enable GPIOs are active.

> diff --git a/drivers/regulator/sgm3804-regulator.c b/drivers/regulator/sgm3804-regulator.c
> new file mode 100644
> index 0000000000000..c3406cfb73d03
> --- /dev/null
> +++ b/drivers/regulator/sgm3804-regulator.c

[ ... ]

> +static int sgm3804_sync_regcache_state(struct sgm3804_data *ctx)
> +{
> +	guard(mutex)(&ctx->lock);
> +
> +	/* If both GPIOs are down, IC is powered down and I2C writes will fail */
> +	if (!gpiod_get_value_cansleep(ctx->gpios[SGM3804_POS_RAIL]) &&
> +	    !gpiod_get_value_cansleep(ctx->gpios[SGM3804_NEG_RAIL])) {
> +		regcache_cache_only(ctx->regmap, true);
> +		regcache_mark_dirty(ctx->regmap);
> +	} else {
> +		int ret;
> +
> +		/* At least a GPIO is up, we can write registers */
> +		regcache_cache_only(ctx->regmap, false);
> +		ret = regcache_sync(ctx->regmap);
> +		if (ret) {
> +			regcache_cache_only(ctx->regmap, true);

[Severity: Medium]
If one regulator is already successfully enabled, and the user attempts to
enable the second one but the I2C transfer fails, does this permanently stick
the regmap cache in cache-only mode?

Any subsequent voltage or discharge changes to the active rail will only update
the software cache and fail to write to the physical hardware.

> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int sgm3804_enable(struct regulator_dev *rdev)
> +{
> +	struct sgm3804_data *ctx = rdev->reg_data;
> +	int ret;
> +
> +	ret = gpiod_set_value_cansleep(ctx->gpios[rdev_get_id(rdev)], 1);
> +	if (ret)
> +		return ret;
> +
> +	ret = sgm3804_sync_regcache_state(ctx);

[Severity: High]
Does this perform I2C communication immediately after asserting the enable
GPIO, bypassing the required IC power-on delay?

The driver specifies .enable_time = 40000, but because the driver issues
I2C commands synchronously inside the enable callback, the regulator core's
delay hasn't occurred yet.

Will sending I2C commands immediately result in I2C NACK errors?

> +	if (ret)
> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	gpiod_set_value_cansleep(ctx->gpios[rdev_get_id(rdev)], 0);
> +	return ret;
> +}
> +
> +static int sgm3804_disable(struct regulator_dev *rdev)
> +{
> +	struct sgm3804_data *ctx = rdev->reg_data;
> +	int ret;
> +
> +	ret = gpiod_set_value_cansleep(ctx->gpios[rdev_get_id(rdev)], 0);
> +	if (ret)
> +		return ret;
> +
> +	return sgm3804_sync_regcache_state(ctx);

[Severity: Medium]
Is there a race condition here if the hardware is powered down before
setting the regmap to cache-only mode?

If this is the last active rail, the physical IC loses power immediately.
If another thread concurrently accesses the shared regmap to set the voltage
on the other rail, it could attempt an I2C transaction on the powered-off IC.

> +}

[ ... ]

> +static int sgm3804_probe(struct i2c_client *i2c)
> +{
> +	struct device *dev = &i2c->dev;
> +	struct sgm3804_data *ctx;
> +	int ret, i;
> +
> +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	mutex_init(&ctx->lock);

[Severity: Low]
Is this missing a corresponding mutex_destroy()?

Since ctx is allocated with devm_kzalloc(), the memory will be freed without
destroying the mutex, which bypasses the kernel's CONFIG_DEBUG_MUTEXES
infrastructure.

Would it be better to use devm_mutex_init() instead?

> +
> +	ctx->regmap = devm_regmap_init_i2c(i2c, &sgm3804_regmap_config);
> +	if (IS_ERR(ctx->regmap))
> +		return dev_err_probe(dev, PTR_ERR(ctx->regmap),
> +				     "failed to init regmap\n");
> +
> +	/* Get enable GPIOs */
> +	for (i = 0; i < ARRAY_SIZE(sgm3804_regulator_desc); i++) {
> +		const struct regulator_desc *reg = &sgm3804_regulator_desc[i];
> +		struct fwnode_handle *child;
> +
> +		child = device_get_named_child_node(dev, reg->of_match);
> +		if (!child) {
> +			dev_err(dev, "missing child '%s'\n", reg->of_match);
> +			return -EINVAL;
> +		}
> +
> +		ctx->gpios[i] = devm_fwnode_gpiod_get(dev, child, "enable",
> +						      GPIOD_ASIS, reg->name);

[Severity: High]
Does this prevent the regulator from powering up?

The GPIOD_ASIS flag leaves the pin's hardware direction unconfigured.
If the bootloader left the pin as an input, calling gpiod_set_value_cansleep()
in sgm3804_enable() will fail and return -EPERM because the pin is not
configured as an output.

Could the GPIO direction be explicitly configured as an output instead?

> +		fwnode_handle_put(child);
> +		if (IS_ERR(ctx->gpios[i]))
> +			return dev_err_probe(dev, PTR_ERR(ctx->gpios[i]),
> +					     "failed to get '%s' enable GPIO\n",
> +					     reg->name);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522-topic-sm8650-ayaneo-pocket-s2-sgm3804-v5-0-bd6b1c300ecc@linaro.org?part=2