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 <[email protected]>
---
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 <[email protected]>
>
> ---
>
>
>
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.
On Friday 17 October 2008 23:36:36 John Daiker wrote:
> - if (1 /*FIXME: the last PSpoll frame was sent successfully */ )
> + if (1) /*FIXME: the last PSpoll frame was sent successfully */ {
Well, go for this, if you're really forced to fix this. ;)
> >
> > 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.
> >
> Will do this next time with updated patch, among other things.
Thanks. Also make sure to enable all kconfig options.
(also remove the BROKEN dependencies that some options have,
if you are changing files related to these options.).
--
Greetings Michael.
On Friday 17 October 2008 23:58:28 John Daiker wrote:
> Michael Buesch wrote:
> > On Friday 17 October 2008 23:36:36 John Daiker wrote:
> >
> >> - if (1 /*FIXME: the last PSpoll frame was sent successfully */ )
> >> + if (1) /*FIXME: the last PSpoll frame was sent successfully */ {
> >>
> >
> > Well, go for this, if you're really forced to fix this. ;)
> >
> >
> >>> 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.
> >>>
> >>>
> >> Will do this next time with updated patch, among other things.
> >>
> >
> > Thanks. Also make sure to enable all kconfig options.
> > (also remove the BROKEN dependencies that some options have,
> > if you are changing files related to these options.)
> I'm assuming you're debugging and forcing PIO over DMA?
> For the BROKEN things, can I just remove 'BROKEN' from the Kconfig
> depends line?
Yeah, just enable all that stuff to make sure the binary code isn't changed.
You can just temporarly remove the "depends on BROKEN" and enable
the option. It will compile fine. It's just runtime broken.
--
Greetings Michael.
Michael Buesch wrote:
> On Friday 17 October 2008 23:36:36 John Daiker wrote:
>
>> - if (1 /*FIXME: the last PSpoll frame was sent successfully */ )
>> + if (1) /*FIXME: the last PSpoll frame was sent successfully */ {
>>
>
> Well, go for this, if you're really forced to fix this. ;)
>
>
>>> 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.
>>>
>>>
>> Will do this next time with updated patch, among other things.
>>
>
> Thanks. Also make sure to enable all kconfig options.
> (also remove the BROKEN dependencies that some options have,
> if you are changing files related to these options.)
I'm assuming you're debugging and forcing PIO over DMA?
For the BROKEN things, can I just remove 'BROKEN' from the Kconfig
depends line?
Michael,
Thanks for the feedback. Bob Copeland had mentioned on a different
patch that I compare the binaries before and after a patch. I will be
sure to do this on the patch rework. Further comments inline.
Michael Buesch wrote:
> 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 <[email protected]>
>>
>> ---
>>
>>
>>
>>
>
> 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.
>
How about this:
- if (1 /*FIXME: the last PSpoll frame was sent successfully */ )
+ if (1) { /*FIXME: the last PSpoll frame was sent successfully */
or this?
- if (1 /*FIXME: the last PSpoll frame was sent successfully */ )
+ if (1) /*FIXME: the last PSpoll frame was sent successfully */ {
I'm just not a fan of putting the comment inside the if conditional.
Your argument about losing the precise meaning of the comment is
understood, but I think there is a middleground here. I would hope that
any 'if (1)' line would immediately raise the 'look for a comment' flag
in someone's brain. :P
>> - 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.
>
I'll strip these out of the next patch. I agree this is a weird rule,
but checkpatch.pl did all the work. I'll put more brainpower behind the
next patch, haha.
>
>> -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.
>
I'm certainly not trying to start a flamewar, either, so thanks for
keeping the torches at bay! :) AFAIK, this is accepted (and preferred?)
kernel style, so it will be included in the updated patch. At least
until I see more vigorous pushback.
>
> 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.
>
Will do this next time with updated patch, among other things.
Thanks,
JD