Re: [PATCH] ath9k: make use of slot time macros

[Felix Fietkau <nbd@openwrt.org>]

On 2011-02-14 3:02 PM, Mohammed Shafi wrote:
> On Monday 14 February 2011 07:18 PM, Felix Fietkau wrote:
>> On 2011-02-14 5:46 AM, Mohammed Shafi wrote:
>>    
>>> On Monday 14 February 2011 10:11 AM, Mohammed Shafi wrote:
>>>      
>>>> On Friday 11 February 2011 09:59 PM, John W. Linville wrote:
>>>>        
>>>>> On Fri, Feb 11, 2011 at 05:21:06PM +0100, Felix Fietkau wrote:
>>>>>          
>>>>>> On 2011-02-11 5:15 PM, John W. Linville wrote:
>>>>>>            
>>>>>>> On Fri, Feb 11, 2011 at 01:52:23PM +0100, Felix Fietkau wrote:
>>>>>>>              
>>>>>>>> On 2011-02-11 8:01 AM, Mohammed Shafi Shajakhan wrote:
>>>>>>>>                
>>>>>>>>> From: Mohammed Shafi Shajakhan<mshajakhan@atheros.com>
>>>>>>>>>
>>>>>>>>> Instead of using raw numbers to assign slot time it would be
>>>>>>>>> better to
>>>>>>>>> make use of predefined slot time macros
>>>>>>>>>                  
>>>>>>>> How does this make it better?
>>>>>>>>                
>>>>>>> Maybe if it was ATH9K_SHORT_SLOT_TIME it would make more sense?
>>>>>>>              
>>>>>> Well, neither the unit of this variable, nor the values that can be
>>>>>> used
>>>>>> are ath9k specific.
>>>>>>            
>>> Felix  then I don't know why these macros are used here and I followed
>>> the same thing:
>>>
>>> htc_drv_beacon.c 242 if (ah->slottime == ATH9K_SLOT_TIME_20)
>>> init.c           517 sc->beacon.slottime = ATH9K_SLOT_TIME_9;
>>>      
>> Just because the macros are there doesn't mean that it was a good idea
>> to use them. As far as I know, these were simply inherited from the
>> Atheros codebase that ath9k was based on.
>> I actually consider the code more readable without the redundant
>> "ATH9K_SLOT_TIME_" part.
>>    
> Felix I agree the first part, but I could still see no harm in using 
> these macros.
> Initially we  using these values 6,9,20(no other values) for the slot 
> time and there are macros defined for them. If we are using some other 
> values I would agree that its wrong.
> Why not make use of it ?
> IMHO if we use these macros it will at least people who are reading the 
> code there are three standard values 6,9 and 20
How is 6 a standard value? And why use driver specific defines when it's
really an 802.11 standard thing?

Using something like this would make the code more readable:
#define IEEE80211_SHORT_SLOT_TIME	9
#define IEEE80211_LONG_SLOT_TIME	20

ATH9K_SLOT_TIME_9 or ATH9K_SLOT_TIME_20? Not so much...

> I am sure it would help us to debug issues easily(just like Fair beacon 
> distribution thing).
I really don't see how this is helpful in any way.
The main reason why I object to stuff like this is because I think that
"other code is like that" is not a good reason for repeating it,
especially if what was done on the other code never made much sense to
begin with. In this case I think it's more of a reason to clean up the
other code first and then make things more consistent :)

- Felix