Re: [PATCH v3 03/13] net: phy: at803x: improve the WOL feature

[Andrew Lunn]

> @@ -348,18 +349,29 @@ static int at803x_set_wol(struct phy_device *phydev,
>  			phy_write_mmd(phydev, MDIO_MMD_PCS, offsets[i],
>  				      mac[(i * 2) + 1] | (mac[(i * 2)] << 8));
>  
> +		/* Enable WOL function */
> +		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL,
> +				0, AT803X_WOL_EN);
> +		if (ret)
> +			return ret;
> +		/* Enable WOL interrupt */
>  		ret = phy_modify(phydev, AT803X_INTR_ENABLE, 0, AT803X_INTR_ENABLE_WOL);
>  		if (ret)
>  			return ret;
> -		value = phy_read(phydev, AT803X_INTR_STATUS);
>  	} else {
> +		/* Disable WoL function */
> +		ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL,
> +				AT803X_WOL_EN, 0);
> +		if (ret)
> +			return ret;
> +		/* Disable WOL interrupt */
>  		ret = phy_modify(phydev, AT803X_INTR_ENABLE, AT803X_INTR_ENABLE_WOL, 0);
>  		if (ret)
>  			return ret;
> -		value = phy_read(phydev, AT803X_INTR_STATUS);
>  	}
>  
> -	return ret;
> +	/* Clear WOL status */
> +	return phy_read(phydev, AT803X_INTR_STATUS);

It looks like you could be clearing other interrupt bits which have
not been serviced yet. Is it possible to clear just WoL?

Also, you are returning the contents of the interrupt status register?
You should probably be returning 0 if the read was successful.

    Andrew