Re: [net-next,v6,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: 1 · Low: 1

- [Medium] Incomplete per-variant schema constraint in
  Documentation/devicetree/bindings/net/wiznet,w5100.yaml: the new…
- [Low] Binding advertises a second (link-status/LINKLED) interrupt that
  no driver consumes.

--

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

This changes the interrupts property in the wiznet,w5100 binding from a
single entry to a two entry tuple with minItems: 1, so an optional second
interrupt tied to the LINKLED pin can be described.  The example is updated
to list two interrupts.

> diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> index 601d828bae927..d0717ab45de6c 100644
> --- a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> +++ b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> @@ -24,7 +24,11 @@ properties:
>        - wiznet,w5500
>  
>    interrupts:
> -    maxItems: 1
> +    minItems: 1
> +    items:
> +      - description: Main interrupt (INT pin on the device).
> +      - description:
> +          Optional interrupt for link status changes (LINKLED pin on w5500).

[Severity: Medium]
The second item description says the LINKLED pin exists on the w5500, but
the interrupts definition here is unconditional, so should w5100 and w5200
nodes also be allowed to carry a second interrupt?

The compatible enum in this file covers all three parts:

  compatible:
    enum:
      - wiznet,w5100
      - wiznet,w5200
      - wiznet,w5500

and the only allOf entries are the two $ref lines:

  allOf:
    - $ref: /schemas/net/ethernet-controller.yaml#
    - $ref: /schemas/spi/spi-peripheral-props.yaml#

With no if/then block restricting interrupts by compatible string, a node
with compatible = "wiznet,w5100" and two interrupt entries passes
dt_binding_check even though the schema's own prose says the pin is w5500
only.

On the driver side, w5100_probe() only asks for the second interrupt on the
w5500:

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

so a w5100 or w5200 device tree written with two interrupts validates
cleanly and the second entry is silently ignored with no diagnostic.

Would it make sense to add an if/then block constraining interrupts to
maxItems: 1 for wiznet,w5100 and wiznet,w5200, or alternatively to reword
the description so it no longer claims the capability is w5500 specific?
The v6 changelog mentions the conditional properties were removed, so was
dropping the per-compatible conditional here intentional?

[ ... ]

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