RE: [PATCH v2 01/11] wifi: rtw88: add the RTL8723B chip type and SDIO helper

[Ping-Ke Shih]

Luka Gejak <luka.gejak@xxxxxxxxx> wrote:
> On July 30, 2026 11:17:41 AM GMT+02:00, Ping-Ke Shih <pkshih@xxxxxxxxxxx> wrote:
> >luka.gejak@xxxxxxxxx <luka.gejak@xxxxxxxxx> wrote:
> >> it would be four callbacks (free page check, page
> >> accounting, output queue wait, transfer sizing) and I did not want to
> >> introduce that structure without asking first.
> > If these are called from sdio.c, just check chip ID in sdio.c.
> 
> That is what v3 already does, so nothing changes there. All four are
> reached from sdio.c only.
> 
> > If they are used by common flow like coex.c, I prefer to implement
> > them by chip_ops.
> 
> Let me list them all so you can rule on all of them at once rather than
> a file at a time.
> 
> Two go regardless of your answer: the chip test inside
> rtw8723bs_apply_basic_rates() and rtw8723bs_apply_bss_cap() is
> redundant, both callers already establish the chip. In the second one
> the NL80211_IFTYPE_STATION half of the same guard is still needed, so
> only the chip half goes there.
> 
> That leaves the following outside sdio.c.
> 
> coex.c, 2 sites:
> 
>   rtw_coex_scan_notify()    -> rtw_coex_8723bs_scan_notify()
>   rtw_coex_connect_notify() -> rtw_coex_8723bs_connect_notify()
> 
>   Each tests the chip and bt_disabled once and returns true when it has
>   handled the notification; everything below them assumes both.
> 
> mac80211.c, 6 sites in rtw_ops_bss_info_changed(): the receive filter

Should receive filter implement in rtw_ops_configure_filter()?

> on assoc and on disassoc, BSSID change bookkeeping, and the
> BSS_CHANGED_ERP_PREAMBLE and BSS_CHANGED_ERP_SLOT handlers.
> 
> mac80211.c, 2 sites in rtw_ops_set_key(): enabling and disabling
> default key search for group keys.

Please check existing codes related to sec->default_key_search to see
how to support RTL8723BS by the same flow.

> 
> rx.c, 1 site in rtw_rx_fill_rx_status(), the zero length packet test.
> This one runs per received frame, so an indirect call there looks like
> the wrong trade to me. I would keep it inline unless you disagree.

I don't object this. More, I'm considering if no need to check chips, just
be a common flow.  

> 
> fw.c, 1 site in rtw_fw_write_data_rsvd_page(), two register
> save/restore blocks interleaved with the generic ones. I do not see how
> to lift those into an op without restructuring the whole function, so I
> would keep that inline too.

That looks fine. I remember I don't have comments on this part, no?

> 
> For coex.c and the two mac80211.c callbacks I am happy to add ops. One
> constraint on coex.c: the 8723BS code calls rtw_coex_set_ant_path() and
> rtw_coex_set_table(), which are static in coex.c. Implementing the ops
> in rtw8723b.c would mean making those two non-static, which seems worse
> than the chip test it replaces. What I had in mind is leaving the
> functions in coex.c, declaring them in coex.h, and having rtw8723b.c
> point the ops at them, the same shape as the chip_ops::prepare_tx that
> v3 adds for the association sequence. The generic helpers stay static
> that way.

Did you mean callees of rtw_coex_8723bs_scan_workaround() ?
I think you can go with your mind.

> 
> Tell me which of those you want as ops and I will do it.

I'd say the version copy many stuffs from vendor driver. However, we should
rewrite and consider the proper places, and if it is actually necessary. 
The vendor driver is based on cfg80211, and many stuffs have been done
by mac80211, so we don't need to implement them in rtw88. 

Ping-Ke