On Thu, 2015-06-25 at 03:48 +0000, David Lin wrote: > Signed-off-by: David Lin <dlin@xxxxxxxxxxx> This really needs a commit message, like saying what chips it supports, perhaps where to find them, and similar. > drivers/net/wireless/Kconfig | 1 + > drivers/net/wireless/Makefile | 2 + > drivers/net/wireless/mwlwifi/Kconfig | 17 + > drivers/net/wireless/mwlwifi/Makefile | 9 + > drivers/net/wireless/mwlwifi/dev.h | 435 ++++++ > drivers/net/wireless/mwlwifi/fwcmd.c | 2328 +++++++++++++++++++++++++++++++ > drivers/net/wireless/mwlwifi/fwcmd.h | 175 +++ > drivers/net/wireless/mwlwifi/fwdl.c | 183 +++ > drivers/net/wireless/mwlwifi/fwdl.h | 27 + > drivers/net/wireless/mwlwifi/hostcmd.h | 753 ++++++++++ > drivers/net/wireless/mwlwifi/isr.c | 148 ++ > drivers/net/wireless/mwlwifi/isr.h | 26 + > drivers/net/wireless/mwlwifi/mac80211.c | 743 ++++++++++ > drivers/net/wireless/mwlwifi/mac80211.h | 25 + > drivers/net/wireless/mwlwifi/main.c | 858 ++++++++++++ > drivers/net/wireless/mwlwifi/rx.c | 523 +++++++ > drivers/net/wireless/mwlwifi/rx.h | 25 + > drivers/net/wireless/mwlwifi/sysadpt.h | 67 + > drivers/net/wireless/mwlwifi/tx.c | 836 +++++++++++ > drivers/net/wireless/mwlwifi/tx.h | 28 + > 20 files changed, 7209 insertions(+) You're missing a MAINTAINERS entry. > +++ b/drivers/net/wireless/mwlwifi/Kconfig > @@ -0,0 +1,17 @@ > +config MWLWIFI > + tristate "Marvell Wireless WiFi driver (mwlwifi)" > + depends on PCI && MAC80211 && MWIFIEX_PCIE=n Uh, what's with the exclusion of MWIFIEX_PCIE? Couldn't use a different PCI ID? I really think you need to get rid of this, otherwise people can't even *build-test* their wifi changes fully. > +++ b/drivers/net/wireless/mwlwifi/Makefile > @@ -0,0 +1,9 @@ > +obj-$(CONFIG_MWLWIFI) += mwlwifi.o > + > +mwlwifi-objs += main.o > +mwlwifi-objs += mac80211.o > +mwlwifi-objs += fwdl.o > +mwlwifi-objs += fwcmd.o > +mwlwifi-objs += tx.o > +mwlwifi-objs += rx.o > +mwlwifi-objs += isr.o Please add ccflags-y += -D__CHECK_ENDIAN__ to this file to always flag sparse errors. You have checked with sparse, right? Ok ... you clearly haven't. Please add the line above, and fix up the results. Also, you really need to try building this driver on 64-bit, it reports a good number of warnings. If you're up to it, also try running smatch on it, it reports a number of more warnings that sparse misses, like this one: mwl_rx_ring_init() warn: variable dereferenced before check 'rx_hndl->psk_buff' (see line 108) This is going to be only a very superficial review until the driver builds cleanly. I do think the firmware API stuff I was concerned about earlier looks better now though I think you went a bit overbaord with wiphy_info(), a number of those (like the beacon info one) surely are more debug messages than operational messages that should be shown all the time. > +> > /* Info for debugging > +> > */ > +> > wiphy_info(hw->wiphy, "%s ...", __func__); > +> > wiphy_info(hw->wiphy, " -->pPhysTxRing[0] = %x", > +> > > priv->desc_data[0].pphys_tx_ring); > +> > wiphy_info(hw->wiphy, " -->pPhysTxRing[1] = %x", > +> > > priv->desc_data[1].pphys_tx_ring); > +> > wiphy_info(hw->wiphy, " -->pPhysTxRing[2] = %x", > +> > > priv->desc_data[2].pphys_tx_ring); > +> > wiphy_info(hw->wiphy, " -->pPhysTxRing[3] = %x", > +> > > priv->desc_data[3].pphys_tx_ring); > +> > wiphy_info(hw->wiphy, " -->pPhysRxRing = %x", > +> > > priv->desc_data[0].pphys_rx_ring); > +> > wiphy_info(hw->wiphy, " -->numtxq %d wcbperq %d totalrxwcb %d", > +> > > SYSADPT_NUM_OF_DESC_DATA, > +> > > SYSADPT_MAX_NUM_TX_DESC, > +> > > SYSADPT_MAX_NUM_RX_DESC); Like that ... I've even read the comment :) > +> > spin_lock_irqsave(&priv->fwcmd_lock, flags) > You really only need _irqsave on the stream_lock, the others aren't used in IRQ context so don't need to disable IRQs. Perhaps BHs, haven't checked. 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
