RE: [PATCH 18/20] wifi: rtw88: Add rtw8821a.{c,h}

[Ping-Ke Shih <pkshih@realtek.com>]

> >
> > #define REG_3WIRE_SWA 0xc00
> > #define REG_3WIRE_SWB 0xe00
> >
> > (0xc00 page for path A, 0xe00 page for path B)
> >
> 
> Previously, you called 0xe00 REG_HSSI_WRITE_B. Are both names correct?
> (I'm not sure why I put 0xc00 and 0xe00 on the list if you already gave
> them a name.)

In fact, there is no _formal_ names for PHY registers, so I gave names by
abbreviation name from RTL code. Previously I may reference to vendor
drivers instead. Just choose one you like. 

> >> 0xe90
> >
> > #define REG_PREDISTB 0xe90
> >
> 
> I put 0xe90 on the list by mistake. We already have a name for it
> in the official driver:
> 
> ./include/Hal8812PhyReg.h:66:#define rB_LSSIWrite_Jaguar                        0xe90 /* RF write addr */
> 
> I translated that as REG_LSSI_WRITE_B. Is REG_PREDISTB also a valid
> name? Do we need both names?

For the use case of 0xe90 in rtw8812a_iqk_tx_fill():

   rtw_write32_mask(rtwdev, 0xc90, BIT(7), 0x1);

"RF write addr" seems not reasonable. I suggest to define both for this case. 


> Some of these names are very different from their counterparts
> from page C. In your previous email you said:
> 
> > I think we can reuse existing definitions:
> >
> > rtw8723x.h:#define REG_OFDM_0_XA_TX_IQ_IMBALANCE        0x0c80
> > rtw8703b.h:#define REG_OFDM0_A_TX_AFE 0x0c84
> > rtw8723x.h:#define REG_OFDM_0_XB_TX_IQ_IMBALANCE        0x0c88
> >
> > #define REG_TSSI_TRK_SW 0xc8c
> >
> > rtw8821a.h:#define REG_TXAGCIDX                         0xc94
> 
> Can we reuse these definitions?

Yes. You can reuse existing first. For non-existing definition, use the names
I gave in this thread. 

Basically we can have two ways to have names. One is from vendor drivers, and
the other is from abbreviation name of RTL code, which bit name instead of
whole register name is given. Also I'm not very familiar with the functionality,
so I did just paste reasonable names for undefined magic numbers.

> >>>> +
> >>>> +const struct rtw_chip_info rtw8812a_hw_spec = {
> >>>
> >>> Is it possible moving 8812a to individual file?
> >>> Since you have rtw8812au.c and rtw8821au.c.
> >>>
> >>
> >> I think it is possible. But most of the code is common to both chips.
> >> Only the IQ calibration could be moved.
> >
> > Yep, depend on how much IQK code echo chip has.
> >
> 
> The IQ calibration for RTL8812AU is about 700 lines.

rtw8812au  -----> (a) rtw8812a 
                        |
                        v
                  (b) rtw8821a_common  (hard to give a name)
                        ^
                        |
rtw8821au  -----> (c) rtw8821a

Put all common code to (b). IQK code in (a) or (c). 

I feel you have thought above picture already. What are problems we will encounter?
Many export symbols? If so, how about below?

rtw8812au  -----> (1) rtw8812a 
    +---------+
              +-> (2) rtw8821a_common  (hard to give a name)
    +---------+
rtw8821au  -----> (3) rtw8821a

Put rtw8812a_hw_spec and rtw8821a_hw_spec in (2). Only IQK code in (1) and (3)
respectively, and export IQK entry only. Does it work?