* [PATCH] Improve MAR register definition and usage for rtl8723 [not found] <CGME20240730155255epcas1p4ab3b5e88d400e2221aa1cf5cf234ea19@epcas1p4.samsung.com> @ 2024-07-30 15:50 ` Manjae Cho 2024-07-30 18:55 ` Philipp Hortmann 2024-07-31 6:16 ` Greg KH 0 siblings, 2 replies; 7+ messages in thread From: Manjae Cho @ 2024-07-30 15:50 UTC (permalink / raw) To: gregkh; +Cc: linux-staging, linux-kernel, Manjae Cho 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Improve MAR register definition and usage for rtl8723 2024-07-30 15:50 ` [PATCH] Improve MAR register definition and usage for rtl8723 Manjae Cho @ 2024-07-30 18:55 ` Philipp Hortmann 2024-07-31 5:28 ` Greg KH 2024-07-31 6:16 ` Greg KH 1 sibling, 1 reply; 7+ messages in thread From: Philipp Hortmann @ 2024-07-30 18:55 UTC (permalink / raw) To: Manjae Cho, gregkh; +Cc: linux-staging, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Improve MAR register definition and usage for rtl8723 2024-07-30 18:55 ` Philipp Hortmann @ 2024-07-31 5:28 ` Greg KH 0 siblings, 0 replies; 7+ messages in thread From: Greg KH @ 2024-07-31 5:28 UTC (permalink / raw) To: Philipp Hortmann; +Cc: Manjae Cho, linux-staging, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Improve MAR register definition and usage for rtl8723 2024-07-30 15:50 ` [PATCH] Improve MAR register definition and usage for rtl8723 Manjae Cho 2024-07-30 18:55 ` Philipp Hortmann @ 2024-07-31 6:16 ` Greg KH 2024-07-31 6:55 ` Manjae Cho 1 sibling, 1 reply; 7+ messages in thread From: Greg KH @ 2024-07-31 6:16 UTC (permalink / raw) To: Manjae Cho; +Cc: linux-staging, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] Improve MAR register definition and usage for rtl8723 2024-07-31 6:16 ` Greg KH @ 2024-07-31 6:55 ` Manjae Cho 2024-07-31 7:26 ` 'Greg KH' 0 siblings, 1 reply; 7+ messages in thread From: Manjae Cho @ 2024-07-31 6:55 UTC (permalink / raw) To: 'Greg KH'; +Cc: linux-staging, linux-kernel > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Improve MAR register definition and usage for rtl8723 2024-07-31 6:55 ` Manjae Cho @ 2024-07-31 7:26 ` 'Greg KH' 2024-07-31 8:01 ` Manjae Cho 0 siblings, 1 reply; 7+ messages in thread From: 'Greg KH' @ 2024-07-31 7:26 UTC (permalink / raw) To: Manjae Cho; +Cc: linux-staging, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] Improve MAR register definition and usage for rtl8723 2024-07-31 7:26 ` 'Greg KH' @ 2024-07-31 8:01 ` Manjae Cho 0 siblings, 0 replies; 7+ messages in thread From: Manjae Cho @ 2024-07-31 8:01 UTC (permalink / raw) To: 'Greg KH'; +Cc: linux-staging, linux-kernel > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-07-31 8:01 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20240730155255epcas1p4ab3b5e88d400e2221aa1cf5cf234ea19@epcas1p4.samsung.com>
2024-07-30 15:50 ` [PATCH] Improve MAR register definition and usage for rtl8723 Manjae Cho
2024-07-30 18:55 ` Philipp Hortmann
2024-07-31 5:28 ` Greg KH
2024-07-31 6:16 ` Greg KH
2024-07-31 6:55 ` Manjae Cho
2024-07-31 7:26 ` 'Greg KH'
2024-07-31 8:01 ` Manjae Cho
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox