Improve Power Management in SMC911X driver.

PATCH New Bertrand Cachet 3 participants 5 messages Nov 15, 2011 – Nov 22, 2011

lore

Share & tools

lore

Bertrand Cachet

author

2011-11-15 13:40 UTC #0 lore raw

PATCH v4 1/1 Improve Power Management in SMC911X driver.

Stefano Babic

2011-11-17 15:52 UTC #1 lore raw

PATCH v4 1/1 Improve Power Management in SMC911X driver.

Quoted 22 lines · Bertrand Cachet

From datasheet, when READY bit is set inside PM_CTRL register, it means that device is already in *normal* (D0) mode => it doesn't need to be wake-up.
With this patch, we only wake-up (writing on TEST_BYTE register) if PM_MODE bits of PM_CTRL register is in sleep (D1/D2) mode.
Signed-off-by Bertrand Cachet <[email protected]>
---
v2: Improve code styling by grouping two narrow comments.
v3: Place versions information under scissors line.
v4: Use tabs instead of spaces (vimrc problem)
 drivers/net/smc911x.h |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)
drivers/net/smc911x.h +0 -0
index 8ce08a9..a290073 100644
@@ -471,8 +471,11 @@ static void smc911x_reset(struct eth_device *dev)
 {
 	int timeout;

drivers/net/smc911x.h static void smc911x_reset(struct eth_device *dev) · line 480

Quoted 10 lines

drivers/net/smc911x.h +5 -2

 
-	/* Take out of PM setting first */
-	if (smc911x_reg_read(dev, PMT_CTRL) & PMT_CTRL_READY) {
+	/*
+	 *  Take out of PM setting first
+	 *  Device is already wake up if PMT_CTRL_READY bit is set
+	 */
+	if ((smc911x_reg_read(dev, PMT_CTRL) & PMT_CTRL_READY) == 0) {
 		/* Write to the bytetest will take out of powerdown */
 		smc911x_reg_write(dev, BYTE_TEST, 0x0);

Agree, this is the correct behavior. However, checkpatch report errors (due to leading white space instead of tabs in the comment you added):

ERROR: code indent should use tabs where possible
#29: FILE: drivers/net/smc911x.h:475:
+ ^I * Take out of PM setting first$

WARNING: please, no space before tabs
#29: FILE: drivers/net/smc911x.h:475:
+ ^I * Take out of PM setting first$

ERROR: code indent should use tabs where possible #30: FILE: drivers/net/smc911x.h:476:
+ ^I * Device is already wake up if PMT_CTRL_READY bit is set$

WARNING: please, no space before tabs
#30: FILE: drivers/net/smc911x.h:476:
+ ^I * Device is already wake up if PMT_CTRL_READY bit is set$

total: 2 errors, 2 warnings, 13 lines checked

NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
scripts/cleanfile

Can you fix and repost (maybe setting me as CC, I will take care of your patch).

Thanks,
Stefano Babic

Signature · 5 lines

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

Igor Grinberg

2011-11-17 16:21 UTC #2 lore raw

PATCH v4 1/1 Improve Power Management in SMC911X driver.

Hi Stefano, Bertrand,

Quoted 24 lines · Stefano Babic

Quoted 23 lines · Bertrand Cachet
From datasheet, when READY bit is set inside PM_CTRL register, it means that device is already in *normal* (D0) mode => it doesn't need to be wake-up.
With this patch, we only wake-up (writing on TEST_BYTE register) if PM_MODE bits of PM_CTRL register is in sleep (D1/D2) mode.
Signed-off-by Bertrand Cachet <[email protected]>
---
v2: Improve code styling by grouping two narrow comments.
v3: Place versions information under scissors line.
v4: Use tabs instead of spaces (vimrc problem)
 drivers/net/smc911x.h |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)
drivers/net/smc911x.h +0 -0
index 8ce08a9..a290073 100644
@@ -471,8 +471,11 @@ static void smc911x_reset(struct eth_device *dev)
 {
 	int timeout;
 

Quoted 12 lines

drivers/net/smc911x.h static void smc911x_reset(struct eth_device *dev) · line 480
Quoted 9 lines
drivers/net/smc911x.h +5 -2
-	/* Take out of PM setting first */
-	if (smc911x_reg_read(dev, PMT_CTRL) & PMT_CTRL_READY) {
+	/*
+	 *  Take out of PM setting first
+	 *  Device is already wake up if PMT_CTRL_READY bit is set
+	 */
+	if ((smc911x_reg_read(dev, PMT_CTRL) & PMT_CTRL_READY) == 0) {
 		/* Write to the bytetest will take out of powerdown */
 		smc911x_reg_write(dev, BYTE_TEST, 0x0);
Agree, this is the correct behavior. However, checkpatch report errors (due to leading white space instead of tabs in the comment you added):

Are you sure you are testing the right version (v4)? Because, I get different errors/warnings on that patch (see below)

Quoted 13 lines

ERROR: code indent should use tabs where possible
#29: FILE: drivers/net/smc911x.h:475:
+ ^I * Take out of PM setting first$
WARNING: please, no space before tabs
#29: FILE: drivers/net/smc911x.h:475:
+ ^I * Take out of PM setting first$
ERROR: code indent should use tabs where possible #30: FILE: drivers/net/smc911x.h:476:
+ ^I * Device is already wake up if PMT_CTRL_READY bit is set$

Quoted 9 lines

WARNING: please, no space before tabs
#30: FILE: drivers/net/smc911x.h:476:
+ ^I * Device is already wake up if PMT_CTRL_READY bit is set$
total: 2 errors, 2 warnings, 13 lines checked
NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
scripts/cleanfile

My checkpatch.pl reports:
----------------cut----------------
ERROR: trailing whitespace
#133: FILE: drivers/net/smc911x.h:474:
+^I/*^M$

ERROR: trailing whitespace
#134: FILE: drivers/net/smc911x.h:475:
+^I * Take out of PM setting first^M$

ERROR: trailing whitespace
#135: FILE: drivers/net/smc911x.h:476:
+^I * Device is already wake up if PMT_CTRL_READY bit is set^M$

ERROR: DOS line endings
#136: FILE: drivers/net/smc911x.h:477:
+^I */^M$

ERROR: DOS line endings
#137: FILE: drivers/net/smc911x.h:478:
+^Iif ((smc911x_reg_read(dev, PMT_CTRL) & PMT_CTRL_READY) == 0) {^M$

total: 5 errors, 0 warnings, 13 lines checked
---------------cut-----------------------------

Anyway, those need to be fixed.

Signature · 2 lines

Regards,
Igor.

Stefano Babic

2011-11-17 16:43 UTC #3 lore raw

PATCH v4 1/1 Improve Power Management in SMC911X driver.

Quoted 6 lines · Igor Grinberg

Quoted 3 lines
Agree, this is the correct behavior. However, checkpatch report errors (due to leading white space instead of tabs in the comment you added):
Are you sure you are testing the right version (v4)? Because, I get different errors/warnings on that patch (see below)

The right version, yes. Maybe I have a an old checkpatch.pl, I must update it.

Quoted 4 lines

Anyway, those need to be fixed.

Right.

Signature · 5 lines

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

Stefano Babic

2011-11-22 08:53 UTC #4 lore raw

PATCH v4 1/1 Improve Power Management in SMC911X driver.

In reply to Bertrand Cachet

Quoted 8 lines · Bertrand Cachet

From datasheet, when READY bit is set inside PM_CTRL register, it means that device is already in *normal* (D0) mode => it doesn't need to be wake-up.
With this patch, we only wake-up (writing on TEST_BYTE register) if PM_MODE bits of PM_CTRL register is in sleep (D1/D2) mode.
Signed-off-by Bertrand Cachet <[email protected]>
---

Applied to u-boot-staging, sbabic at denx.de, thanks.

Best regards,
Stefano Babic

Signature · 5 lines

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================