lkml.org 
[lkml]   [2021]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH net-next V2] net: phy: marvell10g: enable WoL for mv2110
Hi Ling,

some more things, please look at these.

First, the subject line should be
net: phy: marvell10g: enable WoL for 88X3310 and 88E2110
since you are also implementing it for 3310.

> From: Voon Weifeng <weifeng.voon@intel.com>
>
> Basically it is just to enable to WoL interrupt and enable WoL
> detection. Then, configure the MAC address into address detection
> register.

"Implement Wake-on-LAN feature for 88X3310 and 88E2110.

This is done by enabling WoL interrupt and WoL detection and
configuring MAC address into WoL magic packet registers."

> Change Log:
> V2:
> (1) Reviewer Marek request to rorganize code to readable way.
> (2) Reviewer Rusell request to put phy_clear_bits_mmd() outside of
> if(){}else{} and modify return ret to return phy_clear_bits_mmd().
> (3) Reviewer Rusell request to add return on phy_read_mmd() in
> set_wol(). (4) Reorganize register layout to be put before
> MV_V2_TEMP_CTRL. (5) Add the .{get|set}_wol for 88E3110 too as per
> feedback from Russell.

We do not put changelogs into the commit message normally. Mostly we
put it into cover letters or after the "--" line so that it is not
included in the commit message in git history (merge commits are one
of the exceptions).

As Russell wrote, you have only partially reorganized register
constants. The constants should be defined in this order
+ MV_V2_PORT_INTR_STS = 0xf040,
+ MV_V2_PORT_INTR_MASK = 0xf043,
+ MV_V2_WOL_INTR_EN = BIT(8),
+ MV_V2_MAGIC_PKT_WORD0 = 0xf06b,
+ MV_V2_MAGIC_PKT_WORD1 = 0xf06c,
+ MV_V2_MAGIC_PKT_WORD2 = 0xf06d,
+ /* Wake on LAN registers */
+ MV_V2_WOL_CTRL = 0xf06e,
+ MV_V2_WOL_CLEAR_STS = BIT(15),
+ MV_V2_WOL_MAGIC_PKT_EN = BIT(0),
+ MV_V2_WOL_STS = 0xf06f,

Also:
- MV_V2_WOL_STS is not used, you should read the register before
cleaning or don't define the constant at all. (IMO you should read
it).
- register value constants should be prefixed with whole register names,
so:
MV_V2_WOL_INTR_EN rename to MV_V2_PORT_INTR_STS_WOL_EN
MV_V2_WOL_CLEAR_STS rename to MV_V2_WOL_CTRL_CLEAR_STS
MV_V2_WOL_MAGIC_PKT_EN rename to MV_V2_WOL_CTRL_MAGIC_PKT_EN

+ /* Reset the clear WOL status bit as it does not self-clear */
+ return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
+ MV_V2_WOL_CTRL,
+ MV_V2_WOL_CLEAR_STS);

This has wrong indentation, the arguments in new lines should start at
the position of first argument.

Marek

\
 
 \ /
  Last update: 2021-06-30 14:45    [W:0.056 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site