Messages in this thread | | | Subject | Re: [PATCH net 1/2] net: phy: DP83822: Fix WoL in config init to be disabled | From | Dan Murphy <> | Date | Thu, 23 Apr 2020 12:49:11 -0500 |
| |
Heiner
On 4/23/20 12:02 PM, Heiner Kallweit wrote: > On 23.04.2020 18:39, Dan Murphy wrote: >> The WoL feature should be disabled when config_init is called and the >> feature should turned on or off when set_wol is called. >> >> In addition updated the calls to modify the registers to use the set_bit >> and clear_bit function calls. >> >> Fixes: 3b427751a9d0 ("net: phy: DP83822 initial driver submission") >> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> --- >> drivers/net/phy/dp83822.c | 30 ++++++++++++++++-------------- >> 1 file changed, 16 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c >> index fe9aa3ad52a7..40fdfd043947 100644 >> --- a/drivers/net/phy/dp83822.c >> +++ b/drivers/net/phy/dp83822.c >> @@ -137,16 +137,19 @@ static int dp83822_set_wol(struct phy_device *phydev, >> value &= ~DP83822_WOL_SECURE_ON; >> } >> >> - value |= (DP83822_WOL_EN | DP83822_WOL_INDICATION_SEL | >> - DP83822_WOL_CLR_INDICATION); >> - phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, >> - value); >> + /* Clear any pending WoL interrupt */ >> + phy_read(phydev, MII_DP83822_MISR2); >> + >> + value |= DP83822_WOL_EN | DP83822_WOL_INDICATION_SEL | >> + DP83822_WOL_CLR_INDICATION; >> + >> + return phy_set_bits_mmd(phydev, DP83822_DEVADDR, >> + MII_DP83822_WOL_CFG, value); > The switch to phy_set_bits_mmd() doesn't seem to be correct here. > So far bit DP83822_WOL_MAGIC_EN is cleared if WAKE_MAGIC isn't set. > Similar for bit DP83822_WOL_SECURE_ON. With your change they don't > get cleared any longer.
Thanks for the review. I was not watching those bits during my testing I will revert back to the original code.
>> } else { >> - value = phy_read_mmd(phydev, DP83822_DEVADDR, >> - MII_DP83822_WOL_CFG); >> - value &= ~DP83822_WOL_EN; >> - phy_write_mmd(phydev, DP83822_DEVADDR, MII_DP83822_WOL_CFG, >> - value); >> + value = DP83822_WOL_EN | DP83822_WOL_CLR_INDICATION; >> + > You clear one bit more than before. The reason for this may be worth a note > in the commit message.
Thanks this was an artifact from debugging. I can remove the CLR as it was cleared in set_wol enablement.
Dan
| |