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

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

[Gregory Fuchedgi via B4 Relay]

This patch series introduces per-port device tree configuration with poe
class restrictions. Also adds optional reset/shutdown gpios.

Tested with hw poe tester:
 - Auto mode tested with no per-port DT settings as well as explicit port
   DT ti,class=4. Tested that no IRQ is required in this case.
 - Semi-Auto mode with class restricted to 0, 1, 2 or 3. IRQ required.
 - Tested current cut-offs in Semi-Auto mode.
 - On/off by default setting tested for both Auto and Semi-Auto modes.
 - Tested fully disabling the ports in DT.
 - Tested with both reset and ti,ports-shutdown gpios defined, as well as
   with reset only, as well as with neither reset nor shutdown.

Signed-off-by: Gregory Fuchedgi <gfuchedgi@gmail.com>
---
Changes in v3:
 - cleaned up dt bindings in response to v2 review
 - Link to v2: https://lore.kernel.org/r/20250811-hwmon-tps23861-add-class-restrictions-v2-0-ebd122ec5e3b@gmail.com

Changes in v2:
 - code cleanup
 - split bindings into separate patch
 - use patternProperties
 - use labels instead of DT node names
 - add few comments for clarity

---
Gregory Fuchedgi (2):
      dt-bindings: hwmon: update TI TPS23861 with per-port schema
      hwmon: (tps23861) add class restrictions and semi-auto mode support

 .../devicetree/bindings/hwmon/ti,tps23861.yaml     |  93 +++++++-
 Documentation/hwmon/tps23861.rst                   |   6 +-
 drivers/hwmon/tps23861.c                           | 249 ++++++++++++++++++++-
 3 files changed, 341 insertions(+), 7 deletions(-)
---
base-commit: 3db46a82d467bd23d9ebc473d872a865785299d8
change-id: 20250808-hwmon-tps23861-add-class-restrictions-83ce3c02d885

Best regards,
-- 
Gregory Fuchedgi <gfuchedgi@gmail.com>

[PATCH v3 1/2] dt-bindings: hwmon: update TI TPS23861 with per-port schema

[Gregory Fuchedgi via B4 Relay]

From: Gregory Fuchedgi <gfuchedgi@gmail.com>

Update schema after per-port poe class restrictions and a few other options
were implemented.

Signed-off-by: Gregory Fuchedgi <gfuchedgi@gmail.com>
---
 .../devicetree/bindings/hwmon/ti,tps23861.yaml     | 93 +++++++++++++++++++++-
 1 file changed, 92 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
index ee7de53e19184d4c3df7564624532306d885f6e4..7538d1a9c19905ec90c48d34f84a92c1972f566b 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
@@ -24,12 +24,60 @@ 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: GPIO for the reset pin.
+    maxItems: 1
+
+  ti,ports-shutdown-gpios:
+    description:
+      GPIO for the shutdown pin. Used to prevent PoE activity before the driver
+      had a chance to configure the chip.
+    maxItems: 1
+
+  interrupts:
+    description:
+      Only required if PoE class is restricted to less than class 4 in the
+      device tree.
+    maxItems: 1
+
+patternProperties:
+  "^poe-port@[0-3]$":
+    type: object
+    description: Port specific nodes.
+    unevaluatedProperties: false
+    properties:
+      reg:
+        description: Port index.
+        items:
+          maximum: 3
+
+      ti,class:
+        description: The maximum power class a port should accept.
+        $ref: /schemas/types.yaml#/definitions/uint32
+        maximum: 4
+
+      ti,off-by-default:
+        description: Indicates the port is off by default.
+        type: boolean
+
+      label:
+        description: Port label.
+
+    required:
+      - reg
+
 required:
   - compatible
   - reg
@@ -45,9 +93,52 @@ examples:
         #address-cells = <1>;
         #size-cells = <0>;
 
-        tps23861@30 {
+        poe_controller@30 {
             compatible = "ti,tps23861";
             reg = <0x30>;
             shunt-resistor-micro-ohms = <255000>;
         };
     };
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        poe_controller@28 {
+            compatible = "ti,tps23861";
+            reg = <0x28>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+            shunt-resistor-micro-ohms = <255000>;
+            interrupt-parent = <&gpio1>;
+            interrupts = <14 IRQ_TYPE_EDGE_FALLING>;
+            label = "cabinet_poe_controller";
+            reset-gpios = <&gpio1 13 GPIO_ACTIVE_LOW>;
+            ti,ports-shutdown-gpios = <&gpio1 12 GPIO_ACTIVE_LOW>;
+
+            poe-port@0 {
+                    reg = <0>;
+                    ti,class = <2>; // Max PoE class allowed.
+                    ti,off-by-default;
+                    label = "cabinet_port_a";
+            };
+
+            poe-port@1 {
+                    reg = <1>;
+                    status = "disabled";
+            };
+
+            poe-port@2 {
+                    reg = <2>;
+                    status = "disabled";
+            };
+
+            poe-port@3 {
+                    reg = <3>;
+                    status = "disabled";
+            };
+        };
+    };

-- 
2.43.0

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

[Gregory Fuchedgi via B4 Relay]

From: Gregory Fuchedgi <gfuchedgi@gmail.com>

This commit adds optional individual per-port device tree nodes, in which
ports can be:
  - restricted by class. For example, ti,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 ti,ports-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.

Tested with hw poe tester:
 - Auto mode tested with no per-port DT settings as well as explicit port
   DT ti,class=4. Tested that no IRQ is required in this case.
 - Semi-Auto mode with class restricted to 0, 1, 2 or 3. IRQ required.
 - Tested current cut-offs in Semi-Auto mode.
 - On/off by default setting tested for both Auto and Semi-Auto modes.
 - Tested fully disabling the ports in DT.
 - Tested with both reset and ti,ports-shutdown gpios defined, as well as
   with reset only, as well as with neither reset nor shutdown.

Signed-off-by: Gregory Fuchedgi <gfuchedgi@gmail.com>
---
 Documentation/hwmon/tps23861.rst |   6 +-
 drivers/hwmon/tps23861.c         | 249 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 249 insertions(+), 6 deletions(-)

diff --git a/Documentation/hwmon/tps23861.rst b/Documentation/hwmon/tps23861.rst
index 46d121ff3f316d977ab3d1ab4e0d7b7736a57c60..5d14f683d0de11091f4a845a78a613604cbde4cc 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 associated child device tree entry or do not restrict poe
+class to 3 or lower will operate in Auto mode. Otherwise the port will 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 4cb3960d51704f6ad4106adc927f213d090c0f9a..c43c667c5d7e6f7813f8ac6bc5267cb8fe562f9c 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,12 @@
 #define TPS23861_GENERAL_MASK_1		0x17
 #define TPS23861_CURRENT_SHUNT_MASK	BIT(0)
 
+#define TPS23861_TIME_RESET_US		5
+#define TPS23861_TIME_RESET_US_MAX	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 +129,28 @@
 #define RESISTANCE_LSB			110966 /* 11.0966 Ohm*/
 #define RESISTANCE_LSB_LOW		157216 /* 15.7216 Ohm*/
 
+struct tps23861_port_data { // From DT.
+	const char *label;
+	/* 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 +242,19 @@ 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;
+
+	/* Channel may be >=TPS23861_NUM_PORTS for common Input voltage */
+	if (channel < TPS23861_NUM_PORTS && data->ports[channel].fully_disabled)
+		return 0;
 	switch (type) {
 	case hwmon_temp:
 		switch (attr) {
@@ -325,10 +368,15 @@ 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].label;
+		else
+			*str = tps23861_port_label[channel];
 		break;
 	case hwmon_temp:
 		*str = "Die";
@@ -502,17 +550,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].label, 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].label, 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 int 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_US, TPS23861_TIME_RESET_US_MAX);
+		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 int 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 int i;
+	bool need_irq = false;
+	const char *hwmon_name = client->name;
 
 	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].label = 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, "ti,ports-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 +707,55 @@ 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].off_by_default = of_property_read_bool(child, "ti,off-by-default");
+		of_property_read_string(child, "label", &ports[i].label);
+		of_property_read_u32(child, "ti,class", &ports[i].class);
+		if (ports[i].class > MAX_SUPPORTED_POE_CLASS) {
+			dev_err(dev, "%s invalid class, disabling\n", ports[i].label);
+			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].label, 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;
+		}
+		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 +771,11 @@ 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,
+	of_property_read_string(dev->of_node, "label", &hwmon_name);
+
+	tps23861_init_ports(client);
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, hwmon_name,
 							 data, &tps23861_chip_info,
 							 NULL);
 	if (IS_ERR(hwmon_dev))

-- 
2.43.0

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

[Guenter Roeck]

On Thu, Sep 04, 2025 at 10:33:45AM -0700, Gregory Fuchedgi via B4 Relay wrote:
> From: Gregory Fuchedgi <gfuchedgi@gmail.com>
> 
> This commit adds optional individual per-port device tree nodes, in which
> ports can be:
>   - restricted by class. For example, ti,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 ti,ports-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.
> 
> Tested with hw poe tester:
>  - Auto mode tested with no per-port DT settings as well as explicit port
>    DT ti,class=4. Tested that no IRQ is required in this case.
>  - Semi-Auto mode with class restricted to 0, 1, 2 or 3. IRQ required.
>  - Tested current cut-offs in Semi-Auto mode.
>  - On/off by default setting tested for both Auto and Semi-Auto modes.
>  - Tested fully disabling the ports in DT.
>  - Tested with both reset and ti,ports-shutdown gpios defined, as well as
>    with reset only, as well as with neither reset nor shutdown.
> 
> Signed-off-by: Gregory Fuchedgi <gfuchedgi@gmail.com>
> ---
>  Documentation/hwmon/tps23861.rst |   6 +-
>  drivers/hwmon/tps23861.c         | 249 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 249 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/hwmon/tps23861.rst b/Documentation/hwmon/tps23861.rst
> index 46d121ff3f316d977ab3d1ab4e0d7b7736a57c60..5d14f683d0de11091f4a845a78a613604cbde4cc 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 associated child device tree entry or do not restrict poe
> +class to 3 or lower will operate in Auto mode. Otherwise the port will 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 4cb3960d51704f6ad4106adc927f213d090c0f9a..c43c667c5d7e6f7813f8ac6bc5267cb8fe562f9c 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,12 @@
>  #define TPS23861_GENERAL_MASK_1		0x17
>  #define TPS23861_CURRENT_SHUNT_MASK	BIT(0)
>  
> +#define TPS23861_TIME_RESET_US		5
> +#define TPS23861_TIME_RESET_US_MAX	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 +129,28 @@
>  #define RESISTANCE_LSB			110966 /* 11.0966 Ohm*/
>  #define RESISTANCE_LSB_LOW		157216 /* 15.7216 Ohm*/
>  
> +struct tps23861_port_data { // From DT.

C comments or C++ comment, but not both.

> +	const char *label;
> +	/* Maximum class accepted by the port. The hardware cannot be

Standard multi-line comments, please. This is not the networking subsystem.

> +	 * 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 +242,19 @@ 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;
> +
> +	/* Channel may be >=TPS23861_NUM_PORTS for common Input voltage */
> +	if (channel < TPS23861_NUM_PORTS && data->ports[channel].fully_disabled)
> +		return 0;
>  	switch (type) {
>  	case hwmon_temp:
>  		switch (attr) {
> @@ -325,10 +368,15 @@ 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].label;
> +		else
> +			*str = tps23861_port_label[channel];
>  		break;
>  	case hwmon_temp:
>  		*str = "Die";
> @@ -502,17 +550,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].label, class, ports[i].class);
> +				continue;

In an interrupt handler ? Please don't.

> +			}
> +			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].label, class);

Same here. If you want to have debugging messages, make it debug messages.

> +			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 int 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_US, TPS23861_TIME_RESET_US_MAX);
> +		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 int 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 int i;
> +	bool need_irq = false;
> +	const char *hwmon_name = client->name;
>  
>  	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].label = 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, "ti,ports-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 +707,55 @@ 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);

If it is an error, return with an error. If it not an error,
dev_err is not acceptable.

> +			continue;
> +		}
> +		if (!of_device_is_available(child)) {
> +			ports[i].fully_disabled = true;
> +			continue;
> +		}
> +		ports[i].off_by_default = of_property_read_bool(child, "ti,off-by-default");
> +		of_property_read_string(child, "label", &ports[i].label);
> +		of_property_read_u32(child, "ti,class", &ports[i].class);
> +		if (ports[i].class > MAX_SUPPORTED_POE_CLASS) {
> +			dev_err(dev, "%s invalid class, disabling\n", ports[i].label);
> +			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].label, ports[i].class,
> +			 ports[i].off_by_default ? "off" : "on");

Please drop to dev_dbg or remove.

> +	}
> +	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;
> +		}
> +		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 +771,11 @@ 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,
> +	of_property_read_string(dev->of_node, "label", &hwmon_name);
> +
> +	tps23861_init_ports(client);
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, hwmon_name,
>  							 data, &tps23861_chip_info,
>  							 NULL);
>  	if (IS_ERR(hwmon_dev))
> 
> -- 
> 2.43.0
> 
> 

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

[Krzysztof Kozlowski]

On Thu, Sep 04, 2025 at 10:33:43AM -0700, Gregory Fuchedgi wrote:
> This patch series introduces per-port device tree configuration with poe
> class restrictions. Also adds optional reset/shutdown gpios.
> 
> Tested with hw poe tester:
>  - Auto mode tested with no per-port DT settings as well as explicit port
>    DT ti,class=4. Tested that no IRQ is required in this case.
>  - Semi-Auto mode with class restricted to 0, 1, 2 or 3. IRQ required.
>  - Tested current cut-offs in Semi-Auto mode.
>  - On/off by default setting tested for both Auto and Semi-Auto modes.
>  - Tested fully disabling the ports in DT.
>  - Tested with both reset and ti,ports-shutdown gpios defined, as well as
>    with reset only, as well as with neither reset nor shutdown.
> 
> Signed-off-by: Gregory Fuchedgi <gfuchedgi@gmail.com>
> ---
> Changes in v3:
>  - cleaned up dt bindings in response to v2 review

This is very vague. Everything is a change or clean up. I requested
specific things to be changed and you should list them.

Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: hwmon: update TI TPS23861 with per-port schema

[Krzysztof Kozlowski]

On Thu, Sep 04, 2025 at 10:33:44AM -0700, Gregory Fuchedgi wrote:
> Update schema after per-port poe class restrictions and a few other options
> were implemented.
> 
> Signed-off-by: Gregory Fuchedgi <gfuchedgi@gmail.com>
> ---
>  .../devicetree/bindings/hwmon/ti,tps23861.yaml     | 93 +++++++++++++++++++++-
>  1 file changed, 92 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
> index ee7de53e19184d4c3df7564624532306d885f6e4..7538d1a9c19905ec90c48d34f84a92c1972f566b 100644
> --- a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
> @@ -24,12 +24,60 @@ 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: GPIO for the reset pin.
> +    maxItems: 1
> +
> +  ti,ports-shutdown-gpios:

You can drop the vendor prefix, we do not have them for pins and
supplies.

> +    description:
> +      GPIO for the shutdown pin. Used to prevent PoE activity before the driver
> +      had a chance to configure the chip.
> +    maxItems: 1
> +
> +  interrupts:
> +    description:
> +      Only required if PoE class is restricted to less than class 4 in the
> +      device tree.
> +    maxItems: 1
> +
> +patternProperties:
> +  "^poe-port@[0-3]$":

Keep consistent quotes, either ' or ".

> +    type: object
> +    description: Port specific nodes.
> +    unevaluatedProperties: false
> +    properties:
> +      reg:
> +        description: Port index.
> +        items:

Drop items, you want here a scalar, not array, and this suggests you
miss array constrain.

> +          maximum: 3
> +
> +      ti,class:
> +        description: The maximum power class a port should accept.

What's the meaning of values? There are no other generic properties like
that? Last time it was a generic property, but maybe the answer to my
question should be that there is or should be such generic one?

Also, why exactly wouldn't you want to accept here always the highest
power class? What makes it a board-level property?

> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        maximum: 4

default: [ ... ]

> +
> +      ti,off-by-default:
> +        description: Indicates the port is off by default.

I fail to see how this is property of a board... unless you wanted to
figure out which ports are not connected, but status=disabled could be
used for that.

Sorry, but device has FIXED reset values for registers, so whether
something is off or on by default is defined by compatible.


> +        type: boolean
> +
> +      label:
> +        description: Port label.
> +
> +    required:
> +      - reg
> +
>  required:
>    - compatible
>    - reg
> @@ -45,9 +93,52 @@ examples:
>          #address-cells = <1>;
>          #size-cells = <0>;
>  
> -        tps23861@30 {
> +        poe_controller@30 {

See DTS coding style.

>              compatible = "ti,tps23861";
>              reg = <0x30>;
>              shunt-resistor-micro-ohms = <255000>;
>          };
>      };
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        poe_controller@28 {
> +            compatible = "ti,tps23861";
> +            reg = <0x28>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            shunt-resistor-micro-ohms = <255000>;
> +            interrupt-parent = <&gpio1>;
> +            interrupts = <14 IRQ_TYPE_EDGE_FALLING>;
> +            label = "cabinet_poe_controller";
> +            reset-gpios = <&gpio1 13 GPIO_ACTIVE_LOW>;
> +            ti,ports-shutdown-gpios = <&gpio1 12 GPIO_ACTIVE_LOW>;
> +
> +            poe-port@0 {
> +                    reg = <0>;

Mixed up indentation.

> +                    ti,class = <2>; // Max PoE class allowed.
> +                    ti,off-by-default;
> +                    label = "cabinet_port_a";

Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: hwmon: update TI TPS23861 with per-port schema

[Gregory Fuchedgi]

On Fri, Sep 5, 2025 at 1:10 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Thu, Sep 04, 2025 at 10:33:44AM -0700, Gregory Fuchedgi wrote:
> What's the meaning of values? There are no other generic properties like
> that? Last time it was a generic property, but maybe the answer to my
> question should be that there is or should be such generic one?
>
> Also, why exactly wouldn't you want to accept here always the highest
> power class? What makes it a board-level property?
poe class (values from 0 to 8, this chip only supports 0 to 4) defines maximum
power of a poe session. The higher the class the higher the power (with an
exception of 0 for historical reasons).

A board may have a power budget of let's say 7W allocated for a poe device. In
that case the board should only provide power to devices which ask/negotiate poe
class 1 (up to 4W) or class 2 (up to 7W). Devices that ask for more should be
rejected to prevent brown out issues.

I think some of my questions last time got lost in the noise. Given the info
above, should this be a generic property? And if yes where do I put it?
I haven't found an existing one.

> I fail to see how this is property of a board... unless you wanted to
> figure out which ports are not connected, but status=disabled could be
> used for that.
yes, status=disabled is used for ports that are not connected at all.
off-by-default property is different.

Most boards want simple automatic operation, no userspace involved. E.g. enable
power as soon as acceptable class was negotiated.

For some boards, however, it is critical to have control of poe from the
userspace. Without this property the driver may enable power before userspace is
ready.

> Sorry, but device has FIXED reset values for registers, so whether
> something is off or on by default is defined by compatible.
yes, but it is also defined by ports-shutdown pin state.

Here's our board startup sequence (see 2/2 patch):
reset pin has pull resistor keeping reset active until driver takes over the
pin. The driver activates ports-shutdown pin first and only then deactivates
reset. Then configuration over i2c happens, while ports are shut down. Then the
driver either enables a port based on off-by-default property or
doesn't, leaving
this up to the userspace.

This setup guarantees that from soc reset until userspace is ready there's no
poe activity on the ports. This implementation is also flexible and backwards
compatible.

Re: [PATCH v3 1/2] dt-bindings: hwmon: update TI TPS23861 with per-port schema

[Krzysztof Kozlowski]

On 05/09/2025 19:22, Gregory Fuchedgi wrote:
> On Fri, Sep 5, 2025 at 1:10 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On Thu, Sep 04, 2025 at 10:33:44AM -0700, Gregory Fuchedgi wrote:
>> What's the meaning of values? There are no other generic properties like

Where is context here? To which part was I replying / commenting on?

You are not making the process easy. I receive a lot of emails and have
no clue what this refers to.

Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: hwmon: update TI TPS23861 with per-port schema

[Gregory Fuchedgi]

On Sat, Sep 6, 2025 at 12:19 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 05/09/2025 19:22, Gregory Fuchedgi wrote:
> > On Fri, Sep 5, 2025 at 1:10 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >> On Thu, Sep 04, 2025 at 10:33:44AM -0700, Gregory Fuchedgi wrote:
> >> What's the meaning of values? There are no other generic properties like
>
> Where is context here? To which part was I replying / commenting on?
>
> You are not making the process easy. I receive a lot of emails and have
> no clue what this refers to.
You were asking about meaning of ti,class property values,
commenting on this piece:
> +      ti,class:
> +        description: The maximum power class a port should accept.

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

[Guenter Roeck]

+Cc: pse-pd maintainers and netdev mailing list

On 9/4/25 10:33, Gregory Fuchedgi via B4 Relay wrote:
> This patch series introduces per-port device tree configuration with poe
> class restrictions. Also adds optional reset/shutdown gpios.
> 
> Tested with hw poe tester:
>   - Auto mode tested with no per-port DT settings as well as explicit port
>     DT ti,class=4. Tested that no IRQ is required in this case.
>   - Semi-Auto mode with class restricted to 0, 1, 2 or 3. IRQ required.
>   - Tested current cut-offs in Semi-Auto mode.
>   - On/off by default setting tested for both Auto and Semi-Auto modes.
>   - Tested fully disabling the ports in DT.
>   - Tested with both reset and ti,ports-shutdown gpios defined, as well as
>     with reset only, as well as with neither reset nor shutdown.
> 
> Signed-off-by: Gregory Fuchedgi <gfuchedgi@gmail.com>

This entire series makes me more and more unhappy. It is not the responsibility
of the hardware monitoring subsystem to control power. The hardware monitoring
subsystem is for monitoring, not for control.

Please consider adding a driver for this chip to the pse-pd subsystem
(drivers/net/pse-pd). As it turns out, that subsystem already supports
tps23881. This is a similar chip which even has a similar register set.

This driver could then be modified to be an auxiliary driver of that driver.
Alternatively, we could drop this driver entirely since the pse-pd subsystem
registers the chips it supports as regulator which has its own means to handle
telemetry.

Thanks,
Guenter

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

[Oleksij Rempel]

On Sun, Sep 07, 2025 at 09:06:25AM -0700, Guenter Roeck wrote:
> +Cc: pse-pd maintainers and netdev mailing list
> 
> On 9/4/25 10:33, Gregory Fuchedgi via B4 Relay wrote:
> > This patch series introduces per-port device tree configuration with poe
> > class restrictions. Also adds optional reset/shutdown gpios.
> > 
> > Tested with hw poe tester:
> >   - Auto mode tested with no per-port DT settings as well as explicit port
> >     DT ti,class=4. Tested that no IRQ is required in this case.
> >   - Semi-Auto mode with class restricted to 0, 1, 2 or 3. IRQ required.
> >   - Tested current cut-offs in Semi-Auto mode.
> >   - On/off by default setting tested for both Auto and Semi-Auto modes.
> >   - Tested fully disabling the ports in DT.
> >   - Tested with both reset and ti,ports-shutdown gpios defined, as well as
> >     with reset only, as well as with neither reset nor shutdown.
> > 
> > Signed-off-by: Gregory Fuchedgi <gfuchedgi@gmail.com>
> 
> This entire series makes me more and more unhappy. It is not the responsibility
> of the hardware monitoring subsystem to control power. The hardware monitoring
> subsystem is for monitoring, not for control.
> 
> Please consider adding a driver for this chip to the pse-pd subsystem
> (drivers/net/pse-pd). As it turns out, that subsystem already supports
> tps23881. This is a similar chip which even has a similar register set.
> 
> This driver could then be modified to be an auxiliary driver of that driver.
> Alternatively, we could drop this driver entirely since the pse-pd subsystem
> registers the chips it supports as regulator which has its own means to handle
> telemetry.
> 
> Thanks,
> Guenter

Yes, Guenter is right. This driver belongs to the pse-pd framework.

Best Regards,
Oleksik
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Gregory Fuchedgi]

On Sun, Sep 7, 2025 at 9:51 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>
> On Sun, Sep 07, 2025 at 09:06:25AM -0700, Guenter Roeck wrote:
> > +Cc: pse-pd maintainers and netdev mailing list
> >
> > On 9/4/25 10:33, Gregory Fuchedgi via B4 Relay wrote:
> > > This patch series introduces per-port device tree configuration with poe
> > > class restrictions. Also adds optional reset/shutdown gpios.
> > >
> > > Tested with hw poe tester:
> > >   - Auto mode tested with no per-port DT settings as well as explicit port
> > >     DT ti,class=4. Tested that no IRQ is required in this case.
> > >   - Semi-Auto mode with class restricted to 0, 1, 2 or 3. IRQ required.
> > >   - Tested current cut-offs in Semi-Auto mode.
> > >   - On/off by default setting tested for both Auto and Semi-Auto modes.
> > >   - Tested fully disabling the ports in DT.
> > >   - Tested with both reset and ti,ports-shutdown gpios defined, as well as
> > >     with reset only, as well as with neither reset nor shutdown.
> > >
> > > Signed-off-by: Gregory Fuchedgi <gfuchedgi@gmail.com>
> >
> > This entire series makes me more and more unhappy. It is not the responsibility
> > of the hardware monitoring subsystem to control power. The hardware monitoring
> > subsystem is for monitoring, not for control.
> >
> > Please consider adding a driver for this chip to the pse-pd subsystem
> > (drivers/net/pse-pd). As it turns out, that subsystem already supports
> > tps23881. This is a similar chip which even has a similar register set.
> >
> > This driver could then be modified to be an auxiliary driver of that driver.
> > Alternatively, we could drop this driver entirely since the pse-pd subsystem
> > registers the chips it supports as regulator which has its own means to handle
> > telemetry.
> Yes, Guenter is right. This driver belongs to the pse-pd framework.
No disagreement here in principle. However, the current hwmon driver
already implements power control and exposes it via in*_enable sysfs
files. I found this a bit odd, but I don't write drivers often.
My understanding of Guenter's suggestion is that it would require breaking
this userspace API?

From a quick look at the tps23881 datasheet I can see that it is
similar, however, it is quite different in the context of this patch.
tps23881 (unlike tps23861) has Port Power Allocation register that can
limit poe power class. This register can be set prior to
detection/classification. So the extra complexity of an interrupt
handler that decides whether to enable the power may not be required.

Perhaps it still makes sense to merge these drivers, but I don't have
time or hardware to do it at the moment.

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

[Kory Maincent]

On Mon, 8 Sep 2025 09:39:58 -0700
Gregory Fuchedgi <gfuchedgi@gmail.com> wrote:

> On Sun, Sep 7, 2025 at 9:51 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> >
> > On Sun, Sep 07, 2025 at 09:06:25AM -0700, Guenter Roeck wrote:  
> > > +Cc: pse-pd maintainers and netdev mailing list
> > >
> > > On 9/4/25 10:33, Gregory Fuchedgi via B4 Relay wrote:  
>  [...]  
> > >
> > > This entire series makes me more and more unhappy. It is not the
> > > responsibility of the hardware monitoring subsystem to control power. The
> > > hardware monitoring subsystem is for monitoring, not for control.
> > >
> > > Please consider adding a driver for this chip to the pse-pd subsystem
> > > (drivers/net/pse-pd). As it turns out, that subsystem already supports
> > > tps23881. This is a similar chip which even has a similar register set.
> > >
> > > This driver could then be modified to be an auxiliary driver of that
> > > driver. Alternatively, we could drop this driver entirely since the
> > > pse-pd subsystem registers the chips it supports as regulator which has
> > > its own means to handle telemetry.  
> > Yes, Guenter is right. This driver belongs to the pse-pd framework.  
> No disagreement here in principle. However, the current hwmon driver
> already implements power control and exposes it via in*_enable sysfs
> files. I found this a bit odd, but I don't write drivers often.
> My understanding of Guenter's suggestion is that it would require breaking
> this userspace API?
>
> From a quick look at the tps23881 datasheet I can see that it is
> similar, however, it is quite different in the context of this patch.
> tps23881 (unlike tps23861) has Port Power Allocation register that can
> limit poe power class. This register can be set prior to
> detection/classification. So the extra complexity of an interrupt
> handler that decides whether to enable the power may not be required.
> 
> Perhaps it still makes sense to merge these drivers, but I don't have
> time or hardware to do it at the moment.

In either way the tps23861 is a PoE controller therefore the driver should land
into the pse-pd framework. Then tweaking tps23881 driver to support tps2361 or
using a standalone driver is another question.
From a quick look the register map is really similar so indeed using tps23881
driver seems relevant.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

[Guenter Roeck]

On 9/8/25 09:39, Gregory Fuchedgi wrote:
> On Sun, Sep 7, 2025 at 9:51 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>>
>> On Sun, Sep 07, 2025 at 09:06:25AM -0700, Guenter Roeck wrote:
>>> +Cc: pse-pd maintainers and netdev mailing list
>>>
>>> On 9/4/25 10:33, Gregory Fuchedgi via B4 Relay wrote:
>>>> This patch series introduces per-port device tree configuration with poe
>>>> class restrictions. Also adds optional reset/shutdown gpios.
>>>>
>>>> Tested with hw poe tester:
>>>>    - Auto mode tested with no per-port DT settings as well as explicit port
>>>>      DT ti,class=4. Tested that no IRQ is required in this case.
>>>>    - Semi-Auto mode with class restricted to 0, 1, 2 or 3. IRQ required.
>>>>    - Tested current cut-offs in Semi-Auto mode.
>>>>    - On/off by default setting tested for both Auto and Semi-Auto modes.
>>>>    - Tested fully disabling the ports in DT.
>>>>    - Tested with both reset and ti,ports-shutdown gpios defined, as well as
>>>>      with reset only, as well as with neither reset nor shutdown.
>>>>
>>>> Signed-off-by: Gregory Fuchedgi <gfuchedgi@gmail.com>
>>>
>>> This entire series makes me more and more unhappy. It is not the responsibility
>>> of the hardware monitoring subsystem to control power. The hardware monitoring
>>> subsystem is for monitoring, not for control.
>>>
>>> Please consider adding a driver for this chip to the pse-pd subsystem
>>> (drivers/net/pse-pd). As it turns out, that subsystem already supports
>>> tps23881. This is a similar chip which even has a similar register set.
>>>
>>> This driver could then be modified to be an auxiliary driver of that driver.
>>> Alternatively, we could drop this driver entirely since the pse-pd subsystem
>>> registers the chips it supports as regulator which has its own means to handle
>>> telemetry.
>> Yes, Guenter is right. This driver belongs to the pse-pd framework.
> No disagreement here in principle. However, the current hwmon driver
> already implements power control and exposes it via in*_enable sysfs
> files. I found this a bit odd, but I don't write drivers often.
> My understanding of Guenter's suggestion is that it would require breaking
> this userspace API?
> 

If the enable attributes enable power to the ports, that code and functionality
is simply wrong. It should only enable (or have enabled) power _monitoring_.
As such, changing that would from my perspective be a bug fix.

And, yes, that slipped my attention when reviewing the original code.
Sorry to have to say that, but I am not perfect.

>  From a quick look at the tps23881 datasheet I can see that it is
> similar, however, it is quite different in the context of this patch.
> tps23881 (unlike tps23861) has Port Power Allocation register that can
> limit poe power class. This register can be set prior to
> detection/classification. So the extra complexity of an interrupt
> handler that decides whether to enable the power may not be required.
> 
> Perhaps it still makes sense to merge these drivers, but I don't have
> time or hardware to do it at the moment.

I didn't suggest to merge the tps23881 and tps23861 drivers; I just pointed out
that they have a similar register set.

The point here is that a hardware monitoring driver should limit itself
to hardware monitoring. Actual control should, for example, be implemented
through the regulator or thermal subsystems.

Guenter

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

[Gregory Fuchedgi]

On Mon, Sep 8, 2025 at 11:02 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 9/8/25 09:39, Gregory Fuchedgi wrote:
> > On Sun, Sep 7, 2025 at 9:51 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> >>
> >> On Sun, Sep 07, 2025 at 09:06:25AM -0700, Guenter Roeck wrote:
> >>> +Cc: pse-pd maintainers and netdev mailing list
> >>>
> >>> On 9/4/25 10:33, Gregory Fuchedgi via B4 Relay wrote:
> >>>> This patch series introduces per-port device tree configuration with poe
> >>>> class restrictions. Also adds optional reset/shutdown gpios.
> >>>>
> >>>> Tested with hw poe tester:
> >>>>    - Auto mode tested with no per-port DT settings as well as explicit port
> >>>>      DT ti,class=4. Tested that no IRQ is required in this case.
> >>>>    - Semi-Auto mode with class restricted to 0, 1, 2 or 3. IRQ required.
> >>>>    - Tested current cut-offs in Semi-Auto mode.
> >>>>    - On/off by default setting tested for both Auto and Semi-Auto modes.
> >>>>    - Tested fully disabling the ports in DT.
> >>>>    - Tested with both reset and ti,ports-shutdown gpios defined, as well as
> >>>>      with reset only, as well as with neither reset nor shutdown.
> >>>>
> >>>> Signed-off-by: Gregory Fuchedgi <gfuchedgi@gmail.com>
> >>>
> >>> This entire series makes me more and more unhappy. It is not the responsibility
> >>> of the hardware monitoring subsystem to control power. The hardware monitoring
> >>> subsystem is for monitoring, not for control.
> >>>
> >>> Please consider adding a driver for this chip to the pse-pd subsystem
> >>> (drivers/net/pse-pd). As it turns out, that subsystem already supports
> >>> tps23881. This is a similar chip which even has a similar register set.
> >>>
> >>> This driver could then be modified to be an auxiliary driver of that driver.
> >>> Alternatively, we could drop this driver entirely since the pse-pd subsystem
> >>> registers the chips it supports as regulator which has its own means to handle
> >>> telemetry.
> >> Yes, Guenter is right. This driver belongs to the pse-pd framework.
> > No disagreement here in principle. However, the current hwmon driver
> > already implements power control and exposes it via in*_enable sysfs
> > files. I found this a bit odd, but I don't write drivers often.
> > My understanding of Guenter's suggestion is that it would require breaking
> > this userspace API?
> >
>
> If the enable attributes enable power to the ports, that code and functionality
> is simply wrong. It should only enable (or have enabled) power _monitoring_.
> As such, changing that would from my perspective be a bug fix.

Alright, then. I'll try to find some time in the next few months to port this
over to a separate driver in pse-pd. And then remove the in*_enable from hwmon
one. Even if it's there by mistake, probably shouldn't fix it until there's an
alternative in place.

>
> And, yes, that slipped my attention when reviewing the original code.
> Sorry to have to say that, but I am not perfect.
>
> >  From a quick look at the tps23881 datasheet I can see that it is
> > similar, however, it is quite different in the context of this patch.
> > tps23881 (unlike tps23861) has Port Power Allocation register that can
> > limit poe power class. This register can be set prior to
> > detection/classification. So the extra complexity of an interrupt
> > handler that decides whether to enable the power may not be required.
> >
> > Perhaps it still makes sense to merge these drivers, but I don't have
> > time or hardware to do it at the moment.
>
> I didn't suggest to merge the tps23881 and tps23861 drivers; I just pointed out
> that they have a similar register set.
>
> The point here is that a hardware monitoring driver should limit itself
> to hardware monitoring. Actual control should, for example, be implemented
> through the regulator or thermal subsystems.