Re: [net-next][PATCH v3] dt-bindings: dsa: lan9303: Add lan9303 yaml

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 28/09/2022 18:21, Jerry Ray wrote:
> Adding the dt binding yaml for the lan9303 3-port ethernet switch.
> The microchip lan9354 3-port ethernet switch will also use the
> same binding.
> 
> Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
> ---
> v2->v3:
>  - removed cpu labels
>  - now patching against latest net-next
> v1->v2:
>  - fixed dt_binding_check warning
>  - added max-speed setting on the switches 10/100 ports.
> ---
>  .../devicetree/bindings/net/dsa/lan9303.txt   |  99 +-------------
>  .../bindings/net/dsa/microchip,lan9303.yaml   | 129 ++++++++++++++++++
>  MAINTAINERS                                   |   8 ++
>  3 files changed, 139 insertions(+), 97 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/lan9303.txt b/Documentation/devicetree/bindings/net/dsa/lan9303.txt
> index 46a732087f5c..8c8fedd1f00e 100644
> --- a/Documentation/devicetree/bindings/net/dsa/lan9303.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/lan9303.txt
> @@ -1,100 +1,5 @@
>  SMSC/MicroChip LAN9303 three port ethernet switch
>  -------------------------------------------------

Old file should be entirely removed.

>  
> -Required properties:
> -
> -- compatible: should be
> -  - "smsc,lan9303-i2c" for I2C managed mode
> -    or
> -  - "smsc,lan9303-mdio" for mdio managed mode
> -
> -Optional properties:
> -
> -- reset-gpios: GPIO to be used to reset the whole device
> -- reset-duration: reset duration in milliseconds, defaults to 200 ms
> -
> -Subnodes:
> -
> -The integrated switch subnode should be specified according to the binding
> -described in dsa/dsa.txt. The CPU port of this switch is always port 0.
> -
> -Note: always use 'reg = <0/1/2>;' for the three DSA ports, even if the device is
> -configured to use 1/2/3 instead. This hardware configuration will be
> -auto-detected and mapped accordingly.
> -
> -Example:
> -
> -I2C managed mode:
> -
> -	master: masterdevice@X {
> -
> -		fixed-link { /* RMII fixed link to LAN9303 */
> -			speed = <100>;
> -			full-duplex;
> -		};
> -	};
> -
> -	switch: switch@a {
> -		compatible = "smsc,lan9303-i2c";
> -		reg = <0xa>;
> -		reset-gpios = <&gpio7 6 GPIO_ACTIVE_LOW>;
> -		reset-duration = <200>;
> -
> -		ports {
> -			#address-cells = <1>;
> -			#size-cells = <0>;
> -
> -			port@0 { /* RMII fixed link to master */
> -				reg = <0>;
> -				ethernet = <&master>;
> -			};
> -
> -			port@1 { /* external port 1 */
> -				reg = <1>;
> -				label = "lan1";
> -			};
> -
> -			port@2 { /* external port 2 */
> -				reg = <2>;
> -				label = "lan2";
> -			};
> -		};
> -	};
> -
> -MDIO managed mode:
> -
> -	master: masterdevice@X {
> -		phy-handle = <&switch>;
> -
> -		mdio {
> -			#address-cells = <1>;
> -			#size-cells = <0>;
> -
> -			switch: switch-phy@0 {
> -				compatible = "smsc,lan9303-mdio";
> -				reg = <0>;
> -				reset-gpios = <&gpio7 6 GPIO_ACTIVE_LOW>;
> -				reset-duration = <100>;
> -
> -				ports {
> -					#address-cells = <1>;
> -					#size-cells = <0>;
> -
> -					port@0 {
> -						reg = <0>;
> -						ethernet = <&master>;
> -					};
> -
> -					port@1 { /* external port 1 */
> -						reg = <1>;
> -						label = "lan1";
> -					};
> -
> -					port@2 { /* external port 2 */
> -						reg = <2>;
> -						label = "lan2";
> -					};
> -				};
> -			};
> -		};
> -	};
> +See Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml for the
> +device tree documentation covering the LAN9303 and LAN9354 devices.

No, just remove it.

> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml
> new file mode 100644
> index 000000000000..818770092a2c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,lan9303.yaml
> @@ -0,0 +1,129 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/dsa/microchip,lan9303.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LAN9303 Ethernet Switch Series Tree Bindings

Drop "Tree Bindings"

> +
> +allOf:
> +  - $ref: "dsa.yaml#"

Drop quotes.

> +
> +maintainers:
> +  - UNGLinuxDriver@microchip.com
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - enum:
> +          - smsc,lan9303-mdio
> +          - microchip,lan9354-mdio
> +      - enum:
> +          - smsc,lan9303-i2c

oneOf does not make sense. It's just one enum.

> +
> +  reg:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    description: Optional gpio specifier for a reset line

Drop "gpio specifier for a"

> +    maxItems: 1
> +
> +  reset-duration:
> +    description: Reset duration in milliseconds, defaults to 200 ms

This does not look like standard type or unit suffix, so you need here
type. Don't you have warnings for this?

default: 200


> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    //Ethernet switch connected via mdio to the host

Missing space before comments. Use Linux style comments, not your own.

> +    ethernet0 {

Drop "0".

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        phy-handle = <&lan9303switch>;
> +        phy-mode = "rmii";
> +        fixed-link {
> +            speed = <100>;
> +            full-duplex;
> +        };
> +        mdio {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            lan9303switch: switch@0 {
> +                compatible = "smsc,lan9303-mdio";
> +                dsa,member = <0 0>;
> +                reg = <0>;

reg goes after compatible.

> +                ethernet-ports {
> +                    #address-cells = <1>;
> +                    #size-cells = <0>;
> +                        port@0 {
> +                            reg = <0>;
> +                            phy-mode = "rmii";
> +                            ethernet = <&ethernet>;
> +                            fixed-link {
> +                                speed = <100>;
> +                                full-duplex;
> +                            };
> +                        };
> +                        port@1 {
> +                            reg = <1>;
> +                            max-speed = <100>;
> +                            label = "lan1";
> +                        };
> +                        port@2 {
> +                            reg = <2>;
> +                            max-speed = <100>;
> +                            label = "lan2";
> +                        };
> +                    };
> +                };
> +            };
> +        };
> +
> +    //Ethernet switch connected via i2c to the host
> +    ethernet1 {

Just ethernet (and make it a second example)



Best regards,
Krzysztof