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

["Luka Gejak" <luka.gejak@linux.dev>]

On Wed Sep 23, 2026 at 10:30 AM CEST, Ping-Ke Shih wrote:
> Luka Gejak <luka.gejak@linux.dev> wrote:
>> Add the Realtek RTL8723B 802.11n chip driver: the chip operations, the
>> power sequences, the efuse layout, the RF and IQ calibration, and the
>> chip specific coexistence handling.
>> 

Thanks for the review. Everything that could be turned into a change is
in v4, and six of your points are worth an answer as much as a change,
so here they are.

>> +#include <linux/unaligned.h>
>> +#include "main.h"
>> +#include "coex.h"
> [...]
>> +#include "tx.h"
>
> In increasing alphabet order.

The list is already in increasing alphabetical order after main.h, and
main.h cannot be sorted into place. The other rtw88 headers have no
includes of their own and are written expecting main.h to be first: it is
what brings in struct rtw_dev and even BIT() and GENMASK(). With mac.h
ahead of main.h the build stops after 179 error lines, the first of them

  'struct rtw_dev' declared inside parameter list will not be visible
  outside of this definition or declaration
  implicit declaration of function 'GENMASK'
  passing argument 1 of 'rtw_set_channel_mac' from incompatible pointer
  type

I tried the sort and reverted it. A strict order needs main.h added to
those headers first. I can send that as a separate patch ahead of this
series, but I would rather not fold a shared header change into the chip
driver.

>> +#define MASK_NETTYPE   0x30000
>> +#define _NETTYPE(x)    (((x) & 0x3) << 16)
>> +#define NT_LINK_AP     0x2
>
> The PORT_SET_NET_TYPE in rtw_vif_port_config() does similar thing
> relying on static const struct rtw_vif_port rtw_vif_port[].
>
> Is that not suitable for RTL8723B? If so, should the common flow
> avoid RTL8723B?

It is suitable, and the common flow does not need to avoid RTL8723B.
PORT_SET_NET_TYPE writes the same field with the same value:
rtw_vif_port[0].net_type is address 0x0100 with mask 0x30000, which is
REG_CR bits 17:16, and rtwvif->net_type is RTW_NET_MGD_LINKED, 2, the
value NT_LINK_AP carried.

Only the timing differs, and there the common flow is the better one.
rtw_ops_add_interface() sets net_type from the interface type,
RTW_NET_NO_LINK for a station, and the association path moves it to
RTW_NET_MGD_LINKED, writing REG_CR after both. The chip local call ran
during mac_init and forced the linked value before there was a link,
and the core's write replaced it a moment later in any case.

So v4 drops rtw8723b_init_network_type() with MASK_NETTYPE, _NETTYPE()
and NT_LINK_AP, and the chip relies on rtw_vif_port[] like the others.
No other rtw88 chip defines a net type value of its own.

>> +       /* Override the default rcr filter for 8723B */
>> +       rtwdev->hal.rcr = WLAN_RCR_CFG;
>
> Why? The default value doesn't work to RTL8723B?

Two things say it does not.

hal.rcr is not only written at init. fw.c clears and restores
BIT_CBSSID_BCN in it around the beacon filter, and mac80211.c toggles
BIT_AM, so whatever is left there has to keep those bits.

And this is not a private filter. It is the RCR that rtw8723x_mac_init()
writes for the whole 8723x family, 0x700060ce, with BIT_APP_FCS added.
The generic default is a different set: it has BIT_PKTCTL_DLEN and lacks
BIT_CBSSID_DATA, BIT_CBSSID_BCN and BIT_AMF, so it drops exactly the bits
fw.c manipulates on this chip.

BIT_APP_FCS has to be set because rtw88 advertises RX_INCLUDES_FCS for
every chip and the shared 8723x value has bit 31 clear; without it
mac80211 trims four bytes of real frame data. v4 says this at the
assignment.

>> +       rtw8723b_init_adaptive_ctrl(rtwdev);
>> +       rtw8723b_init_edca(rtwdev);
>> +       rtw8723b_init_retry_function(rtwdev);
>
> So RTL8723B is very different from existing chips?

No, and the siblings write the same registers with the same values.

rtw8703b_phy_set_param() writes REG_SPEC_SIFS, REG_MAC_SPEC_SIFS, REG_SIFS
and REG_SIFS + 2 with the same 0x100a, ACKTO is 0x40 in both, and
REG_RETRY_LIMIT uses the same 0x3030 in both. Its four EDCA registers are
the same magnitudes as here:

  8703b  0x002FA226 VO  0x005EA324 VI  0x005EA42B BE  0x0000A44F BK
  8723b  0x002FA226 VO  0x005EA324 VI  0x005EA42B BE  0x0000A44F BK

rtw88xxa.c writes REG_RRSR with the same 0xfffff / 0xffff1 pair used here,
and REG_RESP_SIFS_CCK and REG_RESP_SIFS_OFDM are the pair that only this
chip and 8822c write. So I read the three helpers as the family pattern
rather than as something chip specific. Say the word if you would rather
have them inlined into rtw8723b_phy_set_param() the way 8703b has them;
that is a smaller diff.

>> +static bool rtw8723b_sdio_needs_rx_path_fix(struct rtw_dev *rtwdev)
>
> What does it mean?
>
> In many places using this function are not RX path.

Right, the name hid what it is, and it is a test the preparation series
already provides. The function gates the SDIO only register work: the PAD
mux restore in post_enable_flow, the path control save and restore around
IQK, and the trailing re-assert in set_channel. v4 drops the local helper
and calls rtw_is_8723bs() at those seven places, which is what rx.c, tx.c
and sdio.c already use.

One more, where the review asked for a change that was already there.
The declarations in rtw8723b_reassert_rx_path() that you marked for
reverse X'mas order are already longest first, three u32 lines followed
by two u8 lines, and they are the same in the sent v3 and in v4. If you
had a different order in mind, tell me which and I will apply it.

I re-ran the hardware validation on this exact tip after these changes.
The suite and the soak both pass: association, WPA2, DHCP, throughput,
latency, scans under traffic, 20/20 reload/reassociate, link cycles and
reconnects, with no error, TX-report, H2C, LPS or lockdep lines in dmesg,
and no regression against the branch that was validated before.

Best regards,
Luka Gejak