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?
