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

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

Posted by Adith-Joshua 2 months ago

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

Posted by Luka Gejak 2 months ago

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

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

Posted by Dan Carpenter 2 months ago

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

Posted by Luka Gejak 2 months ago

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