Re: [PATCH 1/9] wifi: rtw88: Shared module for rtw8723x devices

[Fiona Klute <fiona.klute@gmx.de>]

Am 05.02.24 um 02:45 schrieb Ping-Ke Shih:
>> -----Original Message-----
>> From: Fiona Klute <fiona.klute@gmx.de>
>> Sent: Friday, February 2, 2024 8:11 PM
>> To: linux-wireless@vger.kernel.org; Ping-Ke Shih <pkshih@realtek.com>
>> Cc: Kalle Valo <kvalo@kernel.org>; Ulf Hansson <ulf.hansson@linaro.org>; linux-mmc@vger.kernel.org; Pavel
>> Machek <pavel@ucw.cz>; Ondřej Jirman <megi@xff.cz>; Fiona Klute <fiona.klute@gmx.de>
>> Subject: [PATCH 1/9] wifi: rtw88: Shared module for rtw8723x devices
>>
>> The already supported 8723d chip is very similar to 8703b/8723cs,
>> split code that can be shared into a new module. The spec definition
>> tables are combined into a struct so we only need one EXPORT_SYMBOL
>> for them all.
>>
>> Signed-off-by: Fiona Klute <fiona.klute@gmx.de>
>> ---
>> I'm not sure how who should be MODULE_AUTHOR in rtw8723x.c. Most of
>> the code is from rtw8723d, I created the separate module. I don't want
>> to claim authorship over code I didn't write, but assigning authorship
>> unasked (by copying the MODULE_AUTHOR from rtw8723d.c) also seems
>> wrong.
>
> If two MODULE_AUTHOR are allowed, maybe list both?

I just checked the macro documentation, it says to use multiple
MODULE_AUTHOR for multiple authors, so I'll do that. :-)

> [...]
>
>> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8723d.c
>> b/drivers/net/wireless/realtek/rtw88/rtw8723d.c
>> index c575476a00..68245f34b5 100644
>> --- a/drivers/net/wireless/realtek/rtw88/rtw8723d.c
>> +++ b/drivers/net/wireless/realtek/rtw88/rtw8723d.c
>
> [...]
>
>> @@ -930,6 +532,8 @@ static u8 rtw8723d_iqk_check_rx_failed(struct rtw_dev *rtwdev,
>>          return 0;
>>   }
>>
>> +#define IQK_LTE_WRITE_VAL 0x0000ff00
>> +
>>   static void rtw8723d_iqk_one_shot(struct rtw_dev *rtwdev, bool tx,
>>                                    const struct rtw_8723d_iqk_cfg *iqk_cfg)
>>   {
>> @@ -937,7 +541,7 @@ static void rtw8723d_iqk_one_shot(struct rtw_dev *rtwdev, bool tx,
>>
>>          /* enter IQK mode */
>>          rtw_write32_mask(rtwdev, REG_FPGA0_IQK_11N, BIT_MASK_IQK_MOD, EN_IQK);
>> -       rtw8723d_iqk_config_lte_path_gnt(rtwdev);
>> +       rtw8723x_iqk_config_lte_path_gnt(rtwdev, IQK_LTE_WRITE_VAL);
>
> It would be better to discriminate IQK_LTE_WRITE_VAL_8723D and
> IQK_LTE_WRITE_VAL_8703B to prevent misreading.

Good point, I'll do that. Same for the ADDA_ON_VAL.

>> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8723x.c
>> b/drivers/net/wireless/realtek/rtw88/rtw8723x.c
>> new file mode 100644
>> index 0000000000..2b58064547
>> --- /dev/null
>> +++ b/drivers/net/wireless/realtek/rtw88/rtw8723x.c
>> @@ -0,0 +1,561 @@
>
> [...]
>
>> +const struct rtw8723x_common rtw8723x_common = {
>> +       .iqk_adda_regs = {
>> +               0x85c, 0xe6c, 0xe70, 0xe74, 0xe78, 0xe7c, 0xe80, 0xe84,
>> +               0xe88, 0xe8c, 0xed0, 0xed4, 0xed8, 0xedc, 0xee0, 0xeec
>> +       },
>> +       .iqk_mac8_regs = {0x522, 0x550, 0x551},
>> +       .iqk_mac32_regs = {0x40},
>> +       .iqk_bb_regs = {
>> +               0xc04, 0xc08, 0x874, 0xb68, 0xb6c, 0x870, 0x860, 0x864, 0xa04
>> +       },
>> +
>> +       .ltecoex_addr = {
>> +               .ctrl = REG_LTECOEX_CTRL,
>> +               .wdata = REG_LTECOEX_WRITE_DATA,
>> +               .rdata = REG_LTECOEX_READ_DATA,
>> +       },
>> +       .rf_sipi_addr = {
>> +               [RF_PATH_A] = { .hssi_1 = 0x820, .lssi_read    = 0x8a0,
>> +                               .hssi_2 = 0x824, .lssi_read_pi = 0x8b8},
>> +               [RF_PATH_B] = { .hssi_1 = 0x828, .lssi_read    = 0x8a4,
>> +                               .hssi_2 = 0x82c, .lssi_read_pi = 0x8bc},
>> +       },
>> +       .dig = {
>> +               [0] = { .addr = 0xc50, .mask = 0x7f },
>> +               [1] = { .addr = 0xc50, .mask = 0x7f },
>> +       },
>> +       .dig_cck = {
>> +               [0] = { .addr = 0xa0c, .mask = 0x3f00 },
>> +       },
>> +       .prioq_addrs = {
>> +               .prio[RTW_DMA_MAPPING_EXTRA] = {
>> +                       .rsvd = REG_RQPN_NPQ + 2, .avail = REG_RQPN_NPQ + 3,
>> +               },
>> +               .prio[RTW_DMA_MAPPING_LOW] = {
>> +                       .rsvd = REG_RQPN + 1, .avail = REG_FIFOPAGE_CTRL_2 + 1,
>> +               },
>> +               .prio[RTW_DMA_MAPPING_NORMAL] = {
>> +                       .rsvd = REG_RQPN_NPQ, .avail = REG_RQPN_NPQ + 1,
>> +               },
>> +               .prio[RTW_DMA_MAPPING_HIGH] = {
>> +                       .rsvd = REG_RQPN, .avail = REG_FIFOPAGE_CTRL_2,
>> +               },
>> +               .wsize = false,
>> +       },
>> +
>> +       .lck = __rtw8723x_lck,
>> +       .read_efuse = __rtw8723x_read_efuse,
>> +       .mac_init = __rtw8723x_mac_init,
>> +       .cfg_ldo25 = __rtw8723x_cfg_ldo25,
>> +       .set_tx_power_index = __rtw8723x_set_tx_power_index,
>> +       .efuse_grant = __rtw8723x_efuse_grant,
>> +       .false_alarm_statistics = __rtw8723x_false_alarm_statistics,
>> +       .iqk_backup_regs = __rtw8723x_iqk_backup_regs,
>> +       .iqk_restore_regs = __rtw8723x_iqk_restore_regs,
>> +       .iqk_similarity_cmp = __rtw8723x_iqk_similarity_cmp,
>> +       .pwrtrack_get_limit_ofdm = __rtw8723x_pwrtrack_get_limit_ofdm,
>> +       .pwrtrack_set_xtal = __rtw8723x_pwrtrack_set_xtal,
>> +       .coex_cfg_init = __rtw8723x_coex_cfg_init,
>> +       .fill_txdesc_checksum = __rtw8723x_fill_txdesc_checksum,
>> +};
>> +EXPORT_SYMBOL(rtw8723x_common);
>
> I think these are just copy-and-paste stuff. Is there anything special you want
> me look deeper?

It should be just copy-and-paste. If you (or anyone else reading this)
have the opportunity to test with an 8723D device to make sure I didn't
make any mistake that breaks things there that'd be great.

>> +
>> +MODULE_AUTHOR("Fiona Klute <fiona.klute@gmx.de>");
>> +MODULE_DESCRIPTION("Common functions for Realtek 802.11n wireless 8723x drivers");
>> +MODULE_LICENSE("Dual BSD/GPL");
>> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8723x.h
>> b/drivers/net/wireless/realtek/rtw88/rtw8723x.h
>> new file mode 100644
>> index 0000000000..d3930f1f2c
>> --- /dev/null
>> +++ b/drivers/net/wireless/realtek/rtw88/rtw8723x.h
>
> [...]
>
>
>> +/* IQK helper functions, defined as inline so they can be shared
>> + * without needing an EXPORT_SYMBOL each.
>> + */
>> +inline void
>
> These inline functions should be 'static'.
> (I believe you have addressed this, and will change them by v2.)

Yes, that was what broke building the driver into the kernel (not as a
module) as reported by Pavel Machek.

>> +rtw8723x_iqk_backup_path_ctrl(struct rtw_dev *rtwdev,
>> +                             struct rtw8723x_iqk_backup_regs *backup)
>> +{
>> +       backup->btg_sel = rtw_read8(rtwdev, REG_BTG_SEL);
>> +       rtw_dbg(rtwdev, RTW_DBG_RFK, "[IQK] original 0x67 = 0x%x\n",
>> +               backup->btg_sel);
>> +}
>> +
>
> [...]
>