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

Mark Brown
2026-04-30T10:34:13Z

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

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.

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

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

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

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

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

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