Re: [PATCH net-next v3 1/7] net: ngbe: implement phylink to handle PHY device

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Wed, Dec 06, 2023 at 05:53:49PM +0800, Jiawen Wu wrote:
> Add phylink support for Wangxun 1Gb Ethernet controller.
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
>  drivers/net/ethernet/wangxun/libwx/wx_type.h  |   8 ++
>  drivers/net/ethernet/wangxun/ngbe/ngbe_main.c |  20 ++-
>  drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c | 126 +++++++++++-------
>  drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.h |   2 +-
>  4 files changed, 93 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> index 165e82de772e..9225aaf029f8 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> @@ -8,6 +8,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/if_vlan.h>
>  #include <net/ip.h>
> +#include <linux/phylink.h>

Nit: would be better to keep linux/ includes together (and in
alphabetical order to prevent conflicts.)

> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index 8db804543e66..c61f4b9d79fa 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> @@ -9,6 +9,7 @@
>  #include <linux/etherdevice.h>
>  #include <net/ip.h>
>  #include <linux/phy.h>
> +#include <linux/phylink.h>
>  #include <linux/if_vlan.h>
>  
>  #include "../libwx/wx_type.h"

As wx_type.h includes linux/phylink.h, which is now fundamental for the
definition of one of the structures in wx_type.h, the include of
linux/phylink.h seems unnecessary here.

> @@ -336,7 +337,8 @@ static void ngbe_disable_device(struct wx *wx)
>  
>  static void ngbe_down(struct wx *wx)
>  {
> -	phy_stop(wx->phydev);
> +	phylink_stop(wx->phylink);
> +	phylink_disconnect_phy(wx->phylink);

I'm not sure why you're moving the PHY disconnection in this patch -
that seems like a separate change to the actual conversion to phylink.
For a pure conversion, you should be able to just replace the phylib
calls with their phylink equivalents.

It seems to me that there's two changes happening here: a conversion to
phylink and a re-ordering of the open/close methods particularly to do
with connecting and disconnecting the PHY. Either this needs to be
described in the commit message (the fact that it's happening and why)
or it should be two patches.

> -static void ngbe_phy_fixup(struct wx *wx)
> +void ngbe_phylink_start(struct wx *wx)
>  {
> -	struct phy_device *phydev = wx->phydev;
> -	struct ethtool_eee eee;
> -
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
> -	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
> -
> -	phydev->mac_managed_pm = true;
> -	if (wx->mac_type != em_mac_type_mdi)
> -		return;
> -	/* disable EEE, internal phy does not support eee */
> -	memset(&eee, 0, sizeof(eee));
> -	phy_ethtool_set_eee(phydev, &eee);
> +	struct phylink *phylink = wx->phylink;
> +
> +	phylink_connect_phy(phylink, wx->phydev);

Note that phylink_connect_phy() can fail, so it's return value should
be checked.

Apart from the comments above, I think I'm fine with this.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!