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

[Andrew Lunn]

> +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?

> +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.

    Andrew

---
pw-bot: cr