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

[Mark Brown <broonie@kernel.org>]

[-- 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 --]