[PATCH 0/2] regulator: Add TPS65185

[PATCH 0/2] regulator: Add TPS65185

[Andreas Kemnade]

Add a driver for the TPS65185 regulator which provides the
comparatively high voltages needed for electronic paper displays.

Datasheet for the TPS65185 is at https://www.ti.com/lit/gpn/tps65185

To simplify things, include the hwmon part directly which is only
one temperature sensorno other functions besides regulators in this chip.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
Andreas Kemnade (2):
      dt-bindings: regulator: Add TI TPS65185
      regulator: Add TPS65185 driver

 .../devicetree/bindings/regulator/ti,tps65185.yaml |  98 +++
 drivers/regulator/Kconfig                          |  11 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/tps65185.c                       | 658 +++++++++++++++++++++
 4 files changed, 768 insertions(+)
---
base-commit: 9448598b22c50c8a5bb77a9103e2d49f134c9578
change-id: 20251222-tps65185-submit-272cc756e0ad

Best regards,
--  
Andreas Kemnade <andreas@kemnade.info>

[PATCH 1/2] dt-bindings: regulator: Add TI TPS65185

[Andreas Kemnade]

Document the TPS65185. GPIO names are same as in the datasheet except for
the PWRUP pad which is described as "enable". That pin is optional because
the rising edge corresponds to setting one register bit and falling edge
to another register bit.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 .../devicetree/bindings/regulator/ti,tps65185.yaml | 98 ++++++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/ti,tps65185.yaml b/Documentation/devicetree/bindings/regulator/ti,tps65185.yaml
new file mode 100644
index 000000000000..91edd0d5f8c0
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/ti,tps65185.yaml
@@ -0,0 +1,98 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/ti,tps65185.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI TPS65185 Power Management Integrated Circuit
+
+maintainers:
+  - Andreas Kemnade <andreas@kemnade.info>
+
+description:
+  TPS65185 is a Power Management IC to provide Power for EPDs with one 3.3V
+  switch, 2 symmetric LDOs behind 2 DC/DC converters, and one unsymmetric
+  regulator for a compensation voltage.
+
+properties:
+  compatible:
+    const: ti,tps65185
+
+  reg:
+    maxItems: 1
+
+  enable-gpios:
+    description:
+      PWRUP pin
+    maxItems: 1
+
+  pg-gpios:
+    maxItems: 1
+
+  vcom-ctrl-gpios:
+    maxItems: 1
+
+  wakeup-gpios:
+    maxItems: 1
+
+  vin-supply:
+    description:
+      Supply for the whole chip. Some vendor kernels and devicetrees
+      declare this as a non-existing GPIO named "pwrall".
+
+  interrupts:
+    maxItems: 1
+
+  regulators:
+    type: object
+    additionalProperties: false
+    patternProperties:
+      "^(vcom|vposneg|v3p3)$":
+        unevaluatedProperties: false
+        type: object
+        $ref: /schemas/regulator/regulator.yaml
+
+required:
+  - compatible
+  - reg
+  - pg-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        pmic@18 {
+            compatible = "ti,tps65185";
+            reg = <0x18>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&pinctrl_tps65185_gpio>;
+            pg-gpios = <&gpio2 7 GPIO_ACTIVE_HIGH>;
+            vcom-ctrl-gpios = <&gpio2 9 GPIO_ACTIVE_HIGH>;
+            enable-gpios = <&gpio2 8 GPIO_ACTIVE_HIGH>;
+            wakeup-gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>;
+            vin-supply = <&epdc_pmic_supply>;
+            interrupts-extended = <&gpio2 0 IRQ_TYPE_LEVEL_LOW>;
+
+            regulators {
+                vcom {
+                    regulator-name = "vcom";
+                };
+
+                vposneg {
+                    regulator-name = "vposneg";
+                    regulator-min-microvolt = <15000000>;
+                    regulator-max-microvolt = <15000000>;
+                };
+
+                v3p3 {
+                    regulator-name = "v3p3";
+                };
+            };
+        };
+    };

-- 
2.47.3

[PATCH 2/2] regulator: Add TPS65185 driver

[Andreas Kemnade]

Add a driver for the TPS65185 regulator. Implement handling of the various
gpio pins. Because the PWRUP (=enable) pin functionality can be achieved
by just using two bits instead, just ensure that it is set to a stable
value.
Implement the pair of symmetric LDOs as a single regulator because they
share a single voltage set register. As the VCOM regulator sits behind that
machinery, just define that one as a supply.
For simplicity, just add the temperature sensor (depending on external NTC)
directly.

There is a mechanism to measure some kick-back voltage during a defined EPD
operation, to calibrate the VCOM voltage setting and store that
non-volatile in the chip to be the power up default setup. That is not
implemented yet in the driver, but that also means that there is a
non-factory default value in these registers after power-up.

As there MSB of VCOM voltage setting is stored in a register together with
runtime-volatile bit, store that one separately.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/regulator/Kconfig    |  11 +
 drivers/regulator/Makefile   |   1 +
 drivers/regulator/tps65185.c | 658 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 670 insertions(+)

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index d2335276cce5..5c539782e48d 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1690,6 +1690,17 @@ config REGULATOR_TPS65132
 	  This driver supports TPS65132 single inductor - dual output
 	  power supply specifically designed for display panels.
 
+config REGULATOR_TPS65185
+	tristate "TI TPS65185 EPD regulator"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  This driver supports the TPS65185 voltage regulator chip
+	  which is used to provide power to Electronic Paper Displays
+	  so it is found in E-Book readers.
+	  If HWWON is enabled, it also provides temperature measurement.
+
+
 config REGULATOR_TPS65217
 	tristate "TI TPS65217 Power regulators"
 	depends on MFD_TPS65217
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 1beba1493241..240de94cd432 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -192,6 +192,7 @@ obj-$(CONFIG_REGULATOR_TPS65023) += tps65023-regulator.o
 obj-$(CONFIG_REGULATOR_TPS6507X) += tps6507x-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65086) += tps65086-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65090) += tps65090-regulator.o
+obj-$(CONFIG_REGULATOR_TPS65185) += tps65185.o
 obj-$(CONFIG_REGULATOR_TPS65217) += tps65217-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65218) += tps65218-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65219) += tps65219-regulator.o
diff --git a/drivers/regulator/tps65185.c b/drivers/regulator/tps65185.c
new file mode 100644
index 000000000000..3ef86b355094
--- /dev/null
+++ b/drivers/regulator/tps65185.c
@@ -0,0 +1,658 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright (C) 2025 Andreas Kemnade
+
+/* Datasheet: https://www.ti.com/lit/gpn/tps65185 */
+
+#include <linux/cleanup.h>
+#include <linux/completion.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/hwmon.h>
+#include <linux/pm_runtime.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regmap.h>
+
+#define TPS65185_REG_TMST_VALUE 0
+#define TPS65185_REG_ENABLE 1
+#define TPS65185_REG_VADJ 2
+#define TPS65185_REG_VCOM1 3
+#define TPS65185_REG_VCOM2 4
+#define TPS65185_REG_INT_EN1 5
+#define TPS65185_REG_INT_EN2 6
+#define TPS65185_REG_INT1 7
+#define TPS65185_REG_INT2 8
+#define TPS65185_REG_TMST1 0xd
+#define TPS65185_REG_TMST2 0xe
+#define TPS65185_REG_PG 0xf
+#define TPS65185_REG_REVID 0x10
+
+#define TPS65185_READ_THERM BIT(7)
+#define TPS65185_CONV_END BIT(5)
+
+#define TPS65185_ENABLE_ACTIVE BIT(7)
+#define TPS65185_ENABLE_STANDBY BIT(6)
+
+#define PGOOD_TIMEOUT_MSECS 200
+
+struct tps65185_data {
+	struct device *dev;
+	struct regmap *regmap;
+	struct regulator *vin_reg;
+	struct gpio_desc *pgood_gpio;
+	struct gpio_desc *pwrup_gpio;
+	struct gpio_desc *vcom_ctrl_gpio;
+	struct gpio_desc *wakeup_gpio;
+	struct completion pgood_completion;
+	int pgood_irq;
+	struct completion tmst_completion;
+	unsigned int vcom_msb;
+};
+
+static const struct hwmon_channel_info *tps65185_info[] = {
+	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
+	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
+	NULL
+};
+
+static int tps65185_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+			       u32 attr, int channel, long *temp)
+{
+	struct tps65185_data *data = dev_get_drvdata(dev);
+	unsigned int val;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(data->dev);
+	if (ret < 0)
+		return ret;
+
+	reinit_completion(&data->tmst_completion);
+	/* start acquisition */
+	regmap_update_bits(data->regmap, TPS65185_REG_TMST1,
+			   TPS65185_READ_THERM, TPS65185_READ_THERM);
+	wait_for_completion_timeout(&data->tmst_completion,
+				    msecs_to_jiffies(PGOOD_TIMEOUT_MSECS));
+	ret = regmap_read(data->regmap, TPS65185_REG_TMST1, &val);
+	if (!(val & TPS65185_CONV_END)) {
+		ret = -ETIMEDOUT;
+		goto out;
+	}
+	ret = regmap_read(data->regmap, TPS65185_REG_TMST_VALUE, &val);
+	if (ret)
+		goto out;
+
+	*temp = (s8)val * 1000;
+
+out:
+	pm_runtime_put_autosuspend(data->dev);
+
+	return ret;
+}
+
+static umode_t tps65185_hwmon_is_visible(const void *data,
+					 enum hwmon_sensor_types type,
+					 u32 attr, int channel)
+{
+	return 0444;
+}
+
+static const struct hwmon_ops tps65185_hwmon_ops = {
+	.is_visible = tps65185_hwmon_is_visible,
+	.read = tps65185_hwmon_read,
+};
+
+static const struct hwmon_chip_info tps65185_chip_info = {
+	.ops = &tps65185_hwmon_ops,
+	.info = tps65185_info,
+};
+
+static int tps65185_runtime_suspend(struct device *dev)
+{
+	int ret = 0;
+	struct tps65185_data *data = dev_get_drvdata(dev);
+
+	ret = regmap_read(data->regmap, TPS65185_REG_VCOM2, &data->vcom_msb);
+	if (ret)
+		return ret;
+
+	if (data->wakeup_gpio) {
+		ret = gpiod_set_value_cansleep(data->wakeup_gpio, 0);
+		if (ret)
+			return ret;
+	}
+
+	if (data->vin_reg) {
+		ret = regulator_disable(data->vin_reg);
+		if (ret)
+			goto reenable_wkup;
+	}
+
+	regcache_mark_dirty(data->regmap);
+
+	return 0;
+reenable_wkup:
+	if (data->wakeup_gpio)
+		gpiod_set_value_cansleep(data->wakeup_gpio, 1);
+
+	return ret;
+}
+
+static int tps65185_runtime_resume(struct device *dev)
+{
+	int ret = 0;
+	struct tps65185_data *data = dev_get_drvdata(dev);
+
+	if (data->vin_reg)
+		ret = regulator_enable(data->vin_reg);
+
+	if (ret)
+		return ret;
+
+	if (data->wakeup_gpio) {
+		ret = gpiod_set_value_cansleep(data->wakeup_gpio, 1);
+		usleep_range(2000, 4000);
+	}
+
+	if (ret)
+		goto resume_wkup_err;
+
+	ret = regcache_sync(data->regmap);
+	if (ret)
+		goto resume_sync_err;
+
+	ret = regmap_update_bits(data->regmap, TPS65185_REG_VCOM2,
+				 BIT(0), data->vcom_msb);
+	if (ret)
+		goto resume_sync_err;
+
+	return 0;
+
+resume_sync_err:
+	gpiod_set_value_cansleep(data->wakeup_gpio, 0);
+
+resume_wkup_err:
+	regulator_disable(data->vin_reg);
+	return ret;
+}
+
+static bool tps65185_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case TPS65185_REG_TMST_VALUE:
+	case TPS65185_REG_ENABLE:
+	case TPS65185_REG_VCOM2:
+	case TPS65185_REG_INT1:
+	case TPS65185_REG_INT2:
+	case TPS65185_REG_TMST1:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x10,
+	.cache_type = REGCACHE_MAPLE,
+	.volatile_reg = tps65185_volatile_reg,
+};
+
+static void disable_nopm(void *d)
+{
+	struct tps65185_data *data = d;
+
+	tps65185_runtime_suspend(data->dev);
+}
+
+static int tps65185_v3p3_enable(struct regulator_dev *rdev)
+{
+	struct tps65185_data *data = rdev_get_drvdata(rdev);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(data->dev);
+	if (ret < 0)
+		return ret;
+
+	ret = regulator_enable_regmap(rdev);
+	if (ret < 0)
+		pm_runtime_put_autosuspend(data->dev);
+
+	return ret;
+}
+
+static int tps65185_v3p3_disable(struct regulator_dev *rdev)
+{
+	struct tps65185_data *data = rdev_get_drvdata(rdev);
+	int ret;
+
+	ret = regulator_disable_regmap(rdev);
+	pm_runtime_put_autosuspend(data->dev);
+
+	return ret;
+}
+
+static int tps65185_v3p3_is_enabled(struct regulator_dev *rdev)
+{
+	struct tps65185_data *data = rdev_get_drvdata(rdev);
+	int ret;
+
+	if (pm_runtime_status_suspended(data->dev))
+		return 0;
+
+	ret = pm_runtime_resume_and_get(data->dev);
+	if (ret < 0)
+		return 0;
+
+	ret = regulator_is_enabled_regmap(rdev);
+
+	pm_runtime_put_autosuspend(data->dev);
+	return ret;
+}
+
+static const struct regulator_ops tps65185_v3p3ops = {
+	.list_voltage = regulator_list_voltage_linear,
+	.enable = tps65185_v3p3_enable,
+	.disable = tps65185_v3p3_disable,
+	.is_enabled = tps65185_v3p3_is_enabled,
+};
+
+static int tps65185_check_powergood(struct regulator_dev *rdev)
+{
+	struct tps65185_data *data = rdev_get_drvdata(rdev);
+
+	if (pm_runtime_status_suspended(data->dev))
+		return 0;
+
+	return gpiod_get_value_cansleep(data->pgood_gpio);
+}
+
+static int tps65185_vposneg_get_voltage_sel(struct regulator_dev *rdev)
+{
+	struct tps65185_data *data = rdev_get_drvdata(rdev);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(data->dev);
+	if (ret < 0)
+		return ret;
+
+	ret = regulator_get_voltage_sel_regmap(rdev);
+	if (ret < 0)
+		return ret;
+
+	pm_runtime_put_autosuspend(data->dev);
+
+	/* highest value is lowest voltage */
+	return 6 - ret;
+}
+
+static int tps65185_vposneg_set_voltage_sel(struct regulator_dev *rdev, unsigned int selector)
+{
+	struct tps65185_data *data = rdev_get_drvdata(rdev);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(data->dev);
+	if (ret < 0)
+		return ret;
+
+	/* highest value is lowest voltage */
+	ret = regulator_set_voltage_sel_regmap(rdev, 6 - selector);
+	pm_runtime_put_autosuspend(data->dev);
+
+	return ret;
+}
+
+static irqreturn_t pgood_handler(int irq, void *dev_id)
+{
+	struct tps65185_data *data = dev_id;
+
+	complete(&data->pgood_completion);
+
+	return IRQ_HANDLED;
+}
+
+static int tps65185_vposneg_enable(struct regulator_dev *rdev)
+{
+	struct tps65185_data *data = rdev_get_drvdata(rdev);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(data->dev);
+	if (ret < 0)
+		return ret;
+
+	reinit_completion(&data->pgood_completion);
+	regmap_update_bits(data->regmap, TPS65185_REG_ENABLE,
+			   TPS65185_ENABLE_ACTIVE,
+			   TPS65185_ENABLE_ACTIVE);
+	dev_dbg(data->dev, "turning on...");
+	wait_for_completion_timeout(&data->pgood_completion,
+				    msecs_to_jiffies(PGOOD_TIMEOUT_MSECS));
+	dev_dbg(data->dev, "turned on");
+	if (gpiod_get_value_cansleep(data->pgood_gpio) != 1) {
+		pm_runtime_put_autosuspend(data->dev);
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static int tps65185_vposneg_disable(struct regulator_dev *rdev)
+{
+	struct tps65185_data *data = rdev_get_drvdata(rdev);
+
+	regmap_update_bits(data->regmap, TPS65185_REG_ENABLE,
+			   TPS65185_ENABLE_STANDBY,
+			   TPS65185_ENABLE_STANDBY);
+	pm_runtime_put_autosuspend(data->dev);
+	return 0;
+}
+
+static int tps65185_vcom_set_voltage_sel(struct regulator_dev *rdev, unsigned int selector)
+{
+	struct tps65185_data *data = rdev_get_drvdata(rdev);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(data->dev);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_update_bits(data->regmap, TPS65185_REG_VCOM2, BIT(0), selector >> 8);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(data->regmap, TPS65185_REG_VCOM1, selector & 0xFF);
+	pm_runtime_put_autosuspend(data->dev);
+	return ret;
+}
+
+static int tps65185_vcom_get_voltage_sel(struct regulator_dev *rdev)
+{
+	struct tps65185_data *data = rdev_get_drvdata(rdev);
+	int ret;
+	unsigned int sel, sel2;
+
+	ret = pm_runtime_resume_and_get(data->dev);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_read(data->regmap, TPS65185_REG_VCOM1, &sel);
+	if (ret < 0)
+		goto out;
+
+	ret = regmap_read(data->regmap, TPS65185_REG_VCOM2, &sel2);
+	if (ret < 0)
+		goto out;
+
+	if (sel2 & BIT(0))
+		sel |= 0x100;
+
+	ret = sel;
+out:
+	pm_runtime_put_autosuspend(data->dev);
+	return ret;
+}
+
+static int tps65185_vcom_enable(struct regulator_dev *rdev)
+{
+	struct tps65185_data *data = rdev_get_drvdata(rdev);
+
+	if (data->vcom_ctrl_gpio)
+		return gpiod_set_value_cansleep(data->vcom_ctrl_gpio, 1);
+
+	return 0;
+}
+
+static int tps65185_vcom_disable(struct regulator_dev *rdev)
+{
+	struct tps65185_data *data = rdev_get_drvdata(rdev);
+
+	if (data->vcom_ctrl_gpio)
+		return gpiod_set_value_cansleep(data->vcom_ctrl_gpio, 0);
+
+	return 0;
+}
+
+static int tps65185_vcom_is_enabled(struct regulator_dev *rdev)
+{
+	struct tps65185_data *data = rdev_get_drvdata(rdev);
+	int ret;
+
+	ret = tps65185_check_powergood(rdev);
+	if (ret <= 0)
+		return ret;
+
+	if (data->vcom_ctrl_gpio)
+		ret = gpiod_get_value_cansleep(data->vcom_ctrl_gpio);
+
+	return ret;
+}
+
+static const struct regulator_ops tps65185_vcom_ops = {
+	.list_voltage = regulator_list_voltage_linear,
+	.map_voltage = regulator_map_voltage_linear,
+	.enable = tps65185_vcom_enable,
+	.disable = tps65185_vcom_disable,
+	.is_enabled = tps65185_vcom_is_enabled,
+	.set_voltage_sel = tps65185_vcom_set_voltage_sel,
+	.get_voltage_sel = tps65185_vcom_get_voltage_sel,
+};
+
+static const struct regulator_ops tps65185_vposneg_ops = {
+	.list_voltage = regulator_list_voltage_linear,
+	.map_voltage = regulator_map_voltage_linear,
+	.enable = tps65185_vposneg_enable,
+	.disable = tps65185_vposneg_disable,
+	.is_enabled = tps65185_check_powergood,
+	.set_voltage_sel = tps65185_vposneg_set_voltage_sel,
+	.get_voltage_sel = tps65185_vposneg_get_voltage_sel,
+};
+
+static const struct regulator_desc regulators[] = {
+	{
+		.name = "v3p3",
+		.of_match = of_match_ptr("v3p3"),
+		.regulators_node = of_match_ptr("regulators"),
+		.id = 0,
+		.ops = &tps65185_v3p3ops,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE,
+		.enable_reg = TPS65185_REG_ENABLE,
+		.enable_mask = BIT(5),
+		.n_voltages = 1,
+		.min_uV = 3300000,
+	},
+	{
+		.name = "vposneg",
+		.of_match = of_match_ptr("vposneg"),
+		.regulators_node = of_match_ptr("regulators"),
+		.id = 1,
+		.ops = &tps65185_vposneg_ops,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE,
+		.n_voltages = 4,
+		.vsel_reg = TPS65185_REG_VADJ,
+		.vsel_mask = 0x7,
+		.min_uV = 14250000,
+		.uV_step = 250000,
+	},
+	{
+		.name = "vcom",
+		.of_match = of_match_ptr("vcom"),
+		.regulators_node = of_match_ptr("regulators"),
+		.supply_name = "vposneg",
+		.id = 2,
+		.ops = &tps65185_vcom_ops,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE,
+		.n_voltages = 511,
+		.min_uV = 0,
+		.uV_step = 10000,
+	},
+};
+
+static irqreturn_t tps65185_irq_thread(int irq, void *dev_id)
+{
+	struct tps65185_data *data = dev_id;
+	unsigned int int_status_1, int_status_2;
+	int ret;
+
+	/* read both status to have irq cleared */
+	regmap_read(data->regmap, TPS65185_REG_INT1, &int_status_1);
+
+	ret = regmap_read(data->regmap, TPS65185_REG_INT2, &int_status_2);
+	if (!ret) {
+		if (int_status_2 & BIT(0))
+			complete(&data->tmst_completion);
+	}
+
+	dev_dbg(data->dev, "irq status %02x %02x\n", int_status_1, int_status_2);
+
+	return IRQ_HANDLED;
+}
+
+static int tps65185_probe(struct i2c_client *client)
+{
+	struct tps65185_data *data;
+	struct regulator_config config = { };
+	struct regulator_dev *rdev;
+	int ret = 0;
+	int i;
+
+	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
+	data->regmap = devm_regmap_init_i2c(client, &regmap_config);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(&client->dev, PTR_ERR(data->regmap),
+				     "failed to allocate regmap!\n");
+
+	data->pgood_gpio = devm_gpiod_get(&client->dev, "pwr-good", GPIOD_IN);
+	if (IS_ERR(data->pgood_gpio))
+		return dev_err_probe(&client->dev,
+				     PTR_ERR(data->pgood_gpio),
+				     "failed to get power good gpio\n");
+
+	data->pgood_irq = gpiod_to_irq(data->pgood_gpio);
+	if (data->pgood_irq < 0)
+		return data->pgood_irq;
+
+	data->pwrup_gpio = devm_gpiod_get_optional(&client->dev, "enable", GPIOD_OUT_LOW);
+	if (IS_ERR(data->pwrup_gpio))
+		return dev_err_probe(&client->dev, PTR_ERR(data->pwrup_gpio),
+				     "failed to get pwrup gpio\n");
+
+	data->wakeup_gpio = devm_gpiod_get_optional(&client->dev, "wakeup", GPIOD_OUT_LOW);
+	if (IS_ERR(data->wakeup_gpio))
+		return dev_err_probe(&client->dev,
+				     PTR_ERR(data->wakeup_gpio),
+				     "failed to get wakeup gpio\n");
+
+	data->vcom_ctrl_gpio = devm_gpiod_get_optional(&client->dev, "vcom-ctrl", GPIOD_OUT_LOW);
+	if (IS_ERR(data->vcom_ctrl_gpio))
+		return dev_err_probe(&client->dev,
+				     PTR_ERR(data->vcom_ctrl_gpio),
+				     "failed to get vcm ctrl gpio\n");
+
+	data->vin_reg = devm_regulator_get_optional(&client->dev, "vin");
+	if (IS_ERR(data->vin_reg))
+		return dev_err_probe(&client->dev, PTR_ERR(data->vin_reg),
+				     "failed to get vin regulator\n");
+
+	data->dev = &client->dev;
+	i2c_set_clientdata(client, data);
+
+	init_completion(&data->pgood_completion);
+	init_completion(&data->tmst_completion);
+
+	ret = devm_request_threaded_irq(&client->dev, data->pgood_irq, NULL,
+					pgood_handler,
+					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+					"PGOOD", data);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "failed to request power good irq\n");
+
+	if (client->irq) {
+		ret = devm_request_threaded_irq(&client->dev, client->irq,
+						NULL, tps65185_irq_thread,
+						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+						"tps65185", data);
+		if (ret)
+			return dev_err_probe(&client->dev, ret,
+					     "failed to request irq\n");
+	}
+
+	if (IS_ENABLED(CONFIG_PM)) {
+		devm_pm_runtime_enable(&client->dev);
+		pm_runtime_set_autosuspend_delay(&client->dev, 4000);
+		pm_runtime_use_autosuspend(&client->dev);
+	} else {
+		ret = tps65185_runtime_resume(&client->dev);
+		if (ret < 0)
+			return ret;
+
+		devm_add_action_or_reset(&client->dev, disable_nopm, data);
+	}
+
+	ret = pm_runtime_resume_and_get(&client->dev);
+	if (ret < 0)
+		return ret;
+
+	regmap_update_bits(data->regmap, TPS65185_REG_INT_EN2, BIT(0), BIT(0));
+	pm_runtime_put_autosuspend(&client->dev);
+
+	config.driver_data = data;
+	config.dev = &client->dev;
+	config.regmap = data->regmap;
+
+	for (i = 0; i < ARRAY_SIZE(regulators); i++) {
+		rdev = devm_regulator_register(&client->dev, &regulators[i],
+					       &config);
+		if (IS_ERR(rdev))
+			return dev_err_probe(&client->dev, PTR_ERR(rdev),
+					     "failed to register %s regulator\n",
+					     regulators[i].name);
+	}
+
+	if (IS_REACHABLE(CONFIG_HWMON)) {
+		struct device *hwmon_dev;
+
+		hwmon_dev = devm_hwmon_device_register_with_info(&client->dev, "tps65185", data,
+								 &tps65185_chip_info, NULL);
+		if (IS_ERR(hwmon_dev))
+			dev_notice(&client->dev, "failed to register hwmon\n");
+	}
+
+	return 0;
+}
+
+static const struct dev_pm_ops tps65185_pm_ops = {
+	SET_RUNTIME_PM_OPS(tps65185_runtime_suspend, tps65185_runtime_resume, NULL)
+};
+
+static const struct of_device_id tps65185_dt_ids[] = {
+	{
+		.compatible = "ti,tps65185",
+	}, {
+		/* sentinel */
+	}
+};
+MODULE_DEVICE_TABLE(of, tps65185_dt_ids);
+
+static struct i2c_driver tps65185_i2c_driver = {
+	.driver = {
+		   .name = "tps65185",
+		   .of_match_table = tps65185_dt_ids,
+		   .pm = &tps65185_pm_ops,
+	},
+	.probe = tps65185_probe,
+};
+
+module_i2c_driver(tps65185_i2c_driver);
+
+/* Module information */
+MODULE_DESCRIPTION("TPS65185 regulator driver");
+MODULE_LICENSE("GPL");
+

-- 
2.47.3

Re: [PATCH 2/2] regulator: Add TPS65185 driver

[Mark Brown]

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

On Mon, Dec 22, 2025 at 01:18:31PM +0100, Andreas Kemnade wrote:

> Add a driver for the TPS65185 regulator. Implement handling of the various
> gpio pins. Because the PWRUP (=enable) pin functionality can be achieved
> by just using two bits instead, just ensure that it is set to a stable
> value.

The reason for having GPIO controlled enables on devices with register
maps is that it's generally substantially faster to update a GPIO than
to do I2C I/O.

> +static bool tps65185_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case TPS65185_REG_TMST_VALUE:
> +	case TPS65185_REG_ENABLE:

Why is the enable register volatile?  I can't see anything in the
datasheet that suggests that it should be.

> +static int tps65185_runtime_suspend(struct device *dev)
> +{

Implementing runtime suspend in a regulator is *very* non-idiomatic and
is leading to large amounts of open coding throughout the driver.
What's the story here?  I'm very surprised that this wasn't in the
changelog.

+       if (data->wakeup_gpio) {
+               ret = gpiod_set_value_cansleep(data->wakeup_gpio, 0);
+               if (ret)
+                       return ret;
+       }

This would usually be used for system suspend.

+       if (data->vin_reg) {
+               ret = regulator_disable(data->vin_reg);
+               if (ret)
+                       goto reenable_wkup;
+       }

Can the device really operate without power?

> +static irqreturn_t tps65185_irq_thread(int irq, void *dev_id)
> +{
> +	struct tps65185_data *data = dev_id;
> +	unsigned int int_status_1, int_status_2;
> +	int ret;
> +
> +	/* read both status to have irq cleared */
> +	regmap_read(data->regmap, TPS65185_REG_INT1, &int_status_1);
> +
> +	ret = regmap_read(data->regmap, TPS65185_REG_INT2, &int_status_2);
> +	if (!ret) {
> +		if (int_status_2 & BIT(0))
> +			complete(&data->tmst_completion);
> +	}
> +
> +	dev_dbg(data->dev, "irq status %02x %02x\n", int_status_1, int_status_2);
> +
> +	return IRQ_HANDLED;
> +}

This unconditionally reports an interrupt even if none was detected.

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

Re: [PATCH 1/2] dt-bindings: regulator: Add TI TPS65185

[Mark Brown]

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

On Mon, Dec 22, 2025 at 01:18:30PM +0100, Andreas Kemnade wrote:
> Document the TPS65185. GPIO names are same as in the datasheet except for
> the PWRUP pad which is described as "enable". That pin is optional because
> the rising edge corresponds to setting one register bit and falling edge
> to another register bit.

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.

> +required:
> +  - compatible
> +  - reg
> +  - pg-gpios

Unless the device can operate without power the supplies should be
required.

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

Re: [PATCH 2/2] regulator: Add TPS65185 driver

[Andreas Kemnade]

On Mon, 22 Dec 2025 12:36:02 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Mon, Dec 22, 2025 at 01:18:31PM +0100, Andreas Kemnade wrote:
> 
> > Add a driver for the TPS65185 regulator. Implement handling of the various
> > gpio pins. Because the PWRUP (=enable) pin functionality can be achieved
> > by just using two bits instead, just ensure that it is set to a stable
> > value.  
> 
> The reason for having GPIO controlled enables on devices with register
> maps is that it's generally substantially faster to update a GPIO than
> to do I2C I/O.
> 
well we are talking about 30ms turning on time here.

[  130.816647] tps65185 1-0068: turning on...
[  130.849970] tps65185 1-0068: turned on

So if we have 100khz i2c, so, we have around 0.1ms per byte, so
the read/modify/write sequence should be done in <1ms. So I guess that is
neglectible and allows the flexibility to not have that pin.

> > +static bool tps65185_volatile_reg(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case TPS65185_REG_TMST_VALUE:
> > +	case TPS65185_REG_ENABLE:  
> 
> Why is the enable register volatile?  I can't see anything in the
> datasheet that suggests that it should be.
> 
Bit 7/6 are volatile. They reset automatically after operation done
Quote: "NOTE: After transition bit is cleared automatically"

> > +static int tps65185_runtime_suspend(struct device *dev)
> > +{  
> 
> Implementing runtime suspend in a regulator is *very* non-idiomatic and
> is leading to large amounts of open coding throughout the driver.
> What's the story here?  I'm very surprised that this wasn't in the
> changelog.
> 
OK, lets look around in the datasheet. We are apparently dealing
with 130µA here which can be saved. But that should be acceptable to be
only done on system suspend even if the regulator is off most times.
So no really strong technical reason here. I am just too used to testing
power management using runtime suspend.

> +       if (data->wakeup_gpio) {
> +               ret = gpiod_set_value_cansleep(data->wakeup_gpio, 0);
> +               if (ret)
> +                       return ret;
> +       }
> 
> This would usually be used for system suspend.
> 
> +       if (data->vin_reg) {
> +               ret = regulator_disable(data->vin_reg);
> +               if (ret)
> +                       goto reenable_wkup;
> +       }
> 
> Can the device really operate without power?
> 
No, it cannot. So yes, if we require the regulator we have
simplier code here.

> > +static irqreturn_t tps65185_irq_thread(int irq, void *dev_id)
> > +{
> > +	struct tps65185_data *data = dev_id;
> > +	unsigned int int_status_1, int_status_2;
> > +	int ret;
> > +
> > +	/* read both status to have irq cleared */
> > +	regmap_read(data->regmap, TPS65185_REG_INT1, &int_status_1);
> > +
> > +	ret = regmap_read(data->regmap, TPS65185_REG_INT2, &int_status_2);
> > +	if (!ret) {
> > +		if (int_status_2 & BIT(0))
> > +			complete(&data->tmst_completion);
> > +	}
> > +
> > +	dev_dbg(data->dev, "irq status %02x %02x\n", int_status_1, int_status_2);
> > +
> > +	return IRQ_HANDLED;
> > +}  
> 
> This unconditionally reports an interrupt even if none was detected.

Hmm, this seems like some common pattern, if some irq occurs,
check some registers and if something is set, do something about it,
and then unconditionally return IRQ_HANDLED.

Regards,
Andreas

Re: [PATCH 2/2] regulator: Add TPS65185 driver

[Mark Brown]

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

On Mon, Dec 22, 2025 at 02:45:22PM +0100, Andreas Kemnade wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > The reason for having GPIO controlled enables on devices with register
> > maps is that it's generally substantially faster to update a GPIO than
> > to do I2C I/O.

> well we are talking about 30ms turning on time here.

> [  130.816647] tps65185 1-0068: turning on...
> [  130.849970] tps65185 1-0068: turned on

> So if we have 100khz i2c, so, we have around 0.1ms per byte, so
> the read/modify/write sequence should be done in <1ms. So I guess that is
> neglectible and allows the flexibility to not have that pin.

Every little helps, and not every I2C controller is a model of
efficiency and programmability.  Note that we do have core support for
GPIO enables, it's not really any effort to support them.

> > > +{  

> > Implementing runtime suspend in a regulator is *very* non-idiomatic and
> > is leading to large amounts of open coding throughout the driver.
> > What's the story here?  I'm very surprised that this wasn't in the
> > changelog.

> OK, lets look around in the datasheet. We are apparently dealing
> with 130µA here which can be saved. But that should be acceptable to be
> only done on system suspend even if the regulator is off most times.
> So no really strong technical reason here. I am just too used to testing
> power management using runtime suspend.

It does feel like something where if we're going to do it we should
update the core to take runtime PM references rather than open coding it
in a driver that's otherwise able to use the standard helpers.  I do
worry about the impact on enable times (you'd have to power up the
supply and sync the register cache) but I guess people could disable
runtime PM for specific devices if it's an issue, and it'll never apply
to primary PMICs anyway.

> > > +static irqreturn_t tps65185_irq_thread(int irq, void *dev_id)
> > > +{

> > This unconditionally reports an interrupt even if none was detected.

> Hmm, this seems like some common pattern, if some irq occurs,
> check some registers and if something is set, do something about it,
> and then unconditionally return IRQ_HANDLED.

It is a common bug but it's a bug nontheless, it means that genirq can't
detect and mitigate stuck interrupts and that the interrupt can't be
shared.

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

Re: [PATCH 2/2] regulator: Add TPS65185 driver

[Andreas Kemnade]

On Mon, 22 Dec 2025 15:07:28 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Mon, Dec 22, 2025 at 02:45:22PM +0100, Andreas Kemnade wrote:
> > Mark Brown <broonie@kernel.org> wrote:  
> 
> > > The reason for having GPIO controlled enables on devices with register
> > > maps is that it's generally substantially faster to update a GPIO than
> > > to do I2C I/O.  
> 
> > well we are talking about 30ms turning on time here.  
> 
> > [  130.816647] tps65185 1-0068: turning on...
> > [  130.849970] tps65185 1-0068: turned on  
> 
> > So if we have 100khz i2c, so, we have around 0.1ms per byte, so
> > the read/modify/write sequence should be done in <1ms. So I guess that is
> > neglectible and allows the flexibility to not have that pin.  
> 
> Every little helps, and not every I2C controller is a model of
> efficiency and programmability.  Note that we do have core support for
> GPIO enables, it's not really any effort to support them.
> 
If the GPIO is wired... There are a half a dozen different implementations
of this driver in the wild, and I remember one not using a GPIO
probably for a device without the enable gpio wired up to the SoC.
So I think the i2c way of enabling things is required at least
as a fallback option. So we need some if (enable_gpio) somewhere.

> > > > +{    
> 
> > > Implementing runtime suspend in a regulator is *very* non-idiomatic and
> > > is leading to large amounts of open coding throughout the driver.
> > > What's the story here?  I'm very surprised that this wasn't in the
> > > changelog.  
> 
> > OK, lets look around in the datasheet. We are apparently dealing
> > with 130µA here which can be saved. But that should be acceptable to be
> > only done on system suspend even if the regulator is off most times.
> > So no really strong technical reason here. I am just too used to testing
> > power management using runtime suspend.  
> 
> It does feel like something where if we're going to do it we should
> update the core to take runtime PM references rather than open coding it
> in a driver that's otherwise able to use the standard helpers.  I do
> worry about the impact on enable times (you'd have to power up the
> supply and sync the register cache) but I guess people could disable
> runtime PM for specific devices if it's an issue, and it'll never apply
> to primary PMICs anyway.
> 
hmm, we have REGULATOR_MODE_FAST to maybe disable some pm. I have used
the autosuspend mechanism, so we do not do the time-consuming register cache
mechanism on every enable. But putting such into regulator core should be
more sane than having it in drivers.

Using standard helpers is in many places at least not so straight forward.
See the 6- in vposneg vsel.

vcom vsel has 9bits across two registers. So that is also odd.
vposneg has 2 bits used in a kind of RS flipflop mode for enable
if the enable pin is not available.

I would agree to remove the pm runtime stuff for now, so we can get the
basics in, and care about the optimazations later.

Regards,
Andreas

Re: [PATCH 2/2] regulator: Add TPS65185 driver

[Mark Brown]

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

On Wed, Dec 24, 2025 at 09:12:35AM +0100, Andreas Kemnade wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > Every little helps, and not every I2C controller is a model of
> > efficiency and programmability.  Note that we do have core support for
> > GPIO enables, it's not really any effort to support them.

> If the GPIO is wired... There are a half a dozen different implementations
> of this driver in the wild, and I remember one not using a GPIO
> probably for a device without the enable gpio wired up to the SoC.
> So I think the i2c way of enabling things is required at least
> as a fallback option. So we need some if (enable_gpio) somewhere.

This is utterly standard for devices with GPIO enables, the core will
handle this gracefully.

> > It does feel like something where if we're going to do it we should
> > update the core to take runtime PM references rather than open coding it
> > in a driver that's otherwise able to use the standard helpers.  I do
> > worry about the impact on enable times (you'd have to power up the
> > supply and sync the register cache) but I guess people could disable
> > runtime PM for specific devices if it's an issue, and it'll never apply
> > to primary PMICs anyway.

> hmm, we have REGULATOR_MODE_FAST to maybe disable some pm. I have used

That's a very different thing and completely inappropriate here.

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