Re: [RFC net-next 2/2] net: stmmac: Convert to phylink

[Florian Fainelli <f.fainelli@gmail.com>]

On 6/6/2019 4:26 AM, Jose Abreu wrote:
> Convert stmmac driver to phylink.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Joao Pinto <jpinto@synopsys.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> ---
[snip]

			     interface);
> -	}
> +	if (node) {
> +		ret = phylink_of_phy_connect(priv->phylink, node, 0);
> +	} else {
> +		int addr = priv->plat->phy_addr;
> +		struct phy_device *phydev;
>  
> -	if (IS_ERR_OR_NULL(phydev)) {
> -		netdev_err(priv->dev, "Could not attach to PHY\n");
> -		if (!phydev)
> +		phydev = mdiobus_get_phy(priv->mii, addr);
> +		if (!phydev) {
> +			netdev_err(priv->dev, "no phy at addr %d\n", addr);
>  			return -ENODEV;
> +		}

I am not exactly sure removing this is strictly equivalent here, but the
diff makes it hard to review.

For the sake of reviewing the code, could you structure the patches like
you did with an intermediate step being:

- prepare for PHYLINK (patch #1)
- add support for PHYLINK (parts of this patch) but without plugging it
into the probe/connect path just yet
- get rid of all PHYLIB related code (parts of this patch), which would
be a new patch #3

AFAIR, there are several cases that stmmac supports today:

- fixed PHY (covered by PHYLINK)
- PHY designated via phy-handle (covered by PHYLINK)
- PHY address via platform data (not covered by PHYLINK unless we add
support for that), with no Device Tree node
- when a MDIO bus Device Tree node is present, register the stmmac MDIO
bus (which you don't change).

I believe I have convinced myself that the third case is covered with
the change above :)

This looks great, thanks!
-- 
Florian