[PATCH] Drivers:firewire: fix style errors in core-card.c This is a patch in the core-card.c file which fixes up 2 warnings found by the checkpatch.pl tool Signed-off-by: Sander Nemvalts <snemvalts@gmail.com>

[PATCH] Drivers:firewire: fix style errors in core-card.c This is a patch in the core-card.c file which fixes up 2 warnings found by the checkpatch.pl tool Signed-off-by: Sander Nemvalts <snemvalts@gmail.com>

[Sander Nemvalts]

---
 drivers/firewire/core-card.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 57ea7f4..825b260 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -89,7 +89,8 @@ static size_t config_rom_length = 1 + 4 + 1 + 1;
 #define BIB_ISC			((1) << 29)
 #define BIB_CMC			((1) << 30)
 #define BIB_IRMC		((1) << 31)
-#define NODE_CAPABILITIES	0x0c0083c0 /* per IEEE 1394 clause 8.3.2.6.5.2 */
+/* per IEEE 1394 clause 8.3.2.6.5.2 */
+#define NODE_CAPABILITIES	0x0c0083c0
 
 /*
  * IEEE-1394 specifies a default SPLIT_TIMEOUT value of 800 cycles (100 ms),
@@ -132,7 +133,7 @@ static void generate_config_rom(struct fw_card *card, __be32 *config_rom)
 	j = 7 + descriptor_count;
 
 	/* Generate root directory entries for descriptors. */
-	list_for_each_entry (desc, &descriptor_list, link) {
+	list_for_each_entry(desc, &descriptor_list, link) {
 		if (desc->immediate > 0)
 			config_rom[i++] = cpu_to_be32(desc->immediate);
 		config_rom[i] = cpu_to_be32(desc->key | (j - i));
@@ -144,7 +145,7 @@ static void generate_config_rom(struct fw_card *card, __be32 *config_rom)
 	config_rom[5] = cpu_to_be32((i - 5 - 1) << 16);
 
 	/* End of root directory, now copy in descriptors. */
-	list_for_each_entry (desc, &descriptor_list, link) {
+	list_for_each_entry(desc, &descriptor_list, link) {
 		for (k = 0; k < desc->length; k++)
 			config_rom[i + k] = cpu_to_be32(desc->data[k]);
 		i += desc->length;
@@ -164,7 +165,7 @@ static void update_config_roms(void)
 {
 	struct fw_card *card;
 
-	list_for_each_entry (card, &card_list, link) {
+	list_for_each_entry(card, &card_list, link) {
 		generate_config_rom(card, tmp_config_rom);
 		card->driver->set_config_rom(card, tmp_config_rom,
 					     config_rom_length);
-- 
1.9.1

Re: [PATCH] Drivers:firewire: fix style errors in core-card.c This is a patch in the core-card.c file which fixes up 2 warnings found by the checkpatch.pl tool Signed-off-by: Sander Nemvalts <snemvalts@gmail.com>

[Stefan Richter]

On Sep 01 Sander Nemvalts wrote:
> --- a/drivers/firewire/core-card.c
> +++ b/drivers/firewire/core-card.c
> @@ -89,7 +89,8 @@ static size_t config_rom_length = 1 + 4 + 1 + 1;
>  #define BIB_ISC			((1) << 29)
>  #define BIB_CMC			((1) << 30)
>  #define BIB_IRMC		((1) << 31)
> -#define NODE_CAPABILITIES	0x0c0083c0 /* per IEEE 1394 clause 8.3.2.6.5.2 */
> +/* per IEEE 1394 clause 8.3.2.6.5.2 */
> +#define NODE_CAPABILITIES	0x0c0083c0

This is no improvement.

>  /*
>   * IEEE-1394 specifies a default SPLIT_TIMEOUT value of 800 cycles (100 ms),
> @@ -132,7 +133,7 @@ static void generate_config_rom(struct fw_card *card, __be32 *config_rom)
>  	j = 7 + descriptor_count;
>  
>  	/* Generate root directory entries for descriptors. */
> -	list_for_each_entry (desc, &descriptor_list, link) {
> +	list_for_each_entry(desc, &descriptor_list, link) {
>  		if (desc->immediate > 0)
>  			config_rom[i++] = cpu_to_be32(desc->immediate);
>  		config_rom[i] = cpu_to_be32(desc->key | (j - i));

We are writing
	for (a; b; c);
not
	for(a; b; c);
and thus a space after list_for_each and friends makes sense.
-- 
Stefan Richter
-=====-====- =--= ----=
http://arcgraph.de/sr/

Re: [PATCH] Drivers:firewire: fix style errors in core-card.c This is a patch in the core-card.c file which fixes up 2 warnings found by the checkpatch.pl tool Signed-off-by: Sander Nemvalts <snemvalts@gmail.com>

[Paul Bolle]

Quite a crowded subject line!

On Mon, 2014-09-01 at 23:56 +0200, Stefan Richter wrote:
> On Sep 01 Sander Nemvalts wrote:
> > @@ -132,7 +133,7 @@ static void generate_config_rom(struct fw_card *card, __be32 *config_rom)
> >  	j = 7 + descriptor_count;
> >  
> >  	/* Generate root directory entries for descriptors. */
> > -	list_for_each_entry (desc, &descriptor_list, link) {
> > +	list_for_each_entry(desc, &descriptor_list, link) {
> >  		if (desc->immediate > 0)
> >  			config_rom[i++] = cpu_to_be32(desc->immediate);
> >  		config_rom[i] = cpu_to_be32(desc->key | (j - i));
> 
> We are writing
> 	for (a; b; c);
> not
> 	for(a; b; c);
> and thus a space after list_for_each and friends makes sense.

$ git grep "list_for_each_entry (" | wc -l
52
$ git grep "list_for_each_entry(" | wc -l
5991

So "list_for_each_entry(" (without space) seems to be the preferred
style.


Paul Bolle

Re: [PATCH] Drivers:firewire: fix style errors in core-card.c This is a patch in the core-card.c file which fixes up 2 warnings found by the checkpatch.pl tool Signed-off-by: Sander Nemvalts <snemvalts@gmail.com>

[Stefan Richter]

Folding two replies into one.

On Sep 02 Sander Nemvalts wrote:
> On Sep 2, 2014 12:56 AM, "Stefan Richter" <stefanr@s5r6.in-berlin.de> wrote:
> 
> > On Sep 01 Sander Nemvalts wrote:
> > > --- a/drivers/firewire/core-card.c
> > > +++ b/drivers/firewire/core-card.c
> > > @@ -89,7 +89,8 @@ static size_t config_rom_length = 1 + 4 + 1 + 1;
> > >  #define BIB_ISC                      ((1) << 29)
> > >  #define BIB_CMC                      ((1) << 30)
> > >  #define BIB_IRMC             ((1) << 31)
> > > -#define NODE_CAPABILITIES    0x0c0083c0 /* per IEEE 1394 clause 8.3.2.6.5.2 */
> > > +/* per IEEE 1394 clause 8.3.2.6.5.2 */
> > > +#define NODE_CAPABILITIES    0x0c0083c0
> >
> > This is no improvement.
> >
> The comment after NODE_CAPABILITIES was moved to front of statement because
> the line would otherwise be over 80 characters long.

There is no problem with this line's remaining 81 characters long.

> Also, commenting before the line is demonstrated in the next statement.

This block of statements is entirely unrelated to the next statement;
furthermore the nature and form of this and the next comment are unrelated.


On Sep 02 Paul Bolle wrote:
> On Mon, 2014-09-01 at 23:56 +0200, Stefan Richter wrote:
> > On Sep 01 Sander Nemvalts wrote:
> > > @@ -132,7 +133,7 @@ static void generate_config_rom(struct fw_card *card, __be32 *config_rom)
> > >  	j = 7 + descriptor_count;
> > >  
> > >  	/* Generate root directory entries for descriptors. */
> > > -	list_for_each_entry (desc, &descriptor_list, link) {
> > > +	list_for_each_entry(desc, &descriptor_list, link) {
> > >  		if (desc->immediate > 0)
> > >  			config_rom[i++] = cpu_to_be32(desc->immediate);
> > >  		config_rom[i] = cpu_to_be32(desc->key | (j - i));
> > 
> > We are writing
> > 	for (a; b; c);
> > not
> > 	for(a; b; c);
> > and thus a space after list_for_each and friends makes sense.
> 
> $ git grep "list_for_each_entry (" | wc -l
> 52
> $ git grep "list_for_each_entry(" | wc -l
> 5991
> 
> So "list_for_each_entry(" (without space) seems to be the preferred
> style.

'Seems to be' is an appropriate wording indeed, since grep shows
occurrences, not preferences.


All,

here are some tips for whitespace changes and similar patches:

  - As a general rule, do not propose style changes to code on which you
    are not working on.

  - Do not propose style changes to code on which you are working on,
    unless you can significantly improve the code this way.

    Rationale:  Style changes (outside of drivers/staging/) tend to worsen
    rather than improve code.  Style changes obfuscate code history.  Style
    changes are of doubtfule value but create nonzero work for those who
    handle the code together with you or after you.

  - checkpatch.pl is a help for you to evaluate stylistic conformance of
    your own new code submissions.  Use checkpatch.pl for its original
    purpose.
-- 
Stefan Richter
-=====-====- =--= ---=-
http://arcgraph.de/sr/