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

[Manjae Cho]

> 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

Dear Greg,

Thank you for your guidance. I appreciate your perspective on keeping the
code simple and addressing current needs rather than potential future uses.

I agree that adding comments for clarity is a more straightforward approach.
I'll revise the patch accordingly

I'll submit this updated patch shortly. Thank you again for your time and
feedback.

Thank you
Best Regards

Manjae Cho