Messages in this thread | | | Date | Tue, 26 Jul 2022 11:50:33 +0100 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH net-next v1 1/2] net: asix: ax88772: migrate to phylink |
| |
On Sat, Jul 23, 2022 at 07:47:10PM +0200, Oleksij Rempel wrote: > There are some exotic ax88772 based devices which may require > functionality provide by the phylink framework. For example: > - US100A20SFP, USB 2.0 auf LWL Converter with SFP Cage > - AX88772B USB to 100Base-TX Ethernet (with RMII) demo board, where it > is possible to switch between internal PHY and external RMII based > connection. > > So, convert this driver to phylink as soon as possible. > > Tested with: > - AX88772A + internal PHY > - AX88772B + external DP83TD510E T1L PHY > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Sorry, I missed this.
> diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c > index 5b5eb630c4b7..3f93bc46a7eb 100644 > --- a/drivers/net/usb/asix_devices.c > +++ b/drivers/net/usb/asix_devices.c > @@ -327,6 +327,12 @@ static int ax88772_reset(struct usbnet *dev) > struct asix_common_private *priv = dev->driver_priv; > int ret; > > + ret = phylink_connect_phy(priv->phylink, priv->phydev); > + if (ret) { > + netdev_err(dev->net, "Could not connect PHY\n"); > + return ret; > + } > +
As Paolo points out, you need to clean this up with a call to phylink_disconnect_phy() in the error paths in this function. Sorting that out doesn't look like it would be sufficient, however. Looking at usbnet_open(), there are a bunch of paths after ->reset() has been called, where failures can occur and the chip specific driver doesn't get notified to clean up, so in that case, the PHY would remain attached.
So, I would say that the usbnet code is not structured to allow a call to bind the PHY at this point.
However, this is also a functional change to the code - moving the call to connect the PHY to (presumably) the .ndo_open() stage of the network device lifetime, rather than the probe stage. In other words, this now happens when the device is actually opened rather than when the device is being bound to the driver.
That should have at least been described in the commit description, but much better would be a separate patch to make that change. However, as I say above, I think the usbnet code would need additional work to permit this.
> +static void ax88772_mac_link_up(struct phylink_config *config, > + struct phy_device *phy, > + unsigned int mode, phy_interface_t interface, > + int speed, int duplex, > + bool tx_pause, bool rx_pause) > +{ > + struct usbnet *dev = netdev_priv(to_net_dev(config->dev)); > + u16 m = AX_MEDIUM_AC | AX_MEDIUM_RE; > + > + m |= duplex ? AX_MEDIUM_FD : 0;
I'd much rather this was:
m |= duplex == DUPLEX_FULL ? AX_MEDIUM_FD : 0;
which is much clearer, and doesn't result in other values of duplex resulting in full duplex. If you do want that behaviour (because the original code used to), then please use:
m |= duplex != DUPLEX_HALF ? AX_MEDIUM_FD : 0;
Either of these make the intention here explicit.
Thanks.
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
| |