Re: [PATCH net-next v12 4/6] dt-bindings: net: rockchip-dwmac: document pcs-handle

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The rule that makes pcs-handle required is true even when phy-
  mode is absent, and the patch adds a redundant rule that the commit…

--

dt-bindings: net: rockchip-dwmac: document pcs-handle

This adds a pcs-handle property to the Rockchip GMAC binding for the SGMII
support added later in the series. It also adds two allOf rules for
rockchip,rk3568-gmac. The first requires phy-mode. The second requires
pcs-handle when phy-mode is "sgmii".

> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> index 80c252845349c..c434450fd07ff 100644
> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.yaml

[ ... ]

> @@ -159,6 +165,27 @@ allOf:
>          clocks:
>            minItems: 5
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: rockchip,rk3568-gmac
> +    then:
> +      required:
> +        - phy-mode
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: rockchip,rk3568-gmac
> +        phy-mode:
> +          contains:
> +            const: sgmii
> +    then:
> +      required:
> +        - pcs-handle

[Severity: Low]
Is the sgmii if above true when phy-mode is missing?

In JSON Schema, a properties subschema passes when the property is not
there. That means a rockchip,rk3568-gmac node without phy-mode matches
this if. It then gets a "pcs-handle is a required property" error, even
though nothing says the link is SGMII.

The standalone phy-mode block before it does not make this if false.
Each allOf entry is checked on its own, so that block only adds its own
error.

Is the standalone phy-mode block needed at all? rockchip-dwmac.yaml
already pulls in snps,dwmac.yaml with "- $ref: snps,dwmac.yaml#" under
allOf. That schema already requires phy-mode at the top level:

Documentation/devicetree/bindings/net/snps,dwmac.yaml:
required:
  - compatible
  - reg
  - interrupts
  - interrupt-names
  - phy-mode

So an rk3568-gmac node with no phy-mode would get two "phy-mode is a
required property" errors and a misleading pcs-handle error.

The commit message also does not say that phy-mode becomes mandatory for
rk3568. It only says pcs-handle is required when phy-mode is "sgmii".

Could required: [phy-mode] go inside the sgmii if instead, which is the
usual idiom? The standalone phy-mode block could then be dropped.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017054406.1828061-1-coiaprant%40gmail.com