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@xxxxxxxxx>
>
> ---
>
>
>
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
