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

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

[Adith-Joshua]

Replace hardcoded magic numbers in rtl8723b_InitBeaconParameters()
with descriptive macros to improve readability and maintainability.

No functional changes.

Signed-off-by: Adith-Joshua <adithalex29@gmail.com>
---
 .../staging/rtl8723bs/hal/rtl8723b_hal_init.c   | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
index 8d259820f103..41f561120af5 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
@@ -864,6 +864,18 @@ void rtl8723b_read_chip_version(struct adapter *padapter)
 	ReadChipVersion8723B(padapter);
 }
 
+#define TBTT_PROHIBIT_HOLD_TIME_MS   0x64
+#define TBTT_PROHIBIT_SETUP_TIME_MS  0x04
+
+#define TBTT_PROHIBIT_VALUE \
+	((TBTT_PROHIBIT_HOLD_TIME_MS << 8) | TBTT_PROHIBIT_SETUP_TIME_MS)
+
+#define BCNTCFG_AIFS 0x66
+#define BCNTCFG_CW   0x0F
+
+#define BCNTCFG_VALUE \
+	((BCNTCFG_AIFS << 8) | BCNTCFG_CW)
+
 void rtl8723b_InitBeaconParameters(struct adapter *padapter)
 {
 	struct hal_com_data *pHalData = GET_HAL_DATA(padapter);
@@ -878,8 +890,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_VALUE);/*  ms */
 	/*  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 +899,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_VALUE);
 
 	pHalData->RegBcnCtrlVal = rtw_read8(padapter, REG_BCN_CTRL);
 	pHalData->RegTxPause = rtw_read8(padapter, REG_TXPAUSE);
-- 
2.53.0

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

[Dan Carpenter]

On Sat, Apr 11, 2026 at 10:58:17AM +0530, Adith-Joshua wrote:
> Replace hardcoded magic numbers in rtl8723b_InitBeaconParameters()
> with descriptive macros to improve readability and maintainability.
> 
> No functional changes.
> 
> Signed-off-by: Adith-Joshua <adithalex29@gmail.com>
> ---

Where did you find these numbers?  Presumably there is a spec
somewhere?  What does BCNTCFG_CW mean?  This really looks like an AI
patch...

regards,
dan carpenter

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

[Luka Gejak]

On Sat Apr 11, 2026 at 10:07 AM CEST, Dan Carpenter wrote:
> On Sat, Apr 11, 2026 at 10:58:17AM +0530, Adith-Joshua wrote:
>> Replace hardcoded magic numbers in rtl8723b_InitBeaconParameters()
>> with descriptive macros to improve readability and maintainability.
>> 
>> No functional changes.
>> 
>> Signed-off-by: Adith-Joshua <adithalex29@gmail.com>
>> ---
>
> Where did you find these numbers?  Presumably there is a spec
> somewhere?  What does BCNTCFG_CW mean?  This really looks like an AI
> patch...
>
> regards,
> dan carpenter

Hi Dan,
I believe you are right about this patch being written by AI. 
As I noted in my other reply, if you cross-reference these registers 
with the primary upstream driver 
(drivers/net/wireless/realtek/rtw88/reg.h), the field mappings 
introduced in this patch are actually backwards. It looks like an AI saw
the 0x660F magic number and hallucinated that 0x66 was the aifs and 0x0F
was the cw, when in reality aifs occupies the lower 8 bits. It also 
hallucinates an _MS suffix based on a loose code comment, completely 
ignoring that 802.11 beacon offsets use time units (TU).
Best regards,
Luka Gejak

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

[Dan Carpenter]

On Sat, Apr 11, 2026 at 08:04:56PM +0530, Adith Joshua wrote:
> Hi Dan,
> 
> Thanks for taking a look at my patch.
> 
> These values come from the RTL8723BS register layout(as in original vendor
> driver)
> 
> For TBTT_PROHIBIT (0x6404), the register is split into two fields. The
> upper byte(0x64) is the TBTT prohibit hold time. The lower byte(0x04) is
> the setup/margin time before TBTT

Is this available online somewhere?  I did manage to find TBTT_PROHIBIT
on google for the rtl8188e driver and the explanation is here:

https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/drivers/staging/rtl8188eu/include/rtl8188e_spec.h#296

It says that the "TBTT prohibit setup in unit of 32us." but the patch
says it is in units of ms so that's wrong.

> 
> For BCNTCFG(0x660F) the upper byte(0x66) configures AIFS value for beacon
> transmission. The lower byte(0x0F) sets the contention window to the max
> value

I wasn't able to verify any of this.  Please provide us a link to
your original vendor driver.

> 
> The intention of this change was to replace the unclear hardcoded values
> with proper macros(like the todo said)
> 
> If it is needed I’ll send a v2 with more explicit names and brief comments
> for the bit fields
> 

*grumble* It's honestly, still not clear if you used AI to write the
patch or not.

regards,
dan carpenter

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

[Dan Carpenter]

On Mon, Apr 13, 2026 at 10:51:16AM +0530, Adith Joshua wrote:
> Hi Dan, Luka,
> 
> Thanks for the review.
> 
> Made some careless errors there... sorry about that
> 
> Looking at it again, I see that the BCNTCFG field mapping in my patch is
> wrong (I swapped AIFS and CW in the naming). The TBTT_PROHIBIT suffix was
> also an assumption based on a vague comment, which I shouldn’t have relied
> on.
> 
> I was looking at:
> https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/drivers/staging/rtl8188eu/include/rtl8188e_spec.h#296
> 
> and also cross-referencing with rtw88:
> drivers/net/wireless/realtek/rtw88/reg.h
> 
> And no, this wasn’t AI generated 🙂(I know the choice of emoji could be
> better)
> 
> I'll send a v2 with these corrections.
> 

Please don't bother.

This explanation is again far too vague to be checked.  I would
prefer to leave it as-is until we are more sure that the information
is correct.  Misleading documentation is worse than having none.

regards,
dan carpenter

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

[Luka Gejak]

On Sat Apr 11, 2026 at 7:28 AM CEST, Adith-Joshua wrote:
> Replace hardcoded magic numbers in rtl8723b_InitBeaconParameters()
> with descriptive macros to improve readability and maintainability.
>
> No functional changes.
>
> Signed-off-by: Adith-Joshua <adithalex29@gmail.com>
> ---
>  .../staging/rtl8723bs/hal/rtl8723b_hal_init.c   | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> index 8d259820f103..41f561120af5 100644
> --- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> +++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
> @@ -864,6 +864,18 @@ void rtl8723b_read_chip_version(struct adapter *padapter)
>  	ReadChipVersion8723B(padapter);
>  }
>  
> +#define TBTT_PROHIBIT_HOLD_TIME_MS   0x64
> +#define TBTT_PROHIBIT_SETUP_TIME_MS  0x04

Please drop the _MS suffix here. While the legacy comment says "ms", 
802.11 beacon offsets are almost universally measured in time units 
(TU, which are 1024 microseconds). 0x64 is 100 TU. Baking "MS" into the 
macro name based on a loose comment introduces false precision. Please 
fix the field mappings and send a v2. I highly recommend taking a look 
at how rtw88 handles these exact same registers 
(REG_BCNTCFG and REG_TBTT_PROHIBIT) and trying to align your macros with
their structure where possible.

> +
> +#define TBTT_PROHIBIT_VALUE \
> +	((TBTT_PROHIBIT_HOLD_TIME_MS << 8) | TBTT_PROHIBIT_SETUP_TIME_MS)
> +
> +#define BCNTCFG_AIFS 0x66
> +#define BCNTCFG_CW   0x0F
> +
> +#define BCNTCFG_VALUE \
> +	((BCNTCFG_AIFS << 8) | BCNTCFG_CW)

This is completely backwards. If you cross-reference this with the 
primary upstream rtw88 driver (drivers/net/wireless/realtek/rtw88/reg.h)
, aifs is actually the lower 8 bits (GENMASK(7, 0)) and cw is in the 
upper bits. So 0x0F is the aifs (a typical max value of 15), and 0x66 
represents the cwmax and cwmin values. While your macro happens to 
evaluate to the original 0x660F magic number, the field names are 
swapped and would actively mislead future developers.

> +
>  void rtl8723b_InitBeaconParameters(struct adapter *padapter)
>  {
>  	struct hal_com_data *pHalData = GET_HAL_DATA(padapter);
> @@ -878,8 +890,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_VALUE);/*  ms */
>  	/*  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 +899,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_VALUE);
>  
>  	pHalData->RegBcnCtrlVal = rtw_read8(padapter, REG_BCN_CTRL);
>  	pHalData->RegTxPause = rtw_read8(padapter, REG_TXPAUSE);

Best regards,
Luka Gejak

[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