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

[Neil Armstrong <neil.armstrong@linaro.org>]

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

Right

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

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@outlook.com>");
>> +MODULE_LICENSE("GPL");
> 
> More author information here not looking joined up.

I'll add my entry.

Neil