Re: [PATCH net-next 1/1] net: phy: marvell: Add WAKE_PHY support to WOL event
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
Date: 2021-08-16 10:11:08
Also in:
lkml
On Fri, Aug 13, 2021 at 04:45:08PM +0800, Song Yoong Siang wrote:
Add Wake-on-PHY feature support by enabling the Link Up Event. Signed-off-by: Song Yoong Siang <redacted>
Hi, I think this can be greatly simplified - see below.
quoted hunk ↗ jump to hunk
--- drivers/net/phy/marvell.c | 39 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-)diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index 3de93c9f2744..415e2a01c151 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c@@ -155,6 +155,7 @@ #define MII_88E1318S_PHY_WOL_CTRL 0x10 #define MII_88E1318S_PHY_WOL_CTRL_CLEAR_WOL_STATUS BIT(12) +#define MII_88E1318S_PHY_WOL_CTRL_LINK_UP_ENABLE BIT(13) #define MII_88E1318S_PHY_WOL_CTRL_MAGIC_PACKET_MATCH_ENABLE BIT(14) #define MII_PHY_LED_CTRL 16@@ -1746,13 +1747,19 @@ static void m88e1318_get_wol(struct phy_device *phydev, { int ret; - wol->supported = WAKE_MAGIC; + wol->supported = WAKE_MAGIC | WAKE_PHY; wol->wolopts = 0; ret = phy_read_paged(phydev, MII_MARVELL_WOL_PAGE, MII_88E1318S_PHY_WOL_CTRL); - if (ret >= 0 && ret & MII_88E1318S_PHY_WOL_CTRL_MAGIC_PACKET_MATCH_ENABLE) + if (ret < 0) + return; + + if (ret & MII_88E1318S_PHY_WOL_CTRL_MAGIC_PACKET_MATCH_ENABLE) wol->wolopts |= WAKE_MAGIC; + + if (ret & MII_88E1318S_PHY_WOL_CTRL_LINK_UP_ENABLE) + wol->wolopts |= WAKE_PHY; } static int m88e1318_set_wol(struct phy_device *phydev,@@ -1764,7 +1771,7 @@ static int m88e1318_set_wol(struct phy_device *phydev, if (oldpage < 0) goto error; - if (wol->wolopts & WAKE_MAGIC) { + if (wol->wolopts & (WAKE_MAGIC | WAKE_PHY)) { /* Explicitly switch to page 0x00, just to be sure */ err = marvell_write_page(phydev, MII_MARVELL_COPPER_PAGE); if (err < 0)@@ -1796,7 +1803,9 @@ static int m88e1318_set_wol(struct phy_device *phydev, MII_88E1318S_PHY_LED_TCR_INT_ACTIVE_LOW); if (err < 0) goto error; + }
Wouldn't it make more sense to always select the WOL page at this point between these two blocks? From what I can see, the WOL page is selected by both true and false blocks of the next if() statement, and again by the newly added if() statement for WAKE_PHY. Other than that, I don't see any issues. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!