On 5/17/2022 11:08 PM, Wenli Looi wrote:
On Thu, May 12, 2022 at 2:45 PM Jeff Johnson <quic_jjohnson@xxxxxxxxxxx> 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
