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

[Neil Armstrong]

On 4/29/26 02:37, Mark Brown wrote:
> On Tue, Apr 28, 2026 at 03:52:06PM +0200, Neil Armstrong wrote:
> > From: KancyJoe <kancy2333@xxxxxxxxxxx>
> >
> >
> > Signed-off-by: Neil Armstrong <neil.armstrong@xxxxxxxxxx>
> > Signed-off-by: KancyJoe <kancy2333@xxxxxxxxxxx>
>
> Your signoff should appear at the bottom of the list here.

Right

> > @@ -0,0 +1,280 @@
> > +// 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 a C++ one so things look more
> intentional.  The authorship overall appears a bit confused?

Sorry, what's confusing here ?

> > +/*
> > + * The registers are only writable when the gpio is enabled, so we
> > + * can't use the regulator regmap helpers & internal gpio handling
> > + * so we need to track the state and apply the state at enable time.
> > + */
> > +struct sgm3804_data {
> > +	struct regmap *regmap;
> > +	bool active_discharge[SGM3804_RAIL_COUNT];
> > +	unsigned int sel[SGM3804_RAIL_COUNT];
> > +	struct gpio_desc *gpios[SGM3804_RAIL_COUNT];
> > +};
>
> > +static int sgm3804_enable(struct regulator_dev *rdev)
> > +{
>
> > +       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)]);
>
> It seems like this should be a regcache sync then only the enable and
> disable operations need to be custom?  There is a register cache and it
> covers these registers, without it the _active_discharge wouldn't work
> since it does a regmap_update_bits() and all the registers are marked
> unreadable.

I added regcache defaults with the default hardware values so it uses the
cache default to do regmap_update_bits(), and yes none registers can be read
so it uses the cache.

> > +MODULE_DESCRIPTION("SGMicro SGM3804 regulator Driver");
> > +MODULE_AUTHOR("Kancy Joe <kancy2333@xxxxxxxxxxx>");
> > +MODULE_LICENSE("GPL");
>
> More author information here not looking joined up.

I'll add my entry.

Neil