Re: [PATCH v4 3/6] wifi: rtw88: 8723b: add the RTL8723B chip driver

[Bitterblue Smith <rtl8821cerfe2@gmail.com>]

On 29/09/2026 13:10, Luka Gejak wrote:
> September 27, 2026 at 17:21, "Bitterblue Smith" <rtl8821cerfe2@gmail.com mailto:rtl8821cerfe2@gmail.com?to=%22Bitterblue%20Smith%22%20%3Crtl8821cerfe2%40gmail.com%3E > wrote:
> 
>>
>>
>> On 24/09/2026 00:35, Luka Gejak wrote:
> 
>>> +static u8 rtw8723b_default_ofdm_index(struct rtw_dev *rtwdev)
>>> +{
>>> +	u32 val32;
>>> +	u32 swing;
>>> +	u8 i;
>>> +
>>> +	swing = rtw_read32_mask(rtwdev, REG_OFDM_0_XA_TX_IQ_IMBALANCE,
>>> +				OFDM_SWING_MASK);
>>> +
>>> +	for (i = 0; i < RTW_OFDM_SWING_TABLE_SIZE; i++) {
>>> +		val32 = rtw8723b_ofdm_swing_table[i];
>>> +
>>> +		if (val32 >= 0x100000)
>>> +			val32 >>= __ffs(OFDM_SWING_MASK);
> 
>> Isn't this the same as u32_get_bits(val32, OFDM_SWING_MASK) ?
> 
> Yes. It is u32_get_bits() now, and the if goes away with it.
> 
>>> +		if (val32 >= 0x100000)
>>> +			val32 >>= __ffs(OFDM_SWING_MASK);
> 
>> Also, every value in the table is bigger than 0x100000, so no need for if.
> 
> There is no val32 left to test.
> 
>>> +static void rtw8723b_post_enable_flow(struct rtw_dev *rtwdev)
>>> +{
>>> +	/*
>>> +	 * Enable falling edge triggered interrupts and GPIO9 interrupt mode.
>>> +	 * The power-on sequence sets both, but it runs before the firmware is
>>> +	 * downloaded, so reassert them here as the vendor driver does.
>>> +	 */
>>> +	rtw_write8_set(rtwdev, 0x0049, BIT(1));
>>> +	rtw_write8_set(rtwdev, 0x0063, BIT(1));
> 
>> This seems like bit 25 of REG_GPIO_PIN_CTRL_2.
> 
> It is. That write is rtw_write32_set(REG_GPIO_PIN_CTRL_2, BIT(25)) now.
> The write above it is bit 9 of REG_GPIO_INTM.
> 
>>> +static void rtw8723b_phy_lck(struct rtw_dev *rtwdev)
>>> +{
>>> +	rtw_write_rf(rtwdev, RF_PATH_A, 0xb0, RFREG_MASK, 0xdfbe0);
> 
>> We have a name for 0xb0: RF_SYN_PFD. Not sure if it's the right name here.
> 
> I have decided to use it.
> 
>>> +static void rtw8723b_init_tx_buffer_boundary(struct rtw_dev *rtwdev)
>>> +{
>>> +	u8 val8 = TX_TOTAL_PAGE_NUMBER_8723B + 1;
>>> +
>>> +	rtw_write8(rtwdev, REG_BCNQ_BDNY, val8);
>>> +	rtw_write8(rtwdev, REG_MGQ_BDNY, val8);
>>> +	rtw_write8(rtwdev, REG_WMAC_LBK_BF_HD, val8);
>>> +	rtw_write8(rtwdev, REG_TRXFF_BNDY, val8);
>>> +	rtw_write8(rtwdev, REG_DWBCN0_CTRL + 1, val8);
> 
>> __priority_queue_cfg_legacy() already takes care of these. Is it
>> necessary to set them again here?
> 
> No. init_tx_buffer_boundary() and init_page_boundary() are both gone.
> 
>>> +static void rtw8723b_init_transfer_page_size(struct rtw_dev *rtwdev)
>>> +{
>>> +	rtw_write8(rtwdev, REG_PBP, 0x11);
> 
>> We have some macros for this in reg.h right under REG_PBP. You can use
>> them with u8_encode_bits().
> 
> Done, u8_encode_bits(PBP_128, PBP_RX_MASK) and the same for TX.
> 
>>> +static void rtw8723b_init_driver_info_size(struct rtw_dev *rtwdev)
>>> +{
>>> +	/* NOTE: also is done in rtw_drv_info_cfg */
> 
>> If it's done there do you have to do it again here?
> 
> No. init_driver_info_size() is gone.
> 
>>> +static void rtw8723b_init_wmac_setting(struct rtw_dev *rtwdev)
>>> +{
>>> +	/*
>>> +	 * The vendor's 8723x filter value plus BIT_APP_FCS, which rtw88
>>> +	 * needs because it advertises RX_INCLUDES_FCS. It is assigned to
>>> +	 * hal.rcr and not merely written because rtw_core_start()
>>> +	 * rewrites REG_RCR from hal.rcr after power_on, which would undo
>>> +	 * a register-only write; fw.c toggles BIT_CBSSID_BCN in it.
>>> +	 */
>>> +	rtwdev->hal.rcr = WLAN_RCR_CFG;
> 
>> Is it necessary to change the default value assigned in rtw_core_init()?
> 
> No, that is where it belngs. hal.rcr is assigned in rtw_core_init() per
> chip now. It is a patch of its own at the beggining of the series.
> 
>>> +	rtw_write32(rtwdev, REG_RCR, rtwdev->hal.rcr);
>>> +
>>> +	rtw_write32(rtwdev, REG_MAR, 0xffffffff);
>>> +	rtw_write32(rtwdev, REG_MAR + 4, 0xffffffff);
>>> +
>>> +	rtw_write16(rtwdev, REG_RXFLTMAP2, WLAN_RX_FILTER2);
>>> +	rtw_write16(rtwdev, REG_RXFLTMAP1, WLAN_RX_FILTER1);
> 
>> Please also update rtwdev->hal.rxfltmap1 when you change REG_RXFLTMAP1:
>> https://lore.kernel.org/linux-wireless/2a52d718-9e46-47f2-84a1-d8e7b1ed89a8@gmail.com/
> 
> You are right, and the reuse turns out to be the fix. The chip uses
> rtw8723x_mac_init() now, which writes REG_RXFLTMAP0/1/2 and sets
> hal.rxfltmap1. rtw8723b_mac_init() and rtw8723b_init_wmac_setting() are
> both gone.
> 
>>> +static void rtw8723b_init_operation_mode(struct rtw_dev *rtwdev)
>>> +{
>>> +	rtw_write8(rtwdev, REG_BWOPMODE, BIT_BWOPMODE_20MHZ);
> 
>> I don't see this in the vendor driver v5.2.17.1.
> 
> After rechecking it myself, me neither. init_operation_mode() and the
> REG_BWOPMODE defines are gone.
> 
>>> +static void rtw8723b_init_antenna_selection(struct rtw_dev *rtwdev)
>>> +{
>>> +	rtw_write8(rtwdev, REG_LEDCFG2, WLAN_ANT_SEL);
>>> +}
>>> +
>>> +#define RF_AC	0x00
> 
>> You can use the existing RF_MODE name for this.
> 
> Done, the local RF_AC is gone.
> 
>>> +static void rtw8723b_lck(struct rtw_dev *rtwdev)
>>> +{
>>> +	u32 rf_mode = 0, lc_cal;
>>> +	int ret;
>>> +	u8 val_ctx;
>>> +	u8 rf_val;
>>> +
>>> +	val_ctx = rtw_read8(rtwdev, REG_CTX);
>>> +
>>> +	if ((val_ctx & BIT_MASK_CTX_TYPE) != 0)
> 
>> No need to compare.
> 
> Dropped.
> 
>>> +	/* 3. Read RF reg18 */
>>> +	lc_cal = rtw_read_rf(rtwdev, RF_PATH_A, RF_CFGCH, MASK12BITS);
>>> +
>>> +	/* 4. Set LC calibration begin	bit15 */
>>> +	rtw_write_rf(rtwdev, RF_PATH_A, 0xb0, RFREG_MASK, 0xdfbe0);
>>> +	rtw_write_rf(rtwdev, RF_PATH_A, RF_CFGCH, MASK12BITS, lc_cal | BIT_LCK);
>>> +
>>> +	ret = read_poll_timeout(rtw_read_rf, rf_val, rf_val != 0x1,
>>> +				10000, 1000000, false,
>>> +			 rtwdev, RF_PATH_A, RF_CFGCH, BIT_LCK);
> 
>> checkpatch should catch alignment issues like this. Please always run it.
> 
> Fixed, and I am running it. checkpatch --strict is clean except the
> MAINTAINERS reminder on the patches that add files and the comllex-macro
> report on TRANS_SEQ_END, which rtw8703b.c already has.
> 
>>> +	if (ret)
>>> +		rtw_warn(rtwdev, "failed to poll LCK status bit\n");
>>> +
>>> +	rtw_write_rf(rtwdev, RF_PATH_A, 0xb0, RFREG_MASK, 0xdffe0);
>>> +
>>> +	/* Restore original situation */
>>> +	if ((val_ctx & BIT_MASK_CTX_TYPE) != 0) {
>>> +		rtw_write8(rtwdev, REG_CTX, val_ctx);
>>> +
>>> +		rtw_write_rf(rtwdev, RF_PATH_A, RF_AC, MASK12BITS, rf_mode);
>>> +	} else {
>>> +		rtw_write8(rtwdev, REG_TXPAUSE, 0x00);
>>> +	}
> 
>> I think the vendor driver is using MASK12BITS (0xfff) by mistake in
>> this function. It doesn't make any sense, it should be RFREG_MASK
>> (0xfffff) instead.
> 
> Agreed, and that function is gone. The chip had its own lck carrying the
> shared one plus the RF_MODE standby read and write and the two RF_SYN_PFD
> writes. The RF_MODE pair only runs in the continuous TX branch, which
> this driver never enters, and rtw8723b_phy_lck() already does the
> RF_SYN_PFD pair at RF configuration. So rtw8723b calls rtw8723x_lck()
> now, the way 8703b and 8723d do, and that one reads RF_CHNLBW with
> RFREG_MASK. Test results are the same with it dropped.

Why drop this function? You don't know what RF_SYN_PFD is for.
Maybe it's required every time it does LC calibration?

> 
>>> +static u32 rtw8723b_iqk_ant_switch_path(struct rtw_dev *rtwdev)
>>> +{
>>> +	if (rtw_hci_type(rtwdev) != RTW_HCI_TYPE_SDIO)
>>> +		return rtw_hci_type(rtwdev) == RTW_HCI_TYPE_USB ? 0x280 : 0x0;
>>> +
>>> +	/* Scan and connect use the PTA mux, so calibrate the path they use. */
>>> +	return (rtwdev->efuse.bt_setting & BIT(6)) ? 0x80 : 0x200;
> 
>> This function would be clearer if you don't use the ternary
>> operator at all.
> 
> Rewritten without it.
> 
>> Not sure this logic is correct. The check in the vendor driver is
>> like this:
>>
>> bool shared_ant = bt_setting & BIT(0);
>> if (USB) {
>> 	if (shared_ant)
>> 		ant_path = RF_PATH_B;
>> 	else
>> 		ant_path = RF_PATH_A;
>> } else {
>> 	if (bt_setting & BIT(6))
>> 		ant_path = RF_PATH_B;
>> 	else
>> 		ant_path = RF_PATH_A;
>> }
>> if (!shared_ant || ant_path == RF_PATH_A)
>> 	return 0;
>> else
>> 	return 0x280;
>>
>> I didn't see 0x80 and 0x200 in the IQK code.
> 
> It follows the vendor now, shared_ant from bit 0 and ant_path from bit 6,
> returning 0 or 0x280. The 0x80 and 0x200 are gone.
> 
>>> +static void rtw8723b_set_channel_rf(struct rtw_dev *rtwdev, u8 channel, u8 bw)
>>> +{
>>> +	u32 rf_cfgch;
>>> +
>>> +	rf_cfgch = rtw_read_rf(rtwdev, RF_PATH_A, RF_CFGCH, RFREG_MASK);
>>> +
>>> +	rf_cfgch &= ~RFCFGCH_CHANNEL_MASK;
>>> +	rf_cfgch |= channel & RFCFGCH_CHANNEL_MASK;
> 
>> This is what u32(p)_replace_bits is for.
> 
> Done, now using u32p_replace_bits().
> 
>>> +
>>> +	rf_cfgch &= ~RFCFGCH_BW_MASK;
>>> +	switch (bw) {
>>> +	case RTW_CHANNEL_WIDTH_20:
>>> +		rf_cfgch |= RFCFGCH_BW_20M;
>>> +		break;
>>> +	case RTW_CHANNEL_WIDTH_40:
>>> +		rf_cfgch |= RFCFGCH_BW_40M;
>>> +		break;
>>> +	default:
>>> +		break;
>>> +	}
> 
>> bw is only going to be 20 or 40, so a simple if would be shorter.
> 
> This is the one I left alone, the bandwidth assignment is still a single
> ternary.
> 
>>> +static void rtw8723b_set_channel(struct rtw_dev *rtwdev, u8 channel,
>>> +				 u8 bw, u8 primary_chan_idx)
>>> +{
>>> +	rtw8723b_set_channel_rf(rtwdev, channel, bw);
>>> +	rtw_set_channel_mac(rtwdev, channel, bw, primary_chan_idx);
>>> +	rtw8723b_set_channel_bb(rtwdev, bw, primary_chan_idx);
> 
>> I don't think the vendor driver v5.2.17.1 does the stuff below:
> 
>>> +	rtw8723b_reassert_rx_path(rtwdev);
>>> +
>>> +	if (rtw_is_8723bs(rtwdev)) {
>>> +		bool keep_pta_owner;
>>> +
>>> +		keep_pta_owner = test_bit(RTW_FLAG_SCANNING, rtwdev->flags) ||
>>> +				 (rtw_read32(rtwdev, REG_PAD_CTRL1) &
>>> +				  BIT_PAPE_WLBT_SEL);
>>> +		rtw8723b_sdio_restore_pad_ctrl(rtwdev, keep_pta_owner);
>>> +
>>> +		rtw_write8(rtwdev, REG_RF_CTRL, WLAN_RF_CTRL_ENABLE);
>>> +		fsleep(1000);
>>> +
>>> +		/*
>>> +		 * RF_WLINT bits 0-1 gate the data path into the BB and a prior
>>> +		 * IQK or coex run can leave them blocking TX, so re-arm them.
>>> +		 */
>>> +		rtw_write_rf(rtwdev, RF_PATH_A, RF_WLINT, RFREG_MASK,
>>> +			     0x0780);
>>> +	}
> 
>> If this driver doesn't work without it, you probably have a bug
>> somewhere.
> 
> The block is still there. I did not find a bug, and I have not found a
> reason to delete it either. rtw8723b_reassert_rx_path() has a comment
> saying what it puts back, and where the PAD bits come from is in my reply
> to your other mail.

So the driver doesn't work if you delete this part?

> 
>>> +	/* enable path A PA in TX IQK mode */
>>> +	rtw_write_rf(rtwdev, RF_PATH_A, RF_LUTWE, 0x80000, 0x1);
>>> +	rtw_write_rf(rtwdev, RF_PATH_A, RF_MODE_TABLE_ADDR, RFREG_MASK,
>>> +		     sdio_iqk ? 0x18000 : 0x20000);
> 
>> Where does 0x18000 come from? The vendor driver v5.2.17.1 uses 0x20000 here.
> 
> That value was mine and it is wrong. The vendor's value is used now.
> 
>>> +	rtw_write32(rtwdev, REG_TXIQK_PI_A_11N,
>>> +		    sdio_iqk ? 0x821303ea : 0x821403ea);
> 
>> Here, too, it uses the second value.
> 
> Same, the vendor's value.
> 
>>> +	rtw_write_rf(rtwdev, RF_PATH_A, RF_LUTWE, 0x80000, 0x1);
>>> +	rtw_write_rf(rtwdev, RF_PATH_A, RF_MODE_TABLE_ADDR, RFREG_MASK,
>>> +		     sdio_iqk ? 0x18000 : 0x30000);
> 
>> Vendor driver uses the second value.
> 
> 0x30000 now.
> 
>>> +	/* IQK setting */
>>> +	rtw_write32(rtwdev, REG_TXIQK_11N, 0x01007c00);
>>> +	rtw_write32(rtwdev, REG_RXIQK_11N, 0x01004800);
>>> +
>>> +	/* path-A IQK setting */
>>> +	rtw_write32(rtwdev, REG_TXIQK_TONE_A_11N, 0x18008c1c);
>>> +	rtw_write32(rtwdev, REG_RXIQK_TONE_A_11N, 0x38008c1c);
>>> +	rtw_write32(rtwdev, REG_TX_IQK_TONE_B, 0x38008c1c);
>>> +	rtw_write32(rtwdev, REG_RX_IQK_TONE_B, 0x38008c1c);
>>> +
>>> +	rtw_write32(rtwdev, REG_TXIQK_PI_A_11N,
>>> +		    sdio_iqk ? 0x82130ff0 : 0x82160ff0);
> 
>> Here as well.
> 
> Also the vendor's value, in the RX path RF_MODE_TABLE_ADDR write.
> 
>>> +	rtw_dbg(rtwdev, RTW_DBG_RFK, "[IQK] path A RX IQK step2!\n");
>>> +
>>> +	/* modify RX IQK mode */
>>> +	rtw_write32_mask(rtwdev, REG_FPGA0_IQK_11N, MASKH3BYTES, 0x000000);
>>> +	rtw_write_rf(rtwdev, RF_PATH_A, RF_LUTWE, 0x80000, 0x1);
>>> +	rtw_write_rf(rtwdev, RF_PATH_A, RF_MODE_TABLE_ADDR, RFREG_MASK,
>>> +		     sdio_iqk ? 0x18000 : 0x30000);
> 
>> Here as well.
> 
> And in the REG_RXIQK_PI_A_11N write.
> 
>>> +	/* leave IQK mode */
>>> +	rtw_write32_mask(rtwdev, REG_FPGA0_IQK_11N, MASKH3BYTES, 0x000000);
>>> +
>>> +	/* Check failed */
> 
>> No need for a comment that just repeats what the name of the function
>> already says.
> 
>>> +	/* path-A IQK setting */
>>> +	rtw_write32(rtwdev, REG_TXIQK_TONE_A_11N, 0x38008c1c);
>>> +	rtw_write32(rtwdev, REG_RXIQK_TONE_A_11N, 0x18008c1c);
>>> +	rtw_write32(rtwdev, REG_TX_IQK_TONE_B, 0x38008c1c);
>>> +	rtw_write32(rtwdev, REG_RX_IQK_TONE_B, 0x38008c1c);
>>> +
>>> +	rtw_write32(rtwdev, REG_TXIQK_PI_A_11N, 0x82110000);
>>> +	rtw_write32(rtwdev, REG_RXIQK_PI_A_11N,
>>> +		    sdio_iqk ? 0x2813001f : 0x2816001f);
> 
>> Here as well.
> 
> All three are gone.
> 
>>> +	rtw_write32_mask(rtwdev, REG_FPGA0_IQK_11N, MASKH3BYTES, 0x000000);
>>> +	rtw_write_rf(rtwdev, RF_PATH_A, RF_LUTWE, 0x80000, 0x1);
>>> +	rtw_write_rf(rtwdev, RF_PATH_A, RF_MODE_TABLE_ADDR, RFREG_MASK, 0x30000);
>>> +	rtw_write_rf(rtwdev, RF_PATH_A, RF_MODE_TABLE_DATA0, RFREG_MASK, 0x0001f);
>>> +	rtw_write_rf(rtwdev, RF_PATH_A, RF_MODE_TABLE_DATA1, RFREG_MASK, 0xf7fb7);
>>> +	rtw_write_rf(rtwdev, RF_PATH_A, 0xed, 0x20, 0x1);
>>> +	rtw_write_rf(rtwdev, RF_PATH_A, 0x43, RFREG_MASK, 0x60fbd);
>>> +
>>> +	for (i = 0; i < PATH_IQK_RETRY; i++) {
>>> +		a_ok = rtw8723b_iqk_tx_path_a(rtwdev);
>>> +		if (a_ok == IQK_TX_OK) {
> 
>> If you invert the condition, this block with long lines can be
>> indented less.
> 
> Inverted.
> 
>>> +
>>> +	if (a_ok == 0x0)
>>> +		rtw_dbg(rtwdev, RTW_DBG_RFK, "[IQK] path A IQK fail!\n");
>>> +
>>> +	/* rtl8723b is 1T1R, so path B is not calibrated. */
> 
>> It looks like the vendor driver is calibrating path B as well
>> (phy_path_b_iqk_8723b() and phy_path_b_rx_iqk_8723b()) when the device
>> has two antennas, and only path A when the device has one antenna.
> 
> Path B is still not calibrated, so path A only. The comment now says what
> you said, that the vendor calibrates it when the board is not 1-antenna
> and that this driver does not support that case. I would rather not add
> IQK code I cannot test on a 1x1 card.
> 
>>> +	memset(result, 0, sizeof(result));
>>> +
>>> +	rtw8723b_lck(rtwdev);
>>> +	rtw8723b_inform_rfk_status(rtwdev, true);
>>> +
>>> +	/* The LTE path GNT backup that 8723d does is not needed on SDIO. */
>>> +	rtw8723x_iqk_backup_path_ctrl(rtwdev, &backup);
>>> +	rtw8723x_iqk_backup_regs(rtwdev, &backup);
>>> +
>>> +	/* save default GNT_BT */
>>> +	bt_control = rtw_read32(rtwdev, REG_BT_CONTROL_8723B);
>>> +
>>> +	for (i = IQK_ROUND_0; i <= IQK_ROUND_2; i++) {
>>> +		if (!rtw_is_8723bs(rtwdev))
>>> +			rtw8723x_iqk_config_path_ctrl(rtwdev);
> 
>> I didn't see this in the RTL8723BE or RTL8723BU drivers. I guess it was
>> copied from another chip?
> 
> It was. The iqk_config_path_ctrl() and restore calls are gone.
> 
>>> +	rtw_dbg(rtwdev, RTW_DBG_RFK, "[IQK] final_candidate is %x\n",
>>> +		final_candidate);
>>> +
>>> +	for (i = IQK_ROUND_0; i < IQK_ROUND_SIZE; i++)
>>> +		rtw_dbg(rtwdev, RTW_DBG_RFK,
>>> +			"[IQK] Result %u: rege94_s1=%x rege9c_s1=%x regea4_s1=%x regeac_s1=%x rege94_s0=%x rege9c_s0=%x regea4_s0=%x regeac_s0=%x %s\n",
>>> +			i,
>>> +			result[i][0], result[i][1], result[i][2], result[i][3],
>>> +			result[i][4], result[i][5], result[i][6], result[i][7],
>>> +			final_candidate == i ? "(final candidate)" : "");
> 
>> You can make this two calls to rtw_dbg to avoid such a long line.
> 
> Two calls now.
> 
>>> +static void rtw8723b_pwrtrack_set_cck_pwr(struct rtw_dev *rtwdev, s8 swing_idx,
>>> +					  s8 txagc_idx)
>>> +{
>>> +	struct rtw_dm_info *dm_info = &rtwdev->dm_info;
>>> +
>>> +	dm_info->txagc_remnant_cck = txagc_idx;
>>> +
>>> +	swing_idx = clamp_t(s8, swing_idx, 0, RTW_CCK_SWING_TABLE_SIZE - 1);
>>> +
>>> +	BUILD_BUG_ON(ARRAY_SIZE(rtw8723b_cck_pwr_regs) !=
>>> +		     ARRAY_SIZE(rtw8723b_cck_swing_table_ch1_ch13[0]));
>>> +
>>> +	/* Only ch1-13 is wired up; channel 14 is Japan-only and unreachable. */
> 
>> If I change my country code to JP and trigger a scan, rtw88 visits
>> channel 14.
> 
> Right. Channel 14 selects rtw8723b_cck_swing_table_ch14 now.
> 
>>> +	for (int i = 0; i < ARRAY_SIZE(rtw8723b_cck_pwr_regs); i++)
>>> +		rtw_write8(rtwdev, rtw8723b_cck_pwr_regs[i],
> 
>> These register addresses are consecutive values, there is no need
>> to put them in an array.
> 
> The array is gone, it loops over the eight consecutive registers from
> 0x0a22.
> 
>>> +	delta = rtw_phy_pwrtrack_get_delta(rtwdev, RF_PATH_A);
>>> +
>>> +	/* NOTE: also done in rtw_phy_pwrtrack_get_delta */
> 
>> Is it necessary to do it again?
> 
> No. The clamp is only in rtw_phy_pwrtrack_get_delta() now.
> 
>>> +iqk:
>>> +	if (do_iqk)
>>> +		rtw8723b_phy_calibration(rtwdev);
> 
>> The vendor driver doesn't redo IQK here.
> 
> 8723d and 8703b in rtw88 both call phy_calibration() from phy_pwrtrack()
> when rtw_phy_pwrtrack_need_iqk() returns true, so I kept it and match the
> other chips.

Different chips can have different needs...

> 
>>> +static void rtw8723b_pwr_track(struct rtw_dev *rtwdev)
>>> +{
>>> +	struct rtw_efuse *efuse = &rtwdev->efuse;
>>> +	struct rtw_dm_info *dm_info = &rtwdev->dm_info;
>>> +
>>> +	if (efuse->power_track_type != 0) {
>>> +		rtw_warn(rtwdev, "unsupported power track type\n");
> 
>> This function runs every two seconds, forever. Please put this
>> warning elsewhere, like rtw8723b_read_efuse(), to avoid filling
>> the kernel log. (Or create rtw_warn_once()).
> 
> Moved to rtw8723b_read_efuse().
> 
>>> +static void rtw8723b_coex_cfg_ant_buffer(struct rtw_dev *rtwdev)
>>> +{
>>> +	u8 sys_func_before;
>>> +
>>> +	sys_func_before = rtw_read8(rtwdev, REG_SYS_FUNC_EN);
>>> +	if ((sys_func_before & WLAN_SYS_FUNC_BB_ENABLE) !=
>>> +	    WLAN_SYS_FUNC_BB_ENABLE) {
>>> +		rtw_write8_set(rtwdev, REG_SYS_FUNC_EN,
>>> +			       WLAN_SYS_FUNC_BB_ENABLE);
>>> +		usleep_range(10, 11);
>>> +	}
> 
>> Not sure why you need to touch REG_SYS_FUNC_EN here. The vendor
>> driver doesn't do it.
> 
> That is gone from rtw8723b_coex_cfg_ant_buffer().
> 
>>> +	rtw_write8_set(rtwdev, REG_PWR_DATA + 1,
>>> +		       BIT_EEPRPAD_RFE_CTRL_EN >> 8);
>>> +	rtw8723b_coex_write8_verify(rtwdev, REG_RFE_CTRL_E, 0xff);
> 
>> The vendor driver uses a normal rtw_write8() here.
> 
> coex_write8_verify() is gone and the call is a plain rtw_write8() now.
> 
>>> +		rtw_write8_set(rtwdev, REG_SYS_FUNC_EN,
>>> +			       WLAN_SYS_FUNC_BB_ENABLE);
>>> +		usleep_range(10, 11);
>>> +	}
>>> +
>>> +	rtw_write32(rtwdev, REG_BB_SEL_BTG, value);
>>> +	readback = rtw_read32(rtwdev, REG_BB_SEL_BTG);
>>> +	if (readback == value)
>>> +		return readback;
>>> +
>>> +	rtw_write8_set(rtwdev, REG_SYS_FUNC_EN, WLAN_SYS_FUNC_BB_ENABLE);
>>> +	usleep_range(10, 11);
>>> +	rtw_write32(rtwdev, REG_BB_SEL_BTG, value);
>>> +
>>> +	return rtw_read32(rtwdev, REG_BB_SEL_BTG);
> 
>> All this looks strange too. What happens if you eliminate this function
>> and write REG_BB_SEL_BTG with a simple rtw_write32()?
> 
> Nothing, it works. The function is gone and every call site does a plain
> rtw_write32() now.
> 
>>> +	.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,
>>> +	/* REG_CSRATIO does not exist on this chip generation. */
>>> +	.cck_pd_set		= NULL,
> 
>> You can use rtw88xxa_phy_cck_pd_set() from rtw88xxa.c for this
>> chip too.
> 
> Done, and it has a consequence you should know about. The chip driver
> calls into rtw88xxa.c now, so RTW88_8723B selects RTW88_88XXA, which makes
> rtw88_8723b.ko depend on rtw88_88xxa.ko and pulls that PCI chip module
> into an SDIO only build. 8821A and 8812A select it the same way, it
> builds and loads clean, and it was on the card for a suite and a soak.
> Say so if you would rather not have the dependency and I will put the
> function in the chip driver instead. The same file carries the adaptive
> control and EDCA init now as well, because the chip was writing the same
> values, so the dependency does more than that one call.
> 

A better idea: move rtw88xxa_phy_cck_pd_set() to phy.c and don't depend
on rtw88_88xxa. That can be a separate patch, of course.

>>> +	.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 = 1,
> 
>> The RTL8723BU vendor driver v5.2.17 sets this to 6.
> 
> usb_tx_agg_desc_num is 6 now.
> 
>>> +	/* 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,
>>> +	.dig_cck = rtw8723x_common.dig_cck,
> 
>> This chip doesn't have dig_cck.
> 
> You are right, and I had to reread the vendor tree to figure out why.
> ODM_IC_PHY_STATUE_NEW_TYPE is 8197F | 8822B | 8723D | 8821C | 8710B, no
> 8723B in it, so that write never runs here. dig_cck is NULL.
> 
> Best regards,
> Luka Gejak