Re: [PATCH net-next v8 2/3] dt-bindings: net: wiznet,w5100: add link status interrupt

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] The new `allOf` `if/then/else` conditional in
  Documentation/devicetree/bindings/net/wiznet,w5100.yaml encodes a…
- [Low] Concern (assessed as NOT valid): the binding documents an
  optional second (LINKLED) interrupt for wiznet,w5500 and the semantics…

--

dt-bindings: net: wiznet,w5100: add link status interrupt

This documents an optional second interrupt on the wiznet SPI ethernet
binding, sourced from the LINKLED pin, for reporting link status changes.
The interrupts property is turned into a conditional list that allows two
entries for wiznet,w5500 and keeps a single entry for the other compatibles.

No Link: tags present in the commit header.

> diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> index 25098308eb6e5..da03f4e07f29a 100644
> --- a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> +++ b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml

[ ... ]

> @@ -24,8 +20,11 @@ properties:
>        - wiznet,w5500
>  
>    interrupts:
> -    maxItems: 1
> -    description: Main interrupt (INT pin on the device).
> +    description:
> +      The main interrupt (INT pin) is mandatory on all devices. The w5500
> +      additionally supports an optional second interrupt for link status changes
> +      (LINKLED pin). If it is not specified, link state changes will not be
> +      reported.

[Severity: Low]
At this commit, drivers/net/ethernet/wiznet/w5100.c only consumes spi->irq
and does not report carrier state, so the sentence "If it is not specified,
link state changes will not be reported" describes behaviour that has no
consumer yet.

Checking the rest of the series, the driver side does land in the following
patch, where w5100_probe() picks up the second entry:

  priv->link_irq = ops->chip_id == W5500 ?
                   fwnode_irq_get(dev_fwnode(dev), 1) : -EINVAL;

and w5500_report_carrier_state() calls netif_carrier_on()/
netif_carrier_off().  So this looks fine as a binding-before-driver
ordering, and is only noted here for completeness.

>  
>    reg:
>      maxItems: 1
> @@ -42,6 +41,30 @@ required:
>    - reg
>    - interrupts
>  
> +allOf:
> +  - $ref: /schemas/net/ethernet-controller.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - wiznet,w5500
> +    then:
> +      properties:
> +        interrupts:
> +          minItems: 1
> +          items:
> +            - description: Main interrupt (INT pin on the device).
> +            - description:
> +                Optional interrupt for link status changes (LINKLED pin on
> +                w5500).
> +    else:
> +      properties:
> +        interrupts:
> +          maxItems: 1
> +          description: Main interrupt (INT pin on the device).

[Severity: Low]
Is this else branch describing the hardware, or the current Linux driver?

The LINKLED output exists on the w5100 and w5200 packages as well, so a
board that routes w5100/w5200 LINKLED to a host GPIO cannot be described
here:

  compatible = "wiznet,w5200";
  interrupts = <25 IRQ_TYPE_LEVEL_LOW>, <24 IRQ_TYPE_EDGE_BOTH>;

The second entry is rejected by maxItems: 1 during dt_binding_check.

The only in-tree justification found for the w5500 restriction is the
driver gate added later in this series, in w5100_probe():

  priv->link_irq = ops->chip_id == W5500 ?
                   fwnode_irq_get(dev_fwnode(dev), 1) : -EINVAL;

which is a software limitation (register readback of link state via w5500
PHYCFGR) rather than absence of the pin.  Would it be preferable to allow
two interrupts for all three compatibles and note in the interrupts
description which parts can actually report link state, so this constraint
does not have to be relaxed later?

>  
>  unevaluatedProperties: false
>  
>  examples:

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915-wiznet-link-gpio-v8-0-d173622474cc%40marmottus.net