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

[Arthur Crépin Leblond]

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 <arthur@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxx/
>
> 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@xxxxxxxxxx) 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