Messages in this thread | | | Date | Tue, 25 Sep 2012 09:37:44 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH 2/3] Staging:bcm: fix coding style error in InterfaceIsr.c |
| |
On Tue, Sep 25, 2012 at 11:04:28AM +0600, Gorskin Ilya wrote: > - if(((Adapter->bPreparingForLowPowerMode == TRUE) && (Adapter->bDoSuspend == TRUE)) || > - psIntfAdapter->bSuspended || > - psIntfAdapter->bPreparingForBusSuspend) > - { > - BCM_DEBUG_PRINT(Adapter,DBG_TYPE_OTHERS, INTF_INIT, DBG_LVL_ALL,"Interrupt call back is called while suspending the device"); > + if (((Adapter->bPreparingForLowPowerMode == TRUE) && > + (Adapter->bDoSuspend == TRUE)) || > + psIntfAdapter->bSuspended || > + psIntfAdapter->bPreparingForBusSuspend) { > + BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, INTF_INIT, > + DBG_LVL_ALL, > + "Interrupt call back is called > + while suspending the device"); > return ;
Hi,
Thanks for doing this, these changes are welcome. However, they should be done slightly differently.
Take one type of checkpatch warning at a time and fix that one over the file, then do a separate patch for the next type of warning. [patch 1/2] Staging: bcm: move curly braces in InterfaceIsr.c [patch 2/2] Staging: bcm: clean up conditions in InterfaceIsr.c
Something like that.
Also the way you've indented the condition is not right. The conditions which are && together should line up like this:
if (((Adapter->bPreparingForLowPowerMode == TRUE) && (Adapter->bDoSuspend == TRUE)) || psIntfAdapter->bSuspended || psIntfAdapter->bPreparingForBusSuspend) {
Also the condition has too many parenthesis. Everyone knows how the precedence works in: if (foo == 3 || bar == 4) { We don't need to specify: if ((foo == 3) || (bar == 4)) {
Putting extra parenthesis make the code harder to read and has lead to == vs = bugs which would have been caught by gcc: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
Also can we just leave off the "== TRUE", or is this a case where it can "== TRUE", "== FALSE", and "== FILENOTFOUND"?
Finally, this is not quoted correctly. > + "Interrupt call back is called > + while suspending the device"); Don't break those string literals up across multiple lines, but if you do then you need to add quotes. "Interrupt call back is called" ^ "while suspending the device"); ^
regards, dan carpenter
| |