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

[Neil Armstrong]

On 5/22/26 14:01, Mark Brown wrote:
> 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?

Indeed

> > +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.

Right

> > +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.

OK indeed thanks, I overlooked this part

Neil