Re: [PATCH 1/2] net: phy: realtek: add combo mode support for RTL8211FS

[Andrew Lunn <andrew@lunn.ch>]

On Tue, Dec 03, 2024 at 11:08:22AM +0800, 万致远 wrote:
> On 2024/12/3 7:52, Andrew Lunn wrote:
> >> +static int rtl8211f_config_aneg(struct phy_device *phydev)
> >> +{
> >> +    int ret;
> >> +
> >> +    struct rtl821x_priv *priv = phydev->priv;
> >> +
> >> +    ret = genphy_read_abilities(phydev);
> >> +    if (ret < 0)
> >> +            return ret;
> >> +
> >> +    linkmode_copy(phydev->advertising, phydev->supported);
> >
> > This is all very unusual for config_aneg(). genphy_read_abilities()
> > will have been done very early on during phy_probe(). So why do it
> > now? And why overwrite how the user might of configured what is to be
> > advertised?
> >
> 
> These codes are migrated from Rockchip SDK and I'm not familiar with this part.
> 
> I will use `linkmode_and` instead of `linkmode_copy` in my next
> version of patch like Marvell does.

No, it needs a lot more work than just that. Spend some time to really
understand how the marvell driver handles either copper or fibre, and
assume the Rockchip SDK is poor quality code.

It might also be that the marvell scheme does not work. It will depend
on how the PHY actually works.

Andrew