[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <001101dae31f$d5ef44d0$81cdce70$@samsung.com>
Date: Wed, 31 Jul 2024 17:01:23 +0900
From: "Manjae Cho" <manjae.cho@...sung.com>
To: "'Greg KH'" <gregkh@...uxfoundation.org>
Cc: <linux-staging@...ts.linux.dev>, <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] Improve MAR register definition and usage for rtl8723
> 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@...sung.com>
> > > >
> > > > ---
> > > > 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
Powered by blists - more mailing lists