Re: [PATCH 2/4] cfg80211: Add new NL80211_CMD_SET_BTCOEX_PRIORITY to support BTCOEX

[Tamizh chelvam <tamizhchelvam@codeaurora.org>]

Hi Johannes,

Thanks for the comments.

On 2016-12-05 20:19, Johannes Berg wrote:
> On Tue, 2016-11-08 at 18:45 +0530, c_traja@qti.qualcomm.com wrote:
>>  
>> + * struct cfg80211_btcoex_priority - BTCOEX support frame type
>> + *
>> + * This structure defines the driver supporting frame types for
>> BTCOEX
>> + *
>> + * @wlan_be_preferred: best effort frames preferred over bt traffic
>> + * @wlan_bk_preferred: background frames preferred over bt traffic
>> + * @wlan_vi_preferred: video frames preferred over bt traffic
>> + * @wlan_vo_preferred: voice frames preferred over bt traffic
>> + * @wlan_beacon_preferred: beacon preferred over bt traffic
>> + * @wlan_mgmt_preferred: management frames preferred ovet bt traffic
> 
> typo: over
> 
Okay
>>  
>>  /**
>> + * wiphy_btcoex_support_flags
>> + *	This enum has the driver supported frame types for BTCOEX.
>> + * @WIPHY_WLAN_BE_PREFERRED - Supports Best Effort frame for BTCOEX
>> + * @WIPHY_WLAN_BK_PREFERRED - supports Background frame for BTCOEX
>> + * @WIPHY_WLAN_VI_PREFERRED - supports Video frame for BTCOEX
>> + * @WIPHY_WLAN_VO_PREFERRED - supports Voice frame for BTCOEX
>> + * @WIPHY_WLAN_BEACON_PREFERRED - supports Beacon frame for BTCOEX
>> + * @WIPHY_WLAN_MGMT_PREFERRED - supports Management frames for
>> BTCOEX.
>> + */
> 
> That's not making much sense to me?
> 

is it fine to have as WIPHY_BTCOEX_BE_PREFERRED ?

>> +/**
>> + * enum wiphy_btcoex_priority - BTCOEX priority level
>> + *	This enum defines priority level for BTCOEX
>> + * WIPHY_WLAN_PREFERRED_LOW - low priority frames over BT traffic
>> + * WIPHY_WLAN_PREFERRED_HIGH - high priority frames over BT traffic
>> + */
>> +
>> +enum wiphy_btcoex_priority {
>> +	WIPHY_WLAN_PREFERRED_LOW = false,
>> +	WIPHY_WLAN_PREFERRED_HIGH = true,
>> +};
> 
> That false/true seems just strange.
> 

I will just use as a enum without assigning false/true.

>> + * @btcoex_support_flags: This will have the driver supported
>> + *	frame types for BTCOEX. This value filled by using
>> + *	%enum wiphy_btcoex_support_flags while driver
>> + *	initialization.
> 
> The whole "will have" isn't really clear.
> 
>> + * @NL80211_ATTR_SET_BTCOEX_PRIORITY: nested attribute for driver
>> supporting
>> + *	the BTCOEX. When used with
>> %NL80211_CMD_SET_BTCOEX_PRIORITY it contains
>> + *	attributes according &enum nl80211_btcoex_priority to
>> indicate
>> + *	which frame has high priority over BT.
> 
> There should be no "SET" in there.
> 
Okay sure.

>>  /**
>> + * enum nl80211_btcoex_priority - BTCOEX parameter attributes
>> + *	This strcuture has enum values for driver supported wlan
>> + *	frame type for BTCOEX.
>> + * @NL80211_WLAN_BE_PREFERRED - Best Effort frame
>> + * @NL80211_WLAN_BK_PREFERRED - Background frame
>> + * @NL80211_WLAN_VI_PREFERRED - Video frame
>> + * @NL80211_WLAN_VO_PREFERRED - Voice frame
>> + * @NL80211_WLAN_BEACON_PREFERRED - BEACON frame
>> + * @NL80211_WLAN_MGMT_PREFERRED - MGMT frame
>> + */
>> +
>> +enum nl80211_btcoex_priority {
>> +	__NL80211_WLAN_PREFERRED_INVALID,
>> +	NL80211_WLAN_BE_PREFERRED,
>> +	NL80211_WLAN_BK_PREFERRED,
>> +	NL80211_WLAN_VI_PREFERRED,
>> +	NL80211_WLAN_VO_PREFERRED,
>> +	NL80211_WLAN_BEACON_PREFERRED,
>> +	NL80211_WLAN_MGMT_PREFERRED,
>> +	__NL80211_WLAN_PREFERRED_LAST,
>> +	NL80211_WLAN_PREFERRED_MAX =
>> +			__NL80211_WLAN_PREFERRED_LAST - 1,
>> +};
> 
> Wouldn't a bitmap be easier?
> 
since this is to distinguish between different btcoex priorities and we 
are not going to do any manipulations on these parameters.
It is just used as flag attribute.