Return-path: Received: from smtp.nokia.com ([192.100.105.134]:23139 "EHLO mgw-mx09.nokia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753157Ab0JHJGD (ORCPT ); Fri, 8 Oct 2010 05:06:03 -0400 Subject: Re: [PATCH v2 03/03] wl1271: 11n Support, functionality and configuration ability From: Luciano Coelho To: ext Shahar Levi Cc: "linux-wireless@vger.kernel.org" In-Reply-To: <1286388491-28752-5-git-send-email-shahar_levi@ti.com> References: <1286388491-28752-1-git-send-email-shahar_levi@ti.com> <1286388491-28752-5-git-send-email-shahar_levi@ti.com> Content-Type: text/plain; charset="UTF-8" Date: Fri, 08 Oct 2010 12:05:36 +0300 Message-ID: <1286528736.21349.42.camel@chilepepper> Mime-Version: 1.0 Sender: linux-wireless-owner@vger.kernel.org List-ID: On Wed, 2010-10-06 at 20:08 +0200, ext Shahar Levi wrote: > Add 11n ability in scan, connection and using MCS rates. > The configuration is temporary due to the code incomplete and > still in testing process. That plans to be remove in the future. > > Signed-off-by: Shahar Levi > --- Some comments below. Again, mostly style-related. > diff --git a/drivers/net/wireless/wl12xx/Makefile b/drivers/net/wireless/wl12xx/Makefile > index 0d334d6..92465c8 100644 > --- a/drivers/net/wireless/wl12xx/Makefile > +++ b/drivers/net/wireless/wl12xx/Makefile > @@ -19,3 +19,4 @@ obj-$(CONFIG_WL1271_SDIO) += wl1271_sdio.o > > # small builtin driver bit > obj-$(CONFIG_WL12XX_PLATFORM_DATA) += wl12xx_platform_data.o > + I guess this extra empty line was included accidentally here? There are no other changes in the Makefile, so please remove this change. > diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c b/drivers/net/wireless/wl12xx/wl1271_main.c > index bef2c24..7dd1257 100644 > --- a/drivers/net/wireless/wl12xx/wl1271_main.c > +++ b/drivers/net/wireless/wl12xx/wl1271_main.c > @@ -841,10 +841,21 @@ static int wl1271_op_tx(struct ieee80211_hw *hw, struct sk_buff *skb) > > /* peek into the rates configured in the STA entry */ > spin_lock_irqsave(&wl->wl_lock, flags); > - if (sta && sta->supp_rates[conf->channel->band] != wl->sta_rate_set) { > + if (sta && > + (sta->supp_rates[conf->channel->band] != > + (wl->sta_rate_set & HW_BG_RATES_MASK))) { > wl->sta_rate_set = sta->supp_rates[conf->channel->band]; Shouldn't this be |= here like the one below? Otherwise the HT rates will be zeroed. Okay, this would be fixed below, because the zeroed HT rates will not match the configured rates, but in some cases that will be unnecessary. > set_bit(WL1271_FLAG_STA_RATES_CHANGED, &wl->flags); > } > + > + if (sta && > + sta->ht_cap.ht_supported && > + ((wl->sta_rate_set >> HW_HT_RATES_OFFSET) != > + sta->ht_cap.mcs.rx_mask[0])) { > + wl->sta_rate_set |= > + (sta->ht_cap.mcs.rx_mask[0] << HW_HT_RATES_OFFSET); > + set_bit(WL1271_FLAG_STA_RATES_CHANGED, &wl->flags); > + } > spin_unlock_irqrestore(&wl->wl_lock, flags); > > /* queue the packet */ > @@ -1697,6 +1708,7 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw, > { > enum wl1271_cmd_ps_mode mode; > struct wl1271 *wl = hw->priv; > + struct ieee80211_sta *sta = ieee80211_find_sta(vif, bss_conf->bssid); > bool do_join = false; > bool set_assoc = false; > int ret; > @@ -1915,6 +1927,21 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw, > } > } > > + if (sta) { > + if (changed & BSS_CHANGED_HT) { > + ret = wl1271_acx_set_ht_capabilities(wl, > + &sta->ht_cap, > + true); > + ret = wl1271_acx_set_ht_information(wl, > + bss_conf->ht_operation_mode); > + } > + else > + if (changed & BSS_CHANGED_ASSOC) > + ret = wl1271_acx_set_ht_capabilities(wl, > + &sta->ht_cap, > + false); > + } > + The closing brace is indented wrongly here. And you should also use braces in the else block (see Documentation/CodingStyle). But in any case, you could reduce the indentation and prevent unaligned parameters in the wl1271_acx_* calls if you do this: if (sta && changed & BSS_CHANGED_HT) { and if (sta && changed & BSS_CHANGED_ASSOC) > @@ -2255,6 +2285,7 @@ static struct ieee80211_supported_band wl1271_band_5ghz = { > .n_channels = ARRAY_SIZE(wl1271_channels_5ghz), > .bitrates = wl1271_rates_5ghz, > .n_bitrates = ARRAY_SIZE(wl1271_rates_5ghz), > + .ht_cap = WL12xx_HT_CAP, > }; You forgot the #ifdef around .ht_cap here. Actually, it might be nicer to put the #ifdef around the macro itself, so you can explicitly set the .ht_supported flag to false if 11n is not configured. > static const u8 *wl1271_band_rate_to_idx[] = { > diff --git a/drivers/net/wireless/wl12xx/wl1271_rx.c b/drivers/net/wireless/wl12xx/wl1271_rx.c > index 94da5dd..109a470 100644 > --- a/drivers/net/wireless/wl12xx/wl1271_rx.c > +++ b/drivers/net/wireless/wl12xx/wl1271_rx.c > @@ -53,6 +53,10 @@ static void wl1271_rx_status(struct wl1271 *wl, > status->band = wl->band; > status->rate_idx = wl1271_rate_to_idx(wl, desc->rate); > > + /* 11n support */ > + if (desc->rate <= CONF_HW_RXTX_RATE_MCS0) > + status->flag |= RX_FLAG_HT; > + > status->signal = desc->rssi; Should this be #ifdef'ed also? > /* > diff --git a/drivers/net/wireless/wl12xx/wl1271_tx.c b/drivers/net/wireless/wl12xx/wl1271_tx.c > index 1b8295c..af54fef 100644 > --- a/drivers/net/wireless/wl12xx/wl1271_tx.c > +++ b/drivers/net/wireless/wl12xx/wl1271_tx.c > @@ -236,6 +236,15 @@ u32 wl1271_tx_enabled_rates_get(struct wl1271 *wl, u32 rate_set) > rate_set >>= 1; > } > > + /* MCS rates indication are on bits 16 - 23 */ > + rate_set >>= HW_HT_RATES_OFFSET - band->n_bitrates; > + > + for (bit = 0; bit < 8; bit++) { > + if (rate_set & 0x1) > + enabled_rates |= (CONF_HW_BIT_RATE_MCS_0 << bit); > + rate_set >>= 1; > + } > + And this? #ifdef here as well, maybe? -- Cheers, Luca.