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

[Krzysztof Kozlowski <krzk@kernel.org>]

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