Re: [PATCH v2 2/9] ath9k: basic support for QCN550x

[Jeff Johnson <quic_jjohnson@quicinc.com>]

On 5/17/2022 11:08 PM, Wenli Looi wrote:
> On Thu, May 12, 2022 at 2:45 PM Jeff Johnson <quic_jjohnson@quicinc.com> wrote:
>>
>> On 5/12/2022 12:53 PM, Wenli Looi wrote:
>>> QCN550x is very similar to QCA956x. Note that AR_CH0_XTAL is
>>> intentionally unchanged. Certain arrays are no longer static because
>>> they are no longer constant.
>>
>> I don't understand the last sentence. You removed the 'static' keyword
>> but left the 'const' keyword, hence they are still constant.
>>
>> And I didn't actually see any instances where the arrays are being modified.
>>
>> Can you highlight which are being modified?
> 
> I changed several arrays from "static const" to "const" because their
> values now depend on ah, so it won't compile if they are static. For
> example, offset_array contains AR_PHY_RX_IQCAL_CORR_B0 which depends
> on AR_CHAN_BASE (which now depends on ah but did not before).

So the values are no longer constant, hence IMO the 'const' qualifier 
should also be removed

> 
>>> -#define AR_CHAN_BASE 0x9800
>>> +#define AR_CHAN_BASE (AR_SREV_5502(ah) ? 0x29800 : 0x9800)
>>
>> this violates the coding style:
>> <https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl>
>>
>> Things to avoid when using macros:
>> macros that depend on having a local variable with a magic name
>>
>> So you should add ah as a parameter to the macro
>>
>> Repeat for all instances below where ah is being used
>>
> 
> The macros like AR_CHAN_BASE and AR_SM_BASE are referenced by a lot of
> other macros, some of which already have a "magic name" dependency on
> ah like these ones:
> 
> https://github.com/torvalds/linux/blob/210e04ff768142b96452030c4c2627512b30ad95/drivers/net/wireless/ath/ath9k/ar9003_phy.h#L527
> 
> However I think it's probably good to avoid introducing new macros
> with the magic name dependency. If desired, for every macro that I
> have modified, I could try to add the ah parameter to all dependent
> macros. This would probably be a rather large change so it might make
> sense to be a separate commit.

Your current change illustrates one of the many pitfalls of using magic 
names. IMO you should have a precursor patch that changes all of the 
"bad" macros to take ah as a parameter rather than be a magic name.

At a minimum you should add ah as a parameter to the ones you are 
modifying so that it is clear why you need to change the attributes of 
those arrays.

/jeff