All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Staging:rtl8192e: fix comments style issue in r8180_93cx6.c This is a patch to the r8180_93cx6.c file that fixes up the comments styling issues found by the checkpatch.pl tool Signed-off-by: Tim Schofield <tim@weberpafrica.com>
@ 2010-02-16 16:35 tim
  2010-02-18 16:05 ` Greg KH
  0 siblings, 1 reply; 8+ messages in thread
From: tim @ 2010-02-16 16:35 UTC (permalink / raw)
  To: gregkh; +Cc: devel, linux-kernel, Tim Schofield

From: Tim Schofield <tim.schofield1960@googlemail.com>

---
 drivers/staging/rtl8192e/r8180_93cx6.c |   22 ++++++++++++++--------
 1 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/rtl8192e/r8180_93cx6.c b/drivers/staging/rtl8192e/r8180_93cx6.c
index 262ed5f..60fba80 100644
--- a/drivers/staging/rtl8192e/r8180_93cx6.c
+++ b/drivers/staging/rtl8192e/r8180_93cx6.c
@@ -23,12 +23,14 @@
 static void eprom_cs(struct net_device *dev, short bit)
 {
 	if (bit)
+		/* enable EPROM */
 		write_nic_byte(dev, EPROM_CMD,
 			       (1<<EPROM_CS_SHIFT) | \
-			       read_nic_byte(dev, EPROM_CMD)); //enable EPROM
+			       read_nic_byte(dev, EPROM_CMD));
 	else
+		/* disable EPROM */
 		write_nic_byte(dev, EPROM_CMD, read_nic_byte(dev, EPROM_CMD)\
-			       &~(1<<EPROM_CS_SHIFT)); //disable EPROM
+			       &~(1<<EPROM_CS_SHIFT));
 
 	force_pci_posting(dev);
 	udelay(EPROM_DELAY);
@@ -96,7 +98,7 @@ u32 eprom_read(struct net_device *dev, u32 addr)
 	u32 ret;
 
 	ret = 0;
-        //enable EPROM programming
+	/* enable EPROM programming */
 	write_nic_byte(dev, EPROM_CMD,
 		       (EPROM_CMD_PROGRAM<<EPROM_CMD_OPERATING_MODE_SHIFT));
 	force_pci_posting(dev);
@@ -126,13 +128,17 @@ u32 eprom_read(struct net_device *dev, u32 addr)
 	eprom_send_bits_string(dev, read_cmd, 3);
 	eprom_send_bits_string(dev, addr_str, addr_len);
 
-	//keep chip pin D to low state while reading.
-	//I'm unsure if it is necessary, but anyway shouldn't hurt
+	/*
+	 * keep chip pin D to low state while reading.
+	 * I'm unsure if it is necessary, but anyway shouldn't hurt
+	 */
 	eprom_w(dev, 0);
 
 	for (i = 0; i < 16; i++) {
-		//eeprom needs a clk cycle between writing opcode&adr
-		//and reading data. (eeprom outs a dummy 0)
+		/*
+		 * eeprom needs a clk cycle between writing opcode&adr
+		 * and reading data. (eeprom outs a dummy 0)
+		 */
 		eprom_ck_cycle(dev);
 		ret |= (eprom_r(dev)<<(15-i));
 	}
@@ -140,7 +146,7 @@ u32 eprom_read(struct net_device *dev, u32 addr)
 	eprom_cs(dev, 0);
 	eprom_ck_cycle(dev);
 
-	//disable EPROM programming
+	/* disable EPROM programming */
 	write_nic_byte(dev, EPROM_CMD,
 		       (EPROM_CMD_NORMAL<<EPROM_CMD_OPERATING_MODE_SHIFT));
 	return ret;
-- 
1.6.3.3


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] Staging:rtl8192e: fix comments style issue in r8180_93cx6.c This is a patch to the r8180_93cx6.c file that fixes up the comments styling issues found by the checkpatch.pl tool Signed-off-by: Tim Schofield <tim@weberpafrica.com>
  2010-02-16 16:35 [PATCH] Staging:rtl8192e: fix comments style issue in r8180_93cx6.c This is a patch to the r8180_93cx6.c file that fixes up the comments styling issues found by the checkpatch.pl tool Signed-off-by: Tim Schofield <tim@weberpafrica.com> tim
@ 2010-02-18 16:05 ` Greg KH
  2010-02-18 17:31   ` Florian Mickler
  2010-03-03  7:10     ` verifying whitespace patches don't change anything was Re: [PATCH] Staging:rtl8192e: fix comments Dan Carpenter
  0 siblings, 2 replies; 8+ messages in thread
From: Greg KH @ 2010-02-18 16:05 UTC (permalink / raw)
  To: tim; +Cc: gregkh, devel, Tim Schofield, linux-kernel

On Tue, Feb 16, 2010 at 04:35:16PM +0000, tim@weberpafrica.com wrote:
> From: Tim Schofield <tim.schofield1960@googlemail.com>
> 
> ---
>  drivers/staging/rtl8192e/r8180_93cx6.c |   22 ++++++++++++++--------
>  1 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192e/r8180_93cx6.c b/drivers/staging/rtl8192e/r8180_93cx6.c
> index 262ed5f..60fba80 100644
> --- a/drivers/staging/rtl8192e/r8180_93cx6.c
> +++ b/drivers/staging/rtl8192e/r8180_93cx6.c
> @@ -23,12 +23,14 @@
>  static void eprom_cs(struct net_device *dev, short bit)
>  {
>  	if (bit)
> +		/* enable EPROM */
>  		write_nic_byte(dev, EPROM_CMD,
>  			       (1<<EPROM_CS_SHIFT) | \
> -			       read_nic_byte(dev, EPROM_CMD)); //enable EPROM
> +			       read_nic_byte(dev, EPROM_CMD));
>  	else
> +		/* disable EPROM */
>  		write_nic_byte(dev, EPROM_CMD, read_nic_byte(dev, EPROM_CMD)\
> -			       &~(1<<EPROM_CS_SHIFT)); //disable EPROM
> +			       &~(1<<EPROM_CS_SHIFT));

This does not do what you think it does (hint, you need {} if you want
to have more than one line in an if statement...)

Can you always verify that your coding style changes do not actually
break the code?  A simple comparison of the .ko file before and after
should be sufficient.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Staging:rtl8192e: fix comments style issue in r8180_93cx6.c This is a patch to the r8180_93cx6.c file that fixes up the comments styling issues found by the checkpatch.pl tool Signed-off-by: Tim Schofield <tim@weberpafrica.com>
  2010-02-18 16:05 ` Greg KH
@ 2010-02-18 17:31   ` Florian Mickler
  2010-02-18 17:51     ` Greg KH
  2010-03-03  7:10     ` verifying whitespace patches don't change anything was Re: [PATCH] Staging:rtl8192e: fix comments Dan Carpenter
  1 sibling, 1 reply; 8+ messages in thread
From: Florian Mickler @ 2010-02-18 17:31 UTC (permalink / raw)
  To: gregkh; +Cc: Tim Schofield, devel, linux-kernel, tim

On Thu, 18 Feb 2010 08:05:47 -0800
Greg KH <greg@kroah.com> wrote:

> On Tue, Feb 16, 2010 at 04:35:16PM +0000, tim@weberpafrica.com wrote:
> > From: Tim Schofield <tim.schofield1960@googlemail.com>
> > 
> > ---
> >  drivers/staging/rtl8192e/r8180_93cx6.c |   22 ++++++++++++++--------
> >  1 files changed, 14 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8192e/r8180_93cx6.c b/drivers/staging/rtl8192e/r8180_93cx6.c
> > index 262ed5f..60fba80 100644
> > --- a/drivers/staging/rtl8192e/r8180_93cx6.c
> > +++ b/drivers/staging/rtl8192e/r8180_93cx6.c
> > @@ -23,12 +23,14 @@
> >  static void eprom_cs(struct net_device *dev, short bit)
> >  {
> >  	if (bit)
> > +		/* enable EPROM */
> >  		write_nic_byte(dev, EPROM_CMD,
> >  			       (1<<EPROM_CS_SHIFT) | \
> > -			       read_nic_byte(dev, EPROM_CMD)); //enable EPROM
> > +			       read_nic_byte(dev, EPROM_CMD));
> >  	else
> > +		/* disable EPROM */
> >  		write_nic_byte(dev, EPROM_CMD, read_nic_byte(dev, EPROM_CMD)\
> > -			       &~(1<<EPROM_CS_SHIFT)); //disable EPROM
> > +			       &~(1<<EPROM_CS_SHIFT));
> 
> This does not do what you think it does (hint, you need {} if you want
> to have more than one line in an if statement...)
> 
> Can you always verify that your coding style changes do not actually
> break the code?  A simple comparison of the .ko file before and after
> should be sufficient.
> 
> thanks,
> 
> greg k-h

hm... no.. seems to be correct... the comment get's ignored. But I
agree that {} would be nicer to the eye...

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Staging:rtl8192e: fix comments style issue in r8180_93cx6.c This is a patch to the r8180_93cx6.c file that fixes up the comments styling issues found by the checkpatch.pl tool Signed-off-by: Tim Schofield <tim@weberpafrica.com>
  2010-02-18 17:31   ` Florian Mickler
@ 2010-02-18 17:51     ` Greg KH
  0 siblings, 0 replies; 8+ messages in thread
From: Greg KH @ 2010-02-18 17:51 UTC (permalink / raw)
  To: Florian Mickler; +Cc: Tim Schofield, devel, linux-kernel, tim

On Thu, Feb 18, 2010 at 06:31:51PM +0100, Florian Mickler wrote:
> On Thu, 18 Feb 2010 08:05:47 -0800
> Greg KH <greg@kroah.com> wrote:
> 
> > On Tue, Feb 16, 2010 at 04:35:16PM +0000, tim@weberpafrica.com wrote:
> > > From: Tim Schofield <tim.schofield1960@googlemail.com>
> > > 
> > > ---
> > >  drivers/staging/rtl8192e/r8180_93cx6.c |   22 ++++++++++++++--------
> > >  1 files changed, 14 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/staging/rtl8192e/r8180_93cx6.c b/drivers/staging/rtl8192e/r8180_93cx6.c
> > > index 262ed5f..60fba80 100644
> > > --- a/drivers/staging/rtl8192e/r8180_93cx6.c
> > > +++ b/drivers/staging/rtl8192e/r8180_93cx6.c
> > > @@ -23,12 +23,14 @@
> > >  static void eprom_cs(struct net_device *dev, short bit)
> > >  {
> > >  	if (bit)
> > > +		/* enable EPROM */
> > >  		write_nic_byte(dev, EPROM_CMD,
> > >  			       (1<<EPROM_CS_SHIFT) | \
> > > -			       read_nic_byte(dev, EPROM_CMD)); //enable EPROM
> > > +			       read_nic_byte(dev, EPROM_CMD));
> > >  	else
> > > +		/* disable EPROM */
> > >  		write_nic_byte(dev, EPROM_CMD, read_nic_byte(dev, EPROM_CMD)\
> > > -			       &~(1<<EPROM_CS_SHIFT)); //disable EPROM
> > > +			       &~(1<<EPROM_CS_SHIFT));
> > 
> > This does not do what you think it does (hint, you need {} if you want
> > to have more than one line in an if statement...)
> > 
> > Can you always verify that your coding style changes do not actually
> > break the code?  A simple comparison of the .ko file before and after
> > should be sufficient.
> > 
> > thanks,
> > 
> > greg k-h
> 
> hm... no.. seems to be correct... the comment get's ignored. But I
> agree that {} would be nicer to the eye...

wow, you are right, I just tested it out, learn something new every day.

But we should add braces...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 8+ messages in thread

* verifying whitespace patches don't change anything was Re: [PATCH]
  2010-02-18 16:05 ` Greg KH
@ 2010-03-03  7:10     ` Dan Carpenter
  2010-03-03  7:10     ` verifying whitespace patches don't change anything was Re: [PATCH] Staging:rtl8192e: fix comments Dan Carpenter
  1 sibling, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2010-03-03  7:10 UTC (permalink / raw)
  To: Greg KH; +Cc: tim, gregkh, devel, Tim Schofield, linux-kernel, kernel-janitors

On Thu, Feb 18, 2010 at 08:05:47AM -0800, Greg KH wrote:
> On Tue, Feb 16, 2010 at 04:35:16PM +0000, tim@weberpafrica.com wrote:
> > From: Tim Schofield <tim.schofield1960@googlemail.com>
> > 
> > ---
> >  drivers/staging/rtl8192e/r8180_93cx6.c |   22 ++++++++++++++--------
> >  1 files changed, 14 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8192e/r8180_93cx6.c b/drivers/staging/rtl8192e/r8180_93cx6.c
> > index 262ed5f..60fba80 100644
> > --- a/drivers/staging/rtl8192e/r8180_93cx6.c
> > +++ b/drivers/staging/rtl8192e/r8180_93cx6.c
> > @@ -23,12 +23,14 @@
> >  static void eprom_cs(struct net_device *dev, short bit)
> >  {
> >  	if (bit)
> > +		/* enable EPROM */
> >  		write_nic_byte(dev, EPROM_CMD,
> >  			       (1<<EPROM_CS_SHIFT) | \
> > -			       read_nic_byte(dev, EPROM_CMD)); //enable EPROM
> > +			       read_nic_byte(dev, EPROM_CMD));
> >  	else
> > +		/* disable EPROM */
> >  		write_nic_byte(dev, EPROM_CMD, read_nic_byte(dev, EPROM_CMD)\
> > -			       &~(1<<EPROM_CS_SHIFT)); //disable EPROM
> > +			       &~(1<<EPROM_CS_SHIFT));
> 
> This does not do what you think it does (hint, you need {} if you want
> to have more than one line in an if statement...)
> 
> Can you always verify that your coding style changes do not actually
> break the code?  A simple comparison of the .ko file before and after
> should be sufficient.
> 

That's what I thought too, but every debug macro has a __LINE__ in it so 
if you add a new line at the start of the file it makes a ton of changes in
the final binary .ko file.

Is there a trick to this?

I hacked sparse to always use 12345 as the line, but if, for example, you 
remove uneeded parenthesis that would still count as a code change.

regards,
dan carpenter

diff --git a/pre-process.c b/pre-process.c
index 34b21ff..d3ae7a0 100644
--- a/pre-process.c
+++ b/pre-process.c
@@ -154,7 +154,8 @@ static int expand_one_symbol(struct token **list)
 		return expand(list, sym);
 	}
 	if (token->ident = &__LINE___ident) {
-		replace_with_integer(token, token->pos.line);
+//		replace_with_integer(token, token->pos.line);
+		replace_with_integer(token, 12345);
 	} else if (token->ident = &__FILE___ident) {
 		replace_with_string(token, stream_name(token->pos.stream));
 	} else if (token->ident = &__DATE___ident) {

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* verifying whitespace patches don't change anything was Re: [PATCH] Staging:rtl8192e: fix comments
@ 2010-03-03  7:10     ` Dan Carpenter
  0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2010-03-03  7:10 UTC (permalink / raw)
  To: Greg KH; +Cc: tim, gregkh, devel, Tim Schofield, linux-kernel, kernel-janitors

On Thu, Feb 18, 2010 at 08:05:47AM -0800, Greg KH wrote:
> On Tue, Feb 16, 2010 at 04:35:16PM +0000, tim@weberpafrica.com wrote:
> > From: Tim Schofield <tim.schofield1960@googlemail.com>
> > 
> > ---
> >  drivers/staging/rtl8192e/r8180_93cx6.c |   22 ++++++++++++++--------
> >  1 files changed, 14 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8192e/r8180_93cx6.c b/drivers/staging/rtl8192e/r8180_93cx6.c
> > index 262ed5f..60fba80 100644
> > --- a/drivers/staging/rtl8192e/r8180_93cx6.c
> > +++ b/drivers/staging/rtl8192e/r8180_93cx6.c
> > @@ -23,12 +23,14 @@
> >  static void eprom_cs(struct net_device *dev, short bit)
> >  {
> >  	if (bit)
> > +		/* enable EPROM */
> >  		write_nic_byte(dev, EPROM_CMD,
> >  			       (1<<EPROM_CS_SHIFT) | \
> > -			       read_nic_byte(dev, EPROM_CMD)); //enable EPROM
> > +			       read_nic_byte(dev, EPROM_CMD));
> >  	else
> > +		/* disable EPROM */
> >  		write_nic_byte(dev, EPROM_CMD, read_nic_byte(dev, EPROM_CMD)\
> > -			       &~(1<<EPROM_CS_SHIFT)); //disable EPROM
> > +			       &~(1<<EPROM_CS_SHIFT));
> 
> This does not do what you think it does (hint, you need {} if you want
> to have more than one line in an if statement...)
> 
> Can you always verify that your coding style changes do not actually
> break the code?  A simple comparison of the .ko file before and after
> should be sufficient.
> 

That's what I thought too, but every debug macro has a __LINE__ in it so 
if you add a new line at the start of the file it makes a ton of changes in
the final binary .ko file.

Is there a trick to this?

I hacked sparse to always use 12345 as the line, but if, for example, you 
remove uneeded parenthesis that would still count as a code change.

regards,
dan carpenter

diff --git a/pre-process.c b/pre-process.c
index 34b21ff..d3ae7a0 100644
--- a/pre-process.c
+++ b/pre-process.c
@@ -154,7 +154,8 @@ static int expand_one_symbol(struct token **list)
 		return expand(list, sym);
 	}
 	if (token->ident == &__LINE___ident) {
-		replace_with_integer(token, token->pos.line);
+//		replace_with_integer(token, token->pos.line);
+		replace_with_integer(token, 12345);
 	} else if (token->ident == &__FILE___ident) {
 		replace_with_string(token, stream_name(token->pos.stream));
 	} else if (token->ident == &__DATE___ident) {

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: verifying whitespace patches don't change anything was Re:
  2010-03-03  7:10     ` verifying whitespace patches don't change anything was Re: [PATCH] Staging:rtl8192e: fix comments Dan Carpenter
@ 2010-03-03 12:18       ` Matthew Wilcox
  -1 siblings, 0 replies; 8+ messages in thread
From: Matthew Wilcox @ 2010-03-03 12:18 UTC (permalink / raw)
  To: Dan Carpenter, Greg KH, tim, gregkh, devel, Tim Schofield,
	linux-kernel, kernel-janitors

On Wed, Mar 03, 2010 at 10:10:23AM +0300, Dan Carpenter wrote:
> On Thu, Feb 18, 2010 at 08:05:47AM -0800, Greg KH wrote:
> > On Tue, Feb 16, 2010 at 04:35:16PM +0000, tim@weberpafrica.com wrote:
> > > From: Tim Schofield <tim.schofield1960@googlemail.com>
> > > 
> > > ---
> > >  drivers/staging/rtl8192e/r8180_93cx6.c |   22 ++++++++++++++--------
> > >  1 files changed, 14 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/staging/rtl8192e/r8180_93cx6.c b/drivers/staging/rtl8192e/r8180_93cx6.c
> > > index 262ed5f..60fba80 100644
> > > --- a/drivers/staging/rtl8192e/r8180_93cx6.c
> > > +++ b/drivers/staging/rtl8192e/r8180_93cx6.c
> > > @@ -23,12 +23,14 @@
> > >  static void eprom_cs(struct net_device *dev, short bit)
> > >  {
> > >  	if (bit)
> > > +		/* enable EPROM */
> > >  		write_nic_byte(dev, EPROM_CMD,
> > >  			       (1<<EPROM_CS_SHIFT) | \
> > > -			       read_nic_byte(dev, EPROM_CMD)); //enable EPROM
> > > +			       read_nic_byte(dev, EPROM_CMD));
> > >  	else
> > > +		/* disable EPROM */
> > >  		write_nic_byte(dev, EPROM_CMD, read_nic_byte(dev, EPROM_CMD)\
> > > -			       &~(1<<EPROM_CS_SHIFT)); //disable EPROM
> > > +			       &~(1<<EPROM_CS_SHIFT));
> > 
> > This does not do what you think it does (hint, you need {} if you want
> > to have more than one line in an if statement...)

Sorry, Greg, you're wrong in this case.  /* */ does not count as a line.
Here's an example:

$ cat foo.c 
#include <stdio.h>

int main(void)
{
        if (0)
                /* foo */
                printf("Hello, World\n");
        return 0;
}

$ gcc -o foo foo.c -W -Wall -O2 -g
$ ./foo
[no output]


I think a much more cogent criticism of this patch would be that the comment
is entirely unnecessary for this function with a simple change:

-static void eprom_cs(struct net_device *dev, short bit)
+static void eprom_cs(struct net_device *dev, short enable)
 {
-	if (bit)
+	if (enable)
 		write_nic_byte(dev, EPROM_CMD,
 			       (1<<EPROM_CS_SHIFT) | \
-			       read_nic_byte(dev, EPROM_CMD)); //enable EPROM
+			       read_nic_byte(dev, EPROM_CMD));
 	else
 		write_nic_byte(dev, EPROM_CMD, read_nic_byte(dev, EPROM_CMD)\
-			       &~(1<<EPROM_CS_SHIFT)); //disable EPROM
+			       &~(1<<EPROM_CS_SHIFT));

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: verifying whitespace patches don't change anything was Re: [PATCH] Staging:rtl8192e: fix comments
@ 2010-03-03 12:18       ` Matthew Wilcox
  0 siblings, 0 replies; 8+ messages in thread
From: Matthew Wilcox @ 2010-03-03 12:18 UTC (permalink / raw)
  To: Dan Carpenter, Greg KH, tim, gregkh, devel, Tim Schofield,
	linux-kernel, kernel-janitors

On Wed, Mar 03, 2010 at 10:10:23AM +0300, Dan Carpenter wrote:
> On Thu, Feb 18, 2010 at 08:05:47AM -0800, Greg KH wrote:
> > On Tue, Feb 16, 2010 at 04:35:16PM +0000, tim@weberpafrica.com wrote:
> > > From: Tim Schofield <tim.schofield1960@googlemail.com>
> > > 
> > > ---
> > >  drivers/staging/rtl8192e/r8180_93cx6.c |   22 ++++++++++++++--------
> > >  1 files changed, 14 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/staging/rtl8192e/r8180_93cx6.c b/drivers/staging/rtl8192e/r8180_93cx6.c
> > > index 262ed5f..60fba80 100644
> > > --- a/drivers/staging/rtl8192e/r8180_93cx6.c
> > > +++ b/drivers/staging/rtl8192e/r8180_93cx6.c
> > > @@ -23,12 +23,14 @@
> > >  static void eprom_cs(struct net_device *dev, short bit)
> > >  {
> > >  	if (bit)
> > > +		/* enable EPROM */
> > >  		write_nic_byte(dev, EPROM_CMD,
> > >  			       (1<<EPROM_CS_SHIFT) | \
> > > -			       read_nic_byte(dev, EPROM_CMD)); //enable EPROM
> > > +			       read_nic_byte(dev, EPROM_CMD));
> > >  	else
> > > +		/* disable EPROM */
> > >  		write_nic_byte(dev, EPROM_CMD, read_nic_byte(dev, EPROM_CMD)\
> > > -			       &~(1<<EPROM_CS_SHIFT)); //disable EPROM
> > > +			       &~(1<<EPROM_CS_SHIFT));
> > 
> > This does not do what you think it does (hint, you need {} if you want
> > to have more than one line in an if statement...)

Sorry, Greg, you're wrong in this case.  /* */ does not count as a line.
Here's an example:

$ cat foo.c 
#include <stdio.h>

int main(void)
{
        if (0)
                /* foo */
                printf("Hello, World\n");
        return 0;
}

$ gcc -o foo foo.c -W -Wall -O2 -g
$ ./foo
[no output]


I think a much more cogent criticism of this patch would be that the comment
is entirely unnecessary for this function with a simple change:

-static void eprom_cs(struct net_device *dev, short bit)
+static void eprom_cs(struct net_device *dev, short enable)
 {
-	if (bit)
+	if (enable)
 		write_nic_byte(dev, EPROM_CMD,
 			       (1<<EPROM_CS_SHIFT) | \
-			       read_nic_byte(dev, EPROM_CMD)); //enable EPROM
+			       read_nic_byte(dev, EPROM_CMD));
 	else
 		write_nic_byte(dev, EPROM_CMD, read_nic_byte(dev, EPROM_CMD)\
-			       &~(1<<EPROM_CS_SHIFT)); //disable EPROM
+			       &~(1<<EPROM_CS_SHIFT));

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2010-03-03 12:18 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-02-16 16:35 [PATCH] Staging:rtl8192e: fix comments style issue in r8180_93cx6.c This is a patch to the r8180_93cx6.c file that fixes up the comments styling issues found by the checkpatch.pl tool Signed-off-by: Tim Schofield <tim@weberpafrica.com> tim
2010-02-18 16:05 ` Greg KH
2010-02-18 17:31   ` Florian Mickler
2010-02-18 17:51     ` Greg KH
2010-03-03  7:10   ` verifying whitespace patches don't change anything was Re: [PATCH] Dan Carpenter
2010-03-03  7:10     ` verifying whitespace patches don't change anything was Re: [PATCH] Staging:rtl8192e: fix comments Dan Carpenter
2010-03-03 12:18     ` verifying whitespace patches don't change anything was Matthew Wilcox
2010-03-03 12:18       ` verifying whitespace patches don't change anything was Re: [PATCH] Staging:rtl8192e: fix comments Matthew Wilcox

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.