[PATCH 0/2] regulator: add support for SGM3804 Dual Output driver

[PATCH 0/2] regulator: add support for SGM3804 Dual Output driver

[Neil Armstrong]

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.

The SGM3804 is powered by the enable GPIO pins inputs and only
supports I2C write messages. Thus we can't use the regmap
helpers directly and we need to cache the selector and
rail discharge state then setup the rails once the gpio
is enabled.
In order add flexibility, the regmap cache is enabled.

This regulator is used to provide vsn and vsn power to the
Ayaneo Pocket S2 dual-DSI LCD panel.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
KancyJoe (1):
      regulator: add SGM3804 Dual Output driver

Neil Armstrong (1):
      dt-bindings: regulator: document the SGM3804 Dual Output regulator

 .../bindings/regulator/sgmicro,sgm3804.yaml        |  77 ++++++
 drivers/regulator/Kconfig                          |   6 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/sgm3804-regulator.c              | 280 +++++++++++++++++++++
 4 files changed, 364 insertions(+)
---
base-commit: 39704f00f747aba3144289870b5fd8ac230a9aaf
change-id: 20260428-topic-sm8650-ayaneo-pocket-s2-sgm3804-8764fbb72eb7

Best regards,
--  
Neil Armstrong <neil.armstrong@linaro.org>

[PATCH 1/2] dt-bindings: regulator: document the SGM3804 Dual Output regulator

[Neil Armstrong]

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

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 .../bindings/regulator/sgmicro,sgm3804.yaml        | 77 ++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/sgmicro,sgm3804.yaml b/Documentation/devicetree/bindings/regulator/sgmicro,sgm3804.yaml
new file mode 100644
index 000000000000..e75684e910ff
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/sgmicro,sgm3804.yaml
@@ -0,0 +1,77 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/sgmicro,sgm3804.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SG Micro SGM3804 Single Inductor Dual Output Buck/Boost Converter
+
+maintainers:
+  - Neil Armstrong <neil.armstrong@linaro.org>
+
+description:
+  The SGM3804 is a dual voltage regulator, designed to support positive/negative
+  supply for driving LCD panels. It support software-configurable output
+  switching. The output voltages can be programmed via an I2C compatible interface.
+
+properties:
+  compatible:
+    const: sgmicro,sgm3804
+
+  reg:
+    maxItems: 1
+
+  enable-gpios:
+    maxItems: 2
+    description:
+      GPIO specifiers to enable the positive and negative outputs.
+
+  vin-supply: true
+
+patternProperties:
+  "^(pos|neg)$":
+    type: object
+    $ref: regulator.yaml#
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - enable-gpios
+  - pos
+  - neg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        regulator@3e {
+            compatible = "sgmicro,sgm3804";
+            reg = <0x3e>;
+
+            vin-supply = <&vin_reg>;
+
+            enable-gpios = <&gpio 17 GPIO_ACTIVE_LOW>,
+                           <&gpio 18 GPIO_ACTIVE_LOW>;
+
+            pos {
+                regulator-name = "outpos";
+                regulator-min-microvolt = <5000000>;
+                regulator-max-microvolt = <5000000>;
+            };
+
+            neg {
+                regulator-name = "outneg";
+                regulator-min-microvolt = <5000000>;
+                regulator-max-microvolt = <5000000>;
+            };
+        };
+    };
+...
+

-- 
2.34.1

Re: [PATCH 1/2] dt-bindings: regulator: document the SGM3804 Dual Output regulator

[Rob Herring (Arm)]

On Tue, 28 Apr 2026 15:52:05 +0200, Neil Armstrong wrote:
> Document 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.
> 
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  .../bindings/regulator/sgmicro,sgm3804.yaml        | 77 ++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/regulator/sgmicro,sgm3804.example.dtb: regulator@3e (sgmicro,sgm3804): enable-gpios: [[4294967295, 17, 1], [4294967295, 18, 1]] is too long
	from schema $id: http://devicetree.org/schemas/gpio/gpio-consumer-common.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.kernel.org/project/devicetree/patch/20260428-topic-sm8650-ayaneo-pocket-s2-sgm3804-v1-1-1d8dc7620256@linaro.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 1/2] dt-bindings: regulator: document the SGM3804 Dual Output regulator

[Mark Brown]

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

On Tue, Apr 28, 2026 at 03:52:05PM +0200, Neil Armstrong wrote:
> Document 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.

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.

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

Re: [PATCH 1/2] dt-bindings: regulator: document the SGM3804 Dual Output regulator

[Mark Brown]

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

On Tue, Apr 28, 2026 at 03:52:05PM +0200, Neil Armstrong wrote:
> Document 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.

> +  enable-gpios:
> +    maxItems: 2
> +    description:
> +      GPIO specifiers to enable the positive and negative outputs.

The driver requires both to be provided, either it should relax it's
requirements during probe() and allow only one of the regulators to be
instantiated or if that's not a realistic setup the binding should set
minItems too.

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

Re: [PATCH 1/2] dt-bindings: regulator: document the SGM3804 Dual Output regulator

[Neil Armstrong]

On 4/29/26 02:38, Mark Brown wrote:
> On Tue, Apr 28, 2026 at 03:52:05PM +0200, Neil Armstrong wrote:
>> Document 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.
> 
>> +  enable-gpios:
>> +    maxItems: 2
>> +    description:
>> +      GPIO specifiers to enable the positive and negative outputs.
> 
> The driver requires both to be provided, either it should relax it's
> requirements during probe() and allow only one of the regulators to be
> instantiated or if that's not a realistic setup the binding should set
> minItems too.


It's a mismatch on my side, it needs both gpios, I'll fix it.

Neil

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

[Neil Armstrong]

From: KancyJoe <kancy2333@outlook.com>

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.

The SGM3804 is powered by the enable GPIO pins inputs and only
supports I2C write messages. Thus we can't use the regmap
helpers directly and we need to cache the selector and
rail discharge state then setup the rails once the gpio
is enabled.
In order add flexibility, the regmap cache is enabled.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
Signed-off-by: KancyJoe <kancy2333@outlook.com>
---
 drivers/regulator/Kconfig             |   6 +
 drivers/regulator/Makefile            |   1 +
 drivers/regulator/sgm3804-regulator.c | 280 ++++++++++++++++++++++++++++++++++
 3 files changed, 287 insertions(+)

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index e8002526cfb0..7a2ba22716a8 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1590,6 +1590,12 @@ config REGULATOR_SUN20I
 	help
 	  This driver supports the internal LDOs in the Allwinner D1 SoC.
 
+config REGULATOR_SGM3804
+	tristate "SGMicro SGM3804 voltage regulator"
+	depends on I2C && OF
+	help
+	  This driver supports SGMicro SGM3804 dual-output voltage regulator.
+
 config REGULATOR_SY7636A
 	tristate "Silergy SY7636A voltage regulator"
 	depends on MFD_SY7636A
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 35639f3115fd..7c345c7caee7 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -181,6 +181,7 @@ obj-$(CONFIG_REGULATOR_STM32_PWR) += stm32-pwr.o
 obj-$(CONFIG_REGULATOR_STPMIC1) += stpmic1_regulator.o
 obj-$(CONFIG_REGULATOR_STW481X_VMMC) += stw481x-vmmc.o
 obj-$(CONFIG_REGULATOR_SUN20I) += sun20i-regulator.o
+obj-$(CONFIG_REGULATOR_SGM3804) += sgm3804-regulator.o
 obj-$(CONFIG_REGULATOR_SY7636A) += sy7636a-regulator.o
 obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o
 obj-$(CONFIG_REGULATOR_SY8824X) += sy8824x.o
diff --git a/drivers/regulator/sgm3804-regulator.c b/drivers/regulator/sgm3804-regulator.c
new file mode 100644
index 000000000000..5d496e32eaf1
--- /dev/null
+++ b/drivers/regulator/sgm3804-regulator.c
@@ -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>
+ */
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/gpio/consumer.h>
+
+#define SGM3804_POS_RAIL_VOLTAGE_REG	0x0
+#define SGM3804_NEG_RAIL_VOLTAGE_REG	0x1
+#define SGM3804_RAIL_DISCHARGE_REG	0x3
+
+#define RAIL_VOLTAGE_MASK	GENMASK(5, 0)
+
+#define POS_RAIL_DISCHARGE_EN	BIT(1)
+#define NEG_RAIL_DISCHARGE_EN	BIT(0)
+
+#define RAIL_VOLTAGE_INVALID		RAIL_VOLTAGE_MASK
+#define RAIL_DISCHARGE_REG_DEFAULT	(POS_RAIL_DISCHARGE_EN | NEG_RAIL_DISCHARGE_EN)
+
+#define SGM3804_VOLTAGES_COUNT	40
+
+enum {
+	SGM3804_POS_RAIL = 0,
+	SGM3804_NEG_RAIL,
+	SGM3804_RAIL_COUNT,
+};
+
+/*
+ * 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 const struct linear_range sgm3804_voltages[] = {
+	REGULATOR_LINEAR_RANGE(2400000, 0x20, 0x2f, 100000),
+	REGULATOR_LINEAR_RANGE(4000000, 0x00, 0x17, 100000),
+};
+
+static const struct reg_default sgm3804_reg_defaults[] = {
+	{ SGM3804_POS_RAIL_VOLTAGE_REG, RAIL_VOLTAGE_INVALID },
+	{ SGM3804_NEG_RAIL_VOLTAGE_REG, RAIL_VOLTAGE_INVALID },
+	{ SGM3804_RAIL_DISCHARGE_REG, RAIL_DISCHARGE_REG_DEFAULT },
+};
+
+/* Registers are writable & volatile */
+static bool sgm3804_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case SGM3804_POS_RAIL_VOLTAGE_REG:
+	case SGM3804_NEG_RAIL_VOLTAGE_REG:
+	case SGM3804_RAIL_DISCHARGE_REG:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool sgm3804_readable_reg(struct device *dev, unsigned int reg)
+{
+	return false;
+}
+
+static const struct regmap_config sgm3804_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x03,
+	.writeable_reg = sgm3804_writeable_reg,
+	.readable_reg = sgm3804_readable_reg,
+	.volatile_reg = sgm3804_writeable_reg,
+	.cache_type = REGCACHE_MAPLE,
+	.reg_defaults = sgm3804_reg_defaults,
+	.num_reg_defaults = ARRAY_SIZE(sgm3804_reg_defaults),
+};
+
+static int sgm3804_set_voltage_sel(struct regulator_dev *rdev, unsigned int sel)
+{
+	struct sgm3804_data *ctx = rdev->reg_data;
+
+	ctx->sel[rdev_get_id(rdev)] = sel;
+
+	return 0;
+}
+
+static int sgm3804_get_voltage_sel(struct regulator_dev *rdev)
+{
+	struct sgm3804_data *ctx = rdev->reg_data;
+
+	/* Force setting a voltage on probe */
+	if (ctx->sel[rdev_get_id(rdev)] == RAIL_VOLTAGE_INVALID)
+		return -ENOTRECOVERABLE;
+
+	return ctx->sel[rdev_get_id(rdev)];
+}
+
+static int sgm3804_set_active_discharge(struct regulator_dev *rdev, bool enable)
+{
+	struct sgm3804_data *ctx = rdev->reg_data;
+
+	ctx->active_discharge[rdev_get_id(rdev)] = enable;
+
+	return 0;
+}
+
+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;
+
+	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;
+
+	return 0;
+
+err:
+	gpiod_set_value(ctx->gpios[rdev_get_id(rdev)], 0);
+	return ret;
+}
+
+static int sgm3804_disable(struct regulator_dev *rdev)
+{
+	struct sgm3804_data *ctx = rdev->reg_data;
+
+	return gpiod_set_value(ctx->gpios[rdev_get_id(rdev)], 0);
+}
+
+static int sgm3804_is_enabled(struct regulator_dev *rdev)
+{
+	struct sgm3804_data *ctx = rdev->reg_data;
+
+	return gpiod_get_value(ctx->gpios[rdev_get_id(rdev)]);
+}
+
+static const struct regulator_ops sgm3804_ops = {
+	.list_voltage = regulator_list_voltage_linear_range,
+	.map_voltage = regulator_map_voltage_linear_range,
+	.set_voltage_sel = sgm3804_set_voltage_sel,
+	.get_voltage_sel = sgm3804_get_voltage_sel,
+	.set_active_discharge = sgm3804_set_active_discharge,
+	.enable = sgm3804_enable,
+	.disable = sgm3804_disable,
+	.is_enabled = sgm3804_is_enabled,
+};
+
+static const struct regulator_desc sgm3804_regulator_desc[] = {
+	/* Positive Output */
+	{
+		.name = "pos",
+		.of_match = "pos",
+		.supply_name = "vin",
+		.id = SGM3804_POS_RAIL,
+		.ops = &sgm3804_ops,
+		.type = REGULATOR_VOLTAGE,
+		.linear_ranges = sgm3804_voltages,
+		.n_linear_ranges = ARRAY_SIZE(sgm3804_voltages),
+		.n_voltages = SGM3804_VOLTAGES_COUNT,
+		.vsel_reg = SGM3804_POS_RAIL_VOLTAGE_REG,
+		.active_discharge_on = POS_RAIL_DISCHARGE_EN,
+		.active_discharge_mask = POS_RAIL_DISCHARGE_EN,
+		.active_discharge_reg = SGM3804_RAIL_DISCHARGE_REG,
+		.enable_time = 40000,
+		.owner = THIS_MODULE,
+	},
+	/* Negative Output */
+	{
+		.name = "neg",
+		.of_match = "neg",
+		.supply_name = "vin",
+		.id = SGM3804_NEG_RAIL,
+		.ops = &sgm3804_ops,
+		.type = REGULATOR_VOLTAGE,
+		.linear_ranges = sgm3804_voltages,
+		.n_linear_ranges = ARRAY_SIZE(sgm3804_voltages),
+		.n_voltages = SGM3804_VOLTAGES_COUNT,
+		.vsel_reg = SGM3804_NEG_RAIL_VOLTAGE_REG,
+		.active_discharge_on = NEG_RAIL_DISCHARGE_EN,
+		.active_discharge_mask = NEG_RAIL_DISCHARGE_EN,
+		.active_discharge_reg = SGM3804_RAIL_DISCHARGE_REG,
+		.enable_time = 40000,
+		.owner = THIS_MODULE,
+	},
+};
+
+static int sgm3804_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct sgm3804_data *ctx;
+	int i;
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->regmap = devm_regmap_init_i2c(i2c, &sgm3804_regmap_config);
+	if (IS_ERR(ctx->regmap))
+		return dev_err_probe(dev, PTR_ERR(ctx->regmap),
+				     "failed to init regmap\n");
+
+	/* Set default values */
+	for (i = 0; i < ARRAY_SIZE(sgm3804_regulator_desc); i++) {
+		ctx->active_discharge[i] = true;
+		ctx->sel[i] = RAIL_VOLTAGE_INVALID;
+
+		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);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(sgm3804_regulator_desc); i++) {
+		struct regulator_config config = { };
+		struct regulator_dev *rdev;
+
+		config.dev = dev;
+		config.regmap = ctx->regmap;
+		config.of_node = dev->of_node;
+		config.driver_data = ctx;
+		rdev = devm_regulator_register(dev, &sgm3804_regulator_desc[i],
+					       &config);
+		if (IS_ERR(rdev))
+			return dev_err_probe(dev, PTR_ERR(rdev),
+					     "failed to register regulator %d\n", i);
+	}
+
+	return 0;
+}
+
+static const struct i2c_device_id sgm3804_id[] = {
+	{ "sgm3804" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, sgm3804_id);
+
+static const struct of_device_id sgm3804_of_match[] = {
+	{ .compatible = "sgmicro,sgm3804" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sgm3804_of_match);
+
+static struct i2c_driver sgm3804_regulator_driver = {
+	.driver = {
+		.name = "sgm3804",
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+		.of_match_table = sgm3804_of_match,
+	},
+	.probe = sgm3804_probe,
+	.id_table = sgm3804_id,
+};
+
+module_i2c_driver(sgm3804_regulator_driver);
+
+MODULE_DESCRIPTION("SGMicro SGM3804 regulator Driver");
+MODULE_AUTHOR("Kancy Joe <kancy2333@outlook.com>");
+MODULE_LICENSE("GPL");

-- 
2.34.1

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

[Mark Brown]

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

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

[Mark Brown]

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

On Wed, Apr 29, 2026 at 09:37:49AM +0900, Mark Brown wrote:
> On Tue, Apr 28, 2026 at 03:52:06PM +0200, Neil Armstrong wrote:

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

Actually I'm not clear how the cache works here since everything
writable is also marked as volatile but no register is readable.

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

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

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

[Mark Brown]

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

On Wed, Apr 29, 2026 at 10:05:33AM +0200, Neil Armstrong wrote:
> On 4/29/26 02:37, Mark Brown wrote:
> > On Tue, Apr 28, 2026 at 03:52:06PM +0200, Neil Armstrong wrote:

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

At various times both you and Kancy Joe are listed as authors of the
driver, but rarely both of you simulataneously.  I can't tell what your
roles were here.

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

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

[Neil Armstrong]

On 4/30/26 03:06, Mark Brown wrote:
> On Wed, Apr 29, 2026 at 10:05:33AM +0200, Neil Armstrong wrote:
>> On 4/29/26 02:37, Mark Brown wrote:
>>> On Tue, Apr 28, 2026 at 03:52:06PM +0200, Neil Armstrong wrote:
> 
>>>> +// 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 ?
> 
> At various times both you and Kancy Joe are listed as authors of the
> driver, but rarely both of you simulataneously.  I can't tell what your
> roles were here.

Oh yes so I'll add my MODULE_AUTHOR() tag aswell then

Neil