Re: [PATCH 2/2] regulator: sgm3804: add SGMicro SGM3804 charge-pump regulator driver

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

Hi Alexandre,

Le 10/05/2026 à 18:45, Alexandre Hamamdjian via B4 Relay a écrit :
> From: Alexandre Hamamdjian <azkali.limited@gmail.com>
> 
> Add a driver for the SGMicro SGM3804, an I2C-controlled positive and
> negative output charge-pump regulator. The chip is typically used to
> generate the AVDD/AVEE rails of display panels, and is for example
> present on the Ayaneo Pocket DS handheld where it powers the panel.
> 
> The driver exposes a single 5V regulator and uses two reset GPIOs to
> sequence the positive and negative outputs. Initialisation values for
> the charge-pump configuration registers are written on enable, and the
> GPIOs are dropped on disable.
> 
> Co-developed-by: Philippe Simons <simons.philippe@gmail.com>
> Signed-off-by: Philippe Simons <simons.philippe@gmail.com>
> Signed-off-by: Alexandre Hamamdjian <azkali.limited@gmail.com>
> ---
>   drivers/regulator/Kconfig             |  11 +++
>   drivers/regulator/Makefile            |   1 +
>   drivers/regulator/sgm3804-regulator.c | 164 ++++++++++++++++++++++++++++++++++
>   3 files changed, 176 insertions(+)
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index d71dac9436e3..e1adb0bda75d 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -1490,6 +1490,17 @@ config REGULATOR_SC2731
>   	  This driver provides support for the voltage regulators on the
>   	  SC2731 PMIC.
>   
> +config REGULATOR_SGM3804
> +	tristate "SGMicro sgm3804 voltage regulator"
> +	depends on I2C && OF
> +	help
> +	  This driver supports the SGMicro SGM3804 I2C-controlled positive
> +	  and negative output charge-pump regulator, commonly used to supply
> +	  AVDD/AVEE rails to display panels. Two reset GPIOs are used to
> +	  sequence the positive and negative outputs.
> +
> +	  Say M here to build the driver as a module called sgm3804-regulator.
> +
>   config REGULATOR_SKY81452
>   	tristate "Skyworks Solutions SKY81452 voltage regulator"
>   	depends on MFD_SKY81452
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 35639f3115fd..98ecbbc3c6b7 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -172,6 +172,7 @@ obj-$(CONFIG_REGULATOR_S2MPA01) += s2mpa01.o
>   obj-$(CONFIG_REGULATOR_S2MPS11) += s2mps11.o
>   obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
>   obj-$(CONFIG_REGULATOR_SC2731) += sc2731-regulator.o
> +obj-$(CONFIG_REGULATOR_SGM3804) += sgm3804-regulator.o
>   obj-$(CONFIG_REGULATOR_SKY81452) += sky81452-regulator.o
>   obj-$(CONFIG_REGULATOR_SLG51000) += slg51000-regulator.o
>   obj-$(CONFIG_REGULATOR_SPACEMIT_P1) += spacemit-p1.o
> diff --git a/drivers/regulator/sgm3804-regulator.c b/drivers/regulator/sgm3804-regulator.c
> new file mode 100644
> index 000000000000..c52f0596acf3
> --- /dev/null
> +++ b/drivers/regulator/sgm3804-regulator.c
> @@ -0,0 +1,164 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#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>
> +
> +struct sgm3804_data {
> +	struct regmap *regmap;
> +	struct gpio_desc *reset_gpio[2];
> +	bool enabled;
> +};
> +
> +static const struct regmap_config sgm3804_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0x03,

The registers are only writable

> +};
> +
> +static int sgm3804_enable(struct regulator_dev *rdev)
> +{
> +	struct sgm3804_data *data = rdev_get_drvdata(rdev);
> +	struct regmap *regmap = data->regmap;
> +	int ret = 0;
> +
> +	/* Set reset GPIO high to enable the device if available */
> +	if (data->reset_gpio[0])
> +		gpiod_set_value_cansleep(data->reset_gpio[0], 1);
> +
> +	if (data->reset_gpio[1])
> +		gpiod_set_value_cansleep(data->reset_gpio[1], 1);
> +
> +	ret |= regmap_write(regmap, 0x00, 0x0c);
> +	ret |= regmap_write(regmap, 0x01, 0x0c);

According to the spec, writing C enables 5.2V on both outputs.

> +	ret |= regmap_write(regmap, 0x03, 0x03);

And this enables the rail discharge on both outputs

> +	if (ret) {
> +		dev_err(rdev->dev.parent,
> +			"Failed to enable SGM3804 regulator\n");
> +		return ret;
> +	}
> +
> +	data->enabled = true;
> +	return 0;
> +}
> +
> +static int sgm3804_disable(struct regulator_dev *rdev)
> +{
> +	struct sgm3804_data *data = rdev_get_drvdata(rdev);
> +
> +	if (data->reset_gpio[0])
> +		gpiod_set_value_cansleep(data->reset_gpio[0], 0);
> +
> +	if (data->reset_gpio[1])
> +		gpiod_set_value_cansleep(data->reset_gpio[1], 0);
> +
> +	data->enabled = false;
> +	return 0;
> +}
> +
> +static int sgm3804_is_enabled(struct regulator_dev *rdev)
> +{
> +	struct sgm3804_data *data = rdev_get_drvdata(rdev);
> +
> +	return data->enabled;

This should use the gpio status instead

> +}
> +
> +static int sgm3804_get_voltage(struct regulator_dev *rdev)
> +{
> +	return 5000000;

So this is wrong

> +}
> +
> +static const struct regulator_ops sgm3804_ops = {
> +	.enable = sgm3804_enable,
> +	.disable = sgm3804_disable,
> +	.is_enabled = sgm3804_is_enabled,
> +	.get_voltage = sgm3804_get_voltage,
> +};
> +
> +static const struct regulator_desc sgm3804_reg = {
> +	.name = "SGM3804",
> +	.id = 0,
> +	.ops = &sgm3804_ops,
> +	.type = REGULATOR_VOLTAGE,
> +	.n_voltages = 1,
> +	.min_uV = 5000000,
> +	.owner = THIS_MODULE,
> +};

This IC supports programming both outputs independently from
2,4V to 5,5v and the discharge rail enable is configurable.

My implementation handles all that:
https://lore.kernel.org/all/20260506-topic-sm8650-ayaneo-pocket-s2-sgm3804-v4-2-7495e7905693@linaro.org/

> +
> +static int sgm3804_i2c_probe(struct i2c_client *i2c)
> +{
> +	struct device *dev = &i2c->dev;
> +	struct regulator_config config = {};
> +	struct regulator_dev *rdev;
> +	struct sgm3804_data *data;
> +	int error;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->regmap = devm_regmap_init_i2c(i2c, &sgm3804_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(dev, PTR_ERR(data->regmap),
> +				     "failed to init regmap\n");
> +
> +	/* Get reset-gpio from device tree */
> +	data->reset_gpio[0] =
> +		devm_gpiod_get_index(dev, "reset", 0, GPIOD_OUT_HIGH);
> +	if (IS_ERR(data->reset_gpio[0]))
> +		return dev_err_probe(dev, PTR_ERR(data->reset_gpio[0]),
> +				     "failed to get first reset GPIO\n");
> +
> +	data->reset_gpio[1] =
> +		devm_gpiod_get_index(dev, "reset", 1, GPIOD_OUT_HIGH);
> +	if (IS_ERR(data->reset_gpio[1]))
> +		dev_warn(dev, "failed to get second reset GPIO\n");
> +
> +	config.dev = dev;
> +	config.regmap = data->regmap;
> +	config.driver_data = data;
> +	config.of_node = dev->of_node;
> +	config.init_data =
> +		of_get_regulator_init_data(dev, dev->of_node, &sgm3804_reg);
> +	if (!config.init_data)
> +		return -ENOMEM;
> +	data->enabled = false;
> +	rdev = devm_regulator_register(dev, &sgm3804_reg, &config);
> +	if (IS_ERR(rdev)) {
> +		error = PTR_ERR(rdev);
> +		dev_err(dev, "Failed to register SGM3804 regulator: %d\n",
> +			error);
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id sgm3804_i2c_id[] = { { "sgm3804" }, {} };
> +MODULE_DEVICE_TABLE(i2c, sgm3804_i2c_id);
> +
> +static const struct of_device_id sgm3804_i2c_of_match[] = {
> +	{ .compatible = "sgmicro,sgm3804" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, sgm3804_i2c_of_match);
> +
> +static struct i2c_driver sgm3804_regulator_driver = {
> +	.driver = {
> +		.name = "sgm3804",
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> +		.of_match_table = sgm3804_i2c_of_match,
> +	},
> +	.probe = sgm3804_i2c_probe,
> +	.id_table = sgm3804_i2c_id,
> +};
> +
> +module_i2c_driver(sgm3804_regulator_driver);
> +
> +MODULE_DESCRIPTION("SGMicro sgm3804 regulator Driver");
> +MODULE_AUTHOR("Kancy Joe <kancy2333@outlook.com>");

This driver was originally written by Kancy Joe, and I derived my implementation
from the exact same driver, and I kept his authorship & Copyright and
he agreed to me I take over his driver to upstream a complete implementation.

Neil

> +MODULE_LICENSE("GPL");
>