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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]

On Wed, May 06, 2026 at 09:34:07AM +0200, Neil Armstrong wrote:

> +config REGULATOR_SGM3804
> +	tristate "SGMicro SGM3804 voltage regulator"
> +	depends on I2C && OF
> +	select REGMAP_I2C
> +	help
> +	  This driver supports SGMicro SGM3804 dual-output voltage regulator.
> +

The GPIO usage in the driver looks non-optional so we should have a
GPIOLIB dependency shouldn't we?

> +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 {
> +		/* At least a GPIO is up, we can write registers */
> +		regcache_cache_only(ctx->regmap, false);
> +		return regcache_sync(ctx->regmap);

This should really put the regmap back into cache only mode if the sync
fails.

> +static const struct regulator_desc sgm3804_regulator_desc[] = {
> +	/* Positive Output */
> +	{
> +		.name = "pos",
> +		.of_match = "pos",
> +		.supply_name = "vin",
> +		.id = SGM3804_POS_RAIL,
> +		.ops = &sgm3804_ops,
> +		.type = REGULATOR_VOLTAGE,
> +		.linear_ranges = sgm3804_voltages,
> +		.n_linear_ranges = ARRAY_SIZE(sgm3804_voltages),
> +		.n_voltages = SGM3804_VOLTAGES_COUNT,

n_voltages is misnamed and should really be maximum selector, for a
device like this with sparse selectors it doesn't do what you'd expect
unfortunately.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]