1. Patchew
  2. linux
  3. [RESEND net-next v5 0/4] net: phy: realtek: Add support for PHY LEDs on
  4. View patch

[RESEND net-next v5 1/4] net: phy: realtek: Group RTL82* macro definitions

Michael Klein posted 4 patches 10 months, 1 week ago
[RESEND net-next v5 1/4] net: phy: realtek: Group RTL82* macro definitions
Posted by Michael Klein 10 months, 1 week ago 
Group macro definitions by chip number in lexicographic order.

Signed-off-by: Michael Klein <michael@fossekall.de>
---
 drivers/net/phy/realtek/realtek_main.c | 30 +++++++++++++-------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
index 893c82479671..b27c0f995e56 100644
--- a/drivers/net/phy/realtek/realtek_main.c
+++ b/drivers/net/phy/realtek/realtek_main.c
@@ -17,6 +17,15 @@
 
 #include "realtek.h"
 
+#define RTL8201F_ISR				0x1e
+#define RTL8201F_ISR_ANERR			BIT(15)
+#define RTL8201F_ISR_DUPLEX			BIT(13)
+#define RTL8201F_ISR_LINK			BIT(11)
+#define RTL8201F_ISR_MASK			(RTL8201F_ISR_ANERR | \
+						 RTL8201F_ISR_DUPLEX | \
+						 RTL8201F_ISR_LINK)
+#define RTL8201F_IER				0x13
+
 #define RTL821x_PHYSR				0x11
 #define RTL821x_PHYSR_DUPLEX			BIT(13)
 #define RTL821x_PHYSR_SPEED			GENMASK(15, 14)
@@ -31,6 +40,10 @@
 #define RTL821x_EXT_PAGE_SELECT			0x1e
 #define RTL821x_PAGE_SELECT			0x1f
 
+#define RTL8211E_CTRL_DELAY			BIT(13)
+#define RTL8211E_TX_DELAY			BIT(12)
+#define RTL8211E_RX_DELAY			BIT(11)
+
 #define RTL8211F_PHYCR1				0x18
 #define RTL8211F_PHYCR2				0x19
 #define RTL8211F_CLKOUT_EN			BIT(0)
@@ -47,6 +60,8 @@
 #define RTL8211F_LEDCR_MASK			GENMASK(4, 0)
 #define RTL8211F_LEDCR_SHIFT			5
 
+#define RTL8211F_LED_COUNT			3
+
 #define RTL8211F_TX_DELAY			BIT(8)
 #define RTL8211F_RX_DELAY			BIT(3)
 
@@ -54,19 +69,6 @@
 #define RTL8211F_ALDPS_ENABLE			BIT(2)
 #define RTL8211F_ALDPS_XTAL_OFF			BIT(12)
 
-#define RTL8211E_CTRL_DELAY			BIT(13)
-#define RTL8211E_TX_DELAY			BIT(12)
-#define RTL8211E_RX_DELAY			BIT(11)
-
-#define RTL8201F_ISR				0x1e
-#define RTL8201F_ISR_ANERR			BIT(15)
-#define RTL8201F_ISR_DUPLEX			BIT(13)
-#define RTL8201F_ISR_LINK			BIT(11)
-#define RTL8201F_ISR_MASK			(RTL8201F_ISR_ANERR | \
-						 RTL8201F_ISR_DUPLEX | \
-						 RTL8201F_ISR_LINK)
-#define RTL8201F_IER				0x13
-
 #define RTL822X_VND1_SERDES_OPTION			0x697a
 #define RTL822X_VND1_SERDES_OPTION_MODE_MASK		GENMASK(5, 0)
 #define RTL822X_VND1_SERDES_OPTION_MODE_2500BASEX_SGMII		0
@@ -112,8 +114,6 @@
 #define RTL_8221B_VN_CG				0x001cc84a
 #define RTL_8251B				0x001cc862
 
-#define RTL8211F_LED_COUNT			3
-
 MODULE_DESCRIPTION("Realtek PHY driver");
 MODULE_AUTHOR("Johnson Leung");
 MODULE_LICENSE("GPL");
-- 
2.39.5
Re: [RESEND net-next v5 1/4] net: phy: realtek: Group RTL82* macro definitions
Posted by Joe Damato 10 months ago 
On Mon, Apr 07, 2025 at 08:21:40PM +0200, Michael Klein wrote:
> Group macro definitions by chip number in lexicographic order.
> 
> Signed-off-by: Michael Klein <michael@fossekall.de>
> ---
>  drivers/net/phy/realtek/realtek_main.c | 30 +++++++++++++-------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
> index 893c82479671..b27c0f995e56 100644
> --- a/drivers/net/phy/realtek/realtek_main.c
> +++ b/drivers/net/phy/realtek/realtek_main.c
> @@ -17,6 +17,15 @@
>  
>  #include "realtek.h"
>  
> +#define RTL8201F_ISR				0x1e
> +#define RTL8201F_ISR_ANERR			BIT(15)
> +#define RTL8201F_ISR_DUPLEX			BIT(13)
> +#define RTL8201F_ISR_LINK			BIT(11)
> +#define RTL8201F_ISR_MASK			(RTL8201F_ISR_ANERR | \
> +						 RTL8201F_ISR_DUPLEX | \
> +						 RTL8201F_ISR_LINK)
> +#define RTL8201F_IER				0x13

If sorting lexicographically, wouldn't RTL8201F_IER come before
RTL8201F_ISR ?

>  #define RTL821x_PHYSR				0x11
>  #define RTL821x_PHYSR_DUPLEX			BIT(13)
>  #define RTL821x_PHYSR_SPEED			GENMASK(15, 14)
> @@ -31,6 +40,10 @@
>  #define RTL821x_EXT_PAGE_SELECT			0x1e
>  #define RTL821x_PAGE_SELECT			0x1f
>  
> +#define RTL8211E_CTRL_DELAY			BIT(13)
> +#define RTL8211E_TX_DELAY			BIT(12)
> +#define RTL8211E_RX_DELAY			BIT(11)

Maybe I'm reading this wrong but these don't seem sorted
lexicographically ?
Re: [RESEND net-next v5 1/4] net: phy: realtek: Group RTL82* macro definitions
Posted by Andrew Lunn 10 months ago 
On Mon, Apr 07, 2025 at 07:52:50PM -0700, Joe Damato wrote:
> On Mon, Apr 07, 2025 at 08:21:40PM +0200, Michael Klein wrote:
> > Group macro definitions by chip number in lexicographic order.
> > 
> > Signed-off-by: Michael Klein <michael@fossekall.de>
> > ---
> >  drivers/net/phy/realtek/realtek_main.c | 30 +++++++++++++-------------
> >  1 file changed, 15 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
> > index 893c82479671..b27c0f995e56 100644
> > --- a/drivers/net/phy/realtek/realtek_main.c
> > +++ b/drivers/net/phy/realtek/realtek_main.c
> > @@ -17,6 +17,15 @@
> >  
> >  #include "realtek.h"
> >  
> > +#define RTL8201F_ISR				0x1e
> > +#define RTL8201F_ISR_ANERR			BIT(15)
> > +#define RTL8201F_ISR_DUPLEX			BIT(13)
> > +#define RTL8201F_ISR_LINK			BIT(11)
> > +#define RTL8201F_ISR_MASK			(RTL8201F_ISR_ANERR | \
> > +						 RTL8201F_ISR_DUPLEX | \
> > +						 RTL8201F_ISR_LINK)
> > +#define RTL8201F_IER				0x13
> 
> If sorting lexicographically, wouldn't RTL8201F_IER come before
> RTL8201F_ISR ?

The change log says "chip_number" lexicographic order. RTL8201F is the
chip number, ISR is the register name.

You would normally sub sort register number, so i would of put
IER=0x13 before ISR=0x1e, within RTL8201F.

> 
> >  #define RTL821x_PHYSR				0x11
> >  #define RTL821x_PHYSR_DUPLEX			BIT(13)
> >  #define RTL821x_PHYSR_SPEED			GENMASK(15, 14)
> > @@ -31,6 +40,10 @@
> >  #define RTL821x_EXT_PAGE_SELECT			0x1e
> >  #define RTL821x_PAGE_SELECT			0x1f
> >  
> > +#define RTL8211E_CTRL_DELAY			BIT(13)
> > +#define RTL8211E_TX_DELAY			BIT(12)
> > +#define RTL8211E_RX_DELAY			BIT(11)
> 
> Maybe I'm reading this wrong but these don't seem sorted
> lexicographically ?

This i don't follow, you normally keep register bits next to the
register. This is particularly important when the register bits don't
have the register name embedded within it.

    Andrew

---
pw-bot: cr
Re: [RESEND net-next v5 1/4] net: phy: realtek: Group RTL82* macro definitions
Posted by Michael Klein 10 months ago 
On Tue, Apr 08, 2025 at 02:17:19PM +0200, Andrew Lunn wrote:
>On Mon, Apr 07, 2025 at 07:52:50PM -0700, Joe Damato wrote:
>> On Mon, Apr 07, 2025 at 08:21:40PM +0200, Michael Klein wrote:
>> >  #define RTL821x_PHYSR				0x11
>> >  #define RTL821x_PHYSR_DUPLEX			BIT(13)
>> >  #define RTL821x_PHYSR_SPEED			GENMASK(15, 14)
>> > @@ -31,6 +40,10 @@
>> >  #define RTL821x_EXT_PAGE_SELECT			0x1e
>> >  #define RTL821x_PAGE_SELECT			0x1f
>> >
>> > +#define RTL8211E_CTRL_DELAY			BIT(13)
>> > +#define RTL8211E_TX_DELAY			BIT(12)
>> > +#define RTL8211E_RX_DELAY			BIT(11)
>>
>> Maybe I'm reading this wrong but these don't seem sorted
>> lexicographically ?
>
>This i don't follow, you normally keep register bits next to the
>register. This is particularly important when the register bits don't
>have the register name embedded within it.

Well, there is no definition for the register these bits pertain to, 
and adding one in _this_ patch would break the scope of this change. So 
I will address this in a later patch of this series in the next version.

-- 
Michael
Re: [RESEND net-next v5 1/4] net: phy: realtek: Group RTL82* macro definitions
Posted by Russell King (Oracle) 10 months ago 
On Tue, Apr 08, 2025 at 02:17:19PM +0200, Andrew Lunn wrote:
> This i don't follow, you normally keep register bits next to the
> register. This is particularly important when the register bits don't
> have the register name embedded within it.

Agreed - the worst thing is when one reads driver code, where the
registers offsets are all defined one after each other, and the
individual register bits are defined elsewhere and without prefixes
that identify which register they pertain to or comments that identify
that.

So yes, please keep register bits and bitfield definitions next to
the register offset definition they pertain to, it's way nicer to
read that way.

Also, having register offset definitions sorted by offset means when
reading documentation, locating the definitions actually used is much
easier. Using the same value (hex or decimal) as the documentation
also aids this.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.