RE: [PATCH rtw-next v7 4/6] wifi: rtw88: 8723b: add the RTL8723B chip driver

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

Luka Gejak <luka.gejak@linux.dev> wrote:
> On Mon Oct 5, 2026 at 8:08 AM CEST, Ping-Ke Shih wrote:
> > Luka Gejak <luka.gejak@linux.dev> wrote:
> [...]
> >> +/*
> >> + * Shares the receive PHY status layout, the SDIO aggregation burst fields
> >> + * and a few baseband registers with the RTL8703B; reuse that header.
> >> + */
> >> +#include "rtw8703b.h"
> >
> > Which layout you are using?
> > Should you move the layout to rtw8723x.h ?
> >
> 
> The layout I reuse is the RTL8703B receive PHY status structure, struct
> phy_status_8703b, together with the SDIO aggregation burst fields and four
> baseband registers. 

Let's use another patch to move the struct out of rtw8703b.h, and rename
to phy_status_8723x for example.

> However, including rtw8703b.h from another chip driver is
> already established, rtw8723cs.c includes it to reuse rtw8703b_hw_spec. 

As I know, 8723CS and 8723B are mutual alias, no?

> So I
> would rather keep the same include here, and if you want the layout in rtw8723x.h
> I can send that as a separate patch later, so this series does not touch
> additional 2 drivers.

Yes, another patch before this one. 

> 
> >> +/*
> >> + * Row 20 (-6.0 dB) intentionally does not match the v5.2.17 vendor driver,
> >
> > I really don't want to mention vendor driver here. If you really need it,
> > mention it in commit message or cover-letter.
> >
> >> + * which has 0x1c, 0x1a, 0x18, 0x12, 0x0e, 0x08 there. Every other row agrees.
> >> + * The values below are what rtl8723be, the mainline driver for this same
> >> + * chip, uses at the same index, and they are also what the vendor's own
> >> + * cck_swing_table_ch1_ch13_92e and the staging rtl8723bs driver use. They
> >> + * also track the 0.5 dB step of the surrounding rows: against row 32 as 0 dB,
> >> + * 0x1b is within 0.06 of the ideal -6.0 dB value while 0x1c is 0.94 away,
> >> + * the largest error anywhere in the table. Treat the vendor row as the
> >> + * anomaly and do not "fix" this towards it.
> >
> > And you have comments each row. Is it still need this block comment to explain?
> >
> 
> I agree, will drop vendor reference and block comment.

I'm not sure if LLM writes this? LLM always write verbose comments for
each line it added. Just ask LLM to write self-explained code.

[...]

> >> +
> >> +static const struct rtw_chip_ops rtw8723b_ops = {
> >> +       .power_on               = rtw_power_on,
> >> +       .power_off              = rtw_power_off,
> >> +
> >> +       .mac_init               = rtw8723x_mac_init,
> >> +       .mac_postinit           = rtw8723x_mac_postinit,
> >> +
> >> +       .dump_fw_crash          = NULL,
> >> +       /*
> >> +        * 8723d sets REG_HCI_OPT_CTRL BIT_USB_SUS_DIS in its shutdown
> >> +        * function; that is USB-only.
> >> +        */
> >> +       .shutdown               = NULL,
> >> +       .read_efuse             = rtw8723b_read_efuse,
> >> +       .phy_set_param          = rtw8723b_phy_set_param,
> >> +
> >> +       .set_channel            = rtw8723b_set_channel,
> >> +
> >> +       .query_phy_status       = rtw8723b_query_phy_status,
> >> +       .read_rf                = rtw_phy_read_rf_sipi,
> >> +       .write_rf               = rtw_phy_write_rf_reg_sipi,
> >> +       .set_tx_power_index     = rtw8723x_set_tx_power_index,
> >> +       .rsvd_page_dump         = NULL,
> >> +       .set_antenna            = NULL,
> >> +       .cfg_ldo25              = rtw8723b_cfg_ldo25,
> >> +       .efuse_grant            = rtw8723b_efuse_grant,
> >> +       .set_ampdu_factor       = NULL,
> >> +       .false_alarm_statistics = rtw8723x_false_alarm_statistics,
> >> +       .phy_calibration        = rtw8723b_phy_calibration,
> >> +       .dpk_track              = NULL,
> >> +       .cck_pd_set             = rtw_phy_cck_pd_set,
> >> +       .pwr_track              = rtw8723b_pwr_track,
> >> +       .config_bfee            = NULL,
> >> +       .set_gid_table          = NULL,
> >> +       .cfg_csi_rate           = NULL,
> >> +       .adaptivity_init        = NULL,
> >> +       .adaptivity             = NULL,
> >> +       .cfo_init               = NULL,
> >> +       .cfo_track              = NULL,
> >> +       .config_tx_path         = NULL,
> >> +       .config_txrx_mode       = NULL,
> >> +       .led_set                = NULL,
> >> +       .fill_txdesc_checksum   = rtw8723b_fill_txdesc_checksum,
> >> +
> >> +       .coex_set_init          = rtw8723b_coex_cfg_init,
> >> +       .coex_set_ant_switch    = rtw8723b_coex_cfg_ant_switch,
> >> +       .coex_set_gnt_fix       = rtw8723b_coex_set_gnt_fix,
> >> +       .coex_set_gnt_debug     = rtw8723b_coex_set_gnt_debug,
> >> +       .coex_set_rfe_type      = rtw8723b_coex_set_rfe_type,
> >> +       .coex_set_wl_tx_power   = rtw8723b_coex_set_wl_tx_power,
> >> +       .coex_set_wl_rx_gain    = rtw8723b_coex_set_wl_rx_gain,
> >> +};
> >> +
> >> +const struct rtw_chip_info rtw8723b_hw_spec = {
> >> +       .ops = &rtw8723b_ops,
> >> +       .id = RTW_CHIP_TYPE_8723B,
> >> +       .fw_name = "rtw88/rtw8723b_fw.bin",
> >> +       .wlan_cpu = RTW_WCPU_8051,
> >> +       .tx_pkt_desc_sz = 40,
> >> +       .tx_buf_desc_sz = 16,
> >> +       .rx_pkt_desc_sz = 24,
> >> +       .rx_buf_desc_sz = 8,
> >> +       .phy_efuse_size = 512,
> >> +       .log_efuse_size = 512,
> >> +       .ptct_efuse_size = 15,
> >> +       .txff_size = 32768,
> >> +       .rxff_size = 16384,
> >> +       .rsvd_drv_pg_num = 8,
> >> +       .txgi_factor = 1,
> >> +       .is_pwr_by_rate_dec = true,
> >> +       .max_power_index = 0x3f,
> >> +       .csi_buf_pg_num = 0,
> >> +       .band = RTW_BAND_2G,
> >> +       .page_size = TX_PAGE_SIZE,
> >> +       .dig_min = 0x20,
> >> +       .usb_tx_agg_desc_num = 6,
> >> +       /*
> >> +        * The firmware reports id 0xfd instead of C2H_HW_FEATURE_REPORT, so
> >> +        * the hardware feature report is not supported on this chip.
> >> +        */
> >> +       .hw_feature_report = false,
> >> +       .c2h_ra_report_size = 4,
> >> +       .old_datarate_fb_limit = true,
> >> +       .path_div_supported = false,
> >> +       .ht_supported = true,
> >> +       .vht_supported = false,
> >> +       .lps_deep_mode_supported = 0,
> >> +       .sys_func_en = 0xfd,
> >> +       .pwr_on_seq = card_enable_flow_8723b,
> >> +       .pwr_off_seq = card_disable_flow_8723b,
> >> +       .page_table = page_table_8723b,
> >> +       .rqpn_table = rqpn_table_8723b,
> >> +       /* same shared table as the sibling rtw8703b and rtw8723d */
> >> +       .prioq_addrs = &rtw8723x_common.prioq_addrs,
> >> +       /* used only in pci.c, not needed for SDIO devices */
> >> +       .intf_table = NULL,
> >> +       .dig = rtw8723x_common.dig,
> >> +       /* The vendor driver never writes the CCK IGI on this chip. */
> >> +       .dig_cck = NULL,
> >> +       .rf_sipi_addr = {0x840, 0x844},
> >> +       .rf_sipi_read_addr = rtw8723x_common.rf_sipi_addr,
> >> +       .fix_rf_phy_num = 2,
> >> +       /* This chip has no LTE coex registers. */
> >> +       .ltecoex_addr = NULL,
> >> +       .mac_tbl = &rtw8723b_mac_tbl,
> >> +       .agc_tbl = &rtw8723b_agc_tbl,
> >> +       .bb_tbl = &rtw8723b_bb_tbl,
> >> +       .rf_tbl = {&rtw8723b_rf_a_tbl},
> >> +       .rfe_defs = rtw8723b_rfe_defs,
> >> +       .rfe_defs_size = ARRAY_SIZE(rtw8723b_rfe_defs),
> >> +       .iqk_threshold = 8,
> >> +       .rx_ldpc = false,
> >> +       .tx_stbc = false,
> >> +       .ampdu_density = IEEE80211_HT_MPDU_DENSITY_16,
> >> +       .max_scan_ie_len = IEEE80211_MAX_DATA_LEN,
> >> +       .coex_para_ver = 20180201,      /* glcoex_ver_date_8723b_1ant */
> >> +       .bt_desired_ver = 0x6d,
> >> +       .scbd_support = false,
> >> +       .new_scbd10_def = true,
> >> +       .ble_hid_profile_support = false,
> >> +       .wl_mimo_ps_support = false,
> >> +       .pstdma_type = COEX_PSTDMA_FORCE_LPSOFF,
> >> +       .bt_rssi_type = COEX_BTRSSI_RATIO,
> >> +       .ant_isolation = 15,
> >> +       .rssi_tolerance = 2,
> >> +       .wl_rssi_step = wl_rssi_step_8723b,
> >> +       .bt_rssi_step = bt_rssi_step_8723b,
> >> +       .table_sant_num = ARRAY_SIZE(table_sant_8723b),
> >> +       .table_sant = table_sant_8723b,
> >> +       .table_nsant_num = ARRAY_SIZE(table_nsant_8723b),
> >> +       .table_nsant = table_nsant_8723b,
> >> +       .tdma_sant_num = ARRAY_SIZE(tdma_sant_8723b),
> >> +       .tdma_sant = tdma_sant_8723b,
> >> +       .tdma_nsant_num = ARRAY_SIZE(tdma_nsant_8723b),
> >> +       .tdma_nsant = tdma_nsant_8723b,
> >> +       .wl_rf_para_num = ARRAY_SIZE(rf_para_tx_8723b),
> >> +       .wl_rf_para_tx = rf_para_tx_8723b,
> >> +       .wl_rf_para_rx = rf_para_rx_8723b,
> >> +       .bt_afh_span_bw20 = 0x20,
> >> +       .bt_afh_span_bw40 = 0x30,
> >> +       .afh_5g_num =  ARRAY_SIZE(afh_5g_8723b),
> >> +       .afh_5g = afh_5g_8723b,
> >> +       /* BTG_SEL is driven by the cardemu_to_act power sequence instead. */
> >> +       .btg_reg = NULL,
> >> +       .coex_info_hw_regs_num = 0,
> >> +       .coex_info_hw_regs = NULL,
> >> +};
> >> +EXPORT_SYMBOL(rtw8723b_hw_spec);
> >
> > I guess you copy these two tables from somewhere and modify the values.
> > However, when I compare these with RTL8822C's ones. The order is very
> > different... Can you align the order?
> >
> > Realtek WiFi chips are different from one to another, and we add many
> > parameters to support the variants. To prevent the order being messed
> > up, I ask people to add dummy (unused) fields (e.g. .xxx = NULL, .yyy = 0)
> > to keep the order and consistent. But now, rtw88 becomes very different
> > again.
> >
> > Let me know your source, I'd think how we can align them sometime.
> 
> I took the ordering from the sibling rtw8703b and rtw8723d drivers. rtw8723b_ops
> is already in the order of the struct rtw_chip_ops declaration, and it follows the
> same order as rtw8703b_ops. I believe rtw8822c_ops is the one that differs, since it
> groups the fields by function instead of following the struct.

Okay. Please make sure the ordering are the same as the one you copied. 

> The values were taken
> from the v5.2.17 vendor driver and checked against the staging rtl8723bs. 

I have no objection to values. 

> So I would
> prefer to leave both tables as they are. If you want the unused fields listed as dummies
> to pin the order, I can add them, but the sequence itself would not change.

I will think a bit how to align these messed tables. 

Ping-Ke