rtl8723bs: Is REG_TBTT_PROHIBIT (0x6404) a worthwhile magic-number cleanup candidate?
-
Hello, I am a computer science undergraduate currently studying Linux kernel drivers.
While analyzing the drivers/staging/rtl8723bs/hal/rtl8723b_hal_init.c driver, I noticed the following code:
rtw_write16(padapter, REG_TBTT_PROHIBIT, 0x6404);
There is also a nearby TODO comment:
/* TODO: Remove these magic number */
rtw_write16(padapter, REG_TBTT_PROHIBIT, 0x6404);At first I assumed this was simply a magic-number cleanup issue, but after tracing the surrounding code and register definitions, the value seems to carry more hardware-specific meaning than I initially expected.
What I found
Near the REG_TBTT_PROHIBIT definition, I found the following comments:// [3:0] : TBTT prohibit setup in unit of 32us
// [19:8] : TBTT prohibit hold in unit of 32us#define REG_TBTT_PROHIBIT 0x0540
Based on this, I interpreted 0x6404 as:
setup field = 0x04
hold field = 0x64And my understanding is that after write16():
the lower byte maps to 0x540
the upper byte maps to 0x541There are also nearby timing-related register writes such as:
rtw_write8(padapter, REG_ATIMWND, 0x0a); /* 10ms */
rtw_write16(padapter, REG_TSFTR_SYN_OFFSET, 0x7fff);which made me think that 0x6404 is likely a hardware timing/tuning value related to beacon/TBTT handling.
I also checked upstream drivers and found the exact same value still present in rtl8xxxu:
rtl8xxxu_write16(priv, REG_TBTT_PROHIBIT, 0x6404);
Current concern
At this point, my impression is that:the value itself appears meaningful as a hardware timing parameter,
it could be rewritten using symbolic bitfield macros,
but since the same raw value has remained in upstream drivers for years, I am unsure whether such a cleanup would actually be considered valuable.For example, I was considering something like:
#define TBTT_SETUP_TIME(_v) (((_v) & 0xf) << 0 )
#define TBTT_HOLD_TIME(_v) (((_v) & 0xff) << 8 )/* setup = 0x04, hold = 0x64 */
TBTT_SETUP_TIME(0x04) | TBTT_HOLD_TIME(0x64)However, I am not sure whether this would meaningfully improve readability/maintainability, or simply make a hardware tuning constant more verbose.
Questions
Would maintainers generally consider this kind of change a worthwhile cleanup patch?
In cases like this, where the value appears to represent a hardware timing/tuning parameter, is it usually preferred to keep the raw constant?
If a staging TODO remains for many years while the same value also exists upstream, how should that usually be interpreted?I would appreciate any advice regarding both the analysis itself and whether this kind of patch is worth pursuing.