Messages in this thread | | | Date | Mon, 16 Aug 2021 11:10:56 +0100 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH net-next 1/1] net: phy: marvell: Add WAKE_PHY support to WOL event |
| |
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 <yoong.siang.song@intel.com>
Hi,
I think this can be greatly simplified - see below.
> --- > 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!
| |