Re: [net-next,v2,1/3] dt-bindings: net: cdns,macb: add a compatible for Microchip pic64hpsc

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email says "Not sure about the first one but the second one looks
legit"

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
dt-bindings: net: cdns,macb: add a compatible for Microchip pic64hpsc

This commit adds "microchip,pic64hpsc-gem" for "PIC64-HPSC" and
"microchip,pic64hx-gem" for "PIC64HX" compatible strings. It also adds a
conditional schema check to prevent adding PHYs under the GEM node for
hardware where MDIO controllers are unconnected from physical pins.

> diff --git a/Documentation/devicetree/bindings/net/cdns,macb.yaml b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> --- a/Documentation/devicetree/bindings/net/cdns,macb.yaml
> +++ b/Documentation/devicetree/bindings/net/cdns,macb.yaml
> @@ -70,6 +70,14 @@
>                - microchip,sama7d65-gem   # Microchip SAMA7D65 gigabit ethernet interface
>            - const: microchip,sama7g5-gem # Microchip SAMA7G5 gigabit ethernet interface
>
> +      - items:
> +          - const: microchip,pic64hpsc-gem # Microchip PIC64-HPSC
> +          - const: cdns,gem
> +      - items:
> +          - const: microchip,pic64hx-gem   # Microchip PIC64HX
> +          - const: microchip,pic64hpsc-gem # Microchip PIC64-HPSC
> +          - const: cdns,gem

The fallback structure appears reversed from what was recommended in v1
review. Conor Dooley suggested that pic64hpsc should fallback to pic64hx:

  - items:
      - const: microchip,pic64hx-gem
      - const: cdns,gem
  - items:
      - const: microchip,pic64hpsc-gem
      - const: microchip,pic64hx-gem
      - const: cdns,gem

However, the current implementation has pic64hx falling back to pic64hpsc
instead. Was this reversal intentional, or should the order match the
reviewer's recommendation?

https://lore.kernel.org/netdev/20260303-emperor-childlike-6c43d8a91753@spud/

>
>    reg:
>      minItems: 1

[ ... ]

> @@ -196,6 +204,17 @@
>        required:
>          - phys
>
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: microchip,p64h-gem
> +    then:
> +      patternProperties:
> +        "^ethernet-phy@[0-9a-f]$": false
> +      properties:
> +        mdio: false

The conditional check references 'microchip,p64h-gem', but the compatible
strings actually added above are 'microchip,pic64hpsc-gem' and
'microchip,pic64hx-gem'. This causes the validation rule to never trigger
for the new hardware variants.

According to the commit message, the GEM IPs within pic64hpsc have their
MDIO controllers unconnected from physical pins, and this check should
prevent adding PHYs under the GEM node. However, device trees using
'microchip,pic64hpsc-gem' or 'microchip,pic64hx-gem' with invalid
MDIO/ethernet-phy configurations will silently pass schema validation.

Should this check use 'microchip,pic64hpsc-gem' instead?

This appears to be a remnant from v1 of the patch, where 'p64h' was used
as an abbreviation. Conor Dooley requested in v1 that the full 'pic'
spelling be used instead of the shortened form, and the compatible names
were updated accordingly, but this schema check was missed.

https://lore.kernel.org/netdev/20260303-emperor-childlike-6c43d8a91753@spud/

>
>  unevaluatedProperties: false
>
>  examples:
-- 
pw-bot: cr