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

[Ping-Ke Shih]

Luka Gejak <luka.gejak@xxxxxxxxx> wrote:
> Hi Ping-Ke,
> 
> September 24, 2026 at 06:11, "Ping-Ke Shih" <pkshih@xxxxxxxxxxx> wrote:
> >
> > Luka Gejak <luka.gejak@xxxxxxxxx> wrote:
> 
> Answers in the order of your mail. The MAINTAINERS point is in a
> separate reply.
> 
> > Can you also review other RTL8723B specific functions? I didn't review
> > them one by one by v3, but I wonder why it needs specific functions,
> > not common flow. If any of them is necessary, please point out reasons.
> >
> > By the way, I didn't only mention these three functions. At here there
> > are many specific functions. Please analyze them.
> > (Honestly, I don't fully re-examinate your analysis in detail, and
> > believe your results.)
> 
> I went through the whole file. The part that is already common flow can
> be listed exactly, because those ops point at shared code:
> 
>   power_on, power_off        rtw_power_on, rtw_power_off
>   mac_postinit               rtw8723x_mac_postinit
>   set_tx_power_index         rtw8723x_set_tx_power_index
>   false_alarm_statistics     rtw8723x_false_alarm_statistics
>   read_rf, write_rf          rtw_phy_read_rf_sipi,
>                              rtw_phy_write_rf_reg_sipi
>   read_efuse                 rtw8723x_read_efuse, plus the hardware
>                              capability, because this chip has no
>                              hardware feature report: the firmware
>                              reports id 0xfd instead of the C2H
>   efuse_grant                rtw8723x_efuse_grant, plus the 0x6b BT
>                              power cut and output isolation write that
>                              the vendor efuse path does
> 

I think you have checked them. Please reconsider to rewrite them. 

As Johannes mentioned, LLM is a tool, but please not fully believe
and rely on it. LLM can generate a lot of stuff, but I read by my
eyes and then think and type by my hands. To understand and rework
stuff generated by LLM is submitter's business.

> 
> > I think the better way is to assign proper rtwdev->hal.rcr per chip
> > in rtw_core_init().
> 

[..]

> 
> If you prefer, I can put the core change in a small patch before the
> chip series rather than in patch 3, so the chip series stays free of
> core changes.

Yes, that'd be good. 

The 8723BS specific stuff should do in the kind of rewriting. 

> 
> > Can you reuse the existing since they are the same?
> 

I think I can refer a common rule... 

> 
> > (Please quote the code; to reply to this, I need to switch to your
> > patches again).
> On your last sentence, that the reason is hard to explain later if it is
> not written down: whatever we change will carry its reason in the code
> or in the commit message, not only in this mail. The same goes for the
> two things I am asking to keep, cfg_ldo25() and the RESP_SIFS writes,
> which will say why they are there.

It is still hard to me to recall what I wrote at last sentence...

Let's follow Bitterblue's comments on v4, and move to v5.

I noted the copyright aren't all consistent. Check them yourself.

Ping-Ke