Hi Johannes, Thank you so much for your comments! > -----Original Message----- > From: Johannes Berg [mailto:johannes@xxxxxxxxxxxxxxxx] > Sent: Friday, December 03, 2010 10:39 AM > To: Frank Huang > Cc: Bing Zhao; linux-wireless@xxxxxxxxxxxxxxx > Subject: RE: [PATCH 00/31] wireless: Marvell mwifiex driver for SD8787 > > On Fri, 2010-12-03 at 09:01 -0800, Frank Huang wrote: > > > [snip] > > Mind you, there will definitely be more things to change in this > driver ... that was just the tip of the iceberg really. > > Also, there is a lot of stuff talking of "ioctl". I initially wrongly > assumed you were using wireless extensions or something -- I apologise > for that misconception (but you'll have to agree that it was an easy > mistake to make!). However, the "ioctl" layer in the driver needs to be > _dissolved_ entirely, there's no real reason why for example > mwifiex_set_rf_channel() shouldn't directly call into the functions set > that the channel, rather than allocating an "ioctl" request and calling > it. > > A better exmaple might be mwifiex_set_tx_power -- it calls > mwifiex_request_ioctl, which calls mwifiex_ioctl, which calls a > mode-dependent ioctl handler, which is, say, mwifiex_ops_sta_ioctl -- > which then parses the arguments and calls mwifiex_power_ioctl. This in > turn calls mwifiex_power_ioctl_set_power, which finally does something > like allocating a command. > > Almost *everything* described in that paragraph should be dissolved -- > mwifiex_set_tx_power() should call a function pointer like > ops.set_tx_power which is set to a function similar to > mwifiex_power_ioctl_set_power, but that similar function should probably > wait for the command to be processed by the device as well. > > There's really no need to marshal parameters and settings a few times > before they arrive at the hardware! You are right. These ioctls shouldn't be necessary. I will put them on my list. > > A more minor thing: there are a lot of static forward declarations that > aren't really necessary if you shuffle the code a bit -- that would be > appreciated. > Yes, this will be taken care of too. > > On a broader view, I would suggest that you stop thinking of your driver > as special. Clearly, it is dear to you, as are everyone's drivers to > them, but in the overall ecosystem it should fit in with a minimum of > fuzz, and not require special tools or special programming. The custom > wireless events, for example, are entirely useless to existing > applications. Similarly, NETLINK_MARVELL (not to mention the abuse of > the netlink system a driver presents that unilaterally declares that a > given netlink ID is reserved for it without making that known to the > kernel at large). John's posted patches that remove the wireless events, including NETLINK_MARVELL. > > > All this said, though, don't think I'm entirely negative. I'm happy > there's a driver, and that you're starting to engage with the community > (although I don't quite understand why your response was private). And > the driver code is at least clean -- I may not agree with how it works, > but at least I don't have to run it through Lindent to protect myself > from eye cancer :) Thanks! We will continue our efforts to make sure that the submitted patches are clean. > > Mind you, I haven't done a cfg80211 API review at all. I'll do that when > the code flow is easier to follow and not hidden in multiple abstraction > layers and "ioctls". I appreciate it. We will make the changes first. Best regards, Bing > > johannes ÿô.nlj·Ÿ®‰†+%ŠË±é¥Šwÿº{.nlj·¥Š{±ÿ«zW¬³ø¡Ü}©ž²ÆzÚj:+v‰¨þø®w¥þŠàÞ¨è&¢)ß«a¶Úÿûz¹ÞúŽŠÝjÿŠwèf
