Hi Johannes, > -----Original Message----- > From: linux-wireless-owner@xxxxxxxxxxxxxxx [mailto:linux-wireless-owner@xxxxxxxxxxxxxxx] On Behalf Of > Johannes Berg > Sent: Friday, December 03, 2010 4:53 AM > To: Bing Zhao > Cc: linux-wireless@xxxxxxxxxxxxxxx; John W. Linville; Dan Williams; David Woodhouse; Amitkumar Karwar; > Kiran Divekar; Greg KH; David S. Miller > Subject: Re: [PATCH 00/31] wireless: Marvell mwifiex driver for SD8787 > > On Wed, 2010-12-01 at 17:31 -0800, Bing Zhao wrote: > > The mwifiex driver for Marvell 802.11n chipset SD8787 is available at > > git://git.marvell.com/mwifiex.git. Both STA mode and Mobile hotspot > > mode are supported. > > > > git pull git://git.marvell.com/mwifiex.git master > > Please post patches, not git trees. > > But since John is holding us somewhat hostage by saying he'll consider > merging this, and apparently you don't have the taste to even consider > the review comments you have gotten before, I took a brief look to > *prove* that this driver cannot be merged as is. > > > Here's a short list of things to fix: > * __packed > * ENTER/LEAVE must go > * enum mwifiex_status must go -- use errno > * private ioctls were grandfathered in, > but aren't allowed in new drivers > * do you really need your own crypto mode definitions? > * your pseudo-kerneldoc comments "/**" don't conform > to the right format > * private locking wrappers are not allowed (mwifiex_spin_lock etc) > * private debugging like PRINTM is not allowed Is it OK if I change PRINTM (debugging printk) to something like IWL_DEBUG_? <drivers/net/wireless/iwlwifi/iwl-debug.h> ...... #define IWL_DEBUG_INFO(p, f, a...) IWL_DEBUG(p, IWL_DL_INFO, f, ## a) #define IWL_DEBUG_MAC80211(p, f, a...) IWL_DEBUG(p, IWL_DL_MAC80211, f, ## a) #define IWL_DEBUG_MACDUMP(p, f, a...) IWL_DEBUG(p, IWL_DL_MACDUMP, f, ## a) #define IWL_DEBUG_TEMP(p, f, a...) IWL_DEBUG(p, IWL_DL_TEMP, f, ## a) #define IWL_DEBUG_SCAN(p, f, a...) IWL_DEBUG(p, IWL_DL_SCAN, f, ## a) #define IWL_DEBUG_RX(p, f, a...) IWL_DEBUG(p, IWL_DL_RX, f, ## a) #define IWL_DEBUG_TX(p, f, a...) IWL_DEBUG(p, IWL_DL_TX, f, ## a) #define IWL_DEBUG_ISR(p, f, a...) IWL_DEBUG(p, IWL_DL_ISR, f, ## a) ...... Or, you want me to remove PRINTM stuff completely? Thanks, Bing > * there's exactly one space after "#include" > * struct eth_803_hdr? are you serious? and you even have > it twice: struct eth_II_hdr > * enum ieee_types_elementid_e must go > * similarly, almost all of ieee.h > * no bitfields and BIG_ENDIAN_BITFIELD allowed > * not all allocations are GFP_ATOMIC, use wisely and > GFP_KERNEL in most places > * a wifi driver that's doing stuff with sockets (nl_sk, sk_socket)? > explain, in MUCH detail (or preferably just get rid of it) > * there's debugfs_remove_recursive > * some functions like mwifiex_is_network_compatible need to be broken > into smaller ones to avoid the crap indentation they have > * mwifiex_uap_ap_cfg_parse_data. Must I say more? > * struct mwifiex_buffer *mbuf? Is this a BSD driver? We have SKBs, get > rid of this. (Oh and skbs have a CB) > * mwifiex_init_lock/mwifiex_free_lock -- I'd laugh in your general > direction if it wasn't so sad > * mwifiex_util_peek_list etc.: learn about list.h > * mwifiex_init_timer -- still sad, dynamically allocating timers? just > like spinlocks, they can be embedded and make your code simpler > > Ok good enough, that should keep you busy for a while... > > The above is might be a good basis for a TODO file for staging... > > johannes > > -- > 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 ÿô.nlj·Ÿ®‰†+%ŠË±é¥Šwÿº{.nlj·¥Š{±ÿ«zW¬³ø¡Ü}©ž²ÆzÚj:+v‰¨þø®w¥þŠàÞ¨è&¢)ß«a¶Úÿûz¹ÞúŽŠÝjÿŠwèf
