RE: [PATCH] Improve MAR register definition and usage for rtl8723

[Manjae Cho]

> 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