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 08:54:55PM +0900, FUJITA Tomonori wrote:
> On Mon, 3 Jun 2024 10:22:17 +0100
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> > 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.
> 
> I see. Seems that there is no reason to call phylink_disconnect_phy()
> later so I leave this alone.
> 
> >> +
> >>  	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.
> 
> Understood. Calling phylink_connect_phy() first in this function looks
> simpler. I modified the code in the following way:
> 
> @@ -1385,13 +1390,20 @@ static int tn40_open(struct net_device *dev)
>  	struct tn40_priv *priv = netdev_priv(dev);
>  	int ret;
>  
> +	ret = phylink_connect_phy(priv->phylink, priv->phydev);
> +	if (ret) {
> +		netdev_err(dev, "failed to connect to phy %d\n", ret);
> +		return ret;
> +	}
>  	tn40_sw_reset(priv);
>  	ret = tn40_start(priv);
>  	if (ret) {
> +		phylink_disconnect_phy(priv->phylink);
>  		netdev_err(dev, "failed to start %d\n", ret);
>  		return ret;
>  	}
>  	napi_enable(&priv->napi);
> +	phylink_start(priv->phylink);
>  	netif_start_queue(priv->ndev);
>  	return 0;
>  }

LGTM, thanks!

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