[U-Boot] [PATCH v4] Improve Power Management in SMC911X driver.

[U-Boot] [PATCH v4] Improve Power Management in SMC911X driver.

[Bertrand Cachet]

[U-Boot] [PATCH v4] Improve Power Management in SMC911X driver.

[Stefano Babic]

On 11/15/2011 02:40 PM, Bertrand Cachet wrote:
> 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 <bertrand.cachet@heig-vd.ch>
> ---
>  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(-)
> 
> diff --git a/drivers/net/smc911x.h b/drivers/net/smc911x.h
> index 8ce08a9..a290073 100644
> --- a/drivers/net/smc911x.h
> +++ b/drivers/net/smc911x.h
> @@ -471,8 +471,11 @@ static void smc911x_reset(struct eth_device *dev)
>  {
>  	int timeout;
>  
> -	/* 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

-- 
=====================================================================
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
=====================================================================

[U-Boot] [PATCH v4] Improve Power Management in SMC911X driver.

[Igor Grinberg]

Hi Stefano, Bertrand,

On 11/17/11 17:52, Stefano Babic wrote:
> On 11/15/2011 02:40 PM, Bertrand Cachet wrote:
>> 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 <bertrand.cachet@heig-vd.ch>
>> ---
>>  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(-)
>>
>> diff --git a/drivers/net/smc911x.h b/drivers/net/smc911x.h
>> index 8ce08a9..a290073 100644
>> --- a/drivers/net/smc911x.h
>> +++ b/drivers/net/smc911x.h
>> @@ -471,8 +471,11 @@ static void smc911x_reset(struct eth_device *dev)
>>  {
>>  	int timeout;
>>  
>> -	/* 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)

> 
> 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
> 


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.

-- 
Regards,
Igor.

[U-Boot] [PATCH v4] Improve Power Management in SMC911X driver.

[Stefano Babic]

On 11/17/2011 05:21 PM, Igor Grinberg wrote:

>>
>> 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.


> 
> 
> Anyway, those need to be fixed.
> 

Right.

-- 
=====================================================================
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
=====================================================================

[U-Boot] [PATCH v4] Improve Power Management in SMC911X driver.

[Stefano Babic]

On 15/11/2011 14:40, Bertrand Cachet wrote:
> 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 <bertrand.cachet@heig-vd.ch>
> ---

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

Best regards,
Stefano Babic



-- 
=====================================================================
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
=====================================================================