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. - 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 - 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) - 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 - 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
