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:

quoted

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html