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

Neil Armstrong
2026-04-30T13:28:47Z

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