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

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

> -----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? 

[...]

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

> 
>         rtw_write32(rtwdev, REG_LTECOEX_CTRL, 0x800f0054);
>         mdelay(1);

[...]

> +#define ADDA_ON_VAL 0x03c00016
> 
>  static
> -void rtw8723d_iqk_precfg_path(struct rtw_dev *rtwdev, enum rtw8723d_path path)
> +void rtw8723d_iqk_precfg_path(struct rtw_dev *rtwdev, enum rtw8723x_path path)
>  {
>         if (path == PATH_S0) {
>                 rtw8723d_iqk_rf_standby(rtwdev, RF_PATH_A);
> -               rtw8723d_iqk_path_adda_on(rtwdev);
> +               rtw8723x_iqk_path_adda_on(rtwdev, ADDA_ON_VAL);

ditto. (I don't point out more similar things later)

>         }
> 
>         rtw_write32_mask(rtwdev, REG_FPGA0_IQK_11N, BIT_MASK_IQK_MOD, EN_IQK);

[...]

> 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?


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

> +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);
> +}
> +

[...]