Re: [PATCH net-next v3 4/5] net: phy: mediatek: Extend 1G TX/RX link pulse time

[Andrew Lunn]

> +static void extend_an_new_lp_cnt_limit(struct phy_device *phydev)
> +{
> +	int mmd_read_ret;
> +	int ret;
> +	u32 reg_val;
> +
> +	ret = read_poll_timeout(mmd_read_ret = phy_read_mmd, reg_val,
> +				(mmd_read_ret < 0) || reg_val & MTK_PHY_FINAL_SPEED_1000,
> +				10000, 1000000, false, phydev,
> +				MDIO_MMD_VEND1, MTK_PHY_LINK_STATUS_MISC);
> +	if (mmd_read_ret < 0)
> +		ret = mmd_read_ret;
> +	/* If final_speed_1000 is raised, try to extend timeout period
> +	 * of auto downshift.
> +	 */
> + if (!ret) {

If you look at other Linux code, the general pattern is to look if a
function returned an error. If it does, either return immediately, or
jump to the end of the function where the cleanup is.

Since this is a void function:

> +	if (mmd_read_ret < 0)
> +		return;

And then you don't need the

> + if (!ret) {


> +		tr_modify(phydev, 0x0, 0xf, 0x3c, AN_NEW_LP_CNT_LIMIT_MASK,
> +			  FIELD_PREP(AN_NEW_LP_CNT_LIMIT_MASK, 0xf));
> +		mdelay(1500);
> +
> +		ret = read_poll_timeout(mmd_read_ret = tr_read, reg_val,
> +					(mmd_read_ret < 0) ||
> +					(reg_val & AN_STATE_MASK) !=
> +					(AN_STATE_TX_DISABLE << AN_STATE_SHIFT),
> +					10000, 1000000, false, phydev,
> +					0x0, 0xf, 0x2);
> +
> +		if (mmd_read_ret < 0)
> +			ret = mmd_read_ret;
> +
> +		if (!ret) {

This if can also be removed.

> +			mdelay(625);
> +			tr_modify(phydev, 0x0, 0xf, 0x3c, AN_NEW_LP_CNT_LIMIT_MASK,
> +				  FIELD_PREP(AN_NEW_LP_CNT_LIMIT_MASK, 0x8));
> +			mdelay(500);
> +			tr_modify(phydev, 0x0, 0xf, 0x3c, AN_NEW_LP_CNT_LIMIT_MASK,
> +				  FIELD_PREP(AN_NEW_LP_CNT_LIMIT_MASK, 0xf));
> +		}
> +	}

One question i have is, should this really be a void function? What
does it mean if read_poll_timeout() returns an error? Why is it safe
to ignore it? Why not return the error?

> +}
> +
> +int mtk_gphy_cl22_read_status(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = genphy_read_status(phydev);
> +	if (ret)
> +		return ret;
> +
> +	if (phydev->autoneg == AUTONEG_ENABLE && !phydev->autoneg_complete) {
> +		ret = phy_read(phydev, MII_CTRL1000);
> +		if ((ret & ADVERTISE_1000FULL) || (ret & ADVERTISE_1000HALF))
> +			extend_an_new_lp_cnt_limit(phydev);
> +	}
> +
> +	return 0;

If extend_an_new_lp_cnt_limit() fails, what does it mean? Do we
actually want mtk_gphy_cl22_read_status() to indicate something has
gone wrong? Or does extend_an_new_lp_cnt_limit() failing not matter?

	Andrew