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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250905-deft-porcelain-teal-a3bdbf@kuoka>
Date: Fri, 5 Sep 2025 10:10:40 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Gregory Fuchedgi <gfuchedgi@...il.com>
Cc: Robert Marko <robert.marko@...tura.hr>, 
	Luka Perkov <luka.perkov@...tura.hr>, Jean Delvare <jdelvare@...e.com>, 
	Guenter Roeck <linux@...ck-us.net>, Jonathan Corbet <corbet@....net>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-hwmon@...r.kernel.org, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 1/2] dt-bindings: hwmon: update TI TPS23861 with
 per-port schema

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@...il.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