Re: [PATCH net-next v3 2/2] dsa: lan9303: Move to PHYLINK

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: Jerry Ray <jerry.ray@microchip.com>, Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v3 2/2] dsa: lan9303: Move to PHYLINK
Date: Tue, 6 Dec 2022 21:07:54 +0000	[thread overview]
Message-ID: <Y4+vKh8EfA9vtC2B@shell.armlinux.org.uk> (raw)
In-Reply-To: <20221206193224.f3obnsjtphbxole4@skbuf>

On Tue, Dec 06, 2022 at 09:32:24PM +0200, Vladimir Oltean wrote:
> On Tue, Dec 06, 2022 at 12:35:00PM -0600, Jerry Ray wrote:
> > This patch replaces the .adjust_link api with the .phylink_get_caps api.
> 
> Am I supposed to read this commit description and understand what the
> change does?
> 
> You can't "replace" adjust_link with phylink_get_caps, since they don't
> do the same thing. The equivalent set of operations are roughly
> phylink_mac_config and phylink_mac_link_up, probably just the latter in
> your case.
> 
> By deleting adjust_link and not replacing with any of the above, the
> change is telling me that nothing from adjust_link was needed?

...

> > -static void lan9303_adjust_link(struct dsa_switch *ds, int port,
> > -				struct phy_device *phydev)
> > -{
> > -	struct lan9303 *chip = ds->priv;
> > -	int ctl;
> > -
> > -	if (!phy_is_pseudo_fixed_link(phydev))
> > -		return;

If this is a not a fixed link, adjust_link does nothing.

> > -
> > -	ctl = lan9303_phy_read(ds, port, MII_BMCR);
> > -
> > -	ctl &= ~BMCR_ANENABLE;
> > -
> > -	if (phydev->speed == SPEED_100)
> > -		ctl |= BMCR_SPEED100;
> > -	else if (phydev->speed == SPEED_10)
> > -		ctl &= ~BMCR_SPEED100;
> > -	else
> > -		dev_err(ds->dev, "unsupported speed: %d\n", phydev->speed);
> > -
> > -	if (phydev->duplex == DUPLEX_FULL)
> > -		ctl |= BMCR_FULLDPLX;
> > -	else
> > -		ctl &= ~BMCR_FULLDPLX;
> > -
> > -	lan9303_phy_write(ds, port, MII_BMCR, ctl);
> 
> Are you going to explain why modifying this register is no longer needed?

... otherwise it is a fixed link, so the PHY is configured for the fixed
link setting - which I think would end up writing to the an emulation of
the PHY, and would end up writing the same settings back to the PHY as
the PHY was already configured.

So, I don't think adjust_link does anything useful, and I think this is
an entirely appropriate change.

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