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

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

On 4/30/26 12:34, Mark Brown wrote:
> On Thu, Apr 30, 2026 at 10:48:47AM +0200, Neil Armstrong wrote:
> 
>> Add support for the SG Micro SGM3804 Single Inductor Dual Output
>> Buck/Boost Converter used to power LCD panels a provide positive
>> and negative power rails with configurable voltage and active
>> discharge function for each output.
> 
>> +config REGULATOR_SGM3804
>> +	tristate "SGMicro SGM3804 voltage regulator"
>> +	depends on I2C && OF
>> +	help
>> +	  This driver supports SGMicro SGM3804 dual-output voltage regulator.
>> +
> 
> This needs to select REGMAP_I2C.

Oops forgot

> 
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * SGMicro SGM3804 regulator Driver
>> + *
>> + * Copyright (C) 2025 Kancy Joe <kancy2333@outlook.com>
>> + * Copyright (C) 2026 Linaro Limited
>> + * Author: Neil Armstrong <neil.armstrong@linaro.org>
>> + */
> 
> Please make the entire comment block a C++ one so things look more
> intentional.

Sure, converted into:
+ * Copyright (C) 2026 Linaro Limited (Neil Armstrong <neil.armstrong@linaro.org>)

> 
>> +/*
>> + * Since all registers are only writeable & volatile,
>> + * regmap will only read from the cache data.
>> + */
>> +static bool sgm3804_readable_reg(struct device *dev, unsigned int reg)
>> +{
>> +	return false;
>> +}
> 
> Non-readable registers can't be volatile, volatile means always do a
> read.

Right I overlooked volatile and indeed it's incorrect.

> 
>> +static int sgm3804_enable(struct regulator_dev *rdev)
>> +{
>> +	struct sgm3804_data *ctx = rdev->reg_data;
>> +	int ret;
>> +
>> +	ret = gpiod_set_value(ctx->gpios[rdev_get_id(rdev)], 1);
>> +	if (ret)
>> +		return ret;
> 
> This could use _cansleep() for wider interoperability.

Good idea

> 
>> +
>> +	ret = regmap_write(ctx->regmap, rdev->desc->vsel_reg,
>> +			   ctx->sel[rdev_get_id(rdev)]);
>> +	if (ret)
>> +		goto err;
>> +
>> +	ret = regulator_set_active_discharge_regmap(rdev,
>> +						    ctx->active_discharge[rdev_get_id(rdev)]);
>> +	if (ret)
>> +		goto err;
> 
> I'm still not clear why this isn't doing a regcache sync instead of
> writing things out individually.

OK indeed I misunderstood you comment, fully switched to cache_only/cache_sync
which is cleaner and simpler.

> 
>> +		ctx->gpios[i] = devm_gpiod_get_index(dev, "enable",
>> +						     i, GPIOD_OUT_LOW);
>> +		if (IS_ERR(ctx->gpios[i]))
>> +			return dev_err_probe(dev, PTR_ERR(ctx->gpios[i]),
>> +					"failed to get enable GPIO %d\n", i);
> 
> Perhaps use GPIOD_ASIS for a smoother handover?

Done

Thanks,
Neil