[PATCH v5 0/3] Rework TPS25990 direct conversions and add TPS1689 support

[PATCH v5 0/3] Rework TPS25990 direct conversions and add TPS1689 support

[Stoyan Bogdanov]

This series reworks the TPS25990 PMBus driver and adds TPS1689 support.
Both devices share most internal functionality, differing mainly in
supported voltage and current operating ranges.

Link to v4 at [2]

v5:
- Simplify implementation and remove calculations from the driver, as
  they are not needed and were implemented incorrectly. Thanks Guenter
  for taking the time to explain.
- Drop pmbus API changes, as they are not actually needed.
- Add conditioning to separate TPS1689 and TPS25990 by chip_id in
  tps25990_read_word_data() and tps25990_write_word_data() for
  PMBUS_VIN_OV_FAULT_LIMIT and PMBUS_IIN_OC_FAULT_LIMIT. The TPS1689
  value is not 4-bit, so it does not need adjusting. Keep the current
  adjustment logic only for TPS25990.

Link to V4 at [2]

v5:
- Simplify implementaion and remove calculatinos from driver as not needed
  and being implemented in wrong manner. Thanks Gunter for explaination.
- Drop pmbus api changes, as they are not actually needed.
- Add conditioning to separate TPS1689 and TPS25990 by chip_id in
  tps25990_read_word_data and tps25990_write_word_data.
  TPS1689 value is on 4bit so it does not need adjusting. Keep only
  current implementaion for TPS25990

Link to V3 at [1]

v4:
- Fix non-devicetree support as reported by Guenter Roeck
- Rework direct conversion handling to use exported PMBus core helpers
  instead of driver-local implementations
- Update dt-bindings commit message and ti,tps25990.yaml
- Clarify commit messages to better reflect the final implementation
- Add and export direct conversion helpers from pmbus_core
- Eliminate duplicated conversion code in the driver

V3:
- Fix error detected from kernel test bot regarding division

Tests:
- Test builds for x86_64, arm64, i386
- Retest driver on arm64
- Validate driver direct conversion functions manualy


V2:
- Fix error detected from kernel test bot
- Add Acked-by to dt-bindings commit
- Drop "support" from dt-bindings commit subject

[1] https://lore.kernel.org/all/20260217081203.1792025-1-sbogdanov@baylibre.com/
[2] https://lore.kernel.org/all/20260522082349.2749970-1-sbogdanov@baylibre.com/

Stoyan Bogdanov (3):
  hwmon: (pmbus/tps25990): Rework driver for multi-device support
  dt-bindings: hwmon: pmbus/tps25990: Add TPS1689
  hwmon: (pmbus/tps25990): Add TPS1689 support

 .../bindings/hwmon/pmbus/ti,tps25990.yaml     |   8 +-
 Documentation/hwmon/tps25990.rst              |  15 +-
 drivers/hwmon/pmbus/tps25990.c                | 219 ++++++++++++------
 3 files changed, 166 insertions(+), 76 deletions(-)

-- 
2.43.0

[PATCH v5 1/3] hwmon: (pmbus/tps25990): Rework driver for multi-device support

[Stoyan Bogdanov]

Rework existing implementation to allow adding support for new
devices to the existing driver. chip_id is used to identify the
current device and differentiate logic where needed.

Changes include:
 - Add an enum listing supported chips
 - Add a structure to hold per-device m, b, R coefficients

Signed-off-by: Stoyan Bogdanov <sbogdanov@baylibre.com>
---
 drivers/hwmon/pmbus/tps25990.c | 123 +++++++++++++++++++--------------
 1 file changed, 70 insertions(+), 53 deletions(-)

diff --git a/drivers/hwmon/pmbus/tps25990.c b/drivers/hwmon/pmbus/tps25990.c
index 9d318e6509ab..490dcd2a5fa5 100644
--- a/drivers/hwmon/pmbus/tps25990.c
+++ b/drivers/hwmon/pmbus/tps25990.c
@@ -47,6 +47,15 @@
 					 PK_MIN_AVG_RST_AVG  | \
 					 PK_MIN_AVG_RST_MIN)
 
+enum chips {
+	tps25990,
+};
+
+struct tps25990_data {
+	struct pmbus_driver_info info;
+	enum chips chip_id;
+};
+
 /*
  * Arbitrary default Rimon value: 1kOhm
  * This correspond to an overcurrent limit of 55A, close to the specified limit
@@ -337,63 +346,65 @@ static const struct regulator_desc tps25990_reg_desc[] = {
 };
 #endif
 
-static const struct pmbus_driver_info tps25990_base_info = {
-	.pages = 1,
-	.format[PSC_VOLTAGE_IN] = direct,
-	.m[PSC_VOLTAGE_IN] = 5251,
-	.b[PSC_VOLTAGE_IN] = 0,
-	.R[PSC_VOLTAGE_IN] = -2,
-	.format[PSC_VOLTAGE_OUT] = direct,
-	.m[PSC_VOLTAGE_OUT] = 5251,
-	.b[PSC_VOLTAGE_OUT] = 0,
-	.R[PSC_VOLTAGE_OUT] = -2,
-	.format[PSC_TEMPERATURE] = direct,
-	.m[PSC_TEMPERATURE] = 140,
-	.b[PSC_TEMPERATURE] = 32100,
-	.R[PSC_TEMPERATURE] = -2,
-	/*
-	 * Current and Power measurement depends on the ohm value
-	 * of Rimon. m is multiplied by 1000 below to have an integer
-	 * and -3 is added to R to compensate.
-	 */
-	.format[PSC_CURRENT_IN] = direct,
-	.m[PSC_CURRENT_IN] = 9538,
-	.b[PSC_CURRENT_IN] = 0,
-	.R[PSC_CURRENT_IN] = -6,
-	.format[PSC_POWER] = direct,
-	.m[PSC_POWER] = 4901,
-	.b[PSC_POWER] = 0,
-	.R[PSC_POWER] = -7,
-	.func[0] = (PMBUS_HAVE_VIN |
-		    PMBUS_HAVE_VOUT |
-		    PMBUS_HAVE_VMON |
-		    PMBUS_HAVE_IIN |
-		    PMBUS_HAVE_PIN |
-		    PMBUS_HAVE_TEMP |
-		    PMBUS_HAVE_STATUS_VOUT |
-		    PMBUS_HAVE_STATUS_IOUT |
-		    PMBUS_HAVE_STATUS_INPUT |
-		    PMBUS_HAVE_STATUS_TEMP |
-		    PMBUS_HAVE_SAMPLES),
-	.read_word_data = tps25990_read_word_data,
-	.write_word_data = tps25990_write_word_data,
-	.read_byte_data = tps25990_read_byte_data,
-	.write_byte_data = tps25990_write_byte_data,
+static struct pmbus_driver_info tps25990_base_info[] = {
+	[tps25990] = {
+		.pages = 1,
+		.format[PSC_VOLTAGE_IN] = direct,
+		.m[PSC_VOLTAGE_IN] = 5251,
+		.b[PSC_VOLTAGE_IN] = 0,
+		.R[PSC_VOLTAGE_IN] = -2,
+		.format[PSC_VOLTAGE_OUT] = direct,
+		.m[PSC_VOLTAGE_OUT] = 5251,
+		.b[PSC_VOLTAGE_OUT] = 0,
+		.R[PSC_VOLTAGE_OUT] = -2,
+		.format[PSC_TEMPERATURE] = direct,
+		.m[PSC_TEMPERATURE] = 140,
+		.b[PSC_TEMPERATURE] = 32100,
+		.R[PSC_TEMPERATURE] = -2,
+		/*
+		 * Current and Power measurement depends on the ohm value
+		 * of Rimon. m is multiplied by 1000 below to have an integer
+		 * and -3 is added to R to compensate.
+		 */
+		.format[PSC_CURRENT_IN] = direct,
+		.m[PSC_CURRENT_IN] = 9538,
+		.b[PSC_CURRENT_IN] = 0,
+		.R[PSC_CURRENT_IN] = -6,
+		.format[PSC_POWER] = direct,
+		.m[PSC_POWER] = 4901,
+		.b[PSC_POWER] = 0,
+		.R[PSC_POWER] = -7,
+		.func[0] = (PMBUS_HAVE_VIN |
+			    PMBUS_HAVE_VOUT |
+			    PMBUS_HAVE_VMON |
+			    PMBUS_HAVE_IIN |
+			    PMBUS_HAVE_PIN |
+			    PMBUS_HAVE_TEMP |
+			    PMBUS_HAVE_STATUS_VOUT |
+			    PMBUS_HAVE_STATUS_IOUT |
+			    PMBUS_HAVE_STATUS_INPUT |
+			    PMBUS_HAVE_STATUS_TEMP |
+			    PMBUS_HAVE_SAMPLES),
+		.read_word_data = tps25990_read_word_data,
+		.write_word_data = tps25990_write_word_data,
+		.read_byte_data = tps25990_read_byte_data,
+		.write_byte_data = tps25990_write_byte_data,
 
 #if IS_ENABLED(CONFIG_SENSORS_TPS25990_REGULATOR)
-	.reg_desc = tps25990_reg_desc,
-	.num_regulators = ARRAY_SIZE(tps25990_reg_desc),
+		.reg_desc = tps25990_reg_desc,
+		.num_regulators = ARRAY_SIZE(tps25990_reg_desc),
 #endif
+	},
 };
 
 static const struct i2c_device_id tps25990_i2c_id[] = {
-	{ .name = "tps25990" },
-	{ }
+	{ "tps25990", tps25990 },
+	{}
 };
 MODULE_DEVICE_TABLE(i2c, tps25990_i2c_id);
 
 static const struct of_device_id tps25990_of_match[] = {
-	{ .compatible = "ti,tps25990" },
+	{ .compatible = "ti,tps25990", .data = (void *)tps25990 },
 	{}
 };
 MODULE_DEVICE_TABLE(of, tps25990_of_match);
@@ -401,8 +412,9 @@ MODULE_DEVICE_TABLE(of, tps25990_of_match);
 static int tps25990_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
-	struct pmbus_driver_info *info;
+	struct tps25990_data *data;
 	const char *propname;
+	enum chips chip_id;
 	u32 rimon;
 	int ret;
 
@@ -415,15 +427,20 @@ static int tps25990_probe(struct i2c_client *client)
 		rimon = TPS25990_DEFAULT_RIMON;
 	}
 
-	info = devm_kmemdup(dev, &tps25990_base_info, sizeof(*info), GFP_KERNEL);
-	if (!info)
+	chip_id = (enum chips)(unsigned long)i2c_get_match_data(client);
+
+	data = devm_kzalloc(dev, sizeof(struct tps25990_data), GFP_KERNEL);
+	if (!data)
 		return -ENOMEM;
 
+	data->info = tps25990_base_info[chip_id];
+	data->chip_id = chip_id;
+
 	/* Adapt the current and power scale for each instance */
-	tps25990_set_m(&info->m[PSC_CURRENT_IN], rimon);
-	tps25990_set_m(&info->m[PSC_POWER], rimon);
+	tps25990_set_m(&data->info.m[PSC_CURRENT_IN], rimon);
+	tps25990_set_m(&data->info.m[PSC_POWER], rimon);
 
-	return pmbus_do_probe(client, info);
+	return pmbus_do_probe(client, &data->info);
 }
 
 static struct i2c_driver tps25990_driver = {
-- 
2.43.0

[PATCH v5 2/3] dt-bindings: hwmon: pmbus/tps25990: Add TPS1689

[Stoyan Bogdanov]

Add device compatible support for TPS1689

Signed-off-by: Stoyan Bogdanov <sbogdanov@baylibre.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
 .../devicetree/bindings/hwmon/pmbus/ti,tps25990.yaml      | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/ti,tps25990.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/ti,tps25990.yaml
index f4115870e450..63ccb67576df 100644
--- a/Documentation/devicetree/bindings/hwmon/pmbus/ti,tps25990.yaml
+++ b/Documentation/devicetree/bindings/hwmon/pmbus/ti,tps25990.yaml
@@ -5,18 +5,20 @@
 $id: http://devicetree.org/schemas/hwmon/pmbus/ti,tps25990.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Texas Instruments TPS25990 Stackable eFuse
+title: Texas Instruments Stackable eFuses
 
 maintainers:
   - Jerome Brunet <jbrunet@baylibre.com>
 
 description:
-  The TI TPS25990 is an integrated, high-current circuit
+  The TI TPS25990 and TPS1689 are integrated, high-current circuit
   protection and power management device with PMBUS interface
 
 properties:
   compatible:
-    const: ti,tps25990
+    enum:
+      - ti,tps1689
+      - ti,tps25990
 
   reg:
     maxItems: 1
-- 
2.43.0

[PATCH v5 3/3] hwmon: (pmbus/tps25990): Add TPS1689 support

[Stoyan Bogdanov]

Extend the existing TPS25990 driver to support the TPS1689 eFuse,
as both devices share the same command interface and functionality.

Update the documentation to include TPS1689 support.

Signed-off-by: Stoyan Bogdanov <sbogdanov@baylibre.com>
---
 Documentation/hwmon/tps25990.rst | 15 +++--
 drivers/hwmon/pmbus/tps25990.c   | 98 ++++++++++++++++++++++++++------
 2 files changed, 92 insertions(+), 21 deletions(-)

diff --git a/Documentation/hwmon/tps25990.rst b/Documentation/hwmon/tps25990.rst
index 04faec780d26..e8bc9a550bda 100644
--- a/Documentation/hwmon/tps25990.rst
+++ b/Documentation/hwmon/tps25990.rst
@@ -9,26 +9,31 @@ Supported chips:
 
     Prefix: 'tps25990'
 
-  * Datasheet
+    Datasheet: Publicly available at Texas Instruments website: https://www.ti.com/lit/gpn/tps25990
 
-    Publicly available at Texas Instruments website: https://www.ti.com/lit/gpn/tps25990
+  * TI TPS1689
+
+    Prefix: 'tps1689'
+
+    Datasheet: Publicly available at Texas Instruments website: https://www.ti.com/lit/gpn/tps1689
 
 Author:
 
 	Jerome Brunet <jbrunet@baylibre.com>
+	Stoyan Bogdanov <sbogdanov@baylibre.com>
 
 Description
 -----------
 
-This driver implements support for TI TPS25990 eFuse.
+This driver implements support for TI TPS25990 and TI TPS1689 eFuse chips.
 This is an integrated, high-current circuit protection and power
 management device with PMBUS interface
 
-Device compliant with:
+Devices are compliant with:
 
 - PMBus rev 1.3 interface.
 
-Device supports direct format for reading input voltages,
+Devices supports direct format for reading input voltages,
 output voltage, input current, input power and temperature.
 
 Due to the specificities of the chip, all history reset attributes
diff --git a/drivers/hwmon/pmbus/tps25990.c b/drivers/hwmon/pmbus/tps25990.c
index 490dcd2a5fa5..7e2991a85da1 100644
--- a/drivers/hwmon/pmbus/tps25990.c
+++ b/drivers/hwmon/pmbus/tps25990.c
@@ -48,6 +48,7 @@
 					 PK_MIN_AVG_RST_MIN)
 
 enum chips {
+	tps1689,
 	tps25990,
 };
 
@@ -105,6 +106,8 @@ static int tps25990_mfr_write_protect_get(struct i2c_client *client)
 static int tps25990_read_word_data(struct i2c_client *client,
 				   int page, int phase, int reg)
 {
+	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
+	struct tps25990_data *data = container_of(info, struct tps25990_data, info);
 	int ret;
 
 	switch (reg) {
@@ -193,9 +196,11 @@ static int tps25990_read_word_data(struct i2c_client *client,
 		ret = pmbus_read_word_data(client, page, phase, reg);
 		if (ret < 0)
 			break;
-		ret = DIV_ROUND_CLOSEST(ret * TPS25990_VIN_OVF_NUM,
-					TPS25990_VIN_OVF_DIV);
-		ret += TPS25990_VIN_OVF_OFF;
+		if (data->chip_id == tps25990) {
+			ret = DIV_ROUND_CLOSEST(ret * TPS25990_VIN_OVF_NUM,
+						TPS25990_VIN_OVF_DIV);
+			ret += TPS25990_VIN_OVF_OFF;
+		}
 		break;
 
 	case PMBUS_IIN_OC_FAULT_LIMIT:
@@ -207,9 +212,11 @@ static int tps25990_read_word_data(struct i2c_client *client,
 		ret = pmbus_read_byte_data(client, page, TPS25990_VIREF);
 		if (ret < 0)
 			break;
-		ret = DIV_ROUND_CLOSEST(ret * TPS25990_IIN_OCF_NUM,
-					TPS25990_IIN_OCF_DIV);
-		ret += TPS25990_IIN_OCF_OFF;
+		if (data->chip_id == tps25990) {
+			ret = DIV_ROUND_CLOSEST(ret * TPS25990_IIN_OCF_NUM,
+						TPS25990_IIN_OCF_DIV);
+			ret += TPS25990_IIN_OCF_OFF;
+		}
 		break;
 
 	case PMBUS_VIRT_SAMPLES:
@@ -238,6 +245,8 @@ static int tps25990_read_word_data(struct i2c_client *client,
 static int tps25990_write_word_data(struct i2c_client *client,
 				    int page, int reg, u16 value)
 {
+	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
+	struct tps25990_data *data = container_of(info, struct tps25990_data, info);
 	int ret;
 
 	switch (reg) {
@@ -253,20 +262,23 @@ static int tps25990_write_word_data(struct i2c_client *client,
 		value = clamp_val(value, 0, 0xff);
 		ret = pmbus_write_word_data(client, page, reg, value);
 		break;
-
 	case PMBUS_VIN_OV_FAULT_LIMIT:
-		value -= TPS25990_VIN_OVF_OFF;
-		value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_VIN_OVF_DIV,
-					  TPS25990_VIN_OVF_NUM);
-		value = clamp_val(value, 0, 0xf);
+		if (data->chip_id == tps25990) {
+			value -= TPS25990_VIN_OVF_OFF;
+			value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_VIN_OVF_DIV,
+						  TPS25990_VIN_OVF_NUM);
+			value = clamp_val(value, 0, 0xf);
+		}
 		ret = pmbus_write_word_data(client, page, reg, value);
 		break;
 
 	case PMBUS_IIN_OC_FAULT_LIMIT:
-		value -= TPS25990_IIN_OCF_OFF;
-		value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_IIN_OCF_DIV,
-					  TPS25990_IIN_OCF_NUM);
-		value = clamp_val(value, 0, 0x3f);
+		if (data->chip_id == tps25990) {
+			value -= TPS25990_IIN_OCF_OFF;
+			value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_IIN_OCF_DIV,
+						  TPS25990_IIN_OCF_NUM);
+			value = clamp_val(value, 0, 0x3f);
+		}
 		ret = pmbus_write_byte_data(client, page, TPS25990_VIREF, value);
 		break;
 
@@ -347,6 +359,59 @@ static const struct regulator_desc tps25990_reg_desc[] = {
 #endif
 
 static struct pmbus_driver_info tps25990_base_info[] = {
+	[tps1689] = {
+		.pages = 1,
+		.format[PSC_VOLTAGE_IN] = direct,
+		.m[PSC_VOLTAGE_IN] = 1166,
+		.b[PSC_VOLTAGE_IN] = 0,
+		.R[PSC_VOLTAGE_IN] = -2,
+		.format[PSC_VOLTAGE_OUT] = direct,
+		.m[PSC_VOLTAGE_OUT] = 1166,
+		.b[PSC_VOLTAGE_OUT] = 0,
+		.R[PSC_VOLTAGE_OUT] = -2,
+		.format[PSC_TEMPERATURE] = direct,
+		.m[PSC_TEMPERATURE] = 140,
+		.b[PSC_TEMPERATURE] = 32103,
+		.R[PSC_TEMPERATURE] = -2,
+		/*
+		 * Current and Power measurement depends on the ohm value
+		 * of Rimon. m is multiplied by 1000 below to have an integer
+		 * and -3 is added to R to compensate.
+		 */
+		.format[PSC_CURRENT_IN] = direct,
+		.m[PSC_CURRENT_IN] = 9548,
+		.b[PSC_CURRENT_IN] = 0,
+		.R[PSC_CURRENT_IN] = -6,
+		.format[PSC_CURRENT_OUT] = direct,
+		.m[PSC_CURRENT_OUT] = 24347,
+		.b[PSC_CURRENT_OUT] = 0,
+		.R[PSC_CURRENT_OUT] = -3,
+		.format[PSC_POWER] = direct,
+		.m[PSC_POWER] = 2775,
+		.b[PSC_POWER] = 0,
+		.R[PSC_POWER] = -4,
+		.func[0] = (PMBUS_HAVE_VIN |
+			    PMBUS_HAVE_VOUT |
+			    PMBUS_HAVE_VMON |
+			    PMBUS_HAVE_IIN |
+			    PMBUS_HAVE_PIN |
+			    PMBUS_HAVE_TEMP |
+			    PMBUS_HAVE_STATUS_VOUT |
+			    PMBUS_HAVE_STATUS_IOUT |
+			    PMBUS_HAVE_STATUS_INPUT |
+			    PMBUS_HAVE_STATUS_TEMP |
+			    PMBUS_HAVE_SAMPLES),
+
+		.read_word_data = tps25990_read_word_data,
+		.write_word_data = tps25990_write_word_data,
+		.read_byte_data = tps25990_read_byte_data,
+		.write_byte_data = tps25990_write_byte_data,
+
+#if IS_ENABLED(CONFIG_SENSORS_TPS25990_REGULATOR)
+		.reg_desc = tps25990_reg_desc,
+		.num_regulators = ARRAY_SIZE(tps25990_reg_desc),
+#endif
+		},
 	[tps25990] = {
 		.pages = 1,
 		.format[PSC_VOLTAGE_IN] = direct,
@@ -389,7 +454,6 @@ static struct pmbus_driver_info tps25990_base_info[] = {
 		.write_word_data = tps25990_write_word_data,
 		.read_byte_data = tps25990_read_byte_data,
 		.write_byte_data = tps25990_write_byte_data,
-
 #if IS_ENABLED(CONFIG_SENSORS_TPS25990_REGULATOR)
 		.reg_desc = tps25990_reg_desc,
 		.num_regulators = ARRAY_SIZE(tps25990_reg_desc),
@@ -398,12 +462,14 @@ static struct pmbus_driver_info tps25990_base_info[] = {
 };
 
 static const struct i2c_device_id tps25990_i2c_id[] = {
+	{ "tps1689", tps1689 },
 	{ "tps25990", tps25990 },
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, tps25990_i2c_id);
 
 static const struct of_device_id tps25990_of_match[] = {
+	{ .compatible = "ti,tps1689", .data = (void *)tps1689 },
 	{ .compatible = "ti,tps25990", .data = (void *)tps25990 },
 	{}
 };
-- 
2.43.0

Re: [PATCH v5 2/3] dt-bindings: hwmon: pmbus/tps25990: Add TPS1689

[sashiko-bot]

> Add device compatible support for TPS1689
> 
> Signed-off-by: Stoyan Bogdanov <sbogdanov@baylibre.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728015857.193890-1-sbogdanov@baylibre.com?part=2

Re: [PATCH v5 1/3] hwmon: (pmbus/tps25990): Rework driver for multi-device support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The static array `tps25990_base_info` is missing the `const` qualifier, placing it in mutable `.data` memory instead of read-only `.rodata`.
- [Low] The `tps25990_i2c_id` array uses positional initializers, which violates the I2C subsystem's coding guidelines.
--

--- Patch [1]: [PATCH v5 1/3] hwmon: (pmbus/tps25990): Rework driver for multi-device support ---
commit 7c6d03feb4ad36395a77df5021ef6a8c6f2df2a8
Author: Stoyan Bogdanov <sbogdanov@baylibre.com>

hwmon: (pmbus/tps25990): Rework driver for multi-device support

This commit reworks the tps25990 driver to allow adding support for new
devices. It introduces a chip enumeration and a data structure to hold
per-device measurement coefficients.

> diff --git a/drivers/hwmon/pmbus/tps25990.c b/drivers/hwmon/pmbus/tps25990.c
> index 9d318e6509abf..490dcd2a5fa51 100644
> --- a/drivers/hwmon/pmbus/tps25990.c
> +++ b/drivers/hwmon/pmbus/tps25990.c
[ ... ]
> -static const struct pmbus_driver_info tps25990_base_info = {
> -	.pages = 1,
[ ... ]
> -#if IS_ENABLED(CONFIG_SENSORS_TPS25990_REGULATOR)
> -	.reg_desc = tps25990_reg_desc,
> -	.num_regulators = ARRAY_SIZE(tps25990_reg_desc),
> -#endif
> -};
> -
> +static struct pmbus_driver_info tps25990_base_info[] = {
> +	[tps25990] = {
> +		.pages = 1,

[Severity: Medium]
Should this array be declared const?

The previous single structure was const, but the new array drops the const
qualifier. Leaving tps25990_base_info mutable places the struct's function
pointers in mutable .data memory instead of read-only .rodata.

[ ... ]
>  static const struct i2c_device_id tps25990_i2c_id[] = {
> -	{ .name = "tps25990" },
> -	{ }
> +	{ "tps25990", tps25990 },
> +	{}
>  };
>  MODULE_DEVICE_TABLE(i2c, tps25990_i2c_id);

[Severity: Low]
Does this code run counter to the I2C subsystem coding guidelines?

The guidelines specify that initialized arrays of type struct i2c_device_id
must use named initializers, but this patch changes the initialization to
use positional initializers instead.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728015857.193890-1-sbogdanov@baylibre.com?part=1

Re: [PATCH v5 3/3] hwmon: (pmbus/tps25990): Add TPS1689 support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] When writing the `PMBUS_IIN_OC_FAULT_LIMIT` for `tps1689`, the 16-bit `value` is passed directly to `pmbus_write_byte_data()` without clamping, causing silent integer truncation and incorrect hardware limits.
- [Medium] The `tps1689` configuration defines format and coefficients for `PSC_CURRENT_OUT`, but misses the required capability flag and scaling logic, leaving it completely unused and broken.

Pre-existing issues:
- [High] When a user requests a voltage or current limit lower than the base hardware offset, the unsigned 16-bit limit underflows, silently configuring the hardware to its absolute maximum limit.
--

--- Patch [3]: [PATCH v5 3/3] hwmon: (pmbus/tps25990): Add TPS1689 support ---
commit 3362ebf6e63c82cf07dc00cc00e78fd1761514ef
Author: Stoyan Bogdanov <sbogdanov@baylibre.com>

hwmon: (pmbus/tps25990): Add TPS1689 support
    
Extend the existing TPS25990 driver to support the TPS1689 eFuse,
as both devices share the same command interface and functionality.
Update the documentation to include TPS1689 support.

> diff --git a/drivers/hwmon/pmbus/tps25990.c b/drivers/hwmon/pmbus/tps25990.c
> index 490dcd2a5fa51..7e2991a85da13 100644
> --- a/drivers/hwmon/pmbus/tps25990.c
> +++ b/drivers/hwmon/pmbus/tps25990.c

[ ... ]

> @@ -253,20 +262,23 @@ static int tps25990_write_word_data(struct i2c_client *client,
>  		value = clamp_val(value, 0, 0xff);
>  		ret = pmbus_write_word_data(client, page, reg, value);
>  		break;
>  	case PMBUS_VIN_OV_FAULT_LIMIT:
> -		value -= TPS25990_VIN_OVF_OFF;
> -		value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_VIN_OVF_DIV,
> -					  TPS25990_VIN_OVF_NUM);
> -		value = clamp_val(value, 0, 0xf);
> +		if (data->chip_id == tps25990) {
> +			value -= TPS25990_VIN_OVF_OFF;
> +			value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_VIN_OVF_DIV,
> +						  TPS25990_VIN_OVF_NUM);
> +			value = clamp_val(value, 0, 0xf);
> +		}

[Severity: High]
This is a pre-existing issue, but could this calculation underflow if the user
requests a limit lower than the base hardware offset?

If a limit is requested where the value is less than TPS25990_VIN_OVF_OFF (155),
wouldn't the u16 value silently underflow to a very large positive number when
subtracted?

Since this huge value is then scaled and passed to clamp_val() against 0xf,
would this result in setting the absolute maximum possible limit in the hardware
instead of the requested low limit, inadvertently bypassing the protections?

>  		ret = pmbus_write_word_data(client, page, reg, value);
>  		break;
>  
>  	case PMBUS_IIN_OC_FAULT_LIMIT:
> -		value -= TPS25990_IIN_OCF_OFF;
> -		value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_IIN_OCF_DIV,
> -					  TPS25990_IIN_OCF_NUM);
> -		value = clamp_val(value, 0, 0x3f);
> +		if (data->chip_id == tps25990) {
> +			value -= TPS25990_IIN_OCF_OFF;
> +			value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_IIN_OCF_DIV,
> +						  TPS25990_IIN_OCF_NUM);
> +			value = clamp_val(value, 0, 0x3f);
> +		}

[Severity: High]
This is also a pre-existing issue, but does this face the same underflow risk
if the requested limit is less than TPS25990_IIN_OCF_OFF?

>  		ret = pmbus_write_byte_data(client, page, TPS25990_VIREF, value);

[Severity: High]
When data->chip_id == tps1689, this code skips the scaling block and passes
the u16 value directly into pmbus_write_byte_data().

Does this silently truncate the 16-bit value into an 8-bit register write?

If a large limit is requested, could this truncation cause it to wrap around
to a very small limit, potentially causing the eFuse to prematurely cut power?

>  		break;

[ ... ]

> @@ -347,6 +359,59 @@ static const struct regulator_desc tps25990_reg_desc[] = {
>  #endif
>  
>  static struct pmbus_driver_info tps25990_base_info[] = {
> +	[tps1689] = {
> +		.pages = 1,

[ ... ]

> +		.format[PSC_CURRENT_OUT] = direct,
> +		.m[PSC_CURRENT_OUT] = 24347,
> +		.b[PSC_CURRENT_OUT] = 0,
> +		.R[PSC_CURRENT_OUT] = -3,

[ ... ]

> +		.func[0] = (PMBUS_HAVE_VIN |
> +			    PMBUS_HAVE_VOUT |
> +			    PMBUS_HAVE_VMON |
> +			    PMBUS_HAVE_IIN |
> +			    PMBUS_HAVE_PIN |
> +			    PMBUS_HAVE_TEMP |
> +			    PMBUS_HAVE_STATUS_VOUT |
> +			    PMBUS_HAVE_STATUS_IOUT |
> +			    PMBUS_HAVE_STATUS_INPUT |
> +			    PMBUS_HAVE_STATUS_TEMP |
> +			    PMBUS_HAVE_SAMPLES),

[Severity: Medium]
The configuration defines format and coefficients for PSC_CURRENT_OUT, however
the func[0] capability flags do not include PMBUS_HAVE_IOUT.

Does this mean the PMBus core will completely ignore the output current sensor?

Additionally, looking at tps25990_probe(), it appears dynamic scaling for the
rimon resistor is applied for PSC_CURRENT_IN and PSC_POWER, but not
PSC_CURRENT_OUT:

drivers/hwmon/pmbus/tps25990.c:tps25990_probe() {
    ...
	/* Adapt the current and power scale for each instance */
	tps25990_set_m(&data->info.m[PSC_CURRENT_IN], rimon);
	tps25990_set_m(&data->info.m[PSC_POWER], rimon);
    ...
}

Would this reading be incorrect even if the sensor capability were enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728015857.193890-1-sbogdanov@baylibre.com?part=3

Re: [PATCH v5 1/3] hwmon: (pmbus/tps25990): Rework driver for multi-device support

[Guenter Roeck]

On 7/27/26 19:07, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] The static array `tps25990_base_info` is missing the `const` qualifier, placing it in mutable `.data` memory instead of read-only `.rodata`.
> - [Low] The `tps25990_i2c_id` array uses positional initializers, which violates the I2C subsystem's coding guidelines.

Both reported issues are valid. Please fix.

Thasnks,
Guenter

> --
> 
> --- Patch [1]: [PATCH v5 1/3] hwmon: (pmbus/tps25990): Rework driver for multi-device support ---
> commit 7c6d03feb4ad36395a77df5021ef6a8c6f2df2a8
> Author: Stoyan Bogdanov <sbogdanov@baylibre.com>
> 
> hwmon: (pmbus/tps25990): Rework driver for multi-device support
> 
> This commit reworks the tps25990 driver to allow adding support for new
> devices. It introduces a chip enumeration and a data structure to hold
> per-device measurement coefficients.
> 
>> diff --git a/drivers/hwmon/pmbus/tps25990.c b/drivers/hwmon/pmbus/tps25990.c
>> index 9d318e6509abf..490dcd2a5fa51 100644
>> --- a/drivers/hwmon/pmbus/tps25990.c
>> +++ b/drivers/hwmon/pmbus/tps25990.c
> [ ... ]
>> -static const struct pmbus_driver_info tps25990_base_info = {
>> -	.pages = 1,
> [ ... ]
>> -#if IS_ENABLED(CONFIG_SENSORS_TPS25990_REGULATOR)
>> -	.reg_desc = tps25990_reg_desc,
>> -	.num_regulators = ARRAY_SIZE(tps25990_reg_desc),
>> -#endif
>> -};
>> -
>> +static struct pmbus_driver_info tps25990_base_info[] = {
>> +	[tps25990] = {
>> +		.pages = 1,
> 
> [Severity: Medium]
> Should this array be declared const?
> 
> The previous single structure was const, but the new array drops the const
> qualifier. Leaving tps25990_base_info mutable places the struct's function
> pointers in mutable .data memory instead of read-only .rodata.
> 
> [ ... ]
>>   static const struct i2c_device_id tps25990_i2c_id[] = {
>> -	{ .name = "tps25990" },
>> -	{ }
>> +	{ "tps25990", tps25990 },
>> +	{}
>>   };
>>   MODULE_DEVICE_TABLE(i2c, tps25990_i2c_id);
> 
> [Severity: Low]
> Does this code run counter to the I2C subsystem coding guidelines?
> 
> The guidelines specify that initialized arrays of type struct i2c_device_id
> must use named initializers, but this patch changes the initialization to
> use positional initializers instead.
> 

Re: [PATCH v5 3/3] hwmon: (pmbus/tps25990): Add TPS1689 support

[Guenter Roeck]

On 7/27/26 18:58, Stoyan Bogdanov wrote:
> Extend the existing TPS25990 driver to support the TPS1689 eFuse,
> as both devices share the same command interface and functionality.
> 
> Update the documentation to include TPS1689 support.
> 
> Signed-off-by: Stoyan Bogdanov <sbogdanov@baylibre.com>
> ---
>   Documentation/hwmon/tps25990.rst | 15 +++--
>   drivers/hwmon/pmbus/tps25990.c   | 98 ++++++++++++++++++++++++++------
>   2 files changed, 92 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/hwmon/tps25990.rst b/Documentation/hwmon/tps25990.rst
> index 04faec780d26..e8bc9a550bda 100644
> --- a/Documentation/hwmon/tps25990.rst
> +++ b/Documentation/hwmon/tps25990.rst
> @@ -9,26 +9,31 @@ Supported chips:
>   
>       Prefix: 'tps25990'
>   
> -  * Datasheet
> +    Datasheet: Publicly available at Texas Instruments website: https://www.ti.com/lit/gpn/tps25990
>   
> -    Publicly available at Texas Instruments website: https://www.ti.com/lit/gpn/tps25990
> +  * TI TPS1689
> +
> +    Prefix: 'tps1689'
> +
> +    Datasheet: Publicly available at Texas Instruments website: https://www.ti.com/lit/gpn/tps1689
>   
>   Author:
>   
>   	Jerome Brunet <jbrunet@baylibre.com>
> +	Stoyan Bogdanov <sbogdanov@baylibre.com>
>   
>   Description
>   -----------
>   
> -This driver implements support for TI TPS25990 eFuse.
> +This driver implements support for TI TPS25990 and TI TPS1689 eFuse chips.
>   This is an integrated, high-current circuit protection and power
>   management device with PMBUS interface
>   
> -Device compliant with:
> +Devices are compliant with:
>   
>   - PMBus rev 1.3 interface.
>   
> -Device supports direct format for reading input voltages,
> +Devices supports direct format for reading input voltages,
>   output voltage, input current, input power and temperature.
>   
>   Due to the specificities of the chip, all history reset attributes
> diff --git a/drivers/hwmon/pmbus/tps25990.c b/drivers/hwmon/pmbus/tps25990.c
> index 490dcd2a5fa5..7e2991a85da1 100644
> --- a/drivers/hwmon/pmbus/tps25990.c
> +++ b/drivers/hwmon/pmbus/tps25990.c
> @@ -48,6 +48,7 @@
>   					 PK_MIN_AVG_RST_MIN)
>   
>   enum chips {
> +	tps1689,
>   	tps25990,
>   };
>   
> @@ -105,6 +106,8 @@ static int tps25990_mfr_write_protect_get(struct i2c_client *client)
>   static int tps25990_read_word_data(struct i2c_client *client,
>   				   int page, int phase, int reg)
>   {
> +	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> +	struct tps25990_data *data = container_of(info, struct tps25990_data, info);
>   	int ret;
>   
>   	switch (reg) {
> @@ -193,9 +196,11 @@ static int tps25990_read_word_data(struct i2c_client *client,
>   		ret = pmbus_read_word_data(client, page, phase, reg);
>   		if (ret < 0)
>   			break;
> -		ret = DIV_ROUND_CLOSEST(ret * TPS25990_VIN_OVF_NUM,
> -					TPS25990_VIN_OVF_DIV);
> -		ret += TPS25990_VIN_OVF_OFF;
> +		if (data->chip_id == tps25990) {
> +			ret = DIV_ROUND_CLOSEST(ret * TPS25990_VIN_OVF_NUM,
> +						TPS25990_VIN_OVF_DIV);
> +			ret += TPS25990_VIN_OVF_OFF;
> +		}

Unless I am missing something, TPS1689 also has a voltage offset for this register.

>   		break;
>   
>   	case PMBUS_IIN_OC_FAULT_LIMIT:
> @@ -207,9 +212,11 @@ static int tps25990_read_word_data(struct i2c_client *client,
>   		ret = pmbus_read_byte_data(client, page, TPS25990_VIREF);
>   		if (ret < 0)
>   			break;
> -		ret = DIV_ROUND_CLOSEST(ret * TPS25990_IIN_OCF_NUM,
> -					TPS25990_IIN_OCF_DIV);
> -		ret += TPS25990_IIN_OCF_OFF;
> +		if (data->chip_id == tps25990) {
> +			ret = DIV_ROUND_CLOSEST(ret * TPS25990_IIN_OCF_NUM,
> +						TPS25990_IIN_OCF_DIV);
> +			ret += TPS25990_IIN_OCF_OFF;
> +		}

Looking at the datasheet, I see no difference regarding TPS25990_VIREF
and its use across the two chips. Also, it does not make sense to me to pass
the value of TPS25990_VIREF directoy as OC fault limit. Unless I am missing
something, I don't think this works as intended.

>   		break;
>   
>   	case PMBUS_VIRT_SAMPLES:
> @@ -238,6 +245,8 @@ static int tps25990_read_word_data(struct i2c_client *client,
>   static int tps25990_write_word_data(struct i2c_client *client,
>   				    int page, int reg, u16 value)
>   {
> +	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
> +	struct tps25990_data *data = container_of(info, struct tps25990_data, info);
>   	int ret;
>   
>   	switch (reg) {
> @@ -253,20 +262,23 @@ static int tps25990_write_word_data(struct i2c_client *client,
>   		value = clamp_val(value, 0, 0xff);
>   		ret = pmbus_write_word_data(client, page, reg, value);
>   		break;
> -
>   	case PMBUS_VIN_OV_FAULT_LIMIT:
> -		value -= TPS25990_VIN_OVF_OFF;
> -		value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_VIN_OVF_DIV,
> -					  TPS25990_VIN_OVF_NUM);
> -		value = clamp_val(value, 0, 0xf);
> +		if (data->chip_id == tps25990) {
> +			value -= TPS25990_VIN_OVF_OFF;
> +			value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_VIN_OVF_DIV,
> +						  TPS25990_VIN_OVF_NUM);
> +			value = clamp_val(value, 0, 0xf);
> +		}

As pointed out by Sashiko, a clamp is still needed here. Also, again,
according to the datasheet there is still a voltage offset.

>   		ret = pmbus_write_word_data(client, page, reg, value);
>   		break;
>   
>   	case PMBUS_IIN_OC_FAULT_LIMIT:
> -		value -= TPS25990_IIN_OCF_OFF;
> -		value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_IIN_OCF_DIV,
> -					  TPS25990_IIN_OCF_NUM);
> -		value = clamp_val(value, 0, 0x3f);
> +		if (data->chip_id == tps25990) {
> +			value -= TPS25990_IIN_OCF_OFF;
> +			value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_IIN_OCF_DIV,
> +						  TPS25990_IIN_OCF_NUM);
> +			value = clamp_val(value, 0, 0x3f);
> +		}

Again, per datasheets, there does not appear to be a difference between
the two chips regarding TPS25990_VIREF.
>   		ret = pmbus_write_byte_data(client, page, TPS25990_VIREF, value);
>   		break;
>   
> @@ -347,6 +359,59 @@ static const struct regulator_desc tps25990_reg_desc[] = {
>   #endif
>   
>   static struct pmbus_driver_info tps25990_base_info[] = {
> +	[tps1689] = {
> +		.pages = 1,
> +		.format[PSC_VOLTAGE_IN] = direct,
> +		.m[PSC_VOLTAGE_IN] = 1166,
> +		.b[PSC_VOLTAGE_IN] = 0,
> +		.R[PSC_VOLTAGE_IN] = -2,
> +		.format[PSC_VOLTAGE_OUT] = direct,
> +		.m[PSC_VOLTAGE_OUT] = 1166,
> +		.b[PSC_VOLTAGE_OUT] = 0,
> +		.R[PSC_VOLTAGE_OUT] = -2,
> +		.format[PSC_TEMPERATURE] = direct,
> +		.m[PSC_TEMPERATURE] = 140,
> +		.b[PSC_TEMPERATURE] = 32103,
> +		.R[PSC_TEMPERATURE] = -2,
> +		/*
> +		 * Current and Power measurement depends on the ohm value
> +		 * of Rimon. m is multiplied by 1000 below to have an integer
> +		 * and -3 is added to R to compensate.
> +		 */
> +		.format[PSC_CURRENT_IN] = direct,
> +		.m[PSC_CURRENT_IN] = 9548,
> +		.b[PSC_CURRENT_IN] = 0,
> +		.R[PSC_CURRENT_IN] = -6,
> +		.format[PSC_CURRENT_OUT] = direct,
> +		.m[PSC_CURRENT_OUT] = 24347,
> +		.b[PSC_CURRENT_OUT] = 0,
> +		.R[PSC_CURRENT_OUT] = -3,
> +		.format[PSC_POWER] = direct,
> +		.m[PSC_POWER] = 2775,
> +		.b[PSC_POWER] = 0,
> +		.R[PSC_POWER] = -4,
> +		.func[0] = (PMBUS_HAVE_VIN |
> +			    PMBUS_HAVE_VOUT |
> +			    PMBUS_HAVE_VMON |
> +			    PMBUS_HAVE_IIN |
> +			    PMBUS_HAVE_PIN |
> +			    PMBUS_HAVE_TEMP |
> +			    PMBUS_HAVE_STATUS_VOUT |
> +			    PMBUS_HAVE_STATUS_IOUT |
> +			    PMBUS_HAVE_STATUS_INPUT |
> +			    PMBUS_HAVE_STATUS_TEMP |
> +			    PMBUS_HAVE_SAMPLES),
> +
> +		.read_word_data = tps25990_read_word_data,
> +		.write_word_data = tps25990_write_word_data,
> +		.read_byte_data = tps25990_read_byte_data,
> +		.write_byte_data = tps25990_write_byte_data,
> +
> +#if IS_ENABLED(CONFIG_SENSORS_TPS25990_REGULATOR)
> +		.reg_desc = tps25990_reg_desc,
> +		.num_regulators = ARRAY_SIZE(tps25990_reg_desc),
> +#endif
> +		},
>   	[tps25990] = {
>   		.pages = 1,
>   		.format[PSC_VOLTAGE_IN] = direct,
> @@ -389,7 +454,6 @@ static struct pmbus_driver_info tps25990_base_info[] = {
>   		.write_word_data = tps25990_write_word_data,
>   		.read_byte_data = tps25990_read_byte_data,
>   		.write_byte_data = tps25990_write_byte_data,
> -
>   #if IS_ENABLED(CONFIG_SENSORS_TPS25990_REGULATOR)
>   		.reg_desc = tps25990_reg_desc,
>   		.num_regulators = ARRAY_SIZE(tps25990_reg_desc),
> @@ -398,12 +462,14 @@ static struct pmbus_driver_info tps25990_base_info[] = {
>   };
>   
>   static const struct i2c_device_id tps25990_i2c_id[] = {
> +	{ "tps1689", tps1689 },
>   	{ "tps25990", tps25990 },
>   	{}
>   };
>   MODULE_DEVICE_TABLE(i2c, tps25990_i2c_id);
>   
>   static const struct of_device_id tps25990_of_match[] = {
> +	{ .compatible = "ti,tps1689", .data = (void *)tps1689 },
>   	{ .compatible = "ti,tps25990", .data = (void *)tps25990 },
>   	{}
>   };