Improve MAR register definition and usage for rtl8723

[PATCH] Improve MAR register definition and usage for rtl8723

Posted by Manjae Cho 1 year, 4 months ago

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@samsung.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 */
+#define MAR4						(REG_MAR + 4)	/* Multicast Address Register, Offset 0x0624-0x0627 */
+
 #define REG_MAC_SPEC_SIFS				0x063A
 /*  20100719 Joseph: Hardware register definition change. (HW datasheet v54) */
 #define REG_RESP_SIFS_CCK				0x063C	/*  [15:8]SIFS_R2T_OFDM, [7:0]SIFS_R2T_CCK */
-- 
2.25.1

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

Posted by Greg KH 1 year, 4 months ago

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@samsung.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

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

Posted by Manjae Cho 1 year, 4 months ago

> 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@samsung.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.

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

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

Posted by 'Greg KH' 1 year, 4 months ago

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@samsung.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

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

Posted by Manjae Cho 1 year, 4 months ago

> 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@samsung.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

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

Posted by Philipp Hortmann 1 year, 4 months ago

On 7/30/24 17:50, 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@samsung.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 */
> +#define MAR4						(REG_MAR + 4)	/* Multicast Address Register, Offset 0x0624-0x0627 */
> +
>   #define REG_MAC_SPEC_SIFS				0x063A
>   /*  20100719 Joseph: Hardware register definition change. (HW datasheet v54) */
>   #define REG_RESP_SIFS_CCK				0x063C	/*  [15:8]SIFS_R2T_OFDM, [7:0]SIFS_R2T_CCK */


Hi Manjae,

please start your Subject according to the expected standard:
staging: rtl8723bs:

The description seems to have a line break that should not be there.

please use checkpatch for your patches:
File Nr: 0    Patch: ../../../Downloads/20240730-[PATCH] Improve MAR 
register definition and usage -17243.txt
WARNING: line length of 126 exceeds 100 columns
#41: FILE: drivers/staging/rtl8723bs/include/hal_com_reg.h:154:
+#define MAR0						REG_MAR		/* Multicast Address Register, Offset 
0x0620-0x0623 */

WARNING: line length of 126 exceeds 100 columns
#42: FILE: drivers/staging/rtl8723bs/include/hal_com_reg.h:155:
+#define MAR4						(REG_MAR + 4)	/* Multicast Address Register, Offset 
0x0624-0x0627 */


If you send in a second version of this patch please use a change 
history. Description from Dan under:
https://staticthinking.wordpress.com/2022/07/27/how-to-send-a-v2-patch/

In case of questions feel free to contact me directly.

Thanks for your support.

Bye Philipp

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

Posted by Greg KH 1 year, 4 months ago

On Tue, Jul 30, 2024 at 08:55:22PM +0200, Philipp Hortmann wrote:
> In case of questions feel free to contact me directly.

No, please keep discussions and help on the mailing list, direct emails
is not a good way to do kernel development, sorry.

greg k-h