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

Jie Luo
2021-10-19T11:47:00Z

On 10/19/2021 2:41 AM, Andrew Lunn wrote:
>> @@ -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?

Hi Andrew,

when this register AT803X_INTR_STATUS bits are cleared after read, we
can't clear only WOL interrupt here.

>
> Also, you are returning the contents of the interrupt status register?
> You should probably be returning 0 if the read was successful.
thanks for this comments, will correct it in the next patch set.
>
> Andrew