From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michal Kazior <michal.kazior@tieto.com>
Date: Thu, 9 May 2013 10:07:59 +0200
Subject: [ath9k-devel] [PATCH 3/3] ath10k: enable STA UAPSD support
In-Reply-To: <3078A9B976EF864C8DDD0C499FFD07913139B7B8C3@EXMB01.eu.tieto.com>
References: <1367999363-20424-1-git-send-email-janusz.dziedzic@tieto.com>
	<1367999363-20424-4-git-send-email-janusz.dziedzic@tieto.com>
	<8738twy647.fsf@kamboji.qca.qualcomm.com>
	<3078A9B976EF864C8DDD0C499FFD07913139B7B8C3@EXMB01.eu.tieto.com>
Message-ID: <518B595F.4070908@tieto.com>
List-Id: <ath9k-devel.lists.ath9k.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: ath9k-devel@lists.ath9k.org

On 09/05/13 09:47, Janusz.Dziedzic at tieto.com wrote:
>> -----Original Message-----
>> From: Kalle Valo [mailto:kvalo at qca.qualcomm.com]
>> Sent: 9 maja 2013 09:17
>> To: Dziedzic Janusz
>> Cc: ath9k-devel at lists.ath9k.org
>> Subject: Re: [ath9k-devel] [PATCH 3/3] ath10k: enable STA UAPSD support
>>
>> Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:
>>
>>> Enable UAPSD support for STA mode.
>>>
>>> Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>
>>
>> Same here, please explain briefly what you have tested.
>>
>>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>>> @@ -2296,6 +2296,68 @@ static int ath10k_sta_state(struct ieee80211_hw
>> *hw,
>>>   	return ret;
>>>   }
>>>
>>> +#define SET_UAPSD(enable, out, flags) do {	\
>>> +	if ((enable))				\
>>> +		(out) |= (flags);		\
>>> +	else					\
>>> +		(out) &= ~(flags);		\
>>> +} while (0)
>>
>> Why the macro? To workaround a long line warning from checkpatch?
>>
>> If that's a problem we could increate line length limit, for example to
>> 85 or 90. In some cases the 80 char limit is a bit too excessive. Would that
>> help?
>
> Not really. This is because we have to set/clean this flags for every ACs.
> So, this is just to prevent code duplication.
>
>          case IEEE80211_AC_VO:
>                 if (enable)
>                          ...
>                 else
>                        ....
>                 break;
>         case IEEE80211_AC_VI:
>                 if (enable)
>                        ....
>                  else
>                       ....
>                 break;
>          .....
>
> I can remove this macro and create inline function instead.

Hmm, but since this is per-ac can't we do the following?

switch (ac) {
case IEEE80211_AC_VO:
	val = WMI_STA_PS_UAPSD_AC3_DELIVERY_EN |
               WMI_STA_PS_UAPSD_AC3_TRIGGER_EN;
	break;
// ..
}

if (enable)
	arvif->u.sta.uapsd |= val;
else
	arvif->u.sta.uapsd &= ~val;


No macros and no code duplication.


-- Pozdrawiam / Best regards, Michal Kazior.