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