Re: [PATCH v2] ath10k: extend debug code for RX path

[Kalle Valo <kvalo@qca.qualcomm.com>]

Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:

> On 28 July 2014 08:35, Varka Bhadram <varkabhadram@gmail.com> wrote:
>> On 07/28/2014 11:56 AM, Janusz Dziedzic wrote:
>>>
>>> Print sequence number, AMSDU_MORE flag and AC when additional
>>> debug enabled in RX path. This is usefull for debugging purpose.
>>>
>>> Signed-off-by: Janusz Dziedzic<janusz.dziedzic@tieto.com>
>>> ---
>>>   drivers/net/wireless/ath/ath10k/htt_rx.c |   41
>>> ++++++++++++++++++++++++++++--
>>>   1 file changed, 39 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c
>>> b/drivers/net/wireless/ath/ath10k/htt_rx.c
>>> index d80fcda..f7c8021 100644
>>> --- a/drivers/net/wireless/ath/ath10k/htt_rx.c
>>> +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
>>> @@ -818,19 +818,55 @@ static bool ath10k_htt_rx_h_channel(struct ath10k
>>> *ar,
>>>         return true;
>>>   }
>>>   +static const char * const tid_to_ac[] = {
>>
>>
>> No space required '*' and const.
>
> Seems checkpatch didn't find this.

>From a quick grep I saw that kernel has both "* const" and "*const". I
would say keep it as it is.

>>> +static char *ath10k_get_tid(struct ieee80211_hdr *hdr, char *out, size_t
>>> size)
>>> +{
>>> +       u8 *qc;
>>> +       int tid;
>>> +
>>> +       if (!ieee80211_is_data_qos(hdr->frame_control))
>>> +               return "";
>>
>>
>> return NULL would be more readable...?
>>
> We don't need extra "(null)" in printf, empty string is what I expect
> here (same as other params in this dbg info).
> Anyway I can return "out" here and initialize as empty before calling
> ath10k_get_tid().

IMHO return "" is more readable here.

>>> +                  is_multicast_ether_addr(ieee80211_get_DA(hdr)) ?
>>> +                                                       "mcast" : "ucast",
>>
>>
>> should match open paranthesis...
>>
>> is_multicast_ether_addr(ieee80211_get_DA(hdr)) ?
>>
>>                         "mcast" : "ucast",
>>
> Could you describe this more (I didn't find clear description about this)?

AFAICS there is no open parenthesis here to which align the indentation.

-- 
Kalle Valo