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

[Arthur Crepin Leblond <arthur@marmottus.net>]

On Thu, Sep 24, 2026 at 01:55:54AM +0000, netdev-bot+sashiko@kernel.org wrote:
>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.
>
>[ ... ]
>

https://elixir.bootlin.com/linux/v7.2/source/Documentation/devicetree/bindings/submitting-patches.rst#L54

   The Documentation/ portion of the patch should come in the series before
   the code implementing the binding.

Arthur