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

[万致远]

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.

>> +static int rtl8211f_read_status(struct phy_device *phydev)
>> +{
>> +    int ret;
>> +    struct rtl821x_priv *priv = phydev->priv;
>> +    bool changed = false;
>> +
>> +    if (rtl8211f_mode(phydev) != priv->lastmode) {
>> +            changed = true;
>> +            ret = rtl8211f_config_aneg(phydev);
>> +            if (ret < 0)
>> +                    return ret;
>> +
>> +            ret = genphy_restart_aneg(phydev);
>> +            if (ret < 0)
>> +                    return ret;
>> +    }
>> +
>> +    return genphy_c37_read_status(phydev, &changed);
>> +}
>
> So you are assuming read_status() is called once per second? But what
> about when interrupts are used?
>
> You might want to look at how the marvell driver does this. It is not
> great, but better than this.

I'm not familiar with that either, could you please tell me how to do
it properly to automatically switch between copper and fiber mode?

>
>     Andrew
>
> ---
> pw-bot: cr
Sincerely,

Zhiyuan Wan