Re: [PATCH] dt-bindings: net: dsa: realtek-smi: convert to YAML schema

From: Rob Herring
Date: Mon Jan 10 2022 - 13:20:46 EST

On Tue, Dec 28, 2021 at 04:26:45AM -0300, Luiz Angelo Daros de Luca wrote:
> Schema changes:
>
> - "interrupt-controller" was not added as a required property. It might
> still work polling the ports when missing
> - "interrupt" property was mentioned but never used. According to its
> description, it was assumed it was really "interrupt-parent"
>
> Examples changes:
>
> - renamed "switch_intc" to make it unique between examples
> - removed "dsa-mdio" from mdio compatible property
> - renamed phy@0 to ethernet-phy@0 (not tested with real HW)
> phy@ requires #phy-cells
>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@xxxxxxxxx>
> ---
> .../bindings/net/dsa/realtek-smi.txt | 240 --------------
> .../bindings/net/dsa/realtek-smi.yaml | 310 ++++++++++++++++++
> 2 files changed, 310 insertions(+), 240 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/net/dsa/realtek-smi.txt
> create mode 100644 Documentation/devicetree/bindings/net/dsa/realtek-smi.yaml


> diff --git a/Documentation/devicetree/bindings/net/dsa/realtek-smi.yaml b/Documentation/devicetree/bindings/net/dsa/realtek-smi.yaml
> new file mode 100644
> index 000000000000..c4cd0038f092
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/dsa/realtek-smi.yaml
> @@ -0,0 +1,310 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/dsa/realtek-smi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek SMI-based Switches
> +
> +allOf:
> + - $ref: dsa.yaml#
> +
> +maintainers:
> + - Linus Walleij <linus.walleij@xxxxxxxxxx>
> +
> +description:
> + The SMI "Simple Management Interface" is a two-wire protocol using
> + bit-banged GPIO that while it reuses the MDIO lines MCK and MDIO does
> + not use the MDIO protocol. This binding defines how to specify the
> + SMI-based Realtek devices. The realtek-smi driver is a platform driver
> + and it must be inserted inside a platform node.
> +
> +properties:
> + compatible:
> + oneOf:
> + - enum:

Don't need oneOf when there is only 1 entry.

> + - realtek,rtl8365mb
> + - realtek,rtl8366
> + - realtek,rtl8366rb
> + - realtek,rtl8366s
> + - realtek,rtl8367
> + - realtek,rtl8367b
> + - realtek,rtl8368s
> + - realtek,rtl8369
> + - realtek,rtl8370
> + description: |
> + realtek,rtl8365mb: 4+1 ports
> + realtek,rtl8366:
> + realtek,rtl8366rb:
> + realtek,rtl8366s: 4+1 ports
> + realtek,rtl8367:
> + realtek,rtl8367b:
> + realtek,rtl8368s: 8 ports
> + realtek,rtl8369:
> + realtek,rtl8370: 8+2 ports
> + reg:
> + maxItems: 1
> +
> + mdc-gpios:
> + description: GPIO line for the MDC clock line.
> + maxItems: 1
> +
> + mdio-gpios:
> + description: GPIO line for the MDIO data line.
> + maxItems: 1
> +
> + reset-gpios:
> + description: GPIO to be used to reset the whole device
> + maxItems: 1
> +
> + realtek,disable-leds:
> + type: boolean
> + description: |
> + if the LED drivers are not used in the
> + hardware design this will disable them so they are not turned on
> + and wasting power.
> +
> + interrupt-controller:
> + type: object
> + description: |
> + This defines an interrupt controller with an IRQ line (typically
> + a GPIO) that will demultiplex and handle the interrupt from the single
> + interrupt line coming out of one of the SMI-based chips. It most
> + importantly provides link up/down interrupts to the PHY blocks inside
> + the ASIC.
> +
> + properties:
> +
> + interrupt-controller:
> + description: see interrupt-controller/interrupts.txt

Don't need generic descriptions. Just 'true' here is fine.

> +
> + interrupts:
> + description: TODO

You have to define how many interrupts and what they are.

> +
> + '#address-cells':
> + const: 0
> +
> + '#interrupt-cells':
> + const: 1
> +
> + required:
> + - interrupt-parent

'interrupt-parent' is never required. It's valid for the
'interrupt-parent' to be in any parent node.

> + - interrupt-controller
> + - '#address-cells'
> + - '#interrupt-cells'
> +
> + mdio:
> + type: object
> + description:
> + This defines the internal MDIO bus of the SMI device, mostly for the
> + purpose of being able to hook the interrupts to the right PHY and
> + the right PHY to the corresponding port.
> +
> + properties:
> + compatible:
> + const: "realtek,smi-mdio"

Don't need quotes.

blank line between properties.

> + '#address-cells':
> + const: 1

blank line.

> + '#size-cells':
> + const: 0
> +
> + patternProperties:
> + "^(ethernet-)?phy@[0-4]$":
> + type: object
> +
> + allOf:
> + - $ref: "http://devicetree.org/schemas/net/mdio.yaml#";

This is applied to the wrong level. It should be applied to 'mdio' node.
You also need to drop 'http://devicetree.org'. With that, you can drop
most of the above. IOW, just this:

mdio:
$ref: /schemas/net/mdio.yaml#
unevaluatedProperties: false

properties:
compatible:
const: realtek,smi-mdio


> +
> + properties:
> + reg:
> + maxItems: 1
> +
> + required:
> + - reg
> +
> +required:
> + - compatible
> + - mdc-gpios
> + - mdio-gpios
> + - reset-gpios
> +
> +additionalProperties: true

No. 'true' is only allowed for common, incomplete schemas referenced by
other schemas. 'unevaluatedProperties: false' is what you need here.

> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + switch {
> + compatible = "realtek,rtl8366rb";
> + /* 22 = MDIO (has input reads), 21 = MDC (clock, output only) */
> + mdc-gpios = <&gpio0 21 GPIO_ACTIVE_HIGH>;
> + mdio-gpios = <&gpio0 22 GPIO_ACTIVE_HIGH>;
> + reset-gpios = <&gpio0 14 GPIO_ACTIVE_LOW>;
> +
> + switch_intc1: interrupt-controller {
> + /* GPIO 15 provides the interrupt */
> + interrupt-parent = <&gpio0>;
> + interrupts = <15 IRQ_TYPE_LEVEL_LOW>;
> + interrupt-controller;
> + #address-cells = <0>;
> + #interrupt-cells = <1>;
> + };
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + port@0 {
> + reg = <0>;
> + label = "lan0";
> + phy-handle = <&phy0>;
> + };
> + port@1 {
> + reg = <1>;
> + label = "lan1";
> + phy-handle = <&phy1>;
> + };
> + port@2 {
> + reg = <2>;
> + label = "lan2";
> + phy-handle = <&phy2>;
> + };
> + port@3 {
> + reg = <3>;
> + label = "lan3";
> + phy-handle = <&phy3>;
> + };
> + port@4 {
> + reg = <4>;
> + label = "wan";
> + phy-handle = <&phy4>;
> + };
> + port@5 {
> + reg = <5>;
> + label = "cpu";
> + ethernet = <&gmac0>;
> + phy-mode = "rgmii";
> + fixed-link {
> + speed = <1000>;
> + full-duplex;
> + };
> + };
> + };
> +
> + mdio {
> + compatible = "realtek,smi-mdio";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + phy0: ethernet-phy@0 {
> + reg = <0>;
> + interrupt-parent = <&switch_intc1>;
> + interrupts = <0>;
> + };
> + phy1: ethernet-phy@1 {
> + reg = <1>;
> + interrupt-parent = <&switch_intc1>;
> + interrupts = <1>;
> + };
> + phy2: ethernet-phy@2 {
> + reg = <2>;
> + interrupt-parent = <&switch_intc1>;
> + interrupts = <2>;
> + };
> + phy3: ethernet-phy@3 {
> + reg = <3>;
> + interrupt-parent = <&switch_intc1>;
> + interrupts = <3>;
> + };
> + phy4: ethernet-phy@4 {
> + reg = <4>;
> + interrupt-parent = <&switch_intc1>;
> + interrupts = <12>;
> + };
> + };
> + };
> +
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + switch {
> + compatible = "realtek,rtl8365mb";
> + mdc-gpios = <&gpio1 16 GPIO_ACTIVE_HIGH>;
> + mdio-gpios = <&gpio1 17 GPIO_ACTIVE_HIGH>;
> + reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> +
> + switch_intc2: interrupt-controller {
> + interrupt-parent = <&gpio5>;
> + interrupts = <1 IRQ_TYPE_LEVEL_LOW>;
> + interrupt-controller;
> + #address-cells = <0>;
> + #interrupt-cells = <1>;
> + };
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + port@0 {
> + reg = <0>;
> + label = "swp0";
> + phy-handle = <&ethphy0>;
> + };
> + port@1 {
> + reg = <1>;
> + label = "swp1";
> + phy-handle = <&ethphy1>;
> + };
> + port@2 {
> + reg = <2>;
> + label = "swp2";
> + phy-handle = <&ethphy2>;
> + };
> + port@3 {
> + reg = <3>;
> + label = "swp3";
> + phy-handle = <&ethphy3>;
> + };
> + port@6 {
> + reg = <6>;
> + label = "cpu";
> + ethernet = <&fec1>;
> + phy-mode = "rgmii";
> + tx-internal-delay-ps = <2000>;
> + rx-internal-delay-ps = <2000>;
> +
> + fixed-link {
> + speed = <1000>;
> + full-duplex;
> + pause;
> + };
> + };
> + };
> +
> + mdio {
> + compatible = "realtek,smi-mdio";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ethphy0: ethernet-phy@0 {
> + reg = <0>;
> + interrupt-parent = <&switch_intc2>;
> + interrupts = <0>;
> + };
> + ethphy1: ethernet-phy@1 {
> + reg = <1>;
> + interrupt-parent = <&switch_intc2>;
> + interrupts = <1>;
> + };
> + ethphy2: ethernet-phy@2 {
> + reg = <2>;
> + interrupt-parent = <&switch_intc2>;
> + interrupts = <2>;
> + };
> + ethphy3: ethernet-phy@3 {
> + reg = <3>;
> + interrupt-parent = <&switch_intc2>;
> + interrupts = <3>;
> + };
> + };
> + };
> --
> 2.34.0
>
>