On Mon, Mar 05, 2007 at 02:08:25PM +0000, Christoph Hellwig wrote: > On Sun, Mar 04, 2007 at 12:36:27PM -0300, Marcelo Tosatti wrote: > > Hi Christoph, > > > > 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. > > > > http://lists.openwall.net/netdev/2007/02/10/25 > > > > > And to be honest I'd be surprised if it's in a good shape already. > > > > Constructive comments are welcome... All comments made by Arnd (which > > were many!) have been addressed. > > Comments on that versions from a quick read over it: > > - the __le* annotation issue you mentioned in the mail is important. > We definitvely don't want any new drivers without endianess annotations, > because there are far too many endianess problems. Even more so > in a case of a driver like this one that's only tested on LE hardware > and has LE device endianess. > - things like 11d.[ch] don't have business of beeing in a driver, > this should be somewhere in common code. Most wireless drivers implement 11d internally. We certainly need to unify the management code, but thats not a merge blocker. > - 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. Then again most things using this should be inside > generic code anyway.. (and yeah, all that is because the driver > copied braindead linux-wlan-ng code that probably needs a major > revision anyway) > - there seems to be lots of tabs vs spaces messups > - please get rid of the ENTER/LEAVE macros These are useful for debugging. We have removed most of the useless ones already. > - 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 > - please get rid of all your private ioctls and iwpriv stuff > (should I add !!!! here) Rationale being? There are a bunch of device private knobs, and iwpriv is the interface for such configuration. (????) > - scan.h has very strange almost docbook comments, please convert > them to real docbook comments and actually run things through > the tools to make sure it's right. > - scan.h has vi indentation comment helpers that are contrary to > linux coding style.. > - the thread.h abstractions are really useless, opencoding them > would make the code a lot more readable. And make people > notice it's actually wrong: > o the return value from kthread_run needs to be checked > o wlan_deactivate_thread is not needed at all > o storing and checking the pid should go away > o there is no need for an additional waitqueue, you can > just use wake_up_process for kernel threads. > - most of types.h should not be there but you should be using > the types from include/linux/*80211* > - version.h shouldn't exist > - the radiotap header changes should definitively not be in > a "add a new driver" diff Other than this your comments are OK, will address them. Thanks! - 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
