Re: [PATCH v3 03/03] wl1271: 11n Support, functionality and configuration ability

[Luciano Coelho <luciano.coelho@nokia.com>]

On Mon, 2010-10-11 at 17:47 +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@ti.com>
> ---

...

> @@ -1709,6 +1721,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;
> @@ -1927,6 +1940,45 @@ static void wl1271_op_bss_info_changed(struct ieee80211_hw *hw,
>  		}
>  	}
>  
> +	if (sta) {
> +		/*
> +		 * Takes care of: New association with HT enable,
> +		 *                HT information change in beacon.
> +		 */
> +		if ((changed & BSS_CHANGED_HT) &&
> +		    (bss_conf->channel_type != NL80211_CHAN_NO_HT)) {

You could move the if (sta) into the same if, as I proposed before:

	if (sta && changed & BSS_CHANGED_HT) &&
	    (bss_conf->channel_type != NL80211_CHAN_NO_HT)) {



> +			ret = wl1271_acx_set_ht_capabilities(wl,
> +					&sta->ht_cap,
> +					true);

...then you don't need to break this line so much...


> +			if (ret < 0) {
> +				wl1271_warning("Set ht cap true failed %d",
> +						ret);

...nor this...

> +				goto out_sleep;
> +			}
> +
> +			ret = wl1271_acx_set_ht_information(wl,
> +					bss_conf->ht_operation_mode);
> +			if (ret < 0) {
> +				wl1271_warning("Set ht information failed %d",
> +						ret);

...nor this...


> +		/*
> +		 * Takes care of: New association without HT,
> +		 *                Disassociate.
> +		 */

Disassociation.


> +		else if (changed & BSS_CHANGED_ASSOC) {

Same in this block about the if (sta) thingy:

	if (sta && changed & BSS_CHANGED_ASSOC) {


> @@ -2107,14 +2158,14 @@ static struct ieee80211_channel wl1271_channels[] = {
>  /* mapping to indexes for wl1271_rates */
>  static const u8 wl1271_rate_to_idx_2ghz[] = {
>  	/* MCS rates are used only with 11n */
> -	CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS7 */
> -	CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS6 */
> -	CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS5 */
> -	CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS4 */
> -	CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS3 */
> -	CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS2 */
> -	CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS1 */
> -	CONF_HW_RXTX_RATE_UNSUPPORTED, /* CONF_HW_RXTX_RATE_MCS0 */
> +	7,                            /* CONF_HW_RXTX_RATE_MCS7 */
> +	6,                            /* CONF_HW_RXTX_RATE_MCS6 */
> +	5,                            /* CONF_HW_RXTX_RATE_MCS5 */
> +	4,                            /* CONF_HW_RXTX_RATE_MCS4 */
> +	3,                            /* CONF_HW_RXTX_RATE_MCS3 */
> +	2,                            /* CONF_HW_RXTX_RATE_MCS2 */
> +	1,                            /* CONF_HW_RXTX_RATE_MCS1 */
> +	0,                            /* CONF_HW_RXTX_RATE_MCS0 */
>  
>  	11,                            /* CONF_HW_RXTX_RATE_54   */
>  	10,                            /* CONF_HW_RXTX_RATE_48   */
> @@ -2137,6 +2188,7 @@ static const u8 wl1271_rate_to_idx_2ghz[] = {
>  /* 11n STA capabilities */
>  #define HW_RX_HIGHEST_RATE	65
>  
> +#ifdef CONFIG_WL1271_HT
>  #define WL1271_HT_CAP { \
>  	.cap = IEEE80211_HT_CAP_GRN_FLD | IEEE80211_HT_CAP_SGI_20, \
>  	.ht_supported = true, \
> @@ -2148,6 +2200,11 @@ static const u8 wl1271_rate_to_idx_2ghz[] = {
>  		.tx_params = IEEE80211_HT_MCS_TX_DEFINED, \
>  		}, \
>  }
> +#else
> +#define WL1271_HT_CAP { \
> +	.ht_supported = false, \
> +}
> +#endif

Would be nicer to have the ifdefs already when you create this macro
(ie. patch 01/03), but you don't have the CONFIG flag at that point
yet... but well, never mind.  No need to change it.


-- 
Cheers,
Luca.