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

[Charles Perry]

On Thu, Mar 12, 2026 at 06:49:43PM -0700, Jakub Kicinski wrote:
> 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?
> 

Yes, this is intentional.

> 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?
> 

Oops! Good catch! I'll send a v3.

Thanks,
Charles