Re: [PATCH] b43: reduce checkpatch.pl errors

[Michael Buesch <mb@bu3sch.de>]

On Friday 17 October 2008 21:16:09 John Daiker wrote:
> A few changes to reduce checkpatch.pl errors in the b43 driver.  For the 
> most part, I only fixed cosmetic things, and left the actual 'code flow' 
> untouched (hopefully)!
> 
> Diff is against wireless-testing HEAD.
> 
> Signed-off-by: John Daiker <daikerjohn@gmail.com>
> 
> ---
> 
> 
> 

Looks good, except these:

> -		if (1 /*FIXME: the last PSpoll frame was sent successfully */ )
> +		if (1) {
> +			/*FIXME: the last PSpoll frame was sent successfully */

Makes it a lot less obvious what the comment is talking about.
Please leave this untouched.

> -		if (!gphy->aci_enable && 1 /*TODO: not scanning? */ ) {
> -			if (0 /*TODO: bunch of conditions */ ) {
> +		if (!gphy->aci_enable && 1) {
> +			/* TODO: not scanning? */
> +			if (0) {
> +				/*TODO: bunch of conditions */

Same here.

There are probably more of these. I didn't check the whole patch.

> -	//TODO
> +	/* TODO */

Well, that's only generating noise in git and results in merge conflicts
and stuff. I do only use // style comments for temporary comments.
IMO this is OK.

> -char * b43_rfkill_led_name(struct b43_wldev *dev)
> +char *b43_rfkill_led_name(struct b43_wldev *dev)

Well, in my opinion it looks bad to glue the * to the function name.
The pointer * is not related to the function name, but to the return value.

(Note that for variable definitions this is different and I agree that
we should put the * in front of the variable name without spaces)

Well, but I'm not going to create a flamewar for this.
If this is kernel coding style, feel free to change the code.


One additional thing I'd like you to do.
Do a b43 compile before and after applying the patch.
Keep the b43.ko files for both runs and do an md5sum on them.
Add the results to the commit log. The sums _must_ match.
If they don't, please send the changes that change the actual
binary code in seperate patches.

-- 
Greetings Michael.