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

['Greg KH']

On Wed, Jul 31, 2024 at 03:55:05PM +0900, Manjae Cho wrote:
> > 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.

You aren't going to use them elsewhere, worry about this then, not now.

> 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
> */

That seems a lot more sane and simpler.

thanks,

greg k-h