[PATCH] staging: rtl8723bs: replace magic numbers with named constants

[PATCH] staging: rtl8723bs: replace magic numbers with named constants

[Jad Keskes]

Remove the magic numbers in rtl8723b_InitBeaconParameters() as requested
by the outstanding TODO comment. Replace 0x6404 and 0x660F with named
constants defined in rtl8723b_hal.h alongside the existing beacon timing
constants.

Signed-off-by: Jad Keskes <inasj268@gmail.com>
---
 drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 5 ++---
 drivers/staging/rtl8723bs/include/rtl8723b_hal.h  | 2 ++
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
index e794fe3ca..c5da5d0be 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
@@ -878,8 +878,7 @@ void rtl8723b_InitBeaconParameters(struct adapter *padapter)
 
 	rtw_write16(padapter, REG_BCN_CTRL, val16);
 
-	/*  TODO: Remove these magic number */
-	rtw_write16(padapter, REG_TBTT_PROHIBIT, 0x6404);/*  ms */
+	rtw_write16(padapter, REG_TBTT_PROHIBIT, TBTT_PROHIBIT_TIME_8723B);
 	/*  Firmware will control REG_DRVERLYINT when power saving is enable, */
 	/*  so don't set this register on STA mode. */
 	if (check_fwstate(&padapter->mlmepriv, WIFI_STATION_STATE) == false)
@@ -888,7 +887,7 @@ void rtl8723b_InitBeaconParameters(struct adapter *padapter)
 
 	/*  Suggested by designer timchen. Change beacon AIFS to the largest number */
 	/*  because test chip does not contension before sending beacon. by tynli. 2009.11.03 */
-	rtw_write16(padapter, REG_BCNTCFG, 0x660F);
+	rtw_write16(padapter, REG_BCNTCFG, BCN_AIFS_CFG_8723B);
 
 	pHalData->RegBcnCtrlVal = rtw_read8(padapter, REG_BCN_CTRL);
 	pHalData->RegTxPause = rtw_read8(padapter, REG_TXPAUSE);
diff --git a/drivers/staging/rtl8723bs/include/rtl8723b_hal.h b/drivers/staging/rtl8723bs/include/rtl8723b_hal.h
index ffd039278..311acfa0f 100644
--- a/drivers/staging/rtl8723bs/include/rtl8723b_hal.h
+++ b/drivers/staging/rtl8723bs/include/rtl8723b_hal.h
@@ -71,6 +71,8 @@ struct rt_firmware_hdr {
 
 #define DRIVER_EARLY_INT_TIME_8723B  0x05
 #define BCN_DMA_ATIME_INT_TIME_8723B 0x02
+#define TBTT_PROHIBIT_TIME_8723B     0x6404
+#define BCN_AIFS_CFG_8723B           0x660F
 
 /* for 8723B */
 /* TX 32K, RX 16K, Page size 128B for TX, 8B for RX */
-- 
2.54.0

Re: [PATCH] staging: rtl8723bs: replace magic numbers with named constants

[Dan Carpenter]

On Wed, Jun 03, 2026 at 03:31:27PM +0100, Jad Keskes wrote:
> Remove the magic numbers in rtl8723b_InitBeaconParameters() as requested
> by the outstanding TODO comment. Replace 0x6404 and 0x660F with named
> constants defined in rtl8723b_hal.h alongside the existing beacon timing
> constants.
> 
> Signed-off-by: Jad Keskes <inasj268@gmail.com>

Just having the same define but as a goto doesn't add any information
or value.

This is actually the third attempt to do this.  The first two were AI
patches.  On the first time, I asked how the author came up with the
definition and the had the AI generate a halucinated fake spec.  On the
second patch, the AI found the first fake spec and assumed it was
correct.  :P

Your patch at least doesn't try to pass off any wrong information so
that's good.

I was able to find this explanation from an out of tree driver.

https://gitlab.elettra.eu/intel_socfpga/linux-socfpga/-/blob/socfpga-5.15.60-lts/drivers/net/wireless/realtek/rtw88/rtw8822b.c?ref_type=heads
#define WLAN_TBTT_PROHIBIT	0x04 /* unit : 32us */
#define WLAN_TBTT_HOLD_TIME	0x064 /* unit : 32us */

But it's weird that the comment here says ms.  I don't know what
is correct.  That driver doesn't have any explanation for the 0x660f
value.

regards,
dan carpenter

Re: [PATCH] staging: rtl8723bs: replace magic numbers with named constants

[Dan Carpenter]

On Thu, Jun 04, 2026 at 05:31:56PM +0300, Dan Carpenter wrote:
> On Wed, Jun 03, 2026 at 03:31:27PM +0100, Jad Keskes wrote:
> > Remove the magic numbers in rtl8723b_InitBeaconParameters() as requested
> > by the outstanding TODO comment. Replace 0x6404 and 0x660F with named
> > constants defined in rtl8723b_hal.h alongside the existing beacon timing
> > constants.
> > 
> > Signed-off-by: Jad Keskes <inasj268@gmail.com>
> 
> Just having the same define but as a goto doesn't add any information
> or value.

Ugh...  Sorry, I meant to say "just having the same value but as a define
doesn't add any information."

regards,
dan carpenter

[PATCH] staging: rtl8723bs: replace beacon timing magic numbers with named constants

[Jad Keskes]

Decompose the beacon timing magic numbers in
rtl8723b_InitBeaconParameters() into named constants with bitfield
expressions to document the register layout.

REG_TBTT_PROHIBIT (0x0540): The value 0x6404 splits into hold time
(0x64) in the upper byte and prohibit setup time (0x04) in the lower
byte, in 32us units — matching the layout used in the rtw88 driver
(WLAN_TBTT_HOLD_TIME << 8 | WLAN_TBTT_PROHIBIT).

REG_BCNTCFG (0x0510): The value 0x660F follows the same bitfield
layout as the adjacent EDCA AC parameter registers:
AIFS = bits[7:0] = 0x0F (disables contention before beacon Tx);
CWmin = bits[11:8] = 0x06; CWmax = bits[15:12] = 0x06.
This layout is confirmed by rtl8192cu which writes 0x66FF (test chips)
and 0x660F (normal chips) — only AIFS varies, the CW byte stays 0x66.

Drop the misleading "ms" unit comment on REG_TBTT_PROHIBIT and the
outstanding TODO marker since this was the last item it referenced.

Signed-off-by: Jad Keskes <inasj268@gmail.com>
---
 drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c |  5 ++---
 drivers/staging/rtl8723bs/include/rtl8723b_hal.h  | 13 +++++++++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
index e794fe3ca..4afba1252 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
@@ -878,8 +878,7 @@ void rtl8723b_InitBeaconParameters(struct adapter *padapter)
 
 	rtw_write16(padapter, REG_BCN_CTRL, val16);
 
-	/*  TODO: Remove these magic number */
-	rtw_write16(padapter, REG_TBTT_PROHIBIT, 0x6404);/*  ms */
+	rtw_write16(padapter, REG_TBTT_PROHIBIT, TBTT_PROHIBIT_TIME_8723B);
 	/*  Firmware will control REG_DRVERLYINT when power saving is enable, */
 	/*  so don't set this register on STA mode. */
 	if (check_fwstate(&padapter->mlmepriv, WIFI_STATION_STATE) == false)
@@ -888,7 +887,7 @@ void rtl8723b_InitBeaconParameters(struct adapter *padapter)
 
 	/*  Suggested by designer timchen. Change beacon AIFS to the largest number */
 	/*  because test chip does not contension before sending beacon. by tynli. 2009.11.03 */
-	rtw_write16(padapter, REG_BCNTCFG, 0x660F);
+	rtw_write16(padapter, REG_BCNTCFG, BCNTCFG_8723B);
 
 	pHalData->RegBcnCtrlVal = rtw_read8(padapter, REG_BCN_CTRL);
 	pHalData->RegTxPause = rtw_read8(padapter, REG_TXPAUSE);
diff --git a/drivers/staging/rtl8723bs/include/rtl8723b_hal.h b/drivers/staging/rtl8723bs/include/rtl8723b_hal.h
index ffd039278..81105c55a 100644
--- a/drivers/staging/rtl8723bs/include/rtl8723b_hal.h
+++ b/drivers/staging/rtl8723bs/include/rtl8723b_hal.h
@@ -72,6 +72,19 @@ struct rt_firmware_hdr {
 #define DRIVER_EARLY_INT_TIME_8723B  0x05
 #define BCN_DMA_ATIME_INT_TIME_8723B 0x02
 
+/* REG_TBTT_PROHIBIT (0x0540) - TBTT prohibit hold/setup in 32us units */
+#define TBTT_PROHIBIT_SETUP_8723B 0x04
+#define TBTT_PROHIBIT_HOLD_8723B  0x64
+#define TBTT_PROHIBIT_TIME_8723B \
+	((TBTT_PROHIBIT_HOLD_8723B << 8) | TBTT_PROHIBIT_SETUP_8723B)
+
+/* REG_BCNTCFG (0x0510) - beacon AIFS, CWmin, CWmax (EDCA-like layout) */
+#define BCN_AIFS_8723B   0x0F
+#define BCN_CW_MIN_8723B 0x06
+#define BCN_CW_MAX_8723B 0x06
+#define BCNTCFG_8723B \
+	((BCN_CW_MAX_8723B << 12) | (BCN_CW_MIN_8723B << 8) | BCN_AIFS_8723B)
+
 /* for 8723B */
 /* TX 32K, RX 16K, Page size 128B for TX, 8B for RX */
 #define PAGE_SIZE_TX_8723B 128
-- 
2.54.0

Re: [PATCH] staging: rtl8723bs: replace beacon timing magic numbers with named constants

[Greg KH]

On Thu, Jun 04, 2026 at 04:18:28PM +0100, Jad Keskes wrote:
> Decompose the beacon timing magic numbers in
> rtl8723b_InitBeaconParameters() into named constants with bitfield
> expressions to document the register layout.
> 
> REG_TBTT_PROHIBIT (0x0540): The value 0x6404 splits into hold time
> (0x64) in the upper byte and prohibit setup time (0x04) in the lower
> byte, in 32us units — matching the layout used in the rtw88 driver
> (WLAN_TBTT_HOLD_TIME << 8 | WLAN_TBTT_PROHIBIT).

Where did this info come from?

> 
> REG_BCNTCFG (0x0510): The value 0x660F follows the same bitfield
> layout as the adjacent EDCA AC parameter registers:
> AIFS = bits[7:0] = 0x0F (disables contention before beacon Tx);
> CWmin = bits[11:8] = 0x06; CWmax = bits[15:12] = 0x06.
> This layout is confirmed by rtl8192cu which writes 0x66FF (test chips)
> and 0x660F (normal chips) — only AIFS varies, the CW byte stays 0x66.

Same here, where did you get this information from?

Did you use a LLM for this change?

thanks,

greg k-h

Re: [PATCH] staging: rtl8723bs: replace beacon timing magic numbers with named constants

[Jad Keskes]

On 07/08, Greg KH wrote:
> On Wed, Jul 08, 2026 at 03:17:02PM +0100, Jad Keskes wrote:
> 
> <snip>
> 
> For some reason you sent this only to me, which is a bit rude to
> everyone else on the mailing list.  I'll be glad to respond if you
> resend it to everyone.
> 
> thanks,
> 
> greg k-h

Apologies, meant to send that to the list. Resending:

Yeah, that info is from the out-of-tree driver Dan linked in the TODO thread.  
REG_TBTT_PROHIBIT has the same hold/setup layout as rtw88's WLAN_TBTT_HOLD_TIME / WLAN_TBTT_PROHIBIT.

REG_BCNTCFG — check rtl8192cu in the tree. Same register, same 0x66 CW byte. 
Production writes 0x660F, test chips write 0x66FF. Only the AIFS changes.

Re: [PATCH] staging: rtl8723bs: replace beacon timing magic numbers with named constants

[Greg KH]

On Wed, Jul 08, 2026 at 03:36:23PM +0100, Jad Keskes wrote:
> On 07/08, Greg KH wrote:
> > On Wed, Jul 08, 2026 at 03:17:02PM +0100, Jad Keskes wrote:
> > 
> > <snip>
> > 
> > For some reason you sent this only to me, which is a bit rude to
> > everyone else on the mailing list.  I'll be glad to respond if you
> > resend it to everyone.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Apologies, meant to send that to the list. Resending:
> 
> Yeah, that info is from the out-of-tree driver Dan linked in the TODO thread.  

What thread?  Point to it please in the changelog.

> REG_TBTT_PROHIBIT has the same hold/setup layout as rtw88's WLAN_TBTT_HOLD_TIME / WLAN_TBTT_PROHIBIT.
> 
> REG_BCNTCFG — check rtl8192cu in the tree. Same register, same 0x66 CW byte. 
> Production writes 0x660F, test chips write 0x66FF. Only the AIFS changes.

Great, document this and where you got the info from in the changelog so
that it doesn't look like you just made it up out of the air :)

thanks,

greg k-h

Re: [PATCH] staging: rtl8723bs: replace beacon timing magic numbers with named constants

[Jad Keskes]

Ping. Any chance this can be picked up?

Re: [PATCH] staging: rtl8723bs: replace beacon timing magic numbers with named constants

[Greg Kroah-Hartman]

On Tue, Jun 16, 2026 at 05:29:04PM +0100, Jad Keskes wrote:
> Ping. Any chance this can be picked up?
> 

I have no context here :(

[PATCH] staging: rtl8723bs: replace beacon timing magic numbers with named constants

[Jad Keskes]

Break down the 0x6404 and 0x660F in rtl8723b_InitBeaconParameters()
as requested by the TODO in the source.

REG_TBTT_PROHIBIT (0x0540): 0x6404 = hold (0x64) and setup (0x04),
both in 32us ticks.  Same layout as rtw88 (WLAN_TBTT_HOLD_TIME << 8 |
WLAN_TBTT_PROHIBIT in rtw88.h).

REG_BCNTCFG (0x0510): 0x660F is an EDCA-like register.  Lower byte is
AIFS (0x0F = no contention before beacon), next nibble is CWmin (0x06),
top nibble is CWmax (0x06).  Confirmed by rtl8192du/rtl8723ae in the
tree which write 0x66FF (test chips) and 0x660F (production) — only
AIFS varies, the CW byte stays 0x66.

The out-of-tree driver that Dan linked in the TODO discussion was the
reference for the bit assignments:
Link: https://lore.kernel.org/all/aiGMXBNQ0TbIGbpP@stanley.mountain/

Drop the TODO since this was the last thing it referenced.

Signed-off-by: Jad Keskes <inasj268@gmail.com>
Reviewed-by: Dan Carpenter <error27@gmail.com>
---
v2: Added Link: to Dan's TODO thread, expanded commit message per Greg's feedback
v3: Added in-tree references (rtw88, rtl8192du, rtl8723ae) and Dan's R-b tag

 drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c |  5 ++---
 drivers/staging/rtl8723bs/include/rtl8723b_hal.h  | 13 +++++++++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
index e794fe3caf9d..4afba1252161 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
@@ -878,8 +878,7 @@ void rtl8723b_InitBeaconParameters(struct adapter *padapter)
 
 	rtw_write16(padapter, REG_BCN_CTRL, val16);
 
-	/*  TODO: Remove these magic number */
-	rtw_write16(padapter, REG_TBTT_PROHIBIT, 0x6404);/*  ms */
+	rtw_write16(padapter, REG_TBTT_PROHIBIT, TBTT_PROHIBIT_TIME_8723B);
 	/*  Firmware will control REG_DRVERLYINT when power saving is enable, */
 	/*  so don't set this register on STA mode. */
 	if (check_fwstate(&padapter->mlmepriv, WIFI_STATION_STATE) == false)
@@ -888,7 +887,7 @@ void rtl8723b_InitBeaconParameters(struct adapter *padapter)
 
 	/*  Suggested by designer timchen. Change beacon AIFS to the largest number */
 	/*  because test chip does not contension before sending beacon. by tynli. 2009.11.03 */
-	rtw_write16(padapter, REG_BCNTCFG, 0x660F);
+	rtw_write16(padapter, REG_BCNTCFG, BCNTCFG_8723B);
 
 	pHalData->RegBcnCtrlVal = rtw_read8(padapter, REG_BCN_CTRL);
 	pHalData->RegTxPause = rtw_read8(padapter, REG_TXPAUSE);
diff --git a/drivers/staging/rtl8723bs/include/rtl8723b_hal.h b/drivers/staging/rtl8723bs/include/rtl8723b_hal.h
index ffd03927841c..81105c55afb2 100644
--- a/drivers/staging/rtl8723bs/include/rtl8723b_hal.h
+++ b/drivers/staging/rtl8723bs/include/rtl8723b_hal.h
@@ -72,6 +72,19 @@ struct rt_firmware_hdr {
 #define DRIVER_EARLY_INT_TIME_8723B  0x05
 #define BCN_DMA_ATIME_INT_TIME_8723B 0x02
 
+/* REG_TBTT_PROHIBIT (0x0540) - TBTT prohibit hold/setup in 32us units */
+#define TBTT_PROHIBIT_SETUP_8723B 0x04
+#define TBTT_PROHIBIT_HOLD_8723B  0x64
+#define TBTT_PROHIBIT_TIME_8723B \
+	((TBTT_PROHIBIT_HOLD_8723B << 8) | TBTT_PROHIBIT_SETUP_8723B)
+
+/* REG_BCNTCFG (0x0510) - beacon AIFS, CWmin, CWmax (EDCA-like layout) */
+#define BCN_AIFS_8723B   0x0F
+#define BCN_CW_MIN_8723B 0x06
+#define BCN_CW_MAX_8723B 0x06
+#define BCNTCFG_8723B \
+	((BCN_CW_MAX_8723B << 12) | (BCN_CW_MIN_8723B << 8) | BCN_AIFS_8723B)
+
 /* for 8723B */
 /* TX 32K, RX 16K, Page size 128B for TX, 8B for RX */
 #define PAGE_SIZE_TX_8723B 128
-- 
2.55.0