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

["Luka Gejak" <luka.gejak@linux.dev>]

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