> On Wed, Jul 31, 2024 at 12:50:54AM +0900, Manjae Cho wrote:
> > This patch improves the usage of the MAR register by updating the
> > relevant macro definitions and ensuring consistent usage across the
> > codebase.
> >
> > Signed-off-by: Manjae Cho <manjae.cho@xxxxxxxxxxx>
> >
> > ---
> > drivers/staging/rtl8723bs/hal/sdio_halinit.c | 4 ++--
> > drivers/staging/rtl8723bs/include/hal_com_reg.h | 3 +++
> > 2 files changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8723bs/hal/sdio_halinit.c
> > b/drivers/staging/rtl8723bs/hal/sdio_halinit.c
> > index c9cd6578f7f8..9493562c1619 100644
> > --- a/drivers/staging/rtl8723bs/hal/sdio_halinit.c
> > +++ b/drivers/staging/rtl8723bs/hal/sdio_halinit.c
> > @@ -380,8 +380,8 @@ static void _InitWMACSetting(struct adapter
> *padapter)
> > rtw_write32(padapter, REG_RCR, pHalData->ReceiveConfig);
> >
> > /* Accept all multicast address */
> > - rtw_write32(padapter, REG_MAR, 0xFFFFFFFF);
> > - rtw_write32(padapter, REG_MAR + 4, 0xFFFFFFFF);
> > + rtw_write32(padapter, MAR0, 0xFFFFFFFF);
> > + rtw_write32(padapter, MAR4, 0xFFFFFFFF);
> >
> > /* Accept all data frames */
> > value16 = 0xFFFF;
> > diff --git a/drivers/staging/rtl8723bs/include/hal_com_reg.h
> > b/drivers/staging/rtl8723bs/include/hal_com_reg.h
> > index 9a02ae69d7a4..baf326d53a46 100644
> > --- a/drivers/staging/rtl8723bs/include/hal_com_reg.h
> > +++ b/drivers/staging/rtl8723bs/include/hal_com_reg.h
> > @@ -151,6 +151,9 @@
> > #define REG_BSSID 0x0618
> > #define REG_MAR
0x0620
> >
> > +#define MAR0 REG_MAR
> /* Multicast Address Register, Offset 0x0620-0x0623 */
>
> Why redefine this value again? What is wrong with using it as "REG_MAR"?
> Is this fixing anything or making anything more consistent somewhere?
> It's only used in one place that I can see.
>
> thanks,
>
> greg k-h
Dear Greg,
Thank you for your feedback. I appreciate your point about the current usage
of REG_MAR. While it's true that it's only used in one place currently, I
believe there's value in making this change for the following reasons:
- Consistency: Other similar registers in the codebase use this pattern.
For example, we have IDR0 and IDR4 for MACID registers. Adding MAR0 and MAR4
brings consistency to our register naming convention.
- Clarity: The +4 offset in "REG_MAR + 4" isn't immediately clear without
context. MAR4 makes it explicit that we're dealing with the next 4 bytes of
the Multicast Address Register.
- If we need to use these registers elsewhere in the future, having clear,
specific names will make the code more readable.
However, I understand if you feel this change doesn't provide enough benefit
to justify inclusion. If you prefer, I could modify the patch to keep the
REG_MAR usage but add comments for clarity:
/* Multicast Address Register */
rtw_write32(padapter, REG_MAR, 0xFFFFFFFF); /* Offset 0x0620-0x0623
*/
rtw_write32(padapter, REG_MAR + 4, 0xFFFFFFFF); /* Offset 0x0624-0x0627
*/
I'm open to your guidance on which approach you think.
Thank you
Best Regards
