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

[Neil Armstrong]

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@xxxxxxxxxxx>
> > + * Copyright (C) 2026 Linaro Limited
> > + * Author: Neil Armstrong <neil.armstrong@xxxxxxxxxx>
> > + */
>
> 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@xxxxxxxxxx>)

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