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). > > -#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.
