Re: [PATCH v2 01/03] wl1271: 11n Support, Add Definitions

[Shahar Levi <shahar_levi@ti.com>]

Thanks for the review.
  all fixed on v3, one commend inline.
Cheers,
Shahar

Luciano Coelho wrote:
> On Wed, 2010-10-06 at 20:08 +0200, ext Shahar Levi wrote:
>> Two acx commands: ht_capabilities & ht_information, 11n sta capabilities macro.
>>
>> Signed-off-by: Shahar Levi <shahar_levi@ti.com>
>> ---
> 
> Looks good! Some minor, mostly style-related, comments below.
> 
> 
>> diff --git a/drivers/net/wireless/wl12xx/wl1271.h b/drivers/net/wireless/wl12xx/wl1271.h
>> index 779b130..777c937 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271.h
>> +++ b/drivers/net/wireless/wl12xx/wl1271.h
>> @@ -427,7 +427,7 @@ struct wl1271 {
>>  	/* Our association ID */
>>  	u16 aid;
>>  
>> -	/* currently configured rate set */
>> +	/* currently configured rate set: bits0-15 BG, bits 16-23 MCS */
> 
> There a space missing after "bits" and I think you could use something a
> bit clearer, for example: 
> 
> 	/*
> 	 * currently configured rate set:
> 	 *	bits  0-15 - 802.11abg rates
> 	 *	bits 16-23 - 802.11n   MCS index mask
> 	*/
> 
> Maybe also a small comment saying that we support only 1 stream, thus
> only 8 bits for the MCS rates (0-7)?
> 
> 
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.h b/drivers/net/wireless/wl12xx/wl1271_acx.h
>> index ebb341d..5cf5a0d 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_acx.h
>> +++ b/drivers/net/wireless/wl12xx/wl1271_acx.h
>> @@ -964,6 +964,96 @@ struct wl1271_acx_rssi_snr_avg_weights {
>>  	u8 snr_data;
>>  };
>>  
>> +/* 11n command to the FW */
>> +/*	Name:	ACX_PEER_HT_CAP
>> +*	Desc:	Configure HT capabilities - declare the capabilities of the peer
>> +*		we are connected to.
>> +* Type:	Configuration
>> +* Access:	Write Only
>> +* Length:
>> +*/
> 
> We don't have this style of comment elsewhere, wouldn't it be better to
> use something more free format?
> 
> 
>> +struct wl1271_acx_ht_capabilities {
>> +	struct acx_header header;
>> +
>> +	/*
>> +	* bit 0 - Allow HT Operation
>> +	* bit 1 - Allow Greenfield format in TX
>> +	* bit 2 - Allow Short GI in TX
>> +	* bit 3 - Allow L-SIG TXOP Protection in TX
>> +	* bit 4 - Allow HT Control fields in TX.
>> +	*	Note, driver will still leave space for HT control in packets
>> +	*	regardless of the value of this field. FW will be responsible to
>> +	*	drop the HT field from any frame when this Bit is set to 0.
> 
> Please align the Note with the "Allow HT Control..." above to make it
> clear that the note is about bit 4.
> 
> 
>> +	* 5 - Allow RD initiation in TXOP. FW is allowed to initate RD.
>> +	*	Exact policy setting for this feature is TBD.
>> +	*	Note, this bit can only be set to 1 if bit 3 is set to 1.
> 
> "bit 5 -" instead of "5 -" and also align the "Exact policy..." and
> "Note".
> 
> 
>> +	/*
>> +	* This the maximum a-mpdu length supported by the AP. The FW may not
> 
> Upper-case the a-mpdu to A-MPDU for consistency.
> 
>> +	* exceed this length when sending A-MPDUs
>> +	*/
>> +	u8  ampdu_max_length;
>> +
>> +	/*
>> +	* This is the minimal spacing required when sending A-MPDUs to the AP
>> +	*/
>> +	u8 ampdu_min_spacing;
>> +} __packed;
>> +
>> +/*  HT Capabilites Fw Bit Mask Mapping */
>> +#define	WL1271_ACX_FW_CAP_BIT_MASK_HT_OPERATION			BIT(0)
>> +#define	WL1271_ACX_FW_CAP_BIT_MASK_GREENFIELD_FRAME_FORMAT	BIT(1)
>> +#define	WL1271_ACX_FW_CAP_BIT_MASK_SHORT_GI_FOR_20MHZ_PACKETS	BIT(2)
>> +#define	WL1271_ACX_FW_CAP_BIT_MASK_LSIG_TXOP_PROTECTION		BIT(3)
>> +#define	WL1271_ACX_FW_CAP_BIT_MASK_HT_CONTROL_FIELDS		BIT(4)
>> +#define	WL1271_ACX_FW_CAP_BIT_MASK_RD_INITIATION		BIT(5)
> 
> No need for the TABs after the #defines.
> 
> 
>> +/* Name:	ACX_HT_BSS_OPERATION
>> + * Desc:	Configure HT capabilities - AP rules for behavior in the BSS.
>> + * Type:	Configuration
>> + * Access:	Write Only
>> + * Length:
>> + */
> 
> Same as my previous comment.  No directly copied comments from TI's
> reference driver, please.
> 
> 
>> +struct wl1271_acx_ht_information {
>> +	struct acx_header header;
>> +
>> +	u8 rifs_mode;	/* Values: 0 - RIFS not allowed, 1 - RIFS allowed */
> 
> Put the comments before the value, as everywhere else.
> 
> 
>> +	u8 ht_protection;	/* Values: 0 - 3 like in spec */
> 
> Same here.
> 
> 
>> +	/*
>> +	* Values: 0 - GF protection not required, 1 - GF protection required
>> +	*/
>> +	u8 gf_protection;
> Move to wl1271_main.c, not configurable setting (HW depended).


> If the comment fits in one line, don't use the extra lines with /* and
> */.
> 
> 
>> +	/*
>> +	* Values: 0 - TX Burst limit not required, 1 - TX Burst Limit required
>> +	*/
>> +	u8 ht_tx_burst_limit;
> 
> Same here.
> 
> 
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_conf.h b/drivers/net/wireless/wl12xx/wl1271_conf.h
>> index 60c50d1..732e9aa 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_conf.h
>> +++ b/drivers/net/wireless/wl12xx/wl1271_conf.h
>> @@ -91,6 +91,10 @@ enum {
>>  	CONF_HW_RXTX_RATE_UNSUPPORTED = 0xff
>>  };
>>  
>> +#define HW_RX_HIGHEST_RATE	65

Move to wl1271_main.c, not configurable setting (HW depended).

>> +#define HW_BG_RATES_MASK	0xffff
>> +#define HW_HT_RATES_OFFSET	16

Move to wl1271.h, not configurable setting.

> 
> We preceed all the other macros with CONF_, please do the same for
> these.
> 
> 
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c b/drivers/net/wireless/wl12xx/wl1271_main.c
>> index cb18f22..bef2c24 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_main.c
>> +++ b/drivers/net/wireless/wl12xx/wl1271_main.c
>> @@ -2122,6 +2122,19 @@ static const u8 wl1271_rate_to_idx_2ghz[] = {
>>  	0                              /* CONF_HW_RXTX_RATE_1    */
>>  };
>>  
>> +/* 11n sta capabilities */
> 
> Use STA for consistency.
> 
> 
>> +#define WL12xx_HT_CAP { \
> 
> As we discussed separately, please use WL1271_HT_CAP.  Later, when we
> rename the driver, we can change it to WL12XX_HT_CAP together with
> everything else.  Let's keep everything as WL1271 for now, for
> consistency.
> 
>