[PATCH] hwmon: (tps23861) add class restrictions and semi-auto mode support

[PATCH] hwmon: (tps23861) add class restrictions and semi-auto mode support

[Gregory Fuchedgi]

This commit adds optional individual per-port device tree nodes, in which ports
can be:
  - restricted by class. For example, class = <2> for a port would only enable
    power if class 1 or class 2 were negotiated. Requires interrupt handler to
    be configured if class != 4 (max supported). This is because hardware cannot
    be set with acceptable classes in advance. So the driver would enable
    Semi-Auto mode and in the interrupt handler would check negotiated class
    against device tree config and enable power only if it is acceptable class.
  - fully disabled. For boards that do not use all 4 ports. This would put
    disabled ports in Off state and would hide corresponding hwmon files.
  - off by default. The port is kept disabled, until enabled via corresponding
    in_enable in sysfs.

The driver keeps current behavior of using Auto mode for all ports if no
per-port device tree nodes given.

This commit also adds optional reset and shutdown pin support:
  - if reset pin is configured the chip will be reset in probe.
  - if both reset and shutdown pins are configured the driver would keep ports
    shut down while configuring the tps23861 over i2c. Having both shutdown and
    reset pins ensures no PoE activity happens while i2c configuration is
    happening.

Signed-off-by: Gregory Fuchedgi <gfuchedgi@gmail.com>
---
 .../bindings/hwmon/ti,tps23861.yaml           |  78 +++++-
 Documentation/hwmon/tps23861.rst              |   6 +-
 drivers/hwmon/tps23861.c                      | 253 +++++++++++++++++-
 3 files changed, 325 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
index ee7de53e1918..328050656ab8 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
@@ -24,12 +24,34 @@ properties:
   reg:
     maxItems: 1
 
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
   shunt-resistor-micro-ohms:
     description: The value of current sense resistor in microohms.
     default: 255000
     minimum: 250000
     maximum: 255000
 
+  reset-gpios:
+    description: Optional GPIO for the reset pin.
+    maxItems: 1
+
+  shutdown-gpios:
+    description: |
+      Optional GPIO for the shutdown pin. Used to prevent PoE activity before
+      the driver had a chance to configure the chip.
+    maxItems: 1
+
+  interrupts:
+    description: |
+      The interrupt specifier. Only required if PoE class is restricted to less
+      than class 4 in the device tree.
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -37,7 +59,27 @@ required:
 allOf:
   - $ref: hwmon-common.yaml#
 
-unevaluatedProperties: false
+additionalProperties:
+  type: object
+  description: Port specific nodes.
+  required:
+    - reg
+  properties:
+    reg:
+      description: Port index.
+      items:
+        minimum: 0
+        maximum: 3
+
+    class:
+      description: The maximum power class a port should accept.
+      $ref: /schemas/types.yaml#/definitions/uint32
+      minimum: 0
+      maximum: 4
+
+    off-by-default:
+      description: Indicates the port is off by default.
+      type: boolean
 
 examples:
   - |
@@ -51,3 +93,37 @@ examples:
             shunt-resistor-micro-ohms = <255000>;
         };
     };
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        tps23861@28 {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            compatible = "ti,tps23861";
+            reg = <0x28>;
+            shunt-resistor-micro-ohms = <255000>;
+            reset-gpios = <&gpio1 13 GPIO_ACTIVE_LOW>;
+            shutdown-gpios = <&gpio1 12 GPIO_ACTIVE_LOW>;
+            interrupt-parent = <&gpio1>;
+            interrupts = <14 0>;
+            port1@0 {
+                    reg = <0>;
+                    class = <2>; // Max PoE class allowed.
+                    off-by-default;
+            };
+            port2@1 {
+                    reg = <1>;
+                    status = "disabled";
+            };
+            port3@2 {
+                    reg = <2>;
+                    status = "disabled";
+            };
+            port4@3 {
+                    reg = <3>;
+                    status = "disabled";
+            };
+        };
+    };
diff --git a/Documentation/hwmon/tps23861.rst b/Documentation/hwmon/tps23861.rst
index 46d121ff3f31..8deaed0d532f 100644
--- a/Documentation/hwmon/tps23861.rst
+++ b/Documentation/hwmon/tps23861.rst
@@ -22,9 +22,13 @@ and monitoring capabilities.
 
 TPS23861 offers three modes of operation: Auto, Semi-Auto and Manual.
 
-This driver only supports the Auto mode of operation providing monitoring
+This driver supports Auto and Semi-Auto modes of operation providing monitoring
 as well as enabling/disabling the four ports.
 
+Ports that do not have a child device tree entry will operate in Auto mode. If a
+class restriction is defined for a port in the device tree, that port will
+operate in Semi-Auto mode and require an interrupt pin.
+
 Sysfs entries
 -------------
 
diff --git a/drivers/hwmon/tps23861.c b/drivers/hwmon/tps23861.c
index 4cb3960d5170..9d0feffc2597 100644
--- a/drivers/hwmon/tps23861.c
+++ b/drivers/hwmon/tps23861.c
@@ -10,13 +10,26 @@
 #include <linux/bitfield.h>
 #include <linux/debugfs.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/hwmon-sysfs.h>
 #include <linux/hwmon.h>
 #include <linux/i2c.h>
+#include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
 #include <linux/regmap.h>
 
+#define MAX_SUPPORTED_POE_CLASS		4
+#define INTERRUPT_STATUS		0x0
+#define INTERRUPT_ENABLE		0x1
+#define INTERRUPT_CLASS			BIT(4)
+#define DETECTION_EVENT			0x5
+#define POWER_STATUS			0x10
+#define PORT_1_ICUT			0x2A
+#define PORT_1_ICUT_MASK		GENMASK(2, 0)
+#define RESET				0x1a
+#define RESET_CLRAIN			0x80
 #define TEMPERATURE			0x2c
 #define INPUT_VOLTAGE_LSB		0x2e
 #define INPUT_VOLTAGE_MSB		0x2f
@@ -102,6 +115,10 @@
 #define TPS23861_GENERAL_MASK_1		0x17
 #define TPS23861_CURRENT_SHUNT_MASK	BIT(0)
 
+#define TPS23861_TIME_RESET_RANGE_US	5, 1000
+#define TPS23861_TIME_POWER_ON_RESET_MS 23
+#define TPS23861_TIME_I2C_MS		20
+
 #define TEMPERATURE_LSB			652 /* 0.652 degrees Celsius */
 #define VOLTAGE_LSB			3662 /* 3.662 mV */
 #define SHUNT_RESISTOR_DEFAULT		255000 /* 255 mOhm */
@@ -110,10 +127,27 @@
 #define RESISTANCE_LSB			110966 /* 11.0966 Ohm*/
 #define RESISTANCE_LSB_LOW		157216 /* 15.7216 Ohm*/
 
+struct tps23861_port_data { // From DT.
+	const char *name;
+	/* Maximum class accepted by the port. The hardware cannot be
+	 * preconfigured to accept certain class only, so classification
+	 * interrupt handler is required to for power-on decision if class is
+	 * not MAX_SUPPORTED_POE_CLASS. */
+	u32 class;
+	/* true if the port is disabled in the DT */
+	bool fully_disabled;
+	/* true if the port shouldn't be enabled on driver init */
+	bool off_by_default;
+};
+
 struct tps23861_data {
 	struct regmap *regmap;
 	u32 shunt_resistor;
 	struct i2c_client *client;
+	struct gpio_desc *reset_gpio;
+	struct gpio_desc *shutdown_gpio;
+	int irq;
+	struct tps23861_port_data ports[TPS23861_NUM_PORTS];
 };
 
 static const struct regmap_config tps23861_regmap_config = {
@@ -205,13 +239,18 @@ static int tps23861_port_enable(struct tps23861_data *data, int channel)
 	regval |= BIT(channel);
 	regval |= BIT(channel + 4);
 	err = regmap_write(data->regmap, DETECT_CLASS_RESTART, regval);
-
-	return err;
+	if (err)
+		return err;
+	return regmap_write(data->regmap, RESET, RESET_CLRAIN);
 }
 
-static umode_t tps23861_is_visible(const void *data, enum hwmon_sensor_types type,
+static umode_t tps23861_is_visible(const void *input_data, enum hwmon_sensor_types type,
 				   u32 attr, int channel)
 {
+	const struct tps23861_data *data = input_data;
+	if (channel < TPS23861_NUM_PORTS && data->ports[channel].fully_disabled) {
+		return 0;
+	}
 	switch (type) {
 	case hwmon_temp:
 		switch (attr) {
@@ -325,10 +364,14 @@ static int tps23861_read_string(struct device *dev,
 				enum hwmon_sensor_types type,
 				u32 attr, int channel, const char **str)
 {
+	struct tps23861_data *data = dev_get_drvdata(dev);
 	switch (type) {
 	case hwmon_in:
 	case hwmon_curr:
-		*str = tps23861_port_label[channel];
+		if (channel < TPS23861_NUM_PORTS)
+			*str = data->ports[channel].name;
+		else
+			*str = tps23861_port_label[channel];
 		break;
 	case hwmon_temp:
 		*str = "Die";
@@ -371,7 +414,7 @@ static const struct hwmon_chip_info tps23861_chip_info = {
 	.info = tps23861_info,
 };
 
-static char *port_operating_mode_string(uint8_t mode_reg, unsigned int port)
+static const char *port_operating_mode_string(uint8_t mode_reg, unsigned int port)
 {
 	unsigned int mode = ~0;
 
@@ -392,7 +435,7 @@ static char *port_operating_mode_string(uint8_t mode_reg, unsigned int port)
 	}
 }
 
-static char *port_detect_status_string(uint8_t status_reg)
+static const char *port_detect_status_string(uint8_t status_reg)
 {
 	switch (FIELD_GET(PORT_STATUS_DETECT_MASK, status_reg)) {
 	case PORT_DETECT_UNKNOWN:
@@ -424,7 +467,7 @@ static char *port_detect_status_string(uint8_t status_reg)
 	}
 }
 
-static char *port_class_status_string(uint8_t status_reg)
+static const char *port_class_status_string(uint8_t status_reg)
 {
 	switch (FIELD_GET(PORT_STATUS_CLASS_MASK, status_reg)) {
 	case PORT_CLASS_UNKNOWN:
@@ -449,12 +492,12 @@ static char *port_class_status_string(uint8_t status_reg)
 	}
 }
 
-static char *port_poe_plus_status_string(uint8_t poe_plus, unsigned int port)
+static const char *port_poe_plus_status_string(uint8_t poe_plus, unsigned int port)
 {
 	return (BIT(port + 4) & poe_plus) ? "Yes" : "No";
 }
 
-static int tps23861_port_resistance(struct tps23861_data *data, int port)
+static unsigned int tps23861_port_resistance(struct tps23861_data *data, int port)
 {
 	unsigned int raw_val;
 	__le16 regval;
@@ -502,17 +545,155 @@ static int tps23861_port_status_show(struct seq_file *s, void *data)
 
 DEFINE_SHOW_ATTRIBUTE(tps23861_port_status);
 
+static inline bool tps23861_auto_mode(struct tps23861_port_data *port)
+{
+	return port->class == MAX_SUPPORTED_POE_CLASS;
+}
+
+static irqreturn_t tps23861_irq_handler(int irq, void *dev_id) {
+	struct tps23861_data *data = (struct tps23861_data *)dev_id;
+	struct tps23861_port_data *ports = data->ports;
+	struct device *dev = &data->client->dev;
+
+	unsigned int intr_status, status, class, i;
+	unsigned int det_status = 0;
+	int ret;
+
+	ret = regmap_read(data->regmap, INTERRUPT_STATUS, &intr_status);
+	if (ret || intr_status == 0) {
+		return IRQ_NONE;
+	}
+	if (intr_status & INTERRUPT_CLASS) {
+		regmap_read(data->regmap, DETECTION_EVENT, &det_status);
+		for (i = 0; i < TPS23861_NUM_PORTS; i++) {
+			if (tps23861_auto_mode(ports+i)) {
+				continue;
+			}
+			if (!(det_status & BIT(i+4)))
+				continue;
+			ret = regmap_read(data->regmap, PORT_1_STATUS+i, &status);
+			if (ret)
+				continue;
+			class = FIELD_GET(PORT_STATUS_CLASS_MASK, status);
+			if (class == PORT_CLASS_0) {
+				/* class 0 is same power as class 3, change 0 to
+				 * 3 for easy comparison */
+				class = 3;
+			}
+			if (class == PORT_CLASS_UNKNOWN ||
+			    class > MAX_SUPPORTED_POE_CLASS) {
+				continue;
+			}
+			if (class > ports[i].class) {
+				dev_info(dev, "%s class mismatch: got %d, want <= %d",
+					 ports[i].name, class, ports[i].class);
+				continue;
+			}
+			regmap_read(data->regmap, POWER_STATUS, &status);
+			if (status & BIT(i)) {
+				continue; /* already enabled */
+			}
+			/* Set ICUT and poe+ according to class. Values in ICUT table happen
+			 * to match class values, so just set class. */
+			regmap_update_bits(data->regmap,
+					   PORT_1_ICUT + i/2,
+					   PORT_1_ICUT_MASK << ((i % 2) * 4),
+					   class << ((i % 2) * 4));
+			regmap_update_bits(data->regmap, POE_PLUS,
+					   BIT(i + 4),
+					   class > 3 ? BIT(i + 4) : 0);
+			dev_info(dev, "%s class %d, enabling power",
+				 ports[i].name, class);
+			regmap_write(data->regmap, POWER_ENABLE, BIT(i));
+		}
+	}
+	regmap_write(data->regmap, RESET, RESET_CLRAIN);
+	return IRQ_HANDLED;
+}
+
+static int tps23861_reset(struct i2c_client *client)
+{
+	struct tps23861_data *data = i2c_get_clientdata(client);
+	unsigned val;
+
+	if (data->reset_gpio) {
+		gpiod_direction_output(data->reset_gpio, true);
+		/* If shutdown pin is defined, use it to keep ports off, while
+		 * turning the chip on for i2c configuration. */
+		if (data->shutdown_gpio)
+			gpiod_direction_output(data->shutdown_gpio, true);
+		usleep_range(TPS23861_TIME_RESET_RANGE_US);
+		gpiod_set_value_cansleep(data->reset_gpio, false);
+		msleep(TPS23861_TIME_POWER_ON_RESET_MS);
+		if (data->shutdown_gpio)
+			gpiod_set_value_cansleep(data->shutdown_gpio, false);
+		msleep(TPS23861_TIME_I2C_MS);
+	}
+
+	/* Check the device is responsive */
+	return regmap_read(data->regmap, OPERATING_MODE, &val);
+}
+
+static void tps23861_init_ports(struct i2c_client *client)
+{
+	struct tps23861_data *data = i2c_get_clientdata(client);
+	struct tps23861_port_data *ports = data->ports;
+	unsigned i, mode;
+
+	for (i = 0; i < TPS23861_NUM_PORTS; i++) {
+		mode = ports[i].fully_disabled       ? OPERATING_MODE_OFF :
+		       tps23861_auto_mode(&ports[i]) ? OPERATING_MODE_AUTO :
+		                                       OPERATING_MODE_SEMI;
+		regmap_update_bits(data->regmap, OPERATING_MODE,
+				   OPERATING_MODE_PORT_1_MASK << (2*i),
+				   mode << (2*i));
+		if (ports[i].fully_disabled)
+			continue;
+		if (ports[i].off_by_default)
+			tps23861_port_disable(data, i);
+		else
+			tps23861_port_enable(data, i);
+	}
+}
+
+
 static int tps23861_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct tps23861_data *data;
+	struct tps23861_port_data *ports;
 	struct device *hwmon_dev;
+	struct gpio_desc *gpio;
+	struct device_node *child;
 	u32 shunt_resistor;
+	int ret;
+	unsigned i;
+	bool need_irq = false;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
+	ports = data->ports;
+	for (i = 0; i < TPS23861_NUM_PORTS; i++) {
+		ports[i].name = tps23861_port_label[i];
+		ports[i].class = MAX_SUPPORTED_POE_CLASS;
+	}
+
+	gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_ASIS);
+	if (IS_ERR(gpio)) {
+		devm_kfree(dev, data);
+		return -EPROBE_DEFER;
+	}
+	data->reset_gpio = gpio;
+
+	gpio = devm_gpiod_get_optional(dev, "shutdown", GPIOD_ASIS);
+	if (IS_ERR(gpio)) {
+		devm_kfree(dev, data);
+		return -EPROBE_DEFER;
+	}
+	data->shutdown_gpio = gpio;
+
 	data->client = client;
 	i2c_set_clientdata(client, data);
 
@@ -521,6 +702,56 @@ static int tps23861_probe(struct i2c_client *client)
 		dev_err(dev, "failed to allocate register map\n");
 		return PTR_ERR(data->regmap);
 	}
+	ret = tps23861_reset(client);
+	if (ret) {
+		return -ENODEV;
+	}
+	for_each_child_of_node(dev->of_node, child) {
+		ret = of_property_read_u32(child, "reg", &i);
+		if (ret || i >= TPS23861_NUM_PORTS) {
+			dev_err(dev, "node %s must define 'reg' < %d\n",
+				child->name, TPS23861_NUM_PORTS);
+			continue;
+		}
+		if (!of_device_is_available(child)) {
+			ports[i].fully_disabled = true;
+			continue;
+		}
+		ports[i].name = child->name;
+		ports[i].off_by_default = of_property_read_bool(child, "off-by-default");
+		of_property_read_u32(child, "class", &ports[i].class);
+		if (ports[i].class > MAX_SUPPORTED_POE_CLASS) {
+			dev_err(dev, "%s invalid class, disabling\n", child->name);
+			ports[i].fully_disabled = true;
+			continue;
+		}
+		if (ports[i].class == 0) {
+			/* class 0 is same power as class 3, change 0 to 3 for
+			 * easy comparison */
+			ports[i].class = 3;
+		}
+		need_irq = need_irq || !tps23861_auto_mode(&ports[i]);
+		dev_info(dev, "%s: max class: %d, %s by default\n",
+			 ports[i].name, ports[i].class,
+			 ports[i].off_by_default ? "off" : "on");
+	}
+	if (need_irq) {
+		data->irq = irq_of_parse_and_map(dev->of_node, 0);
+		if (!data->irq) {
+			dev_err(dev, "failed to configure irq\n");
+			return -EINVAL;
+		}
+		ret = devm_request_threaded_irq(dev, data->irq, NULL,
+						tps23861_irq_handler,
+						IRQF_TRIGGER_FALLING,
+						"tps23861_irq", data);
+		if (ret) {
+			dev_err(dev, "failed to request irq %d\n", data->irq);
+			return ret;
+		}
+		dev_info(dev, "irq successfully requested\n");
+		regmap_write(data->regmap, INTERRUPT_ENABLE, INTERRUPT_CLASS);
+	}
 
 	if (!of_property_read_u32(dev->of_node, "shunt-resistor-micro-ohms", &shunt_resistor))
 		data->shunt_resistor = shunt_resistor;
@@ -536,7 +767,9 @@ static int tps23861_probe(struct i2c_client *client)
 				TPS23861_GENERAL_MASK_1,
 				TPS23861_CURRENT_SHUNT_MASK);
 
-	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+	tps23861_init_ports(client);
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, dev->of_node->name,
 							 data, &tps23861_chip_info,
 							 NULL);
 	if (IS_ERR(hwmon_dev))
-- 
2.43.0

Re: [PATCH] hwmon: (tps23861) add class restrictions and semi-auto mode support

[Guenter Roeck]

On 8/7/25 14:50, Gregory Fuchedgi wrote:
> This commit adds optional individual per-port device tree nodes, in which ports
> can be:
>    - restricted by class. For example, class = <2> for a port would only enable
>      power if class 1 or class 2 were negotiated. Requires interrupt handler to
>      be configured if class != 4 (max supported). This is because hardware cannot
>      be set with acceptable classes in advance. So the driver would enable
>      Semi-Auto mode and in the interrupt handler would check negotiated class
>      against device tree config and enable power only if it is acceptable class.
>    - fully disabled. For boards that do not use all 4 ports. This would put
>      disabled ports in Off state and would hide corresponding hwmon files.
>    - off by default. The port is kept disabled, until enabled via corresponding
>      in_enable in sysfs.
> 
> The driver keeps current behavior of using Auto mode for all ports if no
> per-port device tree nodes given.
> 
> This commit also adds optional reset and shutdown pin support:
>    - if reset pin is configured the chip will be reset in probe.
>    - if both reset and shutdown pins are configured the driver would keep ports
>      shut down while configuring the tps23861 over i2c. Having both shutdown and
>      reset pins ensures no PoE activity happens while i2c configuration is
>      happening.
> 
> Signed-off-by: Gregory Fuchedgi <gfuchedgi@gmail.com>

Not a complete review.

> ---
>   .../bindings/hwmon/ti,tps23861.yaml           |  78 +++++-
>   Documentation/hwmon/tps23861.rst              |   6 +-
>   drivers/hwmon/tps23861.c                      | 253 +++++++++++++++++-

Bindings and driver changes need to be separate patches.

>   3 files changed, 325 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
> index ee7de53e1918..328050656ab8 100644
> --- a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
> @@ -24,12 +24,34 @@ properties:
>     reg:
>       maxItems: 1
>   
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
>     shunt-resistor-micro-ohms:
>       description: The value of current sense resistor in microohms.
>       default: 255000
>       minimum: 250000
>       maximum: 255000
>   
> +  reset-gpios:
> +    description: Optional GPIO for the reset pin.
> +    maxItems: 1
> +
> +  shutdown-gpios:
> +    description: |
> +      Optional GPIO for the shutdown pin. Used to prevent PoE activity before
> +      the driver had a chance to configure the chip.
> +    maxItems: 1
> +
> +  interrupts:
> +    description: |
> +      The interrupt specifier. Only required if PoE class is restricted to less
> +      than class 4 in the device tree.
> +    maxItems: 1
> +
>   required:
>     - compatible
>     - reg
> @@ -37,7 +59,27 @@ required:
>   allOf:
>     - $ref: hwmon-common.yaml#
>   
> -unevaluatedProperties: false
> +additionalProperties:
> +  type: object
> +  description: Port specific nodes.
> +  required:
> +    - reg
> +  properties:
> +    reg:
> +      description: Port index.
> +      items:
> +        minimum: 0
> +        maximum: 3
> +
> +    class:
> +      description: The maximum power class a port should accept.
> +      $ref: /schemas/types.yaml#/definitions/uint32
> +      minimum: 0
> +      maximum: 4
> +
> +    off-by-default:
> +      description: Indicates the port is off by default.
> +      type: boolean
>   
>   examples:
>     - |
> @@ -51,3 +93,37 @@ examples:
>               shunt-resistor-micro-ohms = <255000>;
>           };
>       };
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        tps23861@28 {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            compatible = "ti,tps23861";
> +            reg = <0x28>;
> +            shunt-resistor-micro-ohms = <255000>;
> +            reset-gpios = <&gpio1 13 GPIO_ACTIVE_LOW>;
> +            shutdown-gpios = <&gpio1 12 GPIO_ACTIVE_LOW>;
> +            interrupt-parent = <&gpio1>;
> +            interrupts = <14 0>;
> +            port1@0 {
> +                    reg = <0>;
> +                    class = <2>; // Max PoE class allowed.
> +                    off-by-default;
> +            };
> +            port2@1 {
> +                    reg = <1>;
> +                    status = "disabled";
> +            };
> +            port3@2 {
> +                    reg = <2>;
> +                    status = "disabled";
> +            };
> +            port4@3 {
> +                    reg = <3>;
> +                    status = "disabled";
> +            };
> +        };
> +    };
> diff --git a/Documentation/hwmon/tps23861.rst b/Documentation/hwmon/tps23861.rst
> index 46d121ff3f31..8deaed0d532f 100644
> --- a/Documentation/hwmon/tps23861.rst
> +++ b/Documentation/hwmon/tps23861.rst
> @@ -22,9 +22,13 @@ and monitoring capabilities.
>   
>   TPS23861 offers three modes of operation: Auto, Semi-Auto and Manual.
>   
> -This driver only supports the Auto mode of operation providing monitoring
> +This driver supports Auto and Semi-Auto modes of operation providing monitoring
>   as well as enabling/disabling the four ports.
>   
> +Ports that do not have a child device tree entry will operate in Auto mode. If a
> +class restriction is defined for a port in the device tree, that port will
> +operate in Semi-Auto mode and require an interrupt pin.
> +
>   Sysfs entries
>   -------------
>   
> diff --git a/drivers/hwmon/tps23861.c b/drivers/hwmon/tps23861.c
> index 4cb3960d5170..9d0feffc2597 100644
> --- a/drivers/hwmon/tps23861.c
> +++ b/drivers/hwmon/tps23861.c
> @@ -10,13 +10,26 @@
>   #include <linux/bitfield.h>
>   #include <linux/debugfs.h>
>   #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
>   #include <linux/hwmon-sysfs.h>
>   #include <linux/hwmon.h>
>   #include <linux/i2c.h>
> +#include <linux/interrupt.h>
>   #include <linux/module.h>
>   #include <linux/of.h>
> +#include <linux/of_irq.h>
>   #include <linux/regmap.h>
>   
> +#define MAX_SUPPORTED_POE_CLASS		4
> +#define INTERRUPT_STATUS		0x0
> +#define INTERRUPT_ENABLE		0x1
> +#define INTERRUPT_CLASS			BIT(4)
> +#define DETECTION_EVENT			0x5
> +#define POWER_STATUS			0x10
> +#define PORT_1_ICUT			0x2A
> +#define PORT_1_ICUT_MASK		GENMASK(2, 0)
> +#define RESET				0x1a
> +#define RESET_CLRAIN			0x80
>   #define TEMPERATURE			0x2c
>   #define INPUT_VOLTAGE_LSB		0x2e
>   #define INPUT_VOLTAGE_MSB		0x2f
> @@ -102,6 +115,10 @@
>   #define TPS23861_GENERAL_MASK_1		0x17
>   #define TPS23861_CURRENT_SHUNT_MASK	BIT(0)
>   
> +#define TPS23861_TIME_RESET_RANGE_US	5, 1000
> +#define TPS23861_TIME_POWER_ON_RESET_MS 23
> +#define TPS23861_TIME_I2C_MS		20
> +
>   #define TEMPERATURE_LSB			652 /* 0.652 degrees Celsius */
>   #define VOLTAGE_LSB			3662 /* 3.662 mV */
>   #define SHUNT_RESISTOR_DEFAULT		255000 /* 255 mOhm */
> @@ -110,10 +127,27 @@
>   #define RESISTANCE_LSB			110966 /* 11.0966 Ohm*/
>   #define RESISTANCE_LSB_LOW		157216 /* 15.7216 Ohm*/
>   
> +struct tps23861_port_data { // From DT.
> +	const char *name;
> +	/* Maximum class accepted by the port. The hardware cannot be
> +	 * preconfigured to accept certain class only, so classification
> +	 * interrupt handler is required to for power-on decision if class is
> +	 * not MAX_SUPPORTED_POE_CLASS. */
> +	u32 class;
> +	/* true if the port is disabled in the DT */
> +	bool fully_disabled;
> +	/* true if the port shouldn't be enabled on driver init */
> +	bool off_by_default;
> +};
> +
>   struct tps23861_data {
>   	struct regmap *regmap;
>   	u32 shunt_resistor;
>   	struct i2c_client *client;
> +	struct gpio_desc *reset_gpio;
> +	struct gpio_desc *shutdown_gpio;
> +	int irq;
> +	struct tps23861_port_data ports[TPS23861_NUM_PORTS];
>   };
>   
>   static const struct regmap_config tps23861_regmap_config = {
> @@ -205,13 +239,18 @@ static int tps23861_port_enable(struct tps23861_data *data, int channel)
>   	regval |= BIT(channel);
>   	regval |= BIT(channel + 4);
>   	err = regmap_write(data->regmap, DETECT_CLASS_RESTART, regval);
> -
> -	return err;
> +	if (err)
> +		return err;
> +	return regmap_write(data->regmap, RESET, RESET_CLRAIN);
>   }
>   
> -static umode_t tps23861_is_visible(const void *data, enum hwmon_sensor_types type,
> +static umode_t tps23861_is_visible(const void *input_data, enum hwmon_sensor_types type,
>   				   u32 attr, int channel)
>   {
> +	const struct tps23861_data *data = input_data;
> +	if (channel < TPS23861_NUM_PORTS && data->ports[channel].fully_disabled) {

Why would it be necessary t check "channel < TPS23861_NUM_PORTS",
and why would it make sense to declare attributes as valid if they
are for a channel >= TPS23861_NUM_PORTS ?

> +		return 0;
> +	}
>   	switch (type) {
>   	case hwmon_temp:
>   		switch (attr) {
> @@ -325,10 +364,14 @@ static int tps23861_read_string(struct device *dev,
>   				enum hwmon_sensor_types type,
>   				u32 attr, int channel, const char **str)
>   {
> +	struct tps23861_data *data = dev_get_drvdata(dev);

Please run checkpatch --strict and fix what it reports.

>   	switch (type) {
>   	case hwmon_in:
>   	case hwmon_curr:
> -		*str = tps23861_port_label[channel];
> +		if (channel < TPS23861_NUM_PORTS)
> +			*str = data->ports[channel].name;
> +		else
> +			*str = tps23861_port_label[channel];
>   		break;
>   	case hwmon_temp:
>   		*str = "Die";
> @@ -371,7 +414,7 @@ static const struct hwmon_chip_info tps23861_chip_info = {
>   	.info = tps23861_info,
>   };
>   
> -static char *port_operating_mode_string(uint8_t mode_reg, unsigned int port)
> +static const char *port_operating_mode_string(uint8_t mode_reg, unsigned int port)

That seems like an unrelated change.

>   {
>   	unsigned int mode = ~0;
>   
> @@ -392,7 +435,7 @@ static char *port_operating_mode_string(uint8_t mode_reg, unsigned int port)
>   	}
>   }
>   
> -static char *port_detect_status_string(uint8_t status_reg)
> +static const char *port_detect_status_string(uint8_t status_reg)

Same here.

>   {
>   	switch (FIELD_GET(PORT_STATUS_DETECT_MASK, status_reg)) {
>   	case PORT_DETECT_UNKNOWN:
> @@ -424,7 +467,7 @@ static char *port_detect_status_string(uint8_t status_reg)
>   	}
>   }
>   
> -static char *port_class_status_string(uint8_t status_reg)
> +static const char *port_class_status_string(uint8_t status_reg)

and here.

>   {
>   	switch (FIELD_GET(PORT_STATUS_CLASS_MASK, status_reg)) {
>   	case PORT_CLASS_UNKNOWN:
> @@ -449,12 +492,12 @@ static char *port_class_status_string(uint8_t status_reg)
>   	}
>   }
>   
> -static char *port_poe_plus_status_string(uint8_t poe_plus, unsigned int port)
> +static const char *port_poe_plus_status_string(uint8_t poe_plus, unsigned int port)
>   {
>   	return (BIT(port + 4) & poe_plus) ? "Yes" : "No";
>   }
>   
> -static int tps23861_port_resistance(struct tps23861_data *data, int port)
> +static unsigned int tps23861_port_resistance(struct tps23861_data *data, int port)
>   {
>   	unsigned int raw_val;
>   	__le16 regval;
> @@ -502,17 +545,155 @@ static int tps23861_port_status_show(struct seq_file *s, void *data)
>   
>   DEFINE_SHOW_ATTRIBUTE(tps23861_port_status);
>   
> +static inline bool tps23861_auto_mode(struct tps23861_port_data *port)
> +{
> +	return port->class == MAX_SUPPORTED_POE_CLASS;
> +}
> +
> +static irqreturn_t tps23861_irq_handler(int irq, void *dev_id) {
> +	struct tps23861_data *data = (struct tps23861_data *)dev_id;
> +	struct tps23861_port_data *ports = data->ports;
> +	struct device *dev = &data->client->dev;
> +
> +	unsigned int intr_status, status, class, i;
> +	unsigned int det_status = 0;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, INTERRUPT_STATUS, &intr_status);
> +	if (ret || intr_status == 0) {
> +		return IRQ_NONE;
> +	}
> +	if (intr_status & INTERRUPT_CLASS) {
> +		regmap_read(data->regmap, DETECTION_EVENT, &det_status);
> +		for (i = 0; i < TPS23861_NUM_PORTS; i++) {
> +			if (tps23861_auto_mode(ports+i)) {
> +				continue;
> +			}
> +			if (!(det_status & BIT(i+4)))
> +				continue;
> +			ret = regmap_read(data->regmap, PORT_1_STATUS+i, &status);
> +			if (ret)
> +				continue;
> +			class = FIELD_GET(PORT_STATUS_CLASS_MASK, status);
> +			if (class == PORT_CLASS_0) {
> +				/* class 0 is same power as class 3, change 0 to
> +				 * 3 for easy comparison */
> +				class = 3;
> +			}
> +			if (class == PORT_CLASS_UNKNOWN ||
> +			    class > MAX_SUPPORTED_POE_CLASS) {
> +				continue;
> +			}
> +			if (class > ports[i].class) {
> +				dev_info(dev, "%s class mismatch: got %d, want <= %d",
> +					 ports[i].name, class, ports[i].class);
> +				continue;
> +			}
> +			regmap_read(data->regmap, POWER_STATUS, &status);
> +			if (status & BIT(i)) {
> +				continue; /* already enabled */
> +			}
> +			/* Set ICUT and poe+ according to class. Values in ICUT table happen
> +			 * to match class values, so just set class. */
> +			regmap_update_bits(data->regmap,
> +					   PORT_1_ICUT + i/2,
> +					   PORT_1_ICUT_MASK << ((i % 2) * 4),
> +					   class << ((i % 2) * 4));
> +			regmap_update_bits(data->regmap, POE_PLUS,
> +					   BIT(i + 4),
> +					   class > 3 ? BIT(i + 4) : 0);
> +			dev_info(dev, "%s class %d, enabling power",
> +				 ports[i].name, class);
> +			regmap_write(data->regmap, POWER_ENABLE, BIT(i));
> +		}
> +	}
> +	regmap_write(data->regmap, RESET, RESET_CLRAIN);
> +	return IRQ_HANDLED;
> +}
> +
> +static int tps23861_reset(struct i2c_client *client)
> +{
> +	struct tps23861_data *data = i2c_get_clientdata(client);
> +	unsigned val;
> +
> +	if (data->reset_gpio) {
> +		gpiod_direction_output(data->reset_gpio, true);
> +		/* If shutdown pin is defined, use it to keep ports off, while
> +		 * turning the chip on for i2c configuration. */
> +		if (data->shutdown_gpio)
> +			gpiod_direction_output(data->shutdown_gpio, true);
> +		usleep_range(TPS23861_TIME_RESET_RANGE_US);
> +		gpiod_set_value_cansleep(data->reset_gpio, false);
> +		msleep(TPS23861_TIME_POWER_ON_RESET_MS);
> +		if (data->shutdown_gpio)
> +			gpiod_set_value_cansleep(data->shutdown_gpio, false);
> +		msleep(TPS23861_TIME_I2C_MS);
> +	}
> +
> +	/* Check the device is responsive */
> +	return regmap_read(data->regmap, OPERATING_MODE, &val);
> +}
> +
> +static void tps23861_init_ports(struct i2c_client *client)
> +{
> +	struct tps23861_data *data = i2c_get_clientdata(client);
> +	struct tps23861_port_data *ports = data->ports;
> +	unsigned i, mode;
> +
> +	for (i = 0; i < TPS23861_NUM_PORTS; i++) {
> +		mode = ports[i].fully_disabled       ? OPERATING_MODE_OFF :
> +		       tps23861_auto_mode(&ports[i]) ? OPERATING_MODE_AUTO :
> +		                                       OPERATING_MODE_SEMI;
> +		regmap_update_bits(data->regmap, OPERATING_MODE,
> +				   OPERATING_MODE_PORT_1_MASK << (2*i),
> +				   mode << (2*i));
> +		if (ports[i].fully_disabled)
> +			continue;
> +		if (ports[i].off_by_default)
> +			tps23861_port_disable(data, i);
> +		else
> +			tps23861_port_enable(data, i);
> +	}
> +}
> +
> +
>   static int tps23861_probe(struct i2c_client *client)
>   {
>   	struct device *dev = &client->dev;
>   	struct tps23861_data *data;
> +	struct tps23861_port_data *ports;
>   	struct device *hwmon_dev;
> +	struct gpio_desc *gpio;
> +	struct device_node *child;
>   	u32 shunt_resistor;
> +	int ret;
> +	unsigned i;
> +	bool need_irq = false;
>   
>   	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>   	if (!data)
>   		return -ENOMEM;
>   
> +	ports = data->ports;
> +	for (i = 0; i < TPS23861_NUM_PORTS; i++) {
> +		ports[i].name = tps23861_port_label[i];
> +		ports[i].class = MAX_SUPPORTED_POE_CLASS;
> +	}
> +
> +	gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_ASIS);
> +	if (IS_ERR(gpio)) {
> +		devm_kfree(dev, data);
> +		return -EPROBE_DEFER;
> +	}
> +	data->reset_gpio = gpio;
> +
> +	gpio = devm_gpiod_get_optional(dev, "shutdown", GPIOD_ASIS);
> +	if (IS_ERR(gpio)) {
> +		devm_kfree(dev, data);
> +		return -EPROBE_DEFER;
> +	}
> +	data->shutdown_gpio = gpio;
> +
>   	data->client = client;
>   	i2c_set_clientdata(client, data);
>   
> @@ -521,6 +702,56 @@ static int tps23861_probe(struct i2c_client *client)
>   		dev_err(dev, "failed to allocate register map\n");
>   		return PTR_ERR(data->regmap);
>   	}
> +	ret = tps23861_reset(client);
> +	if (ret) {
> +		return -ENODEV;
> +	}
> +	for_each_child_of_node(dev->of_node, child) {
> +		ret = of_property_read_u32(child, "reg", &i);
> +		if (ret || i >= TPS23861_NUM_PORTS) {
> +			dev_err(dev, "node %s must define 'reg' < %d\n",
> +				child->name, TPS23861_NUM_PORTS);
> +			continue;
> +		}
> +		if (!of_device_is_available(child)) {
> +			ports[i].fully_disabled = true;
> +			continue;
> +		}
> +		ports[i].name = child->name;
> +		ports[i].off_by_default = of_property_read_bool(child, "off-by-default");
> +		of_property_read_u32(child, "class", &ports[i].class);
> +		if (ports[i].class > MAX_SUPPORTED_POE_CLASS) {
> +			dev_err(dev, "%s invalid class, disabling\n", child->name);
> +			ports[i].fully_disabled = true;
> +			continue;
> +		}
> +		if (ports[i].class == 0) {
> +			/* class 0 is same power as class 3, change 0 to 3 for
> +			 * easy comparison */
> +			ports[i].class = 3;
> +		}
> +		need_irq = need_irq || !tps23861_auto_mode(&ports[i]);
> +		dev_info(dev, "%s: max class: %d, %s by default\n",
> +			 ports[i].name, ports[i].class,
> +			 ports[i].off_by_default ? "off" : "on");
> +	}
> +	if (need_irq) {
> +		data->irq = irq_of_parse_and_map(dev->of_node, 0);
> +		if (!data->irq) {
> +			dev_err(dev, "failed to configure irq\n");
> +			return -EINVAL;
> +		}
> +		ret = devm_request_threaded_irq(dev, data->irq, NULL,
> +						tps23861_irq_handler,
> +						IRQF_TRIGGER_FALLING,
> +						"tps23861_irq", data);
> +		if (ret) {
> +			dev_err(dev, "failed to request irq %d\n", data->irq);
> +			return ret;
> +		}
> +		dev_info(dev, "irq successfully requested\n");
> +		regmap_write(data->regmap, INTERRUPT_ENABLE, INTERRUPT_CLASS);
> +	}
>   
>   	if (!of_property_read_u32(dev->of_node, "shunt-resistor-micro-ohms", &shunt_resistor))
>   		data->shunt_resistor = shunt_resistor;
> @@ -536,7 +767,9 @@ static int tps23861_probe(struct i2c_client *client)
>   				TPS23861_GENERAL_MASK_1,
>   				TPS23861_CURRENT_SHUNT_MASK);
>   
> -	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> +	tps23861_init_ports(client);
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, dev->of_node->name,

Changing the name of the device may impact existing userspace code. On top of that,
this change makes devicetree support mandatory. I didn't check the rest of the code,
but mandating that dev->of_node is not NULL is simply unacceptable.

Guenter

>   							 data, &tps23861_chip_info,
>   							 NULL);
>   	if (IS_ERR(hwmon_dev))

Re: [PATCH] hwmon: (tps23861) add class restrictions and semi-auto mode support

[Gregory Fuchedgi]

> Bindings and driver changes need to be separate patches.
Thanks for the feedback! I'll wait for the complete review and start a
new patch series with the split and other fixes applied?
Would documentation change need to be a separate patch as well or
bundle it with bindings or the driver change?

> > +     const struct tps23861_data *data = input_data;
> > +     if (channel < TPS23861_NUM_PORTS && data->ports[channel].fully_disabled) {
> Why would it be necessary t check "channel < TPS23861_NUM_PORTS",
> and why would it make sense to declare attributes as valid if they
> are for a channel >= TPS23861_NUM_PORTS ?
That's because there's one extra hwmon voltage with index 4 in
addition to per-port voltages(indices 0-3).
So channel here can be equal to 4 for this special case and
data->ports check must be skipped.
See tps23861_info struct.

> > -     hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> > +     tps23861_init_ports(client);
> > +
> > +     hwmon_dev = devm_hwmon_device_register_with_info(dev, dev->of_node->name,
> Changing the name of the device may impact existing userspace code. On top of that,
> this change makes devicetree support mandatory. I didn't check the rest of the code,
> but mandating that dev->of_node is not NULL is simply unacceptable.
My thinking here was that most device trees likely have tps23861 as
the node name anyway,
so this change would unlikely to have a userspace impact, at the same
time it would enable us to
provide more meaningful name via device tree for cases when several
tps23861 present and
userspace code wants to differentiate them.
I also thought that for the probe to run a device tree node must be present,
so of_node would always be available.
Do you think it would be acceptable to use of_node->name if it is
available and fall back to
client->name otherwise? Or should I revert this piece?

Re: [PATCH] hwmon: (tps23861) add class restrictions and semi-auto mode support

[Krzysztof Kozlowski]

On 07/08/2025 23:50, Gregory Fuchedgi wrote:
> This commit adds optional individual per-port device tree nodes, in which ports
> can be:
>   - restricted by class. For example, class = <2> for a port would only enable
>     power if class 1 or class 2 were negotiated. Requires interrupt handler to
>     be configured if class != 4 (max supported). This is because hardware cannot
>     be set with acceptable classes in advance. So the driver would enable
>     Semi-Auto mode and in the interrupt handler would check negotiated class
>     against device tree config and enable power only if it is acceptable class.
>   - fully disabled. For boards that do not use all 4 ports. This would put
>     disabled ports in Off state and would hide corresponding hwmon files.
>   - off by default. The port is kept disabled, until enabled via corresponding
>     in_enable in sysfs.
> 

Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>


> The driver keeps current behavior of using Auto mode for all ports if no
> per-port device tree nodes given.
> 
> This commit also adds optional reset and shutdown pin support:
>   - if reset pin is configured the chip will be reset in probe.
>   - if both reset and shutdown pins are configured the driver would keep ports
>     shut down while configuring the tps23861 over i2c. Having both shutdown and
>     reset pins ensures no PoE activity happens while i2c configuration is
>     happening.
> 
> Signed-off-by: Gregory Fuchedgi <gfuchedgi@gmail.com>
> ---
>  .../bindings/hwmon/ti,tps23861.yaml           |  78 +++++-
>  Documentation/hwmon/tps23861.rst              |   6 +-
>  drivers/hwmon/tps23861.c                      | 253 +++++++++++++++++-
>  3 files changed, 325 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
> index ee7de53e1918..328050656ab8 100644
> --- a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
> @@ -24,12 +24,34 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
>    shunt-resistor-micro-ohms:
>      description: The value of current sense resistor in microohms.
>      default: 255000
>      minimum: 250000
>      maximum: 255000
>  
> +  reset-gpios:
> +    description: Optional GPIO for the reset pin.
> +    maxItems: 1
> +
> +  shutdown-gpios:
> +    description: |
> +      Optional GPIO for the shutdown pin. Used to prevent PoE activity before
> +      the driver had a chance to configure the chip.
> +    maxItems: 1
> +
> +  interrupts:
> +    description: |
> +      The interrupt specifier. Only required if PoE class is restricted to less
> +      than class 4 in the device tree.
> +    maxItems: 1
> +
>  required:
>    - compatible
>    - reg
> @@ -37,7 +59,27 @@ required:
>  allOf:
>    - $ref: hwmon-common.yaml#
>  
> -unevaluatedProperties: false
> +additionalProperties:
> +  type: object
> +  description: Port specific nodes.

Yuo should rather use patternProperties.

...

		IRQF_TRIGGER_FALLING,
> +						"tps23861_irq", data);
> +		if (ret) {
> +			dev_err(dev, "failed to request irq %d\n", data->irq);
> +			return ret;
> +		}
> +		dev_info(dev, "irq successfully requested\n");

Not useful. Drop.



Best regards,
Krzysztof

Re: [PATCH] hwmon: (tps23861) add class restrictions and semi-auto mode support

[Gregory Fuchedgi]

Thank you for the recommendations!
Couple of questions below.

On Thu, Aug 7, 2025 at 11:03 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 07/08/2025 23:50, Gregory Fuchedgi wrote:
> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling. Performing review on untested code might be
> a waste of time.
This change was tested internally using hw poe testers. Auto and Semi-Auto modes
were tested with different poe class restrictions. Current cut-offs were tested
too. It was tested on an older kernel because of the SoC vendor. The driver code
cleanly applied to linux-next though.

I would appreciate a few pointers regarding upstream testing infra. Could you
expand a bit more on the missing devicetree list and what needs to be changed?

> Yuo should rather use patternProperties.
The patch uses port DT node names as hwmon labels for curr and in files. Would
switching to something like patternProperties: ^port@[0-3]$ and adding optional
label instead be a good idea?

Re: [PATCH] hwmon: (tps23861) add class restrictions and semi-auto mode support

[Krzysztof Kozlowski]

On 08/08/2025 19:21, Gregory Fuchedgi wrote:
> Thank you for the recommendations!
> Couple of questions below.
> 
> On Thu, Aug 7, 2025 at 11:03 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On 07/08/2025 23:50, Gregory Fuchedgi wrote:
>> You missed at least devicetree list (maybe more), so this won't be
>> tested by automated tooling. Performing review on untested code might be
>> a waste of time.
> This change was tested internally using hw poe testers. Auto and Semi-Auto modes
> were tested with different poe class restrictions. Current cut-offs were tested
> too. It was tested on an older kernel because of the SoC vendor. The driver code
> cleanly applied to linux-next though.
> 
> I would appreciate a few pointers regarding upstream testing infra. Could you
> expand a bit more on the missing devicetree list and what needs to be changed?

The rest of the message explains everything.

> 
>> Yuo should rather use patternProperties.
> The patch uses port DT node names as hwmon labels for curr and in files. Would

I did not perform full review due to missing testing, so dunno.

Best regards,
Krzysztof