Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <7c05bbc5-c51f-2eb6-84a2-3a20c45b8b22@quicinc.com>
Date:   Wed, 18 May 2022 14:19:25 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Subject: Re: [PATCH v2 2/9] ath9k: basic support for QCN550x
Content-Language: en-US
To:     Wenli Looi <wlooi@ucalgary.ca>
CC:     =?UTF-8?Q?Toke_H=c3=b8iland-J=c3=b8rgensen?= <toke@toke.dk>,
        Kalle Valo <kvalo@kernel.org>, <linux-wireless@vger.kernel.org>
References: <20220512195319.14635-1-wlooi@ucalgary.ca>
 <20220512195319.14635-3-wlooi@ucalgary.ca>
 <69feb893-d73a-46e5-fbe6-2f49e091c625@quicinc.com>
 <CAKe_nd3u+iiCGC7M_i+ovF+_bgDMkFUh0OpJ7HFWMhmgZQa7bw@mail.gmail.com>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <CAKe_nd3u+iiCGC7M_i+ovF+_bgDMkFUh0OpJ7HFWMhmgZQa7bw@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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