Re: [PATCH net-next v6 14/17] dt-bindings: net: pse-pd: Add bindings for PD692x0 PSE controller

[Rob Herring]

On Tue, Mar 26, 2024 at 03:04:51PM +0100, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@xxxxxxxxxxx>
> 
> Add the PD692x0 I2C Power Sourcing Equipment controller device tree
> bindings documentation.
> 
> Signed-off-by: Kory Maincent <kory.maincent@xxxxxxxxxxx>
> ---
> 
> Changes in v2:
> - Enhance ports-matrix description.
> - Replace additionalProperties by unevaluatedProperties.
> - Drop i2c suffix.
> 
> Changes in v3:
> - Remove ports-matrix parameter.
> - Add description of all physical ports and managers.
> - Add pse_pis subnode moving to the API of pse-controller binding.
> - Remove the MAINTAINERS section for this driver as I will be maintaining
>   all pse-pd subsystem.
> 
> Changes in v5:
> - Remove defs used only once.
> - Replace underscore by dash.
> - Add description.
> ---
>  .../bindings/net/pse-pd/microchip,pd692x0.yaml     | 158 +++++++++++++++++++++
>  1 file changed, 158 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/pse-pd/microchip,pd692x0.yaml b/Documentation/devicetree/bindings/net/pse-pd/microchip,pd692x0.yaml
> new file mode 100644
> index 000000000000..62ea4363cba3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pse-pd/microchip,pd692x0.yaml
> @@ -0,0 +1,158 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/pse-pd/microchip,pd692x0.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip PD692x0 Power Sourcing Equipment controller
> +
> +maintainers:
> +  - Kory Maincent <kory.maincent@xxxxxxxxxxx>
> +
> +allOf:
> +  - $ref: pse-controller.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,pd69200
> +      - microchip,pd69210
> +      - microchip,pd69220
> +
> +  reg:
> +    maxItems: 1
> +
> +  managers:
> +    type: object
> +    description:
> +      List of the PD69208T4/PD69204T4/PD69208M PSE managers. Each manager
> +      have 4 or 8 physical ports according to the chip version. No need to
> +      specify the SPI chip select as it is automatically detected by the
> +      PD692x0 PSE controller. The PSE managers have to be described from
> +      the lowest chip select to the greatest one, which is the detection
> +      behavior of the PD692x0 PSE controller. The PD692x0 support up to
> +      12 PSE managers which can expose up to 96 physical ports. All
> +      physical ports available on a manager have to be described in the
> +      incremental order even if they are not used.
> +
> +    properties:
> +      "#address-cells":
> +        const: 1
> +
> +      "#size-cells":
> +        const: 0
> +
> +    required:
> +      - "#address-cells"
> +      - "#size-cells"
> +
> +    patternProperties:
> +      "^manager@0[0-9]|1[0-2]$":

Unit-addresses are typically in hex.

Is 'manager' something specific to this device or should be common?

> +        $ref: /schemas/graph.yaml#/properties/ports

This is not using the graph binding. Furthermore, I don't want to see 
new cases of 'port' node names which are not graph nodes. We have it 
already with ethernet switches, but 'ethernet-port' is preferred over 
'port'.

Why is this one 'managers' and the other device binding 'channels'?

> +        description:
> +          PD69208T4/PD69204T4/PD69208M PSE manager exposing 4 or 8 physical
> +          ports.
> +
> +        properties:
> +          reg:
> +            description:
> +              Incremental index of the PSE manager starting from 0, ranging
> +              from lowest to highest chip select, up to 12.
> +            maxItems: 1
> +
> +        patternProperties:
> +          '^port@[0-7]$':
> +            type: object
> +            required:
> +              - reg

Any property you want is allowed in this node. You are missing 
'additionalProperties'.

> +
> +        required:
> +          - reg
> +
> +required:
> +  - compatible
> +  - reg
> +  - pse-pis
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      ethernet-pse@3c {
> +        compatible = "microchip,pd69200";
> +        reg = <0x3c>;
> +
> +        managers {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          manager@0 {
> +            reg = <0>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            phys0: port@0 {
> +              reg = <0>;
> +            };
> +
> +            phys1: port@1 {
> +              reg = <1>;
> +            };
> +
> +            phys2: port@2 {
> +              reg = <2>;
> +            };
> +
> +            phys3: port@3 {
> +              reg = <3>;
> +            };
> +          };
> +
> +          manager@1 {
> +            reg = <1>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            phys4: port@0 {
> +              reg = <0>;
> +            };
> +
> +            phys5: port@1 {
> +              reg = <1>;
> +            };
> +
> +            phys6: port@2 {
> +              reg = <2>;
> +            };
> +
> +            phys7: port@3 {
> +              reg = <3>;
> +            };
> +          };
> +        };
> +
> +        pse-pis {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          pse_pi0: pse-pi@0 {
> +            reg = <0>;
> +            #pse-cells = <0>;
> +            pairset-names = "alternative-a", "alternative-b";
> +            pairsets = <&phys0>, <&phys1>;

It is very strange that you are describing the connections within a 
device.


> +            polarity-supported = "MDI", "S";
> +          };
> +          pse_pi1: pse-pi@1 {
> +            reg = <1>;
> +            #pse-cells = <0>;
> +            pairset-names = "alternative-a";
> +            pairsets = <&phys2>;
> +            polarity-supported = "MDI";
> +          };
> +        };
> +      };
> +    };
> 
> -- 
> 2.25.1
>