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

[PATCH v2 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
   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 shutdown gpios defined, as well as with reset
   only, as well as with neither reset nor shutdown.

V1->V2:
 - code cleanup
 - split bindings into separate patch
 - use patternProperties
 - use labels instead of DT node names
 - add few comments for clarity

Signed-off-by: Gregory Fuchedgi <gfuchedgi@gmail.com>
---
Gregory Fuchedgi (2):
      hwmon: (tps23861) add class restrictions and semi-auto mode support
      dt-bindings: hwmon: update TI TPS23861 bindings with per-port schema

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

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

[PATCH v2 1/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, 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>
---
 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..a7f7ec0e9c54bd09845ae559b094c388f2b1a9a7 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, "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, "off-by-default");
+		of_property_read_string(child, "label", &ports[i].label);
+		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", 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

[PATCH v2 2/2] dt-bindings: hwmon: update TI TPS23861 bindings 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     | 86 ++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
index ee7de53e19184d4c3df7564624532306d885f6e4..578f4dad7eab630b218e9e30b23fc611a760d332 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
@@ -24,12 +24,62 @@ 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
+
+patternProperties:
+  "^port@[0-3]$":
+    type: object
+    description: Port specific nodes.
+    unevaluatedProperties: false
+    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
+
+      label:
+        description: Optional port label
+
 required:
   - compatible
   - reg
@@ -51,3 +101,39 @@ 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>;
+            label = "my_poe_controller";
+            port@0 {
+                    reg = <0>;
+                    class = <2>; // Max PoE class allowed.
+                    off-by-default;
+                    label = "myport";
+            };
+            port@1 {
+                    reg = <1>;
+                    status = "disabled";
+            };
+            port@2 {
+                    reg = <2>;
+                    status = "disabled";
+            };
+            port@3 {
+                    reg = <3>;
+                    status = "disabled";
+            };
+        };
+    };

-- 
2.43.0

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

[Krzysztof Kozlowski]

On 11/08/2025 18:13, Gregory Fuchedgi via B4 Relay wrote:
> From: Gregory Fuchedgi <gfuchedgi@gmail.com>
> 
> Update schema after per-port poe class restrictions and a few other options
> were implemented.

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> 
> Signed-off-by: Gregory Fuchedgi <gfuchedgi@gmail.com>
> ---
>  .../devicetree/bindings/hwmon/ti,tps23861.yaml     | 86 ++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
> index ee7de53e19184d4c3df7564624532306d885f6e4..578f4dad7eab630b218e9e30b23fc611a760d332 100644
> --- a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
> @@ -24,12 +24,62 @@ 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:

powerdown-gpios, see gpio-consumer-common.yaml

> +    description: |

Drop |

> +      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

Drop first sentence, redundant. Interrupts property cannot be anything
else than interrupt specifier.

> +      than class 4 in the device tree.
> +    maxItems: 1
> +
> +patternProperties:
> +  "^port@[0-3]$":

This goes to ports property.


> +    type: object
> +    description: Port specific nodes.
> +    unevaluatedProperties: false
> +    required:
> +      - reg

required goes to the end.

> +
> +    properties:
> +      reg:
> +        description: Port index.
> +        items:
> +          minimum: 0

Drop minimum.

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

What are the values? Where is the property defined - which schema - that
you do not use vendor prefix?

> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        minimum: 0

Drop minimum.

> +        maximum: 4
> +
> +      off-by-default:

Same question - which common schema defines this?

> +        description: Indicates the port is off by default.
> +        type: boolean
> +
> +      label:
> +        description: Optional port label

Skip all "optional" here and other places. Schema tells it, not free
form text. Say something useful here or just ": true".

> +
>  required:
>    - compatible
>    - reg
> @@ -51,3 +101,39 @@ examples:
>              shunt-resistor-micro-ohms = <255000>;
>          };
>      };
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        tps23861@28 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +            #address-cells = <1>;
> +            #size-cells = <0>;

Follow closely DTS coding style.

> +            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>;

0 looks like invalid flag. Use proper defines and proper values.

> +            label = "my_poe_controller";

Use useful names or just drop it.


> +            port@0 {
> +                    reg = <0>;
> +                    class = <2>; // Max PoE class allowed.


> +                    off-by-default;
> +                    label = "myport";

Also not useful.


Best regards,
Krzysztof

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

[Gregory Fuchedgi]

On Tue, Aug 12, 2025 at 12:20 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > +  shutdown-gpios:
> powerdown-gpios, see gpio-consumer-common.yaml
It is called shutdown in the datasheet, but seems like neither powerdown nor
shutdown truly reflects its purpose. This pin doesn't power down the controller
itself. It shuts down the ports while keeping the controller available for
configuration over i2c. Should I call it ti,ports-shutdown-gpios or maybe
ti,shutdown-gpios? Any other suggestions?

> > +patternProperties:
> > +  "^port@[0-3]$":
> This goes to ports property.
Do you mean I should add another DT node that groups all ports? such as:
compatible = "ti,tps23861"; ports { port@0 {...} port@1 {...} }

If that's the case would it make sense to use "^.*$" pattern to allow any name
and drop the port label? Is patternProperties even needed in this case?

> > +        tps23861@28 {
> Node names should be generic. See also an explanation and list of
Ack. Should I also fix the existing example in this patch?

> > +            label = "my_poe_controller";
> Use useful names or just drop it.
I thought this is good as an example? A useful name would be board specific.

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

[Krzysztof Kozlowski]

On 13/08/2025 05:00, Gregory Fuchedgi wrote:
> On Tue, Aug 12, 2025 at 12:20 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>> +  shutdown-gpios:
>> powerdown-gpios, see gpio-consumer-common.yaml
> It is called shutdown in the datasheet, but seems like neither powerdown nor
> shutdown truly reflects its purpose. This pin doesn't power down the controller
> itself. It shuts down the ports while keeping the controller available for
> configuration over i2c. Should I call it ti,ports-shutdown-gpios or maybe
> ti,shutdown-gpios? Any other suggestions?


Feels more like enable-gpios.

> 
>>> +patternProperties:
>>> +  "^port@[0-3]$":
>> This goes to ports property.
> Do you mean I should add another DT node that groups all ports? such as:
> compatible = "ti,tps23861"; ports { port@0 {...} port@1 {...} }


Yes.

> 
> If that's the case would it make sense to use "^.*$" pattern to allow any name
> and drop the port label? Is patternProperties even needed in this case?


You should use standard graph bindings, so:

git grep 'ref' -- Documentation/devicetree/bindings/ | grep ports


> 
>>> +        tps23861@28 {
>> Node names should be generic. See also an explanation and list of
> Ack. Should I also fix the existing example in this patch?


You can, up to you.

> 
>>> +            label = "my_poe_controller";
>> Use useful names or just drop it.
> I thought this is good as an example? A useful name would be board specific.


Then it should be board specific. You add here real and the most
complete example.


Best regards,
Krzysztof

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

[Rob Herring]

On Sun, Aug 17, 2025 at 09:23:09AM +0200, Krzysztof Kozlowski wrote:
> On 13/08/2025 05:00, Gregory Fuchedgi wrote:
> > On Tue, Aug 12, 2025 at 12:20 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>> +  shutdown-gpios:
> >> powerdown-gpios, see gpio-consumer-common.yaml
> > It is called shutdown in the datasheet, but seems like neither powerdown nor
> > shutdown truly reflects its purpose. This pin doesn't power down the controller
> > itself. It shuts down the ports while keeping the controller available for
> > configuration over i2c. Should I call it ti,ports-shutdown-gpios or maybe
> > ti,shutdown-gpios? Any other suggestions?
> 
> 
> Feels more like enable-gpios.
> 
> > 
> >>> +patternProperties:
> >>> +  "^port@[0-3]$":
> >> This goes to ports property.
> > Do you mean I should add another DT node that groups all ports? such as:
> > compatible = "ti,tps23861"; ports { port@0 {...} port@1 {...} }
> 
> 
> Yes.

Except this is not an OF graph. Don't re-use it when it is not that. 
Maybe 'poe-port@'? Is multiple ports/channels something common on PoE 
chips? I'd guess so. If so, then come up with something common.

Whether you should have a container node like 'ports' is a separate 
question. You get exactly 1 address space for any given node. So if you 
ever might need to address multiple disjoint things, then you probably 
want a container node.

Rob

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

[Gregory Fuchedgi]

On Sun, Aug 17, 2025 at 09:23:09AM +0200, Krzysztof Kozlowski wrote:
> On 13/08/2025 05:00, Gregory Fuchedgi wrote:
> > On Tue, Aug 12, 2025 at 12:20 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>> +  shutdown-gpios:
> >> powerdown-gpios, see gpio-consumer-common.yaml
> > It is called shutdown in the datasheet, but seems like neither powerdown nor
> > shutdown truly reflects its purpose. This pin doesn't power down the controller
> > itself. It shuts down the ports while keeping the controller available for
> > configuration over i2c. Should I call it ti,ports-shutdown-gpios or maybe
> > ti,shutdown-gpios? Any other suggestions?
> Feels more like enable-gpios.
Wouldn't that be confusing, since there's no enable pin in the datasheet? Also
it doesn't enable/disable the controller itself, but its ports.
In my mind ti,ports-shutdown-gpios is the most meaningful name for it. That said
I appreciate the guidance, since I do not know what's the usual way to do this?
Happy to go with enable-gpios if that's the convention.

On Mon, Aug 18, 2025 at 10:31 AM Rob Herring <robh@kernel.org> wrote:
> > >>> +patternProperties:
> > >>> +  "^port@[0-3]$":
> > >> This goes to ports property.
> > > Do you mean I should add another DT node that groups all ports? such as:
> > > compatible = "ti,tps23861"; ports { port@0 {...} port@1 {...} }
> > Yes.
> Except this is not an OF graph. Don't re-use it when it is not that.
> Maybe 'poe-port@'? Is multiple ports/channels something common on PoE
> chips? I'd guess so. If so, then come up with something common.
poe-port@ sounds good to me. When you say come up with something common, does
that imply adding it to a new file, like bindings/hwmon/poe-common.yaml? Or just
using poe-port in this dt without the parent ports node?

> Whether you should have a container node like 'ports' is a separate
> question. You get exactly 1 address space for any given node. So if you
> ever might need to address multiple disjoint things, then you probably
> want a container node.
I do not want to address anything else in this case, so I'd keep it simple.
But let me know if I'm missing any important details.
Appreciate the guidance.