[PATCH v3 0/6] hwmon: pmbus: add tps25990 efuse support

[PATCH v3 0/6] hwmon: pmbus: add tps25990 efuse support

[Jerome Brunet]

This patchset adds initial support for the Texas Instruments TPS25990
eFuse. The TPS25990 is an integrated, high-current circuit protection and
power management device. TPS25895 may be stacked on the TPS25990 for
higher currents.

This patchset provides basic telemetry support for the device.
On boot, the device is write protected. Limits can be changed in sysfs
if the write protection is removed using the introduced pmbus parameter.

Limits will be restored to the default value device on startup, unless
saved to NVM. Writing the NVM is not supported by the driver at the moment.

As part of this series, PMBus regulator support is improved to better
support write-protected devices.

This patchset depends on the regulator patchset available here [1]

[1]: https://lore.kernel.org/r/20241008-regulator-ignored-data-v2-0-d1251e0ee507@baylibre.com

Changes in v3:
- Grouped hwmon write protect patches from:
  https://lore.kernel.org/r/20240920-pmbus-wp-v1-0-d679ef31c483@baylibre.com
- Link to v2: https://lore.kernel.org/r/20240920-tps25990-v2-0-f3e39bce5173@baylibre.com

Changes in v2:
- Drop PGOOD command support
- Use micro-ohms for rimon property and better handle range.
- Adjust read/write callbacks to let PMBus core do the job by default
- Drop history reset specific properties and remap to the generic ones
- Drop debugfs write_protect property and remap to the generic register
- Link to v1: https://lore.kernel.org/r/20240909-tps25990-v1-0-39b37e43e795@baylibre.com

---
Jerome Brunet (6):
      hwmon: (pmbus/core) allow drivers to override WRITE_PROTECT
      hwmon: (pmbus/core) improve handling of write protected regulators
      hwmon: (pmbus/core) add wp module param
      hwmon: (pmbus/core) clear faults after setting smbalert mask
      dt-bindings: hwmon: pmbus: add ti tps25990 support
      hwmon: (pmbus/tps25990): add initial support

 Documentation/admin-guide/kernel-parameters.txt    |   4 +
 .../bindings/hwmon/pmbus/ti,tps25990.yaml          |  83 ++++
 Documentation/hwmon/index.rst                      |   1 +
 Documentation/hwmon/tps25990.rst                   | 148 +++++++
 drivers/hwmon/pmbus/Kconfig                        |  17 +
 drivers/hwmon/pmbus/Makefile                       |   1 +
 drivers/hwmon/pmbus/pmbus.h                        |   4 +
 drivers/hwmon/pmbus/pmbus_core.c                   |  90 ++++-
 drivers/hwmon/pmbus/tps25990.c                     | 427 +++++++++++++++++++++
 include/linux/pmbus.h                              |  14 +
 10 files changed, 780 insertions(+), 9 deletions(-)
---
base-commit: 516ddbfef736c843866a0b2db559ce89b40ce378
change-id: 20240909-tps25990-34c0cff2be06
prerequisite-change-id: 20240920-regulator-ignored-data-78e7a855643e:v2
prerequisite-patch-id: 468882ab023813ffe8a7eeb210d05b5177a1954a
prerequisite-patch-id: 2d88eb941437003c6ba1cebb09a352a65b94f358
prerequisite-patch-id: e64c06b721cda2e3c41a670251335d8a2a66a236

Best regards,
-- 
Jerome

[PATCH v3 1/6] hwmon: (pmbus/core) allow drivers to override WRITE_PROTECT

[Jerome Brunet]

Use _pmbus_read_byte_data() rather than calling smbus directly to check
the write protection status. This give a chance to device implementing
write protection differently to report back on the actual write protection
status.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index ce7fd4ca9d89b0f0a02e6c99db391a7cfca924a8..085a4dc91d9bad3d2aacdd946b74a094ea9ae458 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2719,9 +2719,7 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	 * limit registers need to be disabled.
 	 */
 	if (!(data->flags & PMBUS_NO_WRITE_PROTECT)) {
-		pmbus_wait(client);
-		ret = i2c_smbus_read_byte_data(client, PMBUS_WRITE_PROTECT);
-		pmbus_update_ts(client, false);
+		ret = _pmbus_read_byte_data(client, 0xff, PMBUS_WRITE_PROTECT);
 
 		if (ret > 0 && (ret & PB_WP_ANY))
 			data->flags |= PMBUS_WRITE_PROTECTED | PMBUS_SKIP_STATUS_CHECK;

-- 
2.45.2

[PATCH v3 2/6] hwmon: (pmbus/core) improve handling of write protected regulators

[Jerome Brunet]

Writing PMBus protected registers does succeed from the smbus perspective,
even if the write is ignored by the device and a communication fault is
raised. This fault will silently be caught and cleared by pmbus irq if one
has been registered.

This means that the regulator call may return succeed although the
operation was ignored.

With this change, the operation which are not supported will be properly
flagged as such and the regulator framework won't even try to execute them.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/hwmon/pmbus/pmbus.h      |  4 ++++
 drivers/hwmon/pmbus/pmbus_core.c | 35 ++++++++++++++++++++++++++++++++++-
 include/linux/pmbus.h            | 14 ++++++++++++++
 3 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index d605412a3173b95041524285ad1fde52fb64ce5a..ddb19c9726d62416244f83603b92d81d82e64891 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -487,6 +487,8 @@ struct pmbus_driver_info {
 /* Regulator ops */
 
 extern const struct regulator_ops pmbus_regulator_ops;
+int pmbus_regulator_init_cb(struct regulator_dev *rdev,
+			    struct regulator_config *config);
 
 /* Macros for filling in array of struct regulator_desc */
 #define PMBUS_REGULATOR_STEP(_name, _id, _voltages, _step, _min_uV)  \
@@ -501,6 +503,7 @@ extern const struct regulator_ops pmbus_regulator_ops;
 		.n_voltages = _voltages,			\
 		.uV_step = _step,				\
 		.min_uV = _min_uV,				\
+		.init_cb = pmbus_regulator_init_cb,		\
 	}
 
 #define PMBUS_REGULATOR(_name, _id)   PMBUS_REGULATOR_STEP(_name, _id, 0, 0, 0)
@@ -516,6 +519,7 @@ extern const struct regulator_ops pmbus_regulator_ops;
 		.n_voltages = _voltages,			\
 		.uV_step = _step,				\
 		.min_uV = _min_uV,				\
+		.init_cb = pmbus_regulator_init_cb,		\
 	}
 
 #define PMBUS_REGULATOR_ONE(_name)   PMBUS_REGULATOR_STEP_ONE(_name, 0, 0, 0)
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 085a4dc91d9bad3d2aacdd946b74a094ea9ae458..7bdd8f2ffcabc51500437182f411e9826cd7a55d 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2721,8 +2721,21 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	if (!(data->flags & PMBUS_NO_WRITE_PROTECT)) {
 		ret = _pmbus_read_byte_data(client, 0xff, PMBUS_WRITE_PROTECT);
 
-		if (ret > 0 && (ret & PB_WP_ANY))
+		switch (ret) {
+		case PB_WP_ALL:
+			data->flags |= PMBUS_OP_PROTECTED;
+			fallthrough;
+		case PB_WP_OP:
+			data->flags |= PMBUS_VOUT_PROTECTED;
+			fallthrough;
+		case PB_WP_VOUT:
 			data->flags |= PMBUS_WRITE_PROTECTED | PMBUS_SKIP_STATUS_CHECK;
+			break;
+
+		default:
+			/* Ignore manufacturer specific and invalid as well as errors */
+			break;
+		}
 	}
 
 	ret = i2c_smbus_read_byte_data(client, PMBUS_REVISION);
@@ -3183,8 +3196,12 @@ static int pmbus_regulator_list_voltage(struct regulator_dev *rdev,
 {
 	struct device *dev = rdev_get_dev(rdev);
 	struct i2c_client *client = to_i2c_client(dev->parent);
+	struct pmbus_data *data = i2c_get_clientdata(client);
 	int val, low, high;
 
+	if (data->flags & PMBUS_VOUT_PROTECTED)
+		return 0;
+
 	if (selector >= rdev->desc->n_voltages ||
 	    selector < rdev->desc->linear_min_sel)
 		return -EINVAL;
@@ -3219,6 +3236,22 @@ const struct regulator_ops pmbus_regulator_ops = {
 };
 EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
 
+int pmbus_regulator_init_cb(struct regulator_dev *rdev,
+			    struct regulator_config *config)
+{
+	struct pmbus_data *data = config->driver_data;
+	struct regulation_constraints *constraints = rdev->constraints;
+
+	if (data->flags & PMBUS_OP_PROTECTED)
+		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_STATUS;
+
+	if (data->flags & PMBUS_VOUT_PROTECTED)
+		constraints->valid_ops_mask &= ~REGULATOR_CHANGE_VOLTAGE;
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(pmbus_regulator_init_cb, PMBUS);
+
 static int pmbus_regulator_register(struct pmbus_data *data)
 {
 	struct device *dev = data->dev;
diff --git a/include/linux/pmbus.h b/include/linux/pmbus.h
index fa9f08164c365a541ee1c6480bafd8c3a8f98138..884040e1383bf41d2eb3b6de72c40e2650178dc6 100644
--- a/include/linux/pmbus.h
+++ b/include/linux/pmbus.h
@@ -73,6 +73,20 @@
  */
 #define PMBUS_USE_COEFFICIENTS_CMD		BIT(5)
 
+/*
+ * PMBUS_OP_PROTECTED
+ * Set if the chip OPERATION command is protected and protection is not
+ * determined by the standard WRITE_PROTECT command.
+ */
+#define PMBUS_OP_PROTECTED			BIT(6)
+
+/*
+ * PMBUS_VOUT_PROTECTED
+ * Set if the chip VOUT_COMMAND command is protected and protection is not
+ * determined by the standard WRITE_PROTECT command.
+ */
+#define PMBUS_VOUT_PROTECTED			BIT(7)
+
 struct pmbus_platform_data {
 	u32 flags;		/* Device specific flags */
 

-- 
2.45.2

[PATCH v3 3/6] hwmon: (pmbus/core) add wp module param

[Jerome Brunet]

Add a module parameter to force the write protection mode of pmbus chips.

2 protections modes are provided to start with:
* 0: Remove the write protection if possible
* 1: Enable full write protection if possible

Of course, if the parameter is not provided, the default write protection
status of the pmbus chips is left untouched.

Suggested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  4 ++
 drivers/hwmon/pmbus/pmbus_core.c                | 74 ++++++++++++++++++-------
 2 files changed, 59 insertions(+), 19 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1518343bbe2237f1d577df5656339d6224b769be..aa79242fe0a9238f618182289f18563ed63cba1c 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4733,6 +4733,10 @@
 			Format: { parport<nr> | timid | 0 }
 			See also Documentation/admin-guide/parport.rst.
 
+	pmbus.wp=	[HW] PMBus Chips write protection forced mode
+			Format: { 0 | 1 }
+			See drivers/hwmon/pmbus/pmbus_core.c
+
 	pmtmr=		[X86] Manual setup of pmtmr I/O Port.
 			Override pmtimer IOPort with a hex value.
 			e.g. pmtmr=0x508
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 7bdd8f2ffcabc51500437182f411e9826cd7a55d..ce697ca03de01c0e5a352f8f6b72671137721868 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -31,6 +31,20 @@
 #define PMBUS_ATTR_ALLOC_SIZE	32
 #define PMBUS_NAME_SIZE		24
 
+/*
+ * PMBus write protect forced mode:
+ * PMBus may come up with a variety of write protection configuration.
+ * 'pmbus_wp' may be used if a particular write protection is necessary.
+ * The ability to actually alter the protection may also depend on the chip
+ * so the actual runtime write protection configuration may differ from
+ * the requested one. pmbus_core currently support the following value:
+ * - 0: write protection removed
+ * - 1: write protection fully enabled, including OPERATION and VOUT_COMMAND
+ *      registers. Chips essentially become read-only with this.
+ */
+static int wp = -1;
+module_param(wp, int, 0444);
+
 struct pmbus_sensor {
 	struct pmbus_sensor *next;
 	char name[PMBUS_NAME_SIZE];	/* sysfs sensor name */
@@ -2665,6 +2679,45 @@ static void pmbus_remove_pec(void *dev)
 	device_remove_file(dev, &dev_attr_pec);
 }
 
+static void pmbus_init_wp(struct i2c_client *client, struct pmbus_data *data)
+{
+	int ret;
+
+	switch (wp) {
+	case 0:
+		_pmbus_write_byte_data(client, 0xff,
+				       PMBUS_WRITE_PROTECT, 0);
+		break;
+
+	case 1:
+		_pmbus_write_byte_data(client, 0xff,
+				       PMBUS_WRITE_PROTECT, PB_WP_ALL);
+		break;
+
+	default:
+		/* Ignore the other values */
+		break;
+	}
+
+	ret = _pmbus_read_byte_data(client, 0xff, PMBUS_WRITE_PROTECT);
+
+	switch (ret) {
+	case PB_WP_ALL:
+		data->flags |= PMBUS_OP_PROTECTED;
+		fallthrough;
+	case PB_WP_OP:
+		data->flags |= PMBUS_VOUT_PROTECTED;
+		fallthrough;
+	case PB_WP_VOUT:
+		data->flags |= PMBUS_WRITE_PROTECTED | PMBUS_SKIP_STATUS_CHECK;
+		break;
+
+	default:
+		/* Ignore manufacturer specific and invalid as well as errors */
+		break;
+	}
+}
+
 static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 			     struct pmbus_driver_info *info)
 {
@@ -2718,25 +2771,8 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	 * faults, and we should not try it. Also, in that case, writes into
 	 * limit registers need to be disabled.
 	 */
-	if (!(data->flags & PMBUS_NO_WRITE_PROTECT)) {
-		ret = _pmbus_read_byte_data(client, 0xff, PMBUS_WRITE_PROTECT);
-
-		switch (ret) {
-		case PB_WP_ALL:
-			data->flags |= PMBUS_OP_PROTECTED;
-			fallthrough;
-		case PB_WP_OP:
-			data->flags |= PMBUS_VOUT_PROTECTED;
-			fallthrough;
-		case PB_WP_VOUT:
-			data->flags |= PMBUS_WRITE_PROTECTED | PMBUS_SKIP_STATUS_CHECK;
-			break;
-
-		default:
-			/* Ignore manufacturer specific and invalid as well as errors */
-			break;
-		}
-	}
+	if (!(data->flags & PMBUS_NO_WRITE_PROTECT))
+		pmbus_init_wp(client, data);
 
 	ret = i2c_smbus_read_byte_data(client, PMBUS_REVISION);
 	if (ret >= 0)

-- 
2.45.2

[PATCH v3 4/6] hwmon: (pmbus/core) clear faults after setting smbalert mask

[Jerome Brunet]

pmbus_write_smbalert_mask() ignores the errors if the chip can't set
smbalert mask the standard way. It is not necessarily a problem for the irq
support if the chip is otherwise properly setup but it may leave an
uncleared fault behind.

pmbus_core will pick the fault on the next register_check(). The register
check will fails regardless of the actual register support by the chip.

This leads to missing attributes or debugfs entries for chips that should
provide them.

We cannot rely on register_check() as PMBUS_SMBALERT_MASK may be read-only.

Unconditionally clear the page fault after setting PMBUS_SMBALERT_MASK to
avoid the problem.

Suggested-by: Guenter Roeck <linux@roeck-us.net>
Fixes: 221819ca4c36 ("hwmon: (pmbus/core) Add interrupt support")
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index ce697ca03de01c0e5a352f8f6b72671137721868..a0a397d571caa1a6620ef095f9cf63d94e8bda1d 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -3346,7 +3346,12 @@ static int pmbus_regulator_notify(struct pmbus_data *data, int page, int event)
 
 static int pmbus_write_smbalert_mask(struct i2c_client *client, u8 page, u8 reg, u8 val)
 {
-	return _pmbus_write_word_data(client, page, PMBUS_SMBALERT_MASK, reg | (val << 8));
+	int ret;
+
+	ret = _pmbus_write_word_data(client, page, PMBUS_SMBALERT_MASK, reg | (val << 8));
+	pmbus_clear_fault_page(client, -1);
+
+	return ret;
 }
 
 static irqreturn_t pmbus_fault_handler(int irq, void *pdata)

-- 
2.45.2

[PATCH v3 5/6] dt-bindings: hwmon: pmbus: add ti tps25990 support

[Jerome Brunet]

Add DT binding for the Texas Instruments TPS25990 eFuse

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 .../bindings/hwmon/pmbus/ti,tps25990.yaml          | 83 ++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/ti,tps25990.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/ti,tps25990.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..f4115870e4509425e88c913f350789ffc8d396c2
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/pmbus/ti,tps25990.yaml
@@ -0,0 +1,83 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/hwmon/pmbus/ti,tps25990.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments TPS25990 Stackable eFuse
+
+maintainers:
+  - Jerome Brunet <jbrunet@baylibre.com>
+
+description:
+  The TI TPS25990 is an integrated, high-current circuit
+  protection and power management device with PMBUS interface
+
+properties:
+  compatible:
+    const: ti,tps25990
+
+  reg:
+    maxItems: 1
+
+  ti,rimon-micro-ohms:
+    description:
+      micro Ohms value of the resistance installed between the Imon pin
+      and the ground reference.
+
+  interrupts:
+    description: PMBUS SMB Alert Interrupt.
+    maxItems: 1
+
+  regulators:
+    type: object
+    description:
+      list of regulators provided by this controller.
+
+    properties:
+      vout:
+        $ref: /schemas/regulator/regulator.yaml#
+        type: object
+        unevaluatedProperties: false
+
+      gpdac1:
+        $ref: /schemas/regulator/regulator.yaml#
+        type: object
+        unevaluatedProperties: false
+
+      gpdac2:
+        $ref: /schemas/regulator/regulator.yaml#
+        type: object
+        unevaluatedProperties: false
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - ti,rimon-micro-ohms
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        hw-monitor@46 {
+            compatible = "ti,tps25990";
+            reg = <0x46>;
+
+            interrupt-parent = <&gpio>;
+            interrupts = <42 IRQ_TYPE_LEVEL_LOW>;
+            ti,rimon-micro-ohms = <1370000000>;
+
+            regulators {
+                cpu0_vout: vout {
+                    regulator-name = "main_cpu0";
+                };
+            };
+        };
+    };

-- 
2.45.2

[PATCH v3 6/6] hwmon: (pmbus/tps25990): add initial support

[Jerome Brunet]

Add initial support for the Texas Instruments TPS25990 eFuse.
This adds the basic PMBUS telemetry support for the device.

Tested-by: Vaishnav Achath <vaishnav.a@ti.com>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 Documentation/hwmon/index.rst    |   1 +
 Documentation/hwmon/tps25990.rst | 148 ++++++++++++++
 drivers/hwmon/pmbus/Kconfig      |  17 ++
 drivers/hwmon/pmbus/Makefile     |   1 +
 drivers/hwmon/pmbus/tps25990.c   | 427 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 594 insertions(+)

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 4d15664bc41e3a675154d60759a73d827c5a0b67..7a59f3374439fe39949b46f73fb0f9a68fb168ef 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -235,6 +235,7 @@ Hardware Monitoring Kernel Drivers
    tmp464
    tmp513
    tps23861
+   tps25990
    tps40422
    tps53679
    tps546d24
diff --git a/Documentation/hwmon/tps25990.rst b/Documentation/hwmon/tps25990.rst
new file mode 100644
index 0000000000000000000000000000000000000000..ed9e74d43e2c2f070d3abe987d93bcdfcf2162ec
--- /dev/null
+++ b/Documentation/hwmon/tps25990.rst
@@ -0,0 +1,148 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver tps25990
+======================
+
+Supported chips:
+
+  * TI TPS25990
+
+    Prefix: 'tps25990'
+
+  * Datasheet
+
+    Publicly available at Texas Instruments website: https://www.ti.com/lit/gpn/tps25990
+
+Author:
+
+	Jerome Brunet <jbrunet@baylibre.com>
+
+Description
+-----------
+
+This driver implements support for TI TPS25990 eFuse.
+This is an integrated, high-current circuit protection and power
+management device with PMBUS interface
+
+Device compliant with:
+
+- PMBus rev 1.3 interface.
+
+Device 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
+are tied together. Resetting the history of a sensor, resets the
+history of all the sensors.
+
+The driver exports the following attributes via the 'sysfs' files
+for input current:
+
+**curr1_average**
+
+**curr1_crit**
+
+**curr1_crit_alarm**
+
+**curr1_highest**
+
+**curr1_input**
+
+**curr1_label**
+
+**curr1_max**
+
+**curr1_max_alarm**
+
+**curr1_reset_history**
+
+The driver provides the following attributes for main input voltage:
+
+**in1_average**
+
+**in1_crit**
+
+**in1_crit_alarm**
+
+**in1_highest**
+
+**in1_input**
+
+**in1_label**
+
+**in1_lcrit**
+
+**in1_lcrit_alarm**
+
+**in1_lowest**
+
+**in1_max**
+
+**in1_max_alarm**
+
+**in1_min**
+
+**in1_min_alarm**
+
+**in1_reset_history**
+
+The driver provides the following attributes for auxiliary input voltage:
+
+**in2_input**
+
+**in2_label**
+
+The driver provides the following attributes for output voltage:
+
+**in3_average**
+
+**in3_input**
+
+**in3_label**
+
+**in3_lowest**
+
+**in3_min**
+
+**in3_min_alarm**
+
+**in3_reset_history**
+
+The driver provides the following attributes for input power:
+
+**power1_alarm**
+
+**power1_average**
+
+**power1_input**
+
+**power1_input_highest**
+
+**power1_label**
+
+**power1_max**
+
+**power1_reset_history**
+
+The driver provides the following attributes for temperature:
+
+**temp1_average**
+
+**temp1_crit**
+
+**temp1_crit_alarm**
+
+**temp1_highest**
+
+**temp1_input**
+
+**temp1_max**
+
+**temp1_max_alarm**
+
+**temp1_reset_history**
+
+The driver provides the following attributes for sampling:
+
+**samples**
+
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index a4f02cad92fdadaafb15bb8459bf56f8e1722568..a3fd442e02774bf3909bf348e471e04c4ab0043a 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -510,6 +510,23 @@ config SENSORS_TDA38640_REGULATOR
 	  If you say yes here you get regulator support for Infineon
 	  TDA38640 as regulator.
 
+config SENSORS_TPS25990
+	tristate "TI TPS25990"
+	help
+	  If you say yes here you get hardware monitoring support for TI
+	  TPS25990.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called tps25990.
+
+config SENSORS_TPS25990_REGULATOR
+	bool "Regulator support for TPS25990 and compatibles"
+	depends on SENSORS_TPS25990 && REGULATOR
+	default SENSORS_TPS25990
+	help
+	  If you say yes here you get regulator support for Texas Instruments
+	  TPS25990.
+
 config SENSORS_TPS40422
 	tristate "TI TPS40422"
 	help
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index d00bcc758b97200b80158e33b0ac41e6e5ac3231..3d3183f8d2a7060eb513f54f4f0a78ba37c09393 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_SENSORS_PXE1610)	+= pxe1610.o
 obj-$(CONFIG_SENSORS_Q54SJ108A2)	+= q54sj108a2.o
 obj-$(CONFIG_SENSORS_STPDDC60)	+= stpddc60.o
 obj-$(CONFIG_SENSORS_TDA38640)	+= tda38640.o
+obj-$(CONFIG_SENSORS_TPS25990)	+= tps25990.o
 obj-$(CONFIG_SENSORS_TPS40422)	+= tps40422.o
 obj-$(CONFIG_SENSORS_TPS53679)	+= tps53679.o
 obj-$(CONFIG_SENSORS_TPS546D24)	+= tps546d24.o
diff --git a/drivers/hwmon/pmbus/tps25990.c b/drivers/hwmon/pmbus/tps25990.c
new file mode 100644
index 0000000000000000000000000000000000000000..03848d671c1014bdccabea62c2bce1b94e568df1
--- /dev/null
+++ b/drivers/hwmon/pmbus/tps25990.c
@@ -0,0 +1,427 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) 2024 BayLibre, SAS.
+// Author: Jerome Brunet <jbrunet@baylibre.com>
+
+#include <linux/debugfs.h>
+#include <linux/err.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#include "pmbus.h"
+
+#define TPS25990_READ_VAUX		0xd0
+#define TPS25990_READ_VIN_MIN		0xd1
+#define TPS25990_READ_VIN_PEAK		0xd2
+#define TPS25990_READ_IIN_PEAK		0xd4
+#define TPS25990_READ_PIN_PEAK		0xd5
+#define TPS25990_READ_TEMP_AVG		0xd6
+#define TPS25990_READ_TEMP_PEAK		0xd7
+#define TPS25990_READ_VOUT_MIN		0xda
+#define TPS25990_READ_VIN_AVG		0xdc
+#define TPS25990_READ_VOUT_AVG		0xdd
+#define TPS25990_READ_IIN_AVG		0xde
+#define TPS25990_READ_PIN_AVG		0xdf
+#define TPS25990_VIREF			0xe0
+#define TPS25990_PK_MIN_AVG		0xea
+#define  PK_MIN_AVG_RST_PEAK		BIT(7)
+#define  PK_MIN_AVG_RST_AVG		BIT(6)
+#define  PK_MIN_AVG_RST_MIN		BIT(5)
+#define  PK_MIN_AVG_AVG_CNT		GENMASK(2, 0)
+#define TPS25990_MFR_WRITE_PROTECT	0xf8
+#define  TPS25990_UNLOCKED		BIT(7)
+
+#define TPS25990_8B_SHIFT		2
+#define TPS25990_VIN_OVF_NUM		525100
+#define TPS25990_VIN_OVF_DIV		10163
+#define TPS25990_VIN_OVF_OFF		155
+#define TPS25990_IIN_OCF_NUM		953800
+#define TPS25990_IIN_OCF_DIV		129278
+#define TPS25990_IIN_OCF_OFF		157
+
+#define PK_MIN_AVG_RST_MASK		(PK_MIN_AVG_RST_PEAK | \
+					 PK_MIN_AVG_RST_AVG  | \
+					 PK_MIN_AVG_RST_MIN)
+
+/*
+ * Arbitrary default Rimon value: 1kOhm
+ * This correspond to an overcurrent limit of 55A, close to the specified limit
+ * of un-stacked TPS25990 and makes further calculation easier to setup in
+ * sensor.conf, if necessary
+ */
+#define TPS25990_DEFAULT_RIMON		1000000000
+
+static void tps25990_set_m(int *m, u32 rimon)
+{
+	u64 val = ((u64)*m) * rimon;
+
+	/* Make sure m fits the s32 type */
+	*m = DIV_ROUND_CLOSEST_ULL(val, 1000000);
+}
+
+static int tps25990_mfr_write_protect_set(struct i2c_client *client,
+					  u8 protect)
+{
+	/*
+	 * The chip has a single protection mode, set it regardless of
+	 * the specific protection requested
+	 */
+	return pmbus_write_byte_data(client, -1, TPS25990_MFR_WRITE_PROTECT,
+				     protect ? 0x0 : 0xa2);
+}
+
+static int tps25990_mfr_write_protect_get(struct i2c_client *client)
+{
+	int ret = pmbus_read_byte_data(client, -1, TPS25990_MFR_WRITE_PROTECT);
+
+	if (ret < 0)
+		return ret;
+
+	return (ret & TPS25990_UNLOCKED) ? 0 : PB_WP_ALL;
+}
+
+static int tps25990_read_word_data(struct i2c_client *client,
+				   int page, int phase, int reg)
+{
+	int ret;
+
+	switch (reg) {
+	case PMBUS_VIRT_READ_VIN_MAX:
+		ret = pmbus_read_word_data(client, page, phase,
+					   TPS25990_READ_VIN_PEAK);
+		break;
+
+	case PMBUS_VIRT_READ_VIN_MIN:
+		ret = pmbus_read_word_data(client, page, phase,
+					   TPS25990_READ_VIN_MIN);
+		break;
+
+	case PMBUS_VIRT_READ_VIN_AVG:
+		ret = pmbus_read_word_data(client, page, phase,
+					   TPS25990_READ_VIN_AVG);
+		break;
+
+	case PMBUS_VIRT_READ_VOUT_MIN:
+		ret = pmbus_read_word_data(client, page, phase,
+					   TPS25990_READ_VOUT_MIN);
+		break;
+
+	case PMBUS_VIRT_READ_VOUT_AVG:
+		ret = pmbus_read_word_data(client, page, phase,
+					   TPS25990_READ_VOUT_AVG);
+		break;
+
+	case PMBUS_VIRT_READ_IIN_AVG:
+		ret = pmbus_read_word_data(client, page, phase,
+					   TPS25990_READ_IIN_AVG);
+		break;
+
+	case PMBUS_VIRT_READ_IIN_MAX:
+		return TPS25990_READ_IIN_PEAK;
+		ret = pmbus_read_word_data(client, page, phase,
+					   TPS25990_READ_IIN_PEAK);
+		break;
+
+	case PMBUS_VIRT_READ_TEMP_AVG:
+		ret = pmbus_read_word_data(client, page, phase,
+					   TPS25990_READ_TEMP_AVG);
+		break;
+
+	case PMBUS_VIRT_READ_TEMP_MAX:
+		ret = pmbus_read_word_data(client, page, phase,
+					   TPS25990_READ_TEMP_PEAK);
+		break;
+
+	case PMBUS_VIRT_READ_PIN_AVG:
+		ret = pmbus_read_word_data(client, page, phase,
+					   TPS25990_READ_PIN_AVG);
+		break;
+
+	case PMBUS_VIRT_READ_PIN_MAX:
+		ret = pmbus_read_word_data(client, page, phase,
+					   TPS25990_READ_PIN_PEAK);
+		break;
+
+	case PMBUS_VIRT_READ_VMON:
+		ret = pmbus_read_word_data(client, page, phase,
+					   TPS25990_READ_VAUX);
+		break;
+
+	case PMBUS_VIN_UV_WARN_LIMIT:
+	case PMBUS_VIN_UV_FAULT_LIMIT:
+	case PMBUS_VIN_OV_WARN_LIMIT:
+	case PMBUS_VOUT_UV_WARN_LIMIT:
+	case PMBUS_IIN_OC_WARN_LIMIT:
+	case PMBUS_OT_WARN_LIMIT:
+	case PMBUS_OT_FAULT_LIMIT:
+	case PMBUS_PIN_OP_WARN_LIMIT:
+		/*
+		 * These registers provide an 8 bits value instead of a
+		 * 10bits one. Just shifting twice the register value is
+		 * enough to make the sensor type conversion work, even
+		 * if the datasheet provides different m, b and R for
+		 * those.
+		 */
+		ret = pmbus_read_word_data(client, page, phase, reg);
+		if (ret < 0)
+			break;
+		ret <<= TPS25990_8B_SHIFT;
+		break;
+
+	case PMBUS_VIN_OV_FAULT_LIMIT:
+		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;
+		break;
+
+	case PMBUS_IIN_OC_FAULT_LIMIT:
+		/*
+		 * VIREF directly sets the over-current limit at which the eFuse
+		 * will turn the FET off and trigger a fault. Expose it through
+		 * this generic property instead of a manufacturer specific one.
+		 */
+		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;
+		break;
+
+	case PMBUS_VIRT_SAMPLES:
+		ret = pmbus_read_byte_data(client, page, TPS25990_PK_MIN_AVG);
+		if (ret < 0)
+			break;
+		ret = 1 << FIELD_GET(PK_MIN_AVG_AVG_CNT, ret);
+		break;
+
+	case PMBUS_VIRT_RESET_TEMP_HISTORY:
+	case PMBUS_VIRT_RESET_VIN_HISTORY:
+	case PMBUS_VIRT_RESET_IIN_HISTORY:
+	case PMBUS_VIRT_RESET_PIN_HISTORY:
+	case PMBUS_VIRT_RESET_VOUT_HISTORY:
+		ret = 0;
+		break;
+
+	default:
+		ret = -ENODATA;
+		break;
+	}
+
+	return ret;
+}
+
+static int tps25990_write_word_data(struct i2c_client *client,
+				    int page, int reg, u16 value)
+{
+	int ret;
+
+	switch (reg) {
+	case PMBUS_VIN_UV_WARN_LIMIT:
+	case PMBUS_VIN_UV_FAULT_LIMIT:
+	case PMBUS_VIN_OV_WARN_LIMIT:
+	case PMBUS_VOUT_UV_WARN_LIMIT:
+	case PMBUS_IIN_OC_WARN_LIMIT:
+	case PMBUS_OT_WARN_LIMIT:
+	case PMBUS_OT_FAULT_LIMIT:
+	case PMBUS_PIN_OP_WARN_LIMIT:
+		value >>= TPS25990_8B_SHIFT;
+		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);
+		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);
+		ret = pmbus_write_byte_data(client, page, TPS25990_VIREF, value);
+		break;
+
+	case PMBUS_VIRT_SAMPLES:
+		value = clamp_val(value, 1, 1 << PK_MIN_AVG_AVG_CNT);
+		value = ilog2(value);
+		ret = pmbus_update_byte_data(client, page, TPS25990_PK_MIN_AVG,
+					     PK_MIN_AVG_AVG_CNT,
+					     FIELD_PREP(PK_MIN_AVG_AVG_CNT, value));
+		break;
+
+	case PMBUS_VIRT_RESET_TEMP_HISTORY:
+	case PMBUS_VIRT_RESET_VIN_HISTORY:
+	case PMBUS_VIRT_RESET_IIN_HISTORY:
+	case PMBUS_VIRT_RESET_PIN_HISTORY:
+	case PMBUS_VIRT_RESET_VOUT_HISTORY:
+		/*
+		 * TPS25990 has history resets based on MIN/AVG/PEAK instead of per
+		 * sensor type. Exposing this quirk in hwmon is not desirable so
+		 * reset MIN, AVG and PEAK together. Even is there effectively only
+		 * one reset, which resets everything, expose the 5 entries so
+		 * userspace is not required map a sensor type to another to trigger
+		 * a reset
+		 */
+		ret = pmbus_update_byte_data(client, 0, TPS25990_PK_MIN_AVG,
+					     PK_MIN_AVG_RST_MASK,
+					     PK_MIN_AVG_RST_MASK);
+		break;
+
+	default:
+		ret = -ENODATA;
+		break;
+	}
+
+	return ret;
+}
+
+static int tps25990_read_byte_data(struct i2c_client *client,
+				   int page, int reg)
+{
+	int ret;
+
+	switch (reg) {
+	case PMBUS_WRITE_PROTECT:
+		ret = tps25990_mfr_write_protect_get(client);
+		break;
+
+	default:
+		ret = -ENODATA;
+		break;
+	}
+
+	return ret;
+}
+
+static int tps25990_write_byte_data(struct i2c_client *client,
+				    int page, int reg, u8 byte)
+{
+	int ret;
+
+	switch (reg) {
+	case PMBUS_WRITE_PROTECT:
+		ret = tps25990_mfr_write_protect_set(client, byte);
+		break;
+
+	default:
+		ret = -ENODATA;
+		break;
+	}
+
+	return ret;
+}
+
+#if IS_ENABLED(CONFIG_SENSORS_TPS25990_REGULATOR)
+static const struct regulator_desc tps25990_reg_desc[] = {
+	PMBUS_REGULATOR_ONE("vout"),
+};
+#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,
+
+#if IS_ENABLED(CONFIG_SENSORS_TPS25990_REGULATOR)
+	.reg_desc = tps25990_reg_desc,
+	.num_regulators = ARRAY_SIZE(tps25990_reg_desc),
+#endif
+};
+
+static const struct i2c_device_id tps25990_i2c_id[] = {
+	{ "tps25990" },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, tps25990_i2c_id);
+
+static const struct of_device_id tps25990_of_match[] = {
+	{ .compatible = "ti,tps25990" },
+	{}
+};
+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;
+	u32 rimon = TPS25990_DEFAULT_RIMON;
+	int ret;
+
+	ret = device_property_read_u32(dev, "ti,rimon-micro-ohms", &rimon);
+	if (ret < 0 && ret != -EINVAL)
+		return dev_err_probe(dev, ret, "failed to get rimon\n");
+
+	info = devm_kmemdup(dev, &tps25990_base_info, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	/* 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);
+
+	return pmbus_do_probe(client, info);
+}
+
+static struct i2c_driver tps25990_driver = {
+	.driver = {
+		.name = "tps25990",
+		.of_match_table = tps25990_of_match,
+	},
+	.probe = tps25990_probe,
+	.id_table = tps25990_i2c_id,
+};
+module_i2c_driver(tps25990_driver);
+
+MODULE_AUTHOR("Jerome Brunet <jbrunet@baylibre.com>");
+MODULE_DESCRIPTION("PMBUS driver for TPS25990 eFuse");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(PMBUS);

-- 
2.45.2

Re: [PATCH v3 6/6] hwmon: (pmbus/tps25990): add initial support

[kernel test robot]

Hi Jerome,

kernel test robot noticed the following build errors:

[auto build test ERROR on 516ddbfef736c843866a0b2db559ce89b40ce378]

url:    https://github.com/intel-lab-lkp/linux/commits/Jerome-Brunet/hwmon-pmbus-core-allow-drivers-to-override-WRITE_PROTECT/20241025-021525
base:   516ddbfef736c843866a0b2db559ce89b40ce378
patch link:    https://lore.kernel.org/r/20241024-tps25990-v3-6-b6a6e9d4b506%40baylibre.com
patch subject: [PATCH v3 6/6] hwmon: (pmbus/tps25990): add initial support
config: xtensa-allyesconfig (https://download.01.org/0day-ci/archive/20241025/202410252141.XSGtEsDP-lkp@intel.com/config)
compiler: xtensa-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241025/202410252141.XSGtEsDP-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410252141.XSGtEsDP-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/hwmon/pmbus/tps25990.c: In function 'tps25990_read_word_data':
>> drivers/hwmon/pmbus/tps25990.c:201:28: error: implicit declaration of function 'FIELD_GET' [-Wimplicit-function-declaration]
     201 |                 ret = 1 << FIELD_GET(PK_MIN_AVG_AVG_CNT, ret);
         |                            ^~~~~~~~~
   drivers/hwmon/pmbus/tps25990.c: In function 'tps25990_write_word_data':
>> drivers/hwmon/pmbus/tps25990.c:260:46: error: implicit declaration of function 'FIELD_PREP' [-Wimplicit-function-declaration]
     260 |                                              FIELD_PREP(PK_MIN_AVG_AVG_CNT, value));
         |                                              ^~~~~~~~~~

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for GET_FREE_REGION
   Depends on [n]: SPARSEMEM [=n]
   Selected by [y]:
   - RESOURCE_KUNIT_TEST [=y] && RUNTIME_TESTING_MENU [=y] && KUNIT [=y]


vim +/FIELD_GET +201 drivers/hwmon/pmbus/tps25990.c

    85	
    86	static int tps25990_read_word_data(struct i2c_client *client,
    87					   int page, int phase, int reg)
    88	{
    89		int ret;
    90	
    91		switch (reg) {
    92		case PMBUS_VIRT_READ_VIN_MAX:
    93			ret = pmbus_read_word_data(client, page, phase,
    94						   TPS25990_READ_VIN_PEAK);
    95			break;
    96	
    97		case PMBUS_VIRT_READ_VIN_MIN:
    98			ret = pmbus_read_word_data(client, page, phase,
    99						   TPS25990_READ_VIN_MIN);
   100			break;
   101	
   102		case PMBUS_VIRT_READ_VIN_AVG:
   103			ret = pmbus_read_word_data(client, page, phase,
   104						   TPS25990_READ_VIN_AVG);
   105			break;
   106	
   107		case PMBUS_VIRT_READ_VOUT_MIN:
   108			ret = pmbus_read_word_data(client, page, phase,
   109						   TPS25990_READ_VOUT_MIN);
   110			break;
   111	
   112		case PMBUS_VIRT_READ_VOUT_AVG:
   113			ret = pmbus_read_word_data(client, page, phase,
   114						   TPS25990_READ_VOUT_AVG);
   115			break;
   116	
   117		case PMBUS_VIRT_READ_IIN_AVG:
   118			ret = pmbus_read_word_data(client, page, phase,
   119						   TPS25990_READ_IIN_AVG);
   120			break;
   121	
   122		case PMBUS_VIRT_READ_IIN_MAX:
   123			return TPS25990_READ_IIN_PEAK;
   124			ret = pmbus_read_word_data(client, page, phase,
   125						   TPS25990_READ_IIN_PEAK);
   126			break;
   127	
   128		case PMBUS_VIRT_READ_TEMP_AVG:
   129			ret = pmbus_read_word_data(client, page, phase,
   130						   TPS25990_READ_TEMP_AVG);
   131			break;
   132	
   133		case PMBUS_VIRT_READ_TEMP_MAX:
   134			ret = pmbus_read_word_data(client, page, phase,
   135						   TPS25990_READ_TEMP_PEAK);
   136			break;
   137	
   138		case PMBUS_VIRT_READ_PIN_AVG:
   139			ret = pmbus_read_word_data(client, page, phase,
   140						   TPS25990_READ_PIN_AVG);
   141			break;
   142	
   143		case PMBUS_VIRT_READ_PIN_MAX:
   144			ret = pmbus_read_word_data(client, page, phase,
   145						   TPS25990_READ_PIN_PEAK);
   146			break;
   147	
   148		case PMBUS_VIRT_READ_VMON:
   149			ret = pmbus_read_word_data(client, page, phase,
   150						   TPS25990_READ_VAUX);
   151			break;
   152	
   153		case PMBUS_VIN_UV_WARN_LIMIT:
   154		case PMBUS_VIN_UV_FAULT_LIMIT:
   155		case PMBUS_VIN_OV_WARN_LIMIT:
   156		case PMBUS_VOUT_UV_WARN_LIMIT:
   157		case PMBUS_IIN_OC_WARN_LIMIT:
   158		case PMBUS_OT_WARN_LIMIT:
   159		case PMBUS_OT_FAULT_LIMIT:
   160		case PMBUS_PIN_OP_WARN_LIMIT:
   161			/*
   162			 * These registers provide an 8 bits value instead of a
   163			 * 10bits one. Just shifting twice the register value is
   164			 * enough to make the sensor type conversion work, even
   165			 * if the datasheet provides different m, b and R for
   166			 * those.
   167			 */
   168			ret = pmbus_read_word_data(client, page, phase, reg);
   169			if (ret < 0)
   170				break;
   171			ret <<= TPS25990_8B_SHIFT;
   172			break;
   173	
   174		case PMBUS_VIN_OV_FAULT_LIMIT:
   175			ret = pmbus_read_word_data(client, page, phase, reg);
   176			if (ret < 0)
   177				break;
   178			ret = DIV_ROUND_CLOSEST(ret * TPS25990_VIN_OVF_NUM,
   179						TPS25990_VIN_OVF_DIV);
   180			ret += TPS25990_VIN_OVF_OFF;
   181			break;
   182	
   183		case PMBUS_IIN_OC_FAULT_LIMIT:
   184			/*
   185			 * VIREF directly sets the over-current limit at which the eFuse
   186			 * will turn the FET off and trigger a fault. Expose it through
   187			 * this generic property instead of a manufacturer specific one.
   188			 */
   189			ret = pmbus_read_byte_data(client, page, TPS25990_VIREF);
   190			if (ret < 0)
   191				break;
   192			ret = DIV_ROUND_CLOSEST(ret * TPS25990_IIN_OCF_NUM,
   193						TPS25990_IIN_OCF_DIV);
   194			ret += TPS25990_IIN_OCF_OFF;
   195			break;
   196	
   197		case PMBUS_VIRT_SAMPLES:
   198			ret = pmbus_read_byte_data(client, page, TPS25990_PK_MIN_AVG);
   199			if (ret < 0)
   200				break;
 > 201			ret = 1 << FIELD_GET(PK_MIN_AVG_AVG_CNT, ret);
   202			break;
   203	
   204		case PMBUS_VIRT_RESET_TEMP_HISTORY:
   205		case PMBUS_VIRT_RESET_VIN_HISTORY:
   206		case PMBUS_VIRT_RESET_IIN_HISTORY:
   207		case PMBUS_VIRT_RESET_PIN_HISTORY:
   208		case PMBUS_VIRT_RESET_VOUT_HISTORY:
   209			ret = 0;
   210			break;
   211	
   212		default:
   213			ret = -ENODATA;
   214			break;
   215		}
   216	
   217		return ret;
   218	}
   219	
   220	static int tps25990_write_word_data(struct i2c_client *client,
   221					    int page, int reg, u16 value)
   222	{
   223		int ret;
   224	
   225		switch (reg) {
   226		case PMBUS_VIN_UV_WARN_LIMIT:
   227		case PMBUS_VIN_UV_FAULT_LIMIT:
   228		case PMBUS_VIN_OV_WARN_LIMIT:
   229		case PMBUS_VOUT_UV_WARN_LIMIT:
   230		case PMBUS_IIN_OC_WARN_LIMIT:
   231		case PMBUS_OT_WARN_LIMIT:
   232		case PMBUS_OT_FAULT_LIMIT:
   233		case PMBUS_PIN_OP_WARN_LIMIT:
   234			value >>= TPS25990_8B_SHIFT;
   235			value = clamp_val(value, 0, 0xff);
   236			ret = pmbus_write_word_data(client, page, reg, value);
   237			break;
   238	
   239		case PMBUS_VIN_OV_FAULT_LIMIT:
   240			value -= TPS25990_VIN_OVF_OFF;
   241			value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_VIN_OVF_DIV,
   242						  TPS25990_VIN_OVF_NUM);
   243			value = clamp_val(value, 0, 0xf);
   244			ret = pmbus_write_word_data(client, page, reg, value);
   245			break;
   246	
   247		case PMBUS_IIN_OC_FAULT_LIMIT:
   248			value -= TPS25990_IIN_OCF_OFF;
   249			value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_IIN_OCF_DIV,
   250						TPS25990_IIN_OCF_NUM);
   251			value = clamp_val(value, 0, 0x3f);
   252			ret = pmbus_write_byte_data(client, page, TPS25990_VIREF, value);
   253			break;
   254	
   255		case PMBUS_VIRT_SAMPLES:
   256			value = clamp_val(value, 1, 1 << PK_MIN_AVG_AVG_CNT);
   257			value = ilog2(value);
   258			ret = pmbus_update_byte_data(client, page, TPS25990_PK_MIN_AVG,
   259						     PK_MIN_AVG_AVG_CNT,
 > 260						     FIELD_PREP(PK_MIN_AVG_AVG_CNT, value));
   261			break;
   262	
   263		case PMBUS_VIRT_RESET_TEMP_HISTORY:
   264		case PMBUS_VIRT_RESET_VIN_HISTORY:
   265		case PMBUS_VIRT_RESET_IIN_HISTORY:
   266		case PMBUS_VIRT_RESET_PIN_HISTORY:
   267		case PMBUS_VIRT_RESET_VOUT_HISTORY:
   268			/*
   269			 * TPS25990 has history resets based on MIN/AVG/PEAK instead of per
   270			 * sensor type. Exposing this quirk in hwmon is not desirable so
   271			 * reset MIN, AVG and PEAK together. Even is there effectively only
   272			 * one reset, which resets everything, expose the 5 entries so
   273			 * userspace is not required map a sensor type to another to trigger
   274			 * a reset
   275			 */
   276			ret = pmbus_update_byte_data(client, 0, TPS25990_PK_MIN_AVG,
   277						     PK_MIN_AVG_RST_MASK,
   278						     PK_MIN_AVG_RST_MASK);
   279			break;
   280	
   281		default:
   282			ret = -ENODATA;
   283			break;
   284		}
   285	
   286		return ret;
   287	}
   288	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 6/6] hwmon: (pmbus/tps25990): add initial support

[kernel test robot]

Hi Jerome,

kernel test robot noticed the following build errors:

[auto build test ERROR on 516ddbfef736c843866a0b2db559ce89b40ce378]

url:    https://github.com/intel-lab-lkp/linux/commits/Jerome-Brunet/hwmon-pmbus-core-allow-drivers-to-override-WRITE_PROTECT/20241025-021525
base:   516ddbfef736c843866a0b2db559ce89b40ce378
patch link:    https://lore.kernel.org/r/20241024-tps25990-v3-6-b6a6e9d4b506%40baylibre.com
patch subject: [PATCH v3 6/6] hwmon: (pmbus/tps25990): add initial support
config: i386-randconfig-061-20241029 (https://download.01.org/0day-ci/archive/20241029/202410292137.CftTdTLk-lkp@intel.com/config)
compiler: clang version 19.1.2 (https://github.com/llvm/llvm-project 7ba7d8e2f7b6445b60679da826210cdde29eaf8b)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241029/202410292137.CftTdTLk-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410292137.CftTdTLk-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/hwmon/pmbus/tps25990.c:9:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:21:
   In file included from include/linux/mm.h:2213:
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> drivers/hwmon/pmbus/tps25990.c:201:14: error: call to undeclared function 'FIELD_GET'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     201 |                 ret = 1 << FIELD_GET(PK_MIN_AVG_AVG_CNT, ret);
         |                            ^
>> drivers/hwmon/pmbus/tps25990.c:260:11: error: call to undeclared function 'FIELD_PREP'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     260 |                                              FIELD_PREP(PK_MIN_AVG_AVG_CNT, value));
         |                                              ^
   1 warning and 2 errors generated.


vim +/FIELD_GET +201 drivers/hwmon/pmbus/tps25990.c

    85	
    86	static int tps25990_read_word_data(struct i2c_client *client,
    87					   int page, int phase, int reg)
    88	{
    89		int ret;
    90	
    91		switch (reg) {
    92		case PMBUS_VIRT_READ_VIN_MAX:
    93			ret = pmbus_read_word_data(client, page, phase,
    94						   TPS25990_READ_VIN_PEAK);
    95			break;
    96	
    97		case PMBUS_VIRT_READ_VIN_MIN:
    98			ret = pmbus_read_word_data(client, page, phase,
    99						   TPS25990_READ_VIN_MIN);
   100			break;
   101	
   102		case PMBUS_VIRT_READ_VIN_AVG:
   103			ret = pmbus_read_word_data(client, page, phase,
   104						   TPS25990_READ_VIN_AVG);
   105			break;
   106	
   107		case PMBUS_VIRT_READ_VOUT_MIN:
   108			ret = pmbus_read_word_data(client, page, phase,
   109						   TPS25990_READ_VOUT_MIN);
   110			break;
   111	
   112		case PMBUS_VIRT_READ_VOUT_AVG:
   113			ret = pmbus_read_word_data(client, page, phase,
   114						   TPS25990_READ_VOUT_AVG);
   115			break;
   116	
   117		case PMBUS_VIRT_READ_IIN_AVG:
   118			ret = pmbus_read_word_data(client, page, phase,
   119						   TPS25990_READ_IIN_AVG);
   120			break;
   121	
   122		case PMBUS_VIRT_READ_IIN_MAX:
   123			return TPS25990_READ_IIN_PEAK;
   124			ret = pmbus_read_word_data(client, page, phase,
   125						   TPS25990_READ_IIN_PEAK);
   126			break;
   127	
   128		case PMBUS_VIRT_READ_TEMP_AVG:
   129			ret = pmbus_read_word_data(client, page, phase,
   130						   TPS25990_READ_TEMP_AVG);
   131			break;
   132	
   133		case PMBUS_VIRT_READ_TEMP_MAX:
   134			ret = pmbus_read_word_data(client, page, phase,
   135						   TPS25990_READ_TEMP_PEAK);
   136			break;
   137	
   138		case PMBUS_VIRT_READ_PIN_AVG:
   139			ret = pmbus_read_word_data(client, page, phase,
   140						   TPS25990_READ_PIN_AVG);
   141			break;
   142	
   143		case PMBUS_VIRT_READ_PIN_MAX:
   144			ret = pmbus_read_word_data(client, page, phase,
   145						   TPS25990_READ_PIN_PEAK);
   146			break;
   147	
   148		case PMBUS_VIRT_READ_VMON:
   149			ret = pmbus_read_word_data(client, page, phase,
   150						   TPS25990_READ_VAUX);
   151			break;
   152	
   153		case PMBUS_VIN_UV_WARN_LIMIT:
   154		case PMBUS_VIN_UV_FAULT_LIMIT:
   155		case PMBUS_VIN_OV_WARN_LIMIT:
   156		case PMBUS_VOUT_UV_WARN_LIMIT:
   157		case PMBUS_IIN_OC_WARN_LIMIT:
   158		case PMBUS_OT_WARN_LIMIT:
   159		case PMBUS_OT_FAULT_LIMIT:
   160		case PMBUS_PIN_OP_WARN_LIMIT:
   161			/*
   162			 * These registers provide an 8 bits value instead of a
   163			 * 10bits one. Just shifting twice the register value is
   164			 * enough to make the sensor type conversion work, even
   165			 * if the datasheet provides different m, b and R for
   166			 * those.
   167			 */
   168			ret = pmbus_read_word_data(client, page, phase, reg);
   169			if (ret < 0)
   170				break;
   171			ret <<= TPS25990_8B_SHIFT;
   172			break;
   173	
   174		case PMBUS_VIN_OV_FAULT_LIMIT:
   175			ret = pmbus_read_word_data(client, page, phase, reg);
   176			if (ret < 0)
   177				break;
   178			ret = DIV_ROUND_CLOSEST(ret * TPS25990_VIN_OVF_NUM,
   179						TPS25990_VIN_OVF_DIV);
   180			ret += TPS25990_VIN_OVF_OFF;
   181			break;
   182	
   183		case PMBUS_IIN_OC_FAULT_LIMIT:
   184			/*
   185			 * VIREF directly sets the over-current limit at which the eFuse
   186			 * will turn the FET off and trigger a fault. Expose it through
   187			 * this generic property instead of a manufacturer specific one.
   188			 */
   189			ret = pmbus_read_byte_data(client, page, TPS25990_VIREF);
   190			if (ret < 0)
   191				break;
   192			ret = DIV_ROUND_CLOSEST(ret * TPS25990_IIN_OCF_NUM,
   193						TPS25990_IIN_OCF_DIV);
   194			ret += TPS25990_IIN_OCF_OFF;
   195			break;
   196	
   197		case PMBUS_VIRT_SAMPLES:
   198			ret = pmbus_read_byte_data(client, page, TPS25990_PK_MIN_AVG);
   199			if (ret < 0)
   200				break;
 > 201			ret = 1 << FIELD_GET(PK_MIN_AVG_AVG_CNT, ret);
   202			break;
   203	
   204		case PMBUS_VIRT_RESET_TEMP_HISTORY:
   205		case PMBUS_VIRT_RESET_VIN_HISTORY:
   206		case PMBUS_VIRT_RESET_IIN_HISTORY:
   207		case PMBUS_VIRT_RESET_PIN_HISTORY:
   208		case PMBUS_VIRT_RESET_VOUT_HISTORY:
   209			ret = 0;
   210			break;
   211	
   212		default:
   213			ret = -ENODATA;
   214			break;
   215		}
   216	
   217		return ret;
   218	}
   219	
   220	static int tps25990_write_word_data(struct i2c_client *client,
   221					    int page, int reg, u16 value)
   222	{
   223		int ret;
   224	
   225		switch (reg) {
   226		case PMBUS_VIN_UV_WARN_LIMIT:
   227		case PMBUS_VIN_UV_FAULT_LIMIT:
   228		case PMBUS_VIN_OV_WARN_LIMIT:
   229		case PMBUS_VOUT_UV_WARN_LIMIT:
   230		case PMBUS_IIN_OC_WARN_LIMIT:
   231		case PMBUS_OT_WARN_LIMIT:
   232		case PMBUS_OT_FAULT_LIMIT:
   233		case PMBUS_PIN_OP_WARN_LIMIT:
   234			value >>= TPS25990_8B_SHIFT;
   235			value = clamp_val(value, 0, 0xff);
   236			ret = pmbus_write_word_data(client, page, reg, value);
   237			break;
   238	
   239		case PMBUS_VIN_OV_FAULT_LIMIT:
   240			value -= TPS25990_VIN_OVF_OFF;
   241			value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_VIN_OVF_DIV,
   242						  TPS25990_VIN_OVF_NUM);
   243			value = clamp_val(value, 0, 0xf);
   244			ret = pmbus_write_word_data(client, page, reg, value);
   245			break;
   246	
   247		case PMBUS_IIN_OC_FAULT_LIMIT:
   248			value -= TPS25990_IIN_OCF_OFF;
   249			value = DIV_ROUND_CLOSEST(((unsigned int)value) * TPS25990_IIN_OCF_DIV,
   250						TPS25990_IIN_OCF_NUM);
   251			value = clamp_val(value, 0, 0x3f);
   252			ret = pmbus_write_byte_data(client, page, TPS25990_VIREF, value);
   253			break;
   254	
   255		case PMBUS_VIRT_SAMPLES:
   256			value = clamp_val(value, 1, 1 << PK_MIN_AVG_AVG_CNT);
   257			value = ilog2(value);
   258			ret = pmbus_update_byte_data(client, page, TPS25990_PK_MIN_AVG,
   259						     PK_MIN_AVG_AVG_CNT,
 > 260						     FIELD_PREP(PK_MIN_AVG_AVG_CNT, value));
   261			break;
   262	
   263		case PMBUS_VIRT_RESET_TEMP_HISTORY:
   264		case PMBUS_VIRT_RESET_VIN_HISTORY:
   265		case PMBUS_VIRT_RESET_IIN_HISTORY:
   266		case PMBUS_VIRT_RESET_PIN_HISTORY:
   267		case PMBUS_VIRT_RESET_VOUT_HISTORY:
   268			/*
   269			 * TPS25990 has history resets based on MIN/AVG/PEAK instead of per
   270			 * sensor type. Exposing this quirk in hwmon is not desirable so
   271			 * reset MIN, AVG and PEAK together. Even is there effectively only
   272			 * one reset, which resets everything, expose the 5 entries so
   273			 * userspace is not required map a sensor type to another to trigger
   274			 * a reset
   275			 */
   276			ret = pmbus_update_byte_data(client, 0, TPS25990_PK_MIN_AVG,
   277						     PK_MIN_AVG_RST_MASK,
   278						     PK_MIN_AVG_RST_MASK);
   279			break;
   280	
   281		default:
   282			ret = -ENODATA;
   283			break;
   284		}
   285	
   286		return ret;
   287	}
   288	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 2/6] hwmon: (pmbus/core) improve handling of write protected regulators

[Guenter Roeck]

On Thu, Oct 24, 2024 at 08:10:36PM +0200, Jerome Brunet wrote:
> Writing PMBus protected registers does succeed from the smbus perspective,
> even if the write is ignored by the device and a communication fault is
> raised. This fault will silently be caught and cleared by pmbus irq if one
> has been registered.
> 
> This means that the regulator call may return succeed although the
> operation was ignored.
> 
> With this change, the operation which are not supported will be properly
> flagged as such and the regulator framework won't even try to execute them.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/hwmon/pmbus/pmbus.h      |  4 ++++
>  drivers/hwmon/pmbus/pmbus_core.c | 35 ++++++++++++++++++++++++++++++++++-
>  include/linux/pmbus.h            | 14 ++++++++++++++
>  3 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index d605412a3173b95041524285ad1fde52fb64ce5a..ddb19c9726d62416244f83603b92d81d82e64891 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -487,6 +487,8 @@ struct pmbus_driver_info {
>  /* Regulator ops */
>  
>  extern const struct regulator_ops pmbus_regulator_ops;
> +int pmbus_regulator_init_cb(struct regulator_dev *rdev,
> +			    struct regulator_config *config);
>  
>  /* Macros for filling in array of struct regulator_desc */
>  #define PMBUS_REGULATOR_STEP(_name, _id, _voltages, _step, _min_uV)  \
> @@ -501,6 +503,7 @@ extern const struct regulator_ops pmbus_regulator_ops;
>  		.n_voltages = _voltages,			\
>  		.uV_step = _step,				\
>  		.min_uV = _min_uV,				\
> +		.init_cb = pmbus_regulator_init_cb,		\
>  	}
>  
>  #define PMBUS_REGULATOR(_name, _id)   PMBUS_REGULATOR_STEP(_name, _id, 0, 0, 0)
> @@ -516,6 +519,7 @@ extern const struct regulator_ops pmbus_regulator_ops;
>  		.n_voltages = _voltages,			\
>  		.uV_step = _step,				\
>  		.min_uV = _min_uV,				\
> +		.init_cb = pmbus_regulator_init_cb,		\
>  	}
>  
>  #define PMBUS_REGULATOR_ONE(_name)   PMBUS_REGULATOR_STEP_ONE(_name, 0, 0, 0)
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 085a4dc91d9bad3d2aacdd946b74a094ea9ae458..7bdd8f2ffcabc51500437182f411e9826cd7a55d 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2721,8 +2721,21 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
>  	if (!(data->flags & PMBUS_NO_WRITE_PROTECT)) {
>  		ret = _pmbus_read_byte_data(client, 0xff, PMBUS_WRITE_PROTECT);
>  
> -		if (ret > 0 && (ret & PB_WP_ANY))
> +		switch (ret) {

This changes semantics. The mask for PB_WP_ANY was there explicitly to
avoid situations where vendors set more than those bits for whatever
reason. With this change, write protect status will no longer be
recognized for such devices.

Yes, I know, "the standard says", but standard violations are common
with PMBus devices. That is the whole point of all those callbacks.

Long story short, please do not make changes like this. "ret" still needs
to be masked.

Guenter

Re: [PATCH v3 3/6] hwmon: (pmbus/core) add wp module param

[Guenter Roeck]

On Thu, Oct 24, 2024 at 08:10:37PM +0200, Jerome Brunet wrote:
> Add a module parameter to force the write protection mode of pmbus chips.
> 
> 2 protections modes are provided to start with:
> * 0: Remove the write protection if possible
> * 1: Enable full write protection if possible
> 
> Of course, if the parameter is not provided, the default write protection
> status of the pmbus chips is left untouched.
> 
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  4 ++
>  drivers/hwmon/pmbus/pmbus_core.c                | 74 ++++++++++++++++++-------
>  2 files changed, 59 insertions(+), 19 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 1518343bbe2237f1d577df5656339d6224b769be..aa79242fe0a9238f618182289f18563ed63cba1c 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4733,6 +4733,10 @@
>  			Format: { parport<nr> | timid | 0 }
>  			See also Documentation/admin-guide/parport.rst.
>  
> +	pmbus.wp=	[HW] PMBus Chips write protection forced mode
> +			Format: { 0 | 1 }
> +			See drivers/hwmon/pmbus/pmbus_core.c
> +

I have always seen that file as applicable for core kernel parameters,
not for driver kernel parameters. I can not accept a global change like
this without guidance. Please explain why it is desirable to have this
parameter documented here and not in driver documentation.

>  	pmtmr=		[X86] Manual setup of pmtmr I/O Port.
>  			Override pmtimer IOPort with a hex value.
>  			e.g. pmtmr=0x508
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 7bdd8f2ffcabc51500437182f411e9826cd7a55d..ce697ca03de01c0e5a352f8f6b72671137721868 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -31,6 +31,20 @@
>  #define PMBUS_ATTR_ALLOC_SIZE	32
>  #define PMBUS_NAME_SIZE		24
>  
> +/*
> + * PMBus write protect forced mode:
> + * PMBus may come up with a variety of write protection configuration.
> + * 'pmbus_wp' may be used if a particular write protection is necessary.
> + * The ability to actually alter the protection may also depend on the chip
> + * so the actual runtime write protection configuration may differ from
> + * the requested one. pmbus_core currently support the following value:
> + * - 0: write protection removed
> + * - 1: write protection fully enabled, including OPERATION and VOUT_COMMAND
> + *      registers. Chips essentially become read-only with this.

Would it be desirable to also suppport the ability to set the output voltage
but not limits (PB_WP_VOUT) ?

Guenter

Re: [PATCH v3 4/6] hwmon: (pmbus/core) clear faults after setting smbalert mask

[Guenter Roeck]

On Thu, Oct 24, 2024 at 08:10:38PM +0200, Jerome Brunet wrote:
> pmbus_write_smbalert_mask() ignores the errors if the chip can't set
> smbalert mask the standard way. It is not necessarily a problem for the irq
> support if the chip is otherwise properly setup but it may leave an
> uncleared fault behind.
> 
> pmbus_core will pick the fault on the next register_check(). The register
> check will fails regardless of the actual register support by the chip.
> 
> This leads to missing attributes or debugfs entries for chips that should
> provide them.
> 
> We cannot rely on register_check() as PMBUS_SMBALERT_MASK may be read-only.
> 
> Unconditionally clear the page fault after setting PMBUS_SMBALERT_MASK to
> avoid the problem.
> 
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Fixes: 221819ca4c36 ("hwmon: (pmbus/core) Add interrupt support")
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/hwmon/pmbus/pmbus_core.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index ce697ca03de01c0e5a352f8f6b72671137721868..a0a397d571caa1a6620ef095f9cf63d94e8bda1d 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -3346,7 +3346,12 @@ static int pmbus_regulator_notify(struct pmbus_data *data, int page, int event)
>  
>  static int pmbus_write_smbalert_mask(struct i2c_client *client, u8 page, u8 reg, u8 val)
>  {
> -	return _pmbus_write_word_data(client, page, PMBUS_SMBALERT_MASK, reg | (val << 8));
> +	int ret;
> +
> +	ret = _pmbus_write_word_data(client, page, PMBUS_SMBALERT_MASK, reg | (val << 8));
> +	pmbus_clear_fault_page(client, -1);

Why -1 and not page ?

Guenter

Re: [PATCH v3 4/6] hwmon: (pmbus/core) clear faults after setting smbalert mask

[Jerome Brunet]

On Fri 01 Nov 2024 at 08:10, Guenter Roeck <linux@roeck-us.net> wrote:

> On Thu, Oct 24, 2024 at 08:10:38PM +0200, Jerome Brunet wrote:
>> pmbus_write_smbalert_mask() ignores the errors if the chip can't set
>> smbalert mask the standard way. It is not necessarily a problem for the irq
>> support if the chip is otherwise properly setup but it may leave an
>> uncleared fault behind.
>> 
>> pmbus_core will pick the fault on the next register_check(). The register
>> check will fails regardless of the actual register support by the chip.
>> 
>> This leads to missing attributes or debugfs entries for chips that should
>> provide them.
>> 
>> We cannot rely on register_check() as PMBUS_SMBALERT_MASK may be read-only.
>> 
>> Unconditionally clear the page fault after setting PMBUS_SMBALERT_MASK to
>> avoid the problem.
>> 
>> Suggested-by: Guenter Roeck <linux@roeck-us.net>
>> Fixes: 221819ca4c36 ("hwmon: (pmbus/core) Add interrupt support")
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>  drivers/hwmon/pmbus/pmbus_core.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>> index ce697ca03de01c0e5a352f8f6b72671137721868..a0a397d571caa1a6620ef095f9cf63d94e8bda1d 100644
>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>> @@ -3346,7 +3346,12 @@ static int pmbus_regulator_notify(struct pmbus_data *data, int page, int event)
>>  
>>  static int pmbus_write_smbalert_mask(struct i2c_client *client, u8 page, u8 reg, u8 val)
>>  {
>> -	return _pmbus_write_word_data(client, page, PMBUS_SMBALERT_MASK, reg | (val << 8));
>> +	int ret;
>> +
>> +	ret = _pmbus_write_word_data(client, page, PMBUS_SMBALERT_MASK, reg | (val << 8));
>> +	pmbus_clear_fault_page(client, -1);
>
> Why -1 and not page ?

The idea was to clear the fault on the page we are on, basically just skipping
setting the page again.

I'll change to 'page'

>
> Guenter

-- 
Jerome

Re: [PATCH v3 3/6] hwmon: (pmbus/core) add wp module param

[Jerome Brunet]

On Fri 01 Nov 2024 at 08:08, Guenter Roeck <linux@roeck-us.net> wrote:

> On Thu, Oct 24, 2024 at 08:10:37PM +0200, Jerome Brunet wrote:
>> Add a module parameter to force the write protection mode of pmbus chips.
>> 
>> 2 protections modes are provided to start with:
>> * 0: Remove the write protection if possible
>> * 1: Enable full write protection if possible
>> 
>> Of course, if the parameter is not provided, the default write protection
>> status of the pmbus chips is left untouched.
>> 
>> Suggested-by: Guenter Roeck <linux@roeck-us.net>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>  Documentation/admin-guide/kernel-parameters.txt |  4 ++
>>  drivers/hwmon/pmbus/pmbus_core.c                | 74 ++++++++++++++++++-------
>>  2 files changed, 59 insertions(+), 19 deletions(-)
>> 
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 1518343bbe2237f1d577df5656339d6224b769be..aa79242fe0a9238f618182289f18563ed63cba1c 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -4733,6 +4733,10 @@
>>  			Format: { parport<nr> | timid | 0 }
>>  			See also Documentation/admin-guide/parport.rst.
>>  
>> +	pmbus.wp=	[HW] PMBus Chips write protection forced mode
>> +			Format: { 0 | 1 }
>> +			See drivers/hwmon/pmbus/pmbus_core.c
>> +
>
> I have always seen that file as applicable for core kernel parameters,
> not for driver kernel parameters. I can not accept a global change like
> this without guidance. Please explain why it is desirable to have this
> parameter documented here and not in driver documentation.

No reason other than trying to document things the best I can.
Other subsystemw are documenting things in here too, I just did the same

See 'regulator_ignore_unused' for example.

I don't mind dropping this hunk if you prefer it that way.

>
>>  	pmtmr=		[X86] Manual setup of pmtmr I/O Port.
>>  			Override pmtimer IOPort with a hex value.
>>  			e.g. pmtmr=0x508
>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>> index 7bdd8f2ffcabc51500437182f411e9826cd7a55d..ce697ca03de01c0e5a352f8f6b72671137721868 100644
>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>> @@ -31,6 +31,20 @@
>>  #define PMBUS_ATTR_ALLOC_SIZE	32
>>  #define PMBUS_NAME_SIZE		24
>>  
>> +/*
>> + * PMBus write protect forced mode:
>> + * PMBus may come up with a variety of write protection configuration.
>> + * 'pmbus_wp' may be used if a particular write protection is necessary.
>> + * The ability to actually alter the protection may also depend on the chip
>> + * so the actual runtime write protection configuration may differ from
>> + * the requested one. pmbus_core currently support the following value:
>> + * - 0: write protection removed
>> + * - 1: write protection fully enabled, including OPERATION and VOUT_COMMAND
>> + *      registers. Chips essentially become read-only with this.
>
> Would it be desirable to also suppport the ability to set the output voltage
> but not limits (PB_WP_VOUT) ?

I was starting simple, it is doable sure.
It is not something I will be able to test on actual since does not
support that.

Do you want me to add "2: write protection enable execpt for VOUT_COMMAND." ?

>
> Guenter

-- 
Jerome

Re: [PATCH v3 4/6] hwmon: (pmbus/core) clear faults after setting smbalert mask

[Guenter Roeck]

On 11/4/24 00:28, Jerome Brunet wrote:
> On Fri 01 Nov 2024 at 08:10, Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On Thu, Oct 24, 2024 at 08:10:38PM +0200, Jerome Brunet wrote:
>>> pmbus_write_smbalert_mask() ignores the errors if the chip can't set
>>> smbalert mask the standard way. It is not necessarily a problem for the irq
>>> support if the chip is otherwise properly setup but it may leave an
>>> uncleared fault behind.
>>>
>>> pmbus_core will pick the fault on the next register_check(). The register
>>> check will fails regardless of the actual register support by the chip.
>>>
>>> This leads to missing attributes or debugfs entries for chips that should
>>> provide them.
>>>
>>> We cannot rely on register_check() as PMBUS_SMBALERT_MASK may be read-only.
>>>
>>> Unconditionally clear the page fault after setting PMBUS_SMBALERT_MASK to
>>> avoid the problem.
>>>
>>> Suggested-by: Guenter Roeck <linux@roeck-us.net>
>>> Fixes: 221819ca4c36 ("hwmon: (pmbus/core) Add interrupt support")
>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>> ---
>>>   drivers/hwmon/pmbus/pmbus_core.c | 7 ++++++-
>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>>> index ce697ca03de01c0e5a352f8f6b72671137721868..a0a397d571caa1a6620ef095f9cf63d94e8bda1d 100644
>>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>>> @@ -3346,7 +3346,12 @@ static int pmbus_regulator_notify(struct pmbus_data *data, int page, int event)
>>>   
>>>   static int pmbus_write_smbalert_mask(struct i2c_client *client, u8 page, u8 reg, u8 val)
>>>   {
>>> -	return _pmbus_write_word_data(client, page, PMBUS_SMBALERT_MASK, reg | (val << 8));
>>> +	int ret;
>>> +
>>> +	ret = _pmbus_write_word_data(client, page, PMBUS_SMBALERT_MASK, reg | (val << 8));
>>> +	pmbus_clear_fault_page(client, -1);
>>
>> Why -1 and not page ?
> 
> The idea was to clear the fault on the page we are on, basically just skipping
> setting the page again.
> 
> I'll change to 'page'
> 
Or just add a comment explaining the '-1'.

Thanks,
Guenter

Re: [PATCH v3 3/6] hwmon: (pmbus/core) add wp module param

[Guenter Roeck]

On 11/4/24 00:43, Jerome Brunet wrote:
> On Fri 01 Nov 2024 at 08:08, Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On Thu, Oct 24, 2024 at 08:10:37PM +0200, Jerome Brunet wrote:
>>> Add a module parameter to force the write protection mode of pmbus chips.
>>>
>>> 2 protections modes are provided to start with:
>>> * 0: Remove the write protection if possible
>>> * 1: Enable full write protection if possible
>>>
>>> Of course, if the parameter is not provided, the default write protection
>>> status of the pmbus chips is left untouched.
>>>
>>> Suggested-by: Guenter Roeck <linux@roeck-us.net>
>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>> ---
>>>   Documentation/admin-guide/kernel-parameters.txt |  4 ++
>>>   drivers/hwmon/pmbus/pmbus_core.c                | 74 ++++++++++++++++++-------
>>>   2 files changed, 59 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>>> index 1518343bbe2237f1d577df5656339d6224b769be..aa79242fe0a9238f618182289f18563ed63cba1c 100644
>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>> @@ -4733,6 +4733,10 @@
>>>   			Format: { parport<nr> | timid | 0 }
>>>   			See also Documentation/admin-guide/parport.rst.
>>>   
>>> +	pmbus.wp=	[HW] PMBus Chips write protection forced mode
>>> +			Format: { 0 | 1 }
>>> +			See drivers/hwmon/pmbus/pmbus_core.c
>>> +
>>
>> I have always seen that file as applicable for core kernel parameters,
>> not for driver kernel parameters. I can not accept a global change like
>> this without guidance. Please explain why it is desirable to have this
>> parameter documented here and not in driver documentation.
> 
> No reason other than trying to document things the best I can.
> Other subsystemw are documenting things in here too, I just did the same
> 
> See 'regulator_ignore_unused' for example.
> 

Basic rule: You can find examples for everything in the kernel, including
dell_smm_hwmon in the same file. That doesn't make it better. It only shows
that I did not pay attention. Trying to add all the other 80+ module
parameters of hwmon drivers, or all the 5,000+ module parameters from all
drivers, into the same file would not scale well.

Please document the module parameter in Documentation/hwmon/pmbus-core.rst.

Thanks,
Guenter

Re: [PATCH v3 3/6] hwmon: (pmbus/core) add wp module param

[Guenter Roeck]

On 11/4/24 00:43, Jerome Brunet wrote:

>>> +/*
>>> + * PMBus write protect forced mode:
>>> + * PMBus may come up with a variety of write protection configuration.
>>> + * 'pmbus_wp' may be used if a particular write protection is necessary.
>>> + * The ability to actually alter the protection may also depend on the chip
>>> + * so the actual runtime write protection configuration may differ from
>>> + * the requested one. pmbus_core currently support the following value:
>>> + * - 0: write protection removed
>>> + * - 1: write protection fully enabled, including OPERATION and VOUT_COMMAND
>>> + *      registers. Chips essentially become read-only with this.
>>
>> Would it be desirable to also suppport the ability to set the output voltage
>> but not limits (PB_WP_VOUT) ?
> 
> I was starting simple, it is doable sure.
> It is not something I will be able to test on actual since does not
> support that.
> 
> Do you want me to add "2: write protection enable execpt for VOUT_COMMAND." ?
> 

Please add it. I have a number of PMBus test boards and will be able to test it.

Thee are three options, though. Per specification:

1000 0000 Disable all writes except to the WRITE_PROTECT command
0100 0000 Disable all writes except to the WRITE_PROTECT, OPERATION and
           PAGE commands
0010 0000 Disable all writes except to the WRITE_PROTECT, OPERATION,
           PAGE, ON_OFF_CONFIG and VOUT_COMMAND commands

The driver uses OPERATION and VOUT_COMMAND, so we should have options
to disable them separately. It may be desirable, for example, to be able
to turn on a regulator but not to change the voltages. Also, since
full write protection also disables writes to the page register,
the impact of full write protection on multi-page chips needs to be
documented.

Thanks,
Guenter

Re: [PATCH v3 3/6] hwmon: (pmbus/core) add wp module param

[Jerome Brunet]

On Mon 04 Nov 2024 at 06:18, Guenter Roeck <linux@roeck-us.net> wrote:

> On 11/4/24 00:43, Jerome Brunet wrote:
>
>>>> +/*
>>>> + * PMBus write protect forced mode:
>>>> + * PMBus may come up with a variety of write protection configuration.
>>>> + * 'pmbus_wp' may be used if a particular write protection is necessary.
>>>> + * The ability to actually alter the protection may also depend on the chip
>>>> + * so the actual runtime write protection configuration may differ from
>>>> + * the requested one. pmbus_core currently support the following value:
>>>> + * - 0: write protection removed
>>>> + * - 1: write protection fully enabled, including OPERATION and VOUT_COMMAND
>>>> + *      registers. Chips essentially become read-only with this.
>>>
>>> Would it be desirable to also suppport the ability to set the output voltage
>>> but not limits (PB_WP_VOUT) ?
>> I was starting simple, it is doable sure.
>> It is not something I will be able to test on actual since does not
>> support that.
>> Do you want me to add "2: write protection enable execpt for
>> VOUT_COMMAND." ?
>> 
>
> Please add it. I have a number of PMBus test boards and will be able to test it.
>
> Thee are three options, though. Per specification:

Any preference for the value mapped to each mode ? Using the one from
the spec does not seem practical (32768, 16384, 8192).

The bit number, maybe (7, 6, 5) ?

or just by order strongest locking ?

>
> 1000 0000 Disable all writes except to the WRITE_PROTECT command

3

> 0100 0000 Disable all writes except to the WRITE_PROTECT, OPERATION and
>           PAGE commands

2

> 0010 0000 Disable all writes except to the WRITE_PROTECT, OPERATION,
>           PAGE, ON_OFF_CONFIG and VOUT_COMMAND commands

1 ?

>
> The driver uses OPERATION and VOUT_COMMAND, so we should have options
> to disable them separately. It may be desirable, for example, to be able
> to turn on a regulator but not to change the voltages. Also, since
> full write protection also disables writes to the page register,
> the impact of full write protection on multi-page chips needs to be
> documented.

Noted. I'll update the documentation.

>
> Thanks,
> Guenter

-- 
Jerome

Re: [PATCH v3 3/6] hwmon: (pmbus/core) add wp module param

[Guenter Roeck]

On 11/4/24 06:39, Jerome Brunet wrote:
> On Mon 04 Nov 2024 at 06:18, Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On 11/4/24 00:43, Jerome Brunet wrote:
>>
>>>>> +/*
>>>>> + * PMBus write protect forced mode:
>>>>> + * PMBus may come up with a variety of write protection configuration.
>>>>> + * 'pmbus_wp' may be used if a particular write protection is necessary.
>>>>> + * The ability to actually alter the protection may also depend on the chip
>>>>> + * so the actual runtime write protection configuration may differ from
>>>>> + * the requested one. pmbus_core currently support the following value:
>>>>> + * - 0: write protection removed
>>>>> + * - 1: write protection fully enabled, including OPERATION and VOUT_COMMAND
>>>>> + *      registers. Chips essentially become read-only with this.
>>>>
>>>> Would it be desirable to also suppport the ability to set the output voltage
>>>> but not limits (PB_WP_VOUT) ?
>>> I was starting simple, it is doable sure.
>>> It is not something I will be able to test on actual since does not
>>> support that.
>>> Do you want me to add "2: write protection enable execpt for
>>> VOUT_COMMAND." ?
>>>
>>
>> Please add it. I have a number of PMBus test boards and will be able to test it.
>>
>> Thee are three options, though. Per specification:
> 
> Any preference for the value mapped to each mode ? Using the one from
> the spec does not seem practical (32768, 16384, 8192).
> 
> The bit number, maybe (7, 6, 5) ?
> 
> or just by order strongest locking ?
> 
>>
>> 1000 0000 Disable all writes except to the WRITE_PROTECT command
> 
> 3
> 
>> 0100 0000 Disable all writes except to the WRITE_PROTECT, OPERATION and
>>            PAGE commands
> 
> 2
> 
>> 0010 0000 Disable all writes except to the WRITE_PROTECT, OPERATION,
>>            PAGE, ON_OFF_CONFIG and VOUT_COMMAND commands
> 
> 1 ?
> 

Bit number does not make sense since those are just commands which happen
to use individual bits. Also, module parameters should as much as possible
be abstract and not reflect HW 1:1. Strongest locking as you suggested as
second option makes more sense, since 0 means "no locking".

Thanks,
Guenter