Message-ID: <20111101202259.GC4682@mwanda>
Date:	Tue, 1 Nov 2011 23:22:59 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Paul Bolle <pebolle@...cali.nl>
Cc:	Ciaran McCormick <ciaranmccormick@...il.com>,
	devel@...verdev.osuosl.org, shemminger@...tta.com, gregkh@...e.de,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Stageing: bcm: fixed spacing coding style in
 led_control.c

On Tue, Nov 01, 2011 at 12:07:45PM +0100, Paul Bolle wrote:
> Stageing? And you might as well drop the filename from the summary. By
> the way, are the other files (if any) of this driver any better (I
> haven't checked)?
> 

This needs a Signed-off-by line as well.

I'd say keep the filename.  Otherwise you get a string of cleanup
patches that all have the same name and that's annoying.

> On Tue, 2011-11-01 at 10:44 +0000, Ciaran McCormick wrote:
> > ---
> >  drivers/staging/bcm/led_control.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/bcm/led_control.c b/drivers/staging/bcm/led_control.c
> > index 16e939f..28c3382 100644
> > --- a/drivers/staging/bcm/led_control.c
> > +++ b/drivers/staging/bcm/led_control.c
> > @@ -5,8 +5,8 @@
> >  
> >  static B_UINT16 CFG_CalculateChecksum(B_UINT8 *pu8Buffer, B_UINT32 u32Size)
> >  {
> > -	B_UINT16 	u16CheckSum=0;
> > -	while(u32Size--) {
> > +	B_UINT16	u16CheckSum = 0;

Put a blank line between declarations and code.  Probably you could
make the tab a space as well.

> > +	while (u32Size--) {
> >  		u16CheckSum += (B_UINT8)~(*pu8Buffer);
> 
> I'd say this line might need a space after the cast.

That's not in CodingStyle so it's up to the maintainer to decide, I
guess.  I think we normally wouldn't put a space there.

> 
> >  	    pu8Buffer++;

This line should be indented the same as the previous line.

> >  	}
> > @@ -16,7 +16,7 @@ BOOLEAN IsReqGpioIsLedInNVM(PMINI_ADAPTER Adapter, UINT gpios)
> >  {
> >  	INT Status ;
> >  	Status = (Adapter->gpioBitMap & gpios) ^ gpios ;
> 
> You missed the space before the semicolons here.
> 
> > -	if(Status)
> > +	if (Status)
> >  		return FALSE;
> >  	else
> >  		return TRUE;
> 
> But, more importantly, why only do whitespace related stuff? Almost
> every second word apparently needs to be restyled here. Is doing just
> whitespace related stuff worthwhile? (This is not a rhetorical question.
> I'm actually wondering what Greg prefers).

You're right that this driver needs a lot of work.  Actually this
patch only touches on a tiny bit of the white space changes that are
needed in the file.  Here are the checkpatch complaints:

dcarpenter@...on:~/progs/kernel/devel$ ./scripts/checkpatch.pl -f drivers/staging/bcm/led_control.c | egrep "^(WARN|ERROR):" | sort | uniq -c
     21 ERROR: code indent should use tabs where possible
     74 ERROR: do not use C99 // comments
      9 ERROR: else should follow close brace '}'
      2 ERROR: space prohibited after that open parenthesis '('
      3 ERROR: space prohibited before that close parenthesis ')'
      2 ERROR: space prohibited before that ':' (ctx:WxE)
      2 ERROR: space required after that ',' (ctx:VxO)
     81 ERROR: space required after that ',' (ctx:VxV)
      1 ERROR: space required after that ',' (ctx:WxV)
      2 ERROR: space required before that '&' (ctx:OxV)
      1 ERROR: space required before the open brace '{'
     88 ERROR: space required before the open parenthesis '('
      2 ERROR: spaces required around that '||' (ctx:VxE)
      1 ERROR: spaces required around that ':' (ctx:VxE)
      1 ERROR: spaces required around that '<' (ctx:VxV)
      1 ERROR: spaces required around that '==' (ctx:VxV)
      5 ERROR: spaces required around that '=' (ctx:VxV)
      8 ERROR: spaces required around that '=' (ctx:VxW)
      2 ERROR: spaces required around that '!=' (ctx:VxW)
      1 ERROR: spaces required around that '<' (ctx:WxV)
      2 ERROR: spaces required around that '=' (ctx:WxV)
      1 ERROR: spaces required around that '||' (ctx:WxV)
     70 ERROR: that open brace { should be on the previous line
dcarpenter@...on:~/progs/kernel/devel$ 

I'd prefer a series of patches:

[patch 1/3] Staging: bcm: fix whitespace in led_control.c
This would address tabs vs spaces, extra prohibited spaces, and
spaces required around certain chars.  Also it would add blank lines
between functions and between declarations and code.

[patch 2/3] Staging: bcm: fix C99 comments in led_control.c
This would fix the 74 ERROR: do not use C99 // comments

[patch 3/3] Staging: bcm: fix braces style in led_control.c
This would fix the 70 ERROR: that open brace { should be on the
previous line.

Then another series would go through every file in the driver and
change the types to standard kernel types.

[patch 1/x] Staging: bcm: Change B_UINT16 to u16
[patch 2/x] Staging: bcm: Change B_UINT32 to u32

Then go through each file and rename the local variables to kernel
style variables.

[patch 1/y] Staging: bcm: Rename "Status" to "ret" in led_control.c
[patch 2/y] Staging: bcm: Rename CamelCase variables in led_control.c

Then go through the whole driver and make as many functions as
possible static.  Sparse can help with this.

[patch 1/z] Staging: bcm: mark functions as static
[patch 2/z] Staging: bcm: fix function names in led_control.c

Then go through the entire driver and rename all the non-static
functions.  This would be done one function per patch.  Btw the
coccinelle is could at this.  So much of this stuff could be scripted.

Then get rid of all the typedefs.

Then rename all the struct members.

Delete all the dead code.

Change all the custom printk() macros to normal kernel format.

All this stuff is fairly mechanical and do-able.

Then is when you start to hit the interesting bits.  The driver is at
the hello world stage.  It can connect to the internet but it crashes
soon after that.  I think you need a forked version of WPA supplicant
in userspace to make this work.  I tried a while back and didn't find
the userspace code and now I don't have the hardware.

So there is a long way to go... But anyway, every small step in the
right direction helps.  I think we tend to merge patches even if they
are small like this one.

regards,
dan carpenter

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)

Powered by blists - more mailing lists