Re: [PATCH net-next v10 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] Concern (INVALIDATED by later patch in this series): the binding
  documents an optional second, LINKLED interrupt for wiznet,w5500 and…

--

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

This adds an optional second interrupt to the wiznet,w5100 binding for the
LINKLED pin, described as being supported only on wiznet,w5500.  The
interrupts property is moved to a two-item list with an allOf conditional
restricting non-w5500 compatibles to a single interrupt, and the example is
updated to list both interrupts.

> diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> index 25098308eb6e5..ac2ee725af5d5 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).

[Severity: Low]
At this point in the series, is there anything that consumes the second
interrupt?  The description promises that link state changes are reported
when the LINKLED interrupt is present, but w5100_probe() in
drivers/net/ethernet/wiznet/w5100.c only uses spi->irq (interrupt index 0)
and there is no netif_carrier_on() call in the driver yet.

For the record, this resolves later in the same series: the final patch
("w5100: detect carrier state using link status bit and optional
interrupt") adds

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

and w5500_report_carrier_state() which calls
netif_carrier_on()/netif_carrier_off().  So the documentation and the
implementation only disagree between this patch and that one, which is
nothing more than a transient bisect-window mismatch.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-wiznet-link-gpio-v10-0-5874a7a147a2%40marmottus.net