Re: [PATCH] w5100: restore GPIO-based link detection

On Tue, Aug 04, 2026 at 05:02:48PM +0200, Arnd Bergmann wrote:
>On Tue, Aug 4, 2026, at 16:38, Arthur Crépin Leblond wrote:
>> Commit dacf281771a9 ("w5100: remove unused gpio link detection")
>> dropped the link_gpio/link_irq handling on the grounds that no
>> devicetree user passed a "link" GPIO at the time.
>>
>> Signed-off-by: Arthur Crépin Leblond <[email protected]>
>
>Hi Arthur,

Hi Arnd,

Thanks for the reply and the review!

>
>The patch description could use some more explanation here, and
>a clarification that you don't just bring back the original
>broken code but add devicetree support for it.
>
>>  .../devicetree/bindings/net/wiznet,w5x00.txt       |  8 ++-
>>  drivers/net/ethernet/wiznet/w5100.c                | 84 ++++++++++++++++++++++
>>  2 files changed, 89 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/wiznet,w5x00.txt
>> b/Documentation/devicetree/bindings/net/wiznet,w5x00.txt
>> index e9665798c4be..e97ce3cb9183 100644
>> --- a/Documentation/devicetree/bindings/net/wiznet,w5x00.txt
>> +++ b/Documentation/devicetree/bindings/net/wiznet,w5x00.txt
>> @@ -25,6 +25,7 @@ Optional properties:
>>    According to the w5500 datasheet, the chip allows a maximum of 80
>> MHz, however,
>>    board designs may need to limit this value.
>>  - local-mac-address: See ethernet.txt in the same directory.
>> +- link-gpios: a GPIO line used for the link detection interrupt
>>
>>
>>  Example (for Raspberry Pi with pin control stuff for GPIO irq):
>> @@ -38,13 +39,14 @@ Example (for Raspberry Pi with pin control stuff
>> for GPIO irq):
>>  		interrupt-parent = <&gpio>;
>>  		interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
>>  		spi-max-frequency = <30000000>;
>> +		link-gpios = <&gpio 4 GPIO_ACTIVE_HIGH>;
>>  	};
>>  };
>
>Ok, so you are using the binding I suggested originally,
>which I think is fine here, but note that Rob asked for
>the binding to be converted to yaml format in
>https://lore.kernel.org/all/20260427145010.GA2502144-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org/
>
>I avoiding touching it by just removing the broken implementation,
>but it would be good if you could do this now.

Yes, I started to reintroduce the driver link GPIO code from the 6.18 tree
and converted to gpiod_ and noticed that you already had a patch
(20230127095839.3266452-1-arnd-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org) so I reused most of your code.

>
>> +
>> +		priv->link_irq = gpiod_to_irq(priv->link_gpio);
>> +		if (priv->link_irq < 0) {
>> +			err = priv->link_irq;
>> +			goto err_gpio;
>> +		}
>> +
>> +		err = request_any_context_irq(priv->link_irq, w5100_detect_link,
>> +					      IRQF_TRIGGER_RISING |
>> +						      IRQF_TRIGGER_FALLING,
>> +					      link_name, priv->ndev);
>
>I think you can just use a hardcoded link name here, and
>an open-coded gpiod_to_irq(priv->link_gpio) for simplicity. My
>previous version kept this from the original code, but if you
>reintroduce it, you can improve it further (as you did elsewhere
>already)
>
>I would probably also use devm_request_threaded_irq()
>
>> @@ -840,6 +918,7 @@ static int w5100_suspend(struct device *dev)
>>
>>  	if (netif_running(ndev)) {
>>  		netif_carrier_off(ndev);
>> +
>>  		netif_device_detach(ndev);
>...
>>  		w5100_hw_close(priv);
>>  	}
>> +
>>  	return 0;
>
>The whitespace changes should not be part of the patch.
>
>     Arnd

I took your changes into account and made a v2.

Thanks!

-- 
Arthur Crépin Leblond