On Mon, 2007-05-07 at 11:41 +0100, Christoph Hellwig wrote:
> On Sat, Mar 03, 2007 at 05:21:40AM +0000, Christoph Hellwig wrote:
> > Umm, I can't remember the updated driver ever beeig posted for review.
> > And to be honest I'd be surprised if it's in a good shape already.
>
> Of course it's not anywhere near good shape. Almost all items from my
> review were completely ignored, and we have another totoally substandard
> wireless driver with crappy thread handling, a huge number of broken private
> ioctls and partially absymal codingstyle.
- things like 11d.[ch] don't have business of being in a driver,
this should be somewhere in common code.
There is no common code for this. mac80211 will eventually grow a
utility library that libertas will hopefully be able to attach to.
- please get rid of the ENTER/LEAVE macros
People have said that many drivers do this, and it's quite useful for
debugging.
- please get rid of all your private ioctls and iwpriv stuff
(should I add !!!! here)
Any ioctls that have equivalents in WEXT or ethtool can certainly be
removed. Mesh ones like fwt_reset, bt_reset, mesh_get_ttl, etc have no
current equivalents and will need to stick around. I've already removed
all the ones that were relevant for WPA and moved them to standard WEXT
calls.
- most of types.h should not be there but you should be using
the types from include/linux/*80211*
I've already updated libertas-2.6 git with a ton of updates for this.
In any case, lets push off any merge until 2.6.23 so the rest of the
comments can be dealt with:
- the __le* annotation issue you mentioned in the mail is important.
- there shouldn't be a LICENSE file in individual driver directories,
especially if it's just plain old GPLv2.
- please get rid of setting -DFOO flags in the Makefile, just use
these directly as config symbols.
- there shouldn't be a README file in the driver directory, this
should be in Documentation/
- please don't use wlan_* foo types. a) this should be structs, not
typedefs, and b) wlan is an utterly generic name for beeing inside
a driver.
- there seems to be lots of tabs vs spaces messups
- there's an awful lot of headers without clear divided
responsibilities,
there should be only a few ones left (internal interfaces and hw
interface basically)
- having lowercase names for lots of hw commands is a very bad idea
for readability
- etc
Dan
> And I didn't even get a reply to my mail addressing the concerns above.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
