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

[Bitterblue Smith <rtl8821cerfe2@gmail.com>]

On 13/09/2024 04:50, Ping-Ke Shih wrote:
>>>
>>> #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.
> 

I see. Thank you for the explanations.

>>>>>> +
>>>>>> +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?
> 
>
For the name of the common module, I was thinking rtw88_88xxa.ko.

I wonder, what is the goal? To put the code in separate kernel
modules, or just separate files?

I think we can have rtw88xxa.c (common code), rtw8821a.c (IQK code,
rtw8821a_hw_spec, bluetooth stuff), and rtw8812a.c (IQK code,
rtw8812a_hw_spec, some efuse stuff, channel switching)... if these
three files compile into a single module, rtw88_88xxa.ko.

If each file compiles into a module of its own, we have circular
dependencies: rtw8821a_hw_spec -> common code -> IQK code.
If *_hw_spec go in the common module, it still depends on both of
the other two modules, so what use is it?