[PATCH staging] staging: rtl8723bs: replace magic numbers in rtl8723b_InitBeaconParameters()

[PATCH staging] staging: rtl8723bs: replace magic numbers in rtl8723b_InitBeaconParameters()

[Marcos Garcia]

Replace hardcoded values in rtl8723b_InitBeaconParameters() with defined constants
TBTT_PROHIBIT_VENDOR_DEFAULT (0x6404) and BCNTCFG_AIFS_MAX (0x660F) for clarity and
maintainability, addressing the TODO comment in the code.

The values were sourced from the following documentation:
- REG_TBTT_PROHIBIT (Offset 0x0540): Bits [15:8] = 0x64 (100ms prohibit time, 1ms units),
  Bits [7:0] = 0x04 (2ms margin, 0.5ms units), as per RTL8723BS Datasheet v1.5,
  Section 7.3.1.5 and RTL8723BS Programming Guide, p. 112.
- REG_BCNTCFG (Offset 0x0510): 0x660F sets max AIFS (0x0F) to prioritize beacon
  transmission, as per RTL8723BS Datasheet v1.5, Section 7.3.1.3.

Hi Dan,

Thank you for your detailed feedback — I truly appreciate it. I tried to contact you
earlier, but it seems my email didn't reach you. This is my first kernel contribution,
and I started by addressing TODO comments, thinking they were straightforward. I now
realize even these changes require deep hardware understanding. I used AI to assist with
parts of the commit message, but I didn't review it thoroughly enough, and I take full
responsibility for the vague comments. I could only find limited references to these
values, and the documentation seems restricted. I apologize for any oversight and
promise to research more carefully in the future. Thank you for your guidance.

Signed-off-by: Marcos Garcia <magazo2005@gmail.com>
---
 drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
index 893cab0532ed..009d9010d72d 100644
--- a/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
+++ b/drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c
@@ -1183,6 +1183,10 @@ void rtl8723b_read_chip_version(struct adapter *padapter)
 	ReadChipVersion8723B(padapter);
 }
 
+#define TBTT_PROHIBIT_VENDOR_DEFAULT  0x6404
+
+#define BCNTCFG_AIFS_MAX  0x660F
+
 void rtl8723b_InitBeaconParameters(struct adapter *padapter)
 {
 	struct hal_com_data *pHalData = GET_HAL_DATA(padapter);
@@ -1197,8 +1201,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_VENDOR_DEFAULT);
 	/*  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)
@@ -1207,7 +1210,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_AIFS_MAX);
 
 	pHalData->RegBcnCtrlVal = rtw_read8(padapter, REG_BCN_CTRL);
 	pHalData->RegTxPause = rtw_read8(padapter, REG_TXPAUSE);
-- 
2.50.0

Re: [PATCH staging] staging: rtl8723bs: replace magic numbers in rtl8723b_InitBeaconParameters()

[Greg KH]

On Tue, Jul 01, 2025 at 09:40:49AM +0200, Marcos Garcia wrote:
> 
> 
> Replace hardcoded values in rtl8723b_InitBeaconParameters() with defined constants
> TBTT_PROHIBIT_VENDOR_DEFAULT (0x6404) and BCNTCFG_AIFS_MAX (0x660F) for clarity and
> maintainability, addressing the TODO comment in the code.
> 
> The values were sourced from the following documentation:
> - REG_TBTT_PROHIBIT (Offset 0x0540): Bits [15:8] = 0x64 (100ms prohibit time, 1ms units),
>   Bits [7:0] = 0x04 (2ms margin, 0.5ms units), as per RTL8723BS Datasheet v1.5,
>   Section 7.3.1.5 and RTL8723BS Programming Guide, p. 112.
> - REG_BCNTCFG (Offset 0x0510): 0x660F sets max AIFS (0x0F) to prioritize beacon
>   transmission, as per RTL8723BS Datasheet v1.5, Section 7.3.1.3.
> 
> Hi Dan,
> 
> Thank you for your detailed feedback — I truly appreciate it. I tried to contact you
> earlier, but it seems my email didn't reach you. This is my first kernel contribution,
> and I started by addressing TODO comments, thinking they were straightforward. I now
> realize even these changes require deep hardware understanding. I used AI to assist with
> parts of the commit message, but I didn't review it thoroughly enough, and I take full
> responsibility for the vague comments. I could only find limited references to these
> values, and the documentation seems restricted. I apologize for any oversight and
> promise to research more carefully in the future. Thank you for your guidance.

None of this needs to be in the changelog text, please read the
documentation for how to properly submit patches, especially second
version of patches.

thanks,

greg k-h

Re: [PATCH staging] staging: rtl8723bs: replace magic numbers in rtl8723b_InitBeaconParameters()

[Greg KH]

On Tue, Jul 01, 2025 at 10:02:54AM +0200, Greg KH wrote:
> On Tue, Jul 01, 2025 at 09:40:49AM +0200, Marcos Garcia wrote:
> > 
> > 
> > Replace hardcoded values in rtl8723b_InitBeaconParameters() with defined constants
> > TBTT_PROHIBIT_VENDOR_DEFAULT (0x6404) and BCNTCFG_AIFS_MAX (0x660F) for clarity and
> > maintainability, addressing the TODO comment in the code.
> > 
> > The values were sourced from the following documentation:
> > - REG_TBTT_PROHIBIT (Offset 0x0540): Bits [15:8] = 0x64 (100ms prohibit time, 1ms units),
> >   Bits [7:0] = 0x04 (2ms margin, 0.5ms units), as per RTL8723BS Datasheet v1.5,
> >   Section 7.3.1.5 and RTL8723BS Programming Guide, p. 112.
> > - REG_BCNTCFG (Offset 0x0510): 0x660F sets max AIFS (0x0F) to prioritize beacon
> >   transmission, as per RTL8723BS Datasheet v1.5, Section 7.3.1.3.
> > 
> > Hi Dan,
> > 
> > Thank you for your detailed feedback — I truly appreciate it. I tried to contact you
> > earlier, but it seems my email didn't reach you. This is my first kernel contribution,
> > and I started by addressing TODO comments, thinking they were straightforward. I now
> > realize even these changes require deep hardware understanding. I used AI to assist with
> > parts of the commit message, but I didn't review it thoroughly enough, and I take full
> > responsibility for the vague comments. I could only find limited references to these
> > values, and the documentation seems restricted. I apologize for any oversight and
> > promise to research more carefully in the future. Thank you for your guidance.
> 
> None of this needs to be in the changelog text, please read the
> documentation for how to properly submit patches, especially second
> version of patches.

Make that the third version of this patch.

Re: [PATCH staging] staging: rtl8723bs: replace magic numbers in rtl8723b_InitBeaconParameters()

[Dan Carpenter]

On Tue, Jul 01, 2025 at 09:40:49AM +0200, Marcos Garcia wrote:
> 
> 
> Replace hardcoded values in rtl8723b_InitBeaconParameters() with defined constants
> TBTT_PROHIBIT_VENDOR_DEFAULT (0x6404) and BCNTCFG_AIFS_MAX (0x660F) for clarity and
> maintainability, addressing the TODO comment in the code.
> 
> The values were sourced from the following documentation:
> - REG_TBTT_PROHIBIT (Offset 0x0540): Bits [15:8] = 0x64 (100ms prohibit time, 1ms units),
>   Bits [7:0] = 0x04 (2ms margin, 0.5ms units), as per RTL8723BS Datasheet v1.5,
>   Section 7.3.1.5 and RTL8723BS Programming Guide, p. 112.
> - REG_BCNTCFG (Offset 0x0510): 0x660F sets max AIFS (0x0F) to prioritize beacon
>   transmission, as per RTL8723BS Datasheet v1.5, Section 7.3.1.3.
> 
> Hi Dan,
> 
> Thank you for your detailed feedback — I truly appreciate it. I tried to contact you
> earlier, but it seems my email didn't reach you. This is my first kernel contribution,
> and I started by addressing TODO comments, thinking they were straightforward. I now
> realize even these changes require deep hardware understanding. I used AI to assist with
> parts of the commit message, but I didn't review it thoroughly enough, and I take full
> responsibility for the vague comments. I could only find limited references to these
> values, and the documentation seems restricted. I apologize for any oversight and
> promise to research more carefully in the future. Thank you for your guidance.
> 

I feel like there needs to be some kind of notice on patches which are
generated with AI.  I was so puzzled when it said that the units were
in half miliseconds, but AI sounds so confident and professional and
people do crazy things all the time.

I have sent my fair share of guesswork patch where it's like "I think
the code is correct, but it's just indented incorrectly" or whatever.
And I try to put a note to say that it's guesswork so, "Please, review
this one extra carefully."  With AI it's all 100% guesswork, but it's
formatted as if it's quoting a spec.  There is a lot of potential for
confusion.

regards,
dan carpenter