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

[Mark Brown <broonie@kernel.org>]

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

On Tue, Apr 28, 2026 at 03:52:06PM +0200, Neil Armstrong wrote:
> From: KancyJoe <kancy2333@outlook.com>
> 
> 
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> Signed-off-by: KancyJoe <kancy2333@outlook.com>

Your signoff should appear at the bottom of the list here.

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

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

> +MODULE_DESCRIPTION("SGMicro SGM3804 regulator Driver");
> +MODULE_AUTHOR("Kancy Joe <kancy2333@outlook.com>");
> +MODULE_LICENSE("GPL");

More author information here not looking joined up.

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