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

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,

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.

> +
> +		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