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 <shahar_levi@xxxxxx>
> ---
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.
--
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
