Marvell Libertas 8388 802.11b/g USB driver (v2)
Subsystems (2)
Resending, apparently the first message got dropped somehow...
From: Marcelo Tosatti <[email protected]>
Date: Thu, 18 Jan 2007 13:02:13 -0200
To: Jiri Benc <[email protected]>
Cc: Dan Williams <[email protected]>, Marcelo Tosatti <[email protected]>,
netdev <[email protected]>, Jeff Garzik <[email protected]>, "John W. Linville" <[email protected]>,
"Luis R. Rodriguez" <[email protected]>,
Arnd Bergmann <[email protected]>,
Arnaldo Carvalho de Melo <[email protected]>
In-Reply-To: <[email protected]>
Subject: Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)
Hi Jiri,
Quoted 9 lines · Jiri Benc
Quoted 2 lines · Dan Williams
But could we please be constructive here, and actually take a look at the driver and point out problems?
Just from a quick glance:
-
drivers/net/wireless/libertas/radiotap.hfile with a lot of constants already defined elsewhere (and not respecting kernel coding style, either)
Such as? The bit definitions (IEEE80211_RADIOTAP_F_{TX,RX}...) have been moved to include/net/ieee80211_radiotap.h in the second version of the patch. Is that what you were referring to?
As for kernel coding style in that file, can you specify what looks wrong? It appears alright to me.
Quoted 32 lines
- regulatory domain management (already in ieee80211, in progress for d80211)
- the full authentication and association state machine (or am I understanding it wrong?)And I'm omitting things like
Quoted 25 lines
(quoted diff) +25
-0+/** Double-Word(32Bit) Bit definition */ +#define DW_BIT_0 0x00000001 +#define DW_BIT_1 0x00000002 +#define DW_BIT_2 0x00000004 +#define DW_BIT_3 0x00000008 +#define DW_BIT_4 0x00000010 +#define DW_BIT_5 0x00000020 +#define DW_BIT_6 0x00000040 +#define DW_BIT_7 0x00000080 +#define DW_BIT_8 0x00000100 +#define DW_BIT_9 0x00000200 +#define DW_BIT_10 0x00000400 +#define DW_BIT_11 0x00000800 +#define DW_BIT_12 0x00001000 +#define DW_BIT_13 0x00002000 +#define DW_BIT_14 0x00004000 +#define DW_BIT_15 0x00008000 +#define DW_BIT_16 0x00010000 +#define DW_BIT_17 0x00020000 +#define DW_BIT_18 0x00040000 +#define DW_BIT_19 0x00080000 +#define DW_BIT_20 0x00100000 +#define DW_BIT_21 0x00200000 +#define DW_BIT_22 0x00400000 +#define DW_BIT_23 0x00800000
Quoted 8 lines
Review
Quoted 8 lines
(quoted diff) +8
-0+#define DW_BIT_24 0x01000000 +#define DW_BIT_25 0x02000000 +#define DW_BIT_26 0x04000000 +#define DW_BIT_27 0x08000000 +#define DW_BIT_28 0x10000000 +#define DW_BIT_29 0x20000000 +#define DW_BIT_30 0x40000000 +#define DW_BIT_31 0x80000000
Removed.
Quoted 1 line
or those ENTER and LEAVE statements in each other function,
What is the problem with it? The entry/exit debug points have been pretty useful for debugging. A bunch of in-tree drivers contain similar code.
You might argue that ENTER/LEAVE should be in lower caps?
Quoted 1 line
non-conforming coding style,
Send patches or point them out? :)
I've ran scripts/Lindent over it, but there might still be some sections in need of love.
Quoted 3 lines
comments like "All rights reserved" or
"This software file (the "File") is distributed by Marvell International Ltd.",
Removed them all, moved the Marvell copyright + GNU header to LICENSE file.
Quoted 1 line
a lot of duplicated code, etc.
Again, please point it out.
Quoted 1 line
I stopped looking t it in the half, asorry.
Thanks for your comments so far!
----- End forwarded message -----
Hi Jiri,
Quoted 7 lines · Marcelo Tosatti
Quoted 6 lines · Jiri Benc
Quoted 2 lines · Dan Williams
But could we please be constructive here, and actually take a look at the driver and point out problems?
Just from a quick glance:
Quoted 7 lines
Quoted 3 lines
-
drivers/net/wireless/libertas/radiotap.hfile with a lot of constants already defined elsewhere (and not respecting kernel coding style, either)Such as? The bit definitions (IEEE80211_RADIOTAP_F_{TX,RX}...) have been moved to
include/net/ieee80211_radiotap.hin the second version of the patch. Is that what you were referring to?
I also converted the last piece of code which could be shared from generic Linux code in radiotap.h, which is "struct ieee80211_hdr_4addr".
Can you go over it again and point any problems you might see?
Quoted 2 lines
As for kernel coding style in that file, can you specify what looks wrong? It appears alright to me.
Again, coding style looks fine to me.
Quoted 4 lines
Quoted 4 lines
- regulatory domain management (already in ieee80211, in progress for d80211)
- the full authentication and association state machine (or am I understanding it wrong?)