Re: [PATCH 01/03] wl1271: BA Initiator support, Add Definitions

[Shahar Levi <shahar_levi@ti.com>]

Thanks for the review.
All will be fix on v2.
Two comments inline.
regards,
Shahar

Luciano Coelho wrote:
> On Thu, 2010-10-07 at 15:06 +0200, ext Shahar Levi wrote:
>> New acx cmd wl1271_acx_ba_session_policy to set BA policy to the FW.
>> Macros to use with BA setting.
>>
>> Signed-off-by: Shahar Levi <shahar_levi@ti.com>
>> ---
> 
> You can merge this patch with the next patch, no need to make the
> changes first in the header files and then in the c files, since they go
> very much hand in hand.
> 
> 
>>  drivers/net/wireless/wl12xx/wl1271_acx.h  |   16 ++++++++++++++++
>>  drivers/net/wireless/wl12xx/wl1271_conf.h |    3 +++
>>  2 files changed, 19 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.h b/drivers/net/wireless/wl12xx/wl1271_acx.h
>> index b8da1bc..6c3c7c0 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_acx.h
>> +++ b/drivers/net/wireless/wl12xx/wl1271_acx.h
>> @@ -1055,6 +1055,22 @@ struct wl1271_acx_ht_information {
>>  	u8 padding[3];
>>  } __packed;
>>  
>> +/*
>> + * BA sessen interface structure
>> + */
> 
> BA session.  Actually you can get rid of this comment, as it is obvious
> from the name of the structure that this is about BA sessions.
> 
>> +struct wl1271_acx_ba_session_policy {
>> +	struct acx_header header;
>> +	/* Mac address of: SA as receiver / RA as initiator */
>> +	u8 mac_address[ETH_ALEN];
> 
> Is this really SA for receiver and RA for initiator? Not SA and DA? Or
> TA and RA?
yes, as initiator it is the receiver address, as receiver it is source 
address. I change the comment to:
Mac address of the peer: SA as receiver / RA as initiator

> 
> 
>> +	u8 tid;			/* TID */
> 
> Comment here is unnecessary.
> 
>> +	u8 policy;		/* Enable / Disable */
> 
> Could we change policy to enable here? Then the comment can go away too,
> because the name of the element will be clear enough already.
> 
> 
>> +	u16 win_size;		/* windows size in num of packet */
> 
> "number of packets"
> 
> 
>> +	u16 inactivity_timeout;	/* as initiator inactivity timeout
>> +				 * in time units(TU) of 1024us.
>> +				 * as receiver reserved
>> +				 */
> 
> The comment style is wrong.
> 
> 
>> diff --git a/drivers/net/wireless/wl12xx/wl1271_conf.h b/drivers/net/wireless/wl12xx/wl1271_conf.h
>> index b3e608e..63a0a9a 100644
>> --- a/drivers/net/wireless/wl12xx/wl1271_conf.h
>> +++ b/drivers/net/wireless/wl12xx/wl1271_conf.h
>> @@ -94,6 +94,9 @@ enum {
>>  #define HW_BG_RATES_MASK	0xffff
>>  #define HW_HT_RATES_OFFSET	16
>>  
>> +#define BA_RECEIVER_WIN_SIZE 8
move to wl1271_acx.h, not configurable.

>> +#define BA_INACTIVITY_TIMEOUT 10000
> 
> This should be added as a real configuration, like the other stuff in
> the wl1271_conf.h file, which is then used in wl1271_main.c
> default_conf.  Please define a struct conf_ba with win_size and
> inactivity_timeout elements and set the actual values in the main file.
> 
>