Re: [PATCH net-next v8 6/6] net: tn40xx: add phylink support

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

On Mon, Jun 03, 2024 at 03:49:55PM +0900, FUJITA Tomonori wrote:
> @@ -1374,6 +1375,10 @@ static void tn40_stop(struct tn40_priv *priv)
>  static int tn40_close(struct net_device *ndev)
>  {
>  	struct tn40_priv *priv = netdev_priv(ndev);
> +
> +	phylink_stop(priv->phylink);
> +	phylink_disconnect_phy(priv->phylink);

There is no need to pair both of these together - you can disconnect
from the PHY later if it's more convenient.

> +
>  	napi_disable(&priv->napi);
>  	netif_napi_del(&priv->napi);
>  	tn40_stop(priv);
> @@ -1392,6 +1397,14 @@ static int tn40_open(struct net_device *dev)
>  		return ret;
>  	}
>  	napi_enable(&priv->napi);
> +	ret = phylink_connect_phy(priv->phylink, priv->phydev);
> +	if (ret) {
> +		napi_disable(&priv->napi);
> +		tn40_stop(priv);
> +		netdev_err(dev, "failed to connect to phy %d\n", ret);
> +		return ret;
> +	}

Again, no need to pair phylink_connect_phy() close to phylink_start()
if there's somewhere more convenient to place it. Operation with the
PHY doesn't begin until phylink_start() is called.

My review comment last time was purely about where phylink_start()
and phylink_stop() were being called. It's the placement of these
two functions that are key.

Thanks.

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