Message-ID: <4CB486D8.5080506@ti.com>
Date: Tue, 12 Oct 2010 18:03:36 +0200
From: Shahar Levi <shahar_levi@ti.com>
MIME-Version: 1.0
To: Luciano Coelho <luciano.coelho@nokia.com>
CC: "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH v3 03/03] wl1271: 11n Support, functionality and configuration
 ability
References: <1286812038-19013-1-git-send-email-shahar_levi@ti.com>	 <1286812038-19013-4-git-send-email-shahar_levi@ti.com> <1286893095.12528.75.camel@chilepepper>
In-Reply-To: <1286893095.12528.75.camel@chilepepper>
Content-Type: text/plain; charset=utf-8; format=flowed
Sender: linux-wireless-owner@vger.kernel.org

Thanks for the review. All below will be fix in v4.
Cheers,
Shahar

Luciano Coelho wrote:
> 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.
> 
>