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

From: Rob Herring
Date: Tue Apr 02 2024 - 09:29:36 EST


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
>