Re: [PATCH net-next v7 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] Documentation/devicetree/bindings/net/wiznet,w5100.yaml: the
  newly added second `interrupts` entry is described as being…
- [Low] Concern (assessed as a FALSE POSITIVE; retained only because
  requires_validation=true): the claim that the newly documented second…

--

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

The wiznet,w5100 binding is extended so that the interrupts property can
carry a second entry for a link status interrupt wired to the LINKLED pin,
and the example is updated to show both 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: Low]

The description of the second entry says the pin is on the w5500, but the
constraint sits in the top-level properties block with no allOf/if-then
keyed on compatible.  The compatible enum in this same file is:

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

so a node such as:

  ethernet@0 {
      compatible = "wiznet,w5100";
      interrupts = <25 IRQ_TYPE_EDGE_FALLING>,
                   <24 IRQ_TYPE_EDGE_BOTH>;
      ...
  };

passes make dt_binding_check even though the schema text says the pin only
exists on the w5500.

Should the item count be tied to the compatible string, for example:

  allOf:
    - if:
        properties:
          compatible:
            contains:
              enum:
                - wiznet,w5100
                - wiznet,w5200
      then:
        properties:
          interrupts:
            maxItems: 1

The driver side of the series treats index 1 as w5500-only:

drivers/net/ethernet/wiznet/w5100.c:w5100_probe() {
    ...
    priv->link_irq = ops->chip_id == W5500 ?
                     fwnode_irq_get(dev_fwnode(dev), 1) : -ENODEV;
    ...
}

with w5100_get_link() returning a hardcoded 1 for the parts that have no
PHYCFGR link-status register.  So for w5100/w5200 a second interrupt in DT
would validate but then be silently ignored, since fwnode_irq_get() is
never called for index 1 on those chip ids.

Alternatively, if LINKLED wiring is equally describable on w5100 and w5200,
should the description be widened instead of naming only the w5500?

Nothing in the rest of the series changes this block, so the published
schema stays permissive for all three compatibles.  Adding the conditional
later would tighten the schema rather than break the ABI, so it seems it
could still be done, but it seems better to get it right here.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-wiznet-link-gpio-v7-0-69f01b70c96f%40marmottus.net