Re: [PATCH net-next v11 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 more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new allOf conditional in
  Documentation/devicetree/bindings/net/wiznet,w5100.yaml (`if: not:…

--

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

This patch adds an optional second interrupt to the wiznet,w5100 binding.
The second interrupt is meant to be wired to the LINKLED pin so link
status changes can be detected. A new allOf conditional limits every
compatible except wiznet,w5500 to a single interrupt.

> diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> index df83f90b3919..b0d1e1d699dc 100644
> --- a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> +++ b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml

[ ... ]

> @@ -24,8 +20,16 @@ properties:
>        - wiznet,w5500
>  
>    interrupts:
> -    maxItems: 1
> -    description: Main interrupt (INT pin on the device).
> +    minItems: 1
> +    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.
> +    items:
> +      - description: Main interrupt (INT pin on the device).
> +      - description:
> +          Optional interrupt for link status changes (LINKLED pin on w5500).

[ ... ]

> +allOf:
> +  - $ref: /schemas/net/ethernet-controller.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +  - if:
> +      not:
> +        properties:
> +          compatible:
> +            contains:
> +              enum:
> +                - wiznet,w5500
> +    then:
> +      properties:
> +        interrupts:
> +          maxItems: 1

[Severity: Low]
Is this restriction based on a hardware difference, or does it follow the
driver implementation?

As far as I can tell, the w5200 (and the w5100) also have a LINKLED
output. The w5200 also has a readable link bit in PHYSTATUS, so the same
"LINKLED edge, then read the link register" approach looks like it would
work there too. I haven't been able to confirm this against the
datasheets, so please correct me if that's wrong.

In the follow-up driver patch, w5100_probe() only reads the second
interrupt on the W5500:

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

The older link_gpio / w5100_detect_link() logic that commit dacf281771a9
("w5100: remove unused gpio link detection") removed was not tied to a
particular chip, so link status wiring was not limited to the w5500
before.

With this patch, a w5200 board that routes LINKLED to a SoC interrupt
line would fail dtbs_check with "too many items". The w5100 seems to
have no link-status register, so the case is weaker there, but the w5200
case still seems to apply.

The commit message only says:

  "This second interrupt is only supported on wiznet,w5500."

The item description says "LINKLED pin on w5500". Could the commit
message name the hardware reason, or could the conditional be relaxed
to cover the w5200?

A smaller point: because the conditional uses "not: contains:", any
WIZnet compatible added later will also get maxItems: 1 by default.

> +
>  unevaluatedProperties: false
>  
>  examples:

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-wiznet-link-gpio-v11-0-42a332517b30%40marmottus.net