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

[Neil Armstrong <neil.armstrong@linaro.org>]

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