> -----Original Message-----
> From: Fiona Klute <fiona.klute@xxxxxx>
> Sent: Friday, February 2, 2024 8:11 PM
> To: linux-wireless@xxxxxxxxxxxxxxx; Ping-Ke Shih <pkshih@xxxxxxxxxxx>
> Cc: Kalle Valo <kvalo@xxxxxxxxxx>; Ulf Hansson <ulf.hansson@xxxxxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx; Pavel
> Machek <pavel@xxxxxx>; Ondřej Jirman <megi@xxxxxx>; Fiona Klute <fiona.klute@xxxxxx>
> 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@xxxxxx>
> ---
> 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@xxxxxx>");
> +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);
> +}
> +
[...]
