Re: [PATCH net-next v6 2/7] dt-bindings: net: ftgmac100: Add AST2700 compatible

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] dt-bindings: net: ftgmac100: requiring phy-mode on AST2700
  breaks use-ncsi
- [Low] dt-bindings: net: ftgmac100: split schema instead of adding more
  if-thens
--

commit d4c79ebecc8b3f53e3b3064e58bdd5d5f51b3ece
Author: Jacky Chou <jacky_chou@aspeedtech.com>

dt-bindings: net: ftgmac100: Add AST2700 compatible

This commit adds the standalone aspeed,ast2700-mac compatible to the
Faraday FTGMAC100 Device Tree binding schema along with its clock, reset,
and phy-mode rules.

> diff --git a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> index 9d352e3686aaf..613d4f1caa4fa 100644
> --- a/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml
> +++ b/Documentation/devicetree/bindings/net/faraday,ftgmac100.yaml

[ ... ]

> @@ -79,12 +80,21 @@ allOf:
>            contains:
>              enum:
>                - aspeed,ast2600-mac
> +              - aspeed,ast2700-mac
>      then:
>        properties:
>          resets: true
>      else:
>        properties:
>          resets: false
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: aspeed,ast2700-mac
> +    then:
> +      required:
> +        - phy-mode

[Severity: Medium]
Does unconditionally requiring phy-mode on aspeed,ast2700-mac break
dtbs_check validation for nodes configured with use-ncsi?

The binding allows use-ncsi without an MDIO PHY or phy-mode (as shown in
the schema's first example), and ftgmac100_probe_dt() handles use-ncsi
via ftgmac100_probe_ncsi() with PHY_INTERFACE_MODE_RMII directly without
reading phy-mode.

Also, the commit message explains this rule as matching the driver's
probe-time requirement, whereas bindings should describe hardware
constraints, and the check added in commit "net: ftgmac100: Require
phy-mode for AST2700" is in ftgmac100_mii_probe(), which is only reached
for legacy MDIO bus scanning on AST2400/AST2500/Faraday.

>    - if:
>        properties:
>          compatible:
> @@ -105,6 +115,7 @@ allOf:
>              enum:
>                - aspeed,ast2500-mac
>                - aspeed,ast2600-mac
> +              - aspeed,ast2700-mac
>      then:
>        properties:
>          clocks:
> @@ -124,6 +135,7 @@ allOf:
>                  - aspeed,ast2400-mac
>                  - aspeed,ast2500-mac
>                  - aspeed,ast2600-mac
> +                - aspeed,ast2700-mac
>      then:
>        properties:
>          clocks: false

[Severity: Low]
Now that commit "dt-bindings: net: ftgmac100: Make Aspeed compatibles
standalone" decoupled the Aspeed variants from faraday,ftgmac100, allOf
uses five per-compatible if-then blocks to partition resets, phy-mode,
clocks, and clock-names across the variants.  Should the Aspeed bindings
be split into a separate schema document instead of adding more
per-compatible conditionals here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-support_ast2700-v6-0-238020bf46cf@aspeedtech.com?part=2