Re: [PATCH v5 2/7] dt-bindings: net: amlogic,meson-dwmac: add amlogic,t7-dwmac

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dt-bindings: net: amlogic,meson-dwmac: unwieldy per-compatible conditionals
--

commit bbd012595ac6470b2960164c914c68e801ea5738
Author: Lucas Tanure <tanure@linux.com>

dt-bindings: net: amlogic,meson-dwmac: add amlogic,t7-dwmac

This commit adds the amlogic,t7-dwmac compatible string to the Amlogic Meson
DWMAC device tree binding schema, specifying its five-clock list and
restricting its delay properties to rx-internal-delay-ps.

> diff --git a/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.yaml b/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.yaml
> index e769c9bb70d84..6de013e3e8f63 100644
> --- a/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.yaml

[ ... ]

> @@ -57,6 +58,59 @@ allOf:
>              - const: clkin1
>              - const: timing-adjustment
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: amlogic,t7-dwmac
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: GMAC main clock
> +            - description: First parent clock of the internal mux
> +            - description: Second parent clock of the internal mux
> +            - description:
> +                clock of the pipeline stage inserted in the bus path between
> +                the controller and the DRAM. Without it, the controller cannot
> +                complete DMA transfers.
> +            - description:
> +                clock of the register block holding PRG_ETH0 and PRG_ETH1.
> +                Without it, the block reads as zero and ignores writes.
> +
> +        clock-names:
> +          items:
> +            - const: stmmaceth
> +            - const: clkin0
> +            - const: clkin1
> +            - const: pipeline
> +            - const: pclk
> +
> +        # The PHY adds the 2ns RGMII delays, see ethernet-controller.yaml.
> +        # The RX delay line of this glue is fine tuning in 200ps steps,
> +        # added on top of what phy-mode describes. The TX side has no fine
> +        # tuning: its only step is a quarter of the clock period, the full
> +        # 2ns at 1Gbit/s, so the TX property is not accepted.
> +        tx-internal-delay-ps: false
> +
> +        # Deprecated: new SoCs use the standard *-internal-delay-ps
> +        # properties instead.
> +        amlogic,tx-delay-ns: false
> +        amlogic,rx-delay-ns: false
> +
> +  # Delay definitions for Meson8b and newer
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - amlogic,meson8b-dwmac
> +              - amlogic,meson8m2-dwmac
> +              - amlogic,meson-gxbb-dwmac
> +              - amlogic,meson-axg-dwmac
> +              - amlogic,meson-g12a-dwmac
> +    then:
> +      properties:
>          amlogic,tx-delay-ns:
>            enum: [0, 2, 4, 6]
>            default: 2

[Severity: Low]
This isn't a runtime bug, but the per-compatible if-then conditionals under
allOf in Documentation/devicetree/bindings/net/amlogic,meson-dwmac.yaml are
getting quite unwieldy with the addition of amlogic,t7-dwmac.

The T7 variant differs substantially from the older variants in this binding:
it uses snps,dwmac-5.10a instead of snps,dwmac-3.70a, has a distinct five-clock
list (stmmaceth, clkin0, clkin1, pipeline, pclk without timing-adjustment), and
disallows tx-internal-delay-ps as well as amlogic,tx-delay-ns and
amlogic,rx-delay-ns.

Inserting the amlogic,t7-dwmac block into the middle of allOf also splits the
existing conditional for the five Meson8b-through-G12A compatibles into two
separate if-then blocks with identical enum lists:

  - if:
      properties:
        compatible:
          contains:
            enum:
              - amlogic,meson8b-dwmac
              - amlogic,meson8m2-dwmac
              - amlogic,meson-gxbb-dwmac
              - amlogic,meson-axg-dwmac
              - amlogic,meson-g12a-dwmac

Furthermore, because amlogic,tx-delay-ns and amlogic,rx-delay-ns are only
defined inside that second Meson8b-through-G12A conditional and not in the
top-level properties block (with unevaluatedProperties: false at the bottom of
the schema), setting them to false in the amlogic,t7-dwmac block is redundant,
while leaving rx-internal-delay-ps: default: 0 inside the Meson8b-through-G12A
block omits that default for amlogic,t7-dwmac.

Would it be cleaner to split amlogic,t7-dwmac into a separate binding schema
document rather than adding more per-compatible conditionals to
amlogic,meson-dwmac.yaml?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103163715.385394-1-tanure@linux.com?part=2