Re: [PATCH net-next v6 3/9] net: dsa: realtek: rtl8365mb: set speed for family D
flat view
From: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Date: 2026-10-18 22:41:01
Also in:
lkml
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c index 840db5a..2dabc4d 100644 --- a/drivers/net/dsa/realtek/rtl8365mb_main.c +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c@@ -344,6 +344,7 @@ #define RTL8365MB_PORT_SPEED_10M 0 #define RTL8365MB_PORT_SPEED_100M 1 #define RTL8365MB_PORT_SPEED_1000M 2 +#define RTL8365MB_D_PORT_SPEED_2500M 5
I also used the same _<FAMILY>_ tag trick previously, but it gets confusing quickly. Does RTL8365MB_D_xxx mean something exclusive to family D, or family D and later? I hope a more seasoned reviewer can weigh in on this. When I implemented support for the RTL8367R (pre-family B), I ran into this exact problem of macro definitions (and code conditions) turning into unmaintainable spaghetti. In this particular case, as 5 does not exist in Family C and the semantics of the existing values still hold, you can omit the _D_. A simple comment will be enough if the semantics change for a future family.
quoted hunk ↗ jump to hunk
/* External interface force configuration registers 0~2 */ #define RTL8365MB_DIGITAL_INTERFACE_FORCE_REG0 0x1310 /* EXT0 */@@ -361,6 +362,28 @@ #define RTL8365MB_DIGITAL_INTERFACE_FORCE_LINK_MASK BIT(4) #define RTL8365MB_DIGITAL_INTERFACE_FORCE_DUPLEX_MASK BIT(2) #define RTL8365MB_DIGITAL_INTERFACE_FORCE_SPEED_MASK GENMASK(1, 0) +/* Width of the FORCE_SPEED field, bits[1:0] */ +#define RTL8365MB_DIGITAL_INTERFACE_FORCE_SPEED_WIDTH 2
This is what bitfield masks/macros are for; there is no need for an explicit WIDTH define. More comments below on the code using it.
quoted hunk ↗ jump to hunk
+/* bits[13:12]; bit 13 reserved, no defined speed value uses it yet */ +#define RTL8365MB_D_DIGITAL_INTERFACE_FORCE_SPEED2_MASK GENMASK(13, 12) + +#define RTL8365MB_D_DIGITAL_INTERFACE_FORCE_REG_BASE 0x12c0 +#define RTL8365MB_D_DIGITAL_INTERFACE_FORCE_REG(_port) \ + (RTL8365MB_D_DIGITAL_INTERFACE_FORCE_REG_BASE + (_port)) + +#define RTL8365MB_D_DIGITAL_INTERFACE_FORCE_EN_BASE 0x12c8 +#define RTL8365MB_D_DIGITAL_INTERFACE_FORCE_REG_EN(_port) \ + (RTL8365MB_D_DIGITAL_INTERFACE_FORCE_EN_BASE + (_port)) + +/* Per-port force-mode enable. The vendor RTL8367D API + * (dal_rtl8367d_port_macForceLink_set()) treats it as an on/off switch for + * the whole forced configuration: all bits set when the MAC is forced, zero + * when it is not. The meaning of the individual bits is not documented. + * This driver always forces the MAC, as on family C, so all bits are set + * on every call, including link down. + */ +#define RTL8365MB_D_DIGITAL_INTERFACE_FORCE_EN_ALL_MASK GENMASK(15, 0) /* CPU port mask register - controls which ports are treated as CPU ports */ #define RTL8365MB_CPU_PORT_MASK_REG 0x1219@@ -1646,23 +1669,32 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port, u32 r_duplex; u32 r_speed; u32 r_link; + bool is_d;
Thinking about future families like E, F, G..., a boolean flag like
is_d might not scale well.
You can still keep the family id in a variable to keep lines short and clean:
enum rtl8365mb_family family;
int val;
int ret;[...]
quoted hunk ↗ jump to hunk
FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_LINK_MASK, r_link) | FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_DUPLEX_MASK, r_duplex) | - FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_SPEED_MASK, r_speed); - ret = regmap_write(priv->map, - RTL8365MB_DIGITAL_INTERFACE_FORCE_REG(extint->id), - val); - if (ret) - return ret; + FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_SPEED_MASK, + r_speed & + RTL8365MB_DIGITAL_INTERFACE_FORCE_SPEED_MASK);
Isn't the point of FIELD_GET/FIELD_PREP to avoid bitwise operations like this? For fields split between bit ranges, you can use the same trick as in RTL8365MB_CVLAN_MBR_LO_MASK, RTL8365MB_CVLAN_MBR_HI_MASK.
+
+ if (is_d) {
For multiple/bigger differences, we will need to have different
functions for each family. In this case, for small differences, I
think is_d, is_e, is_f... are better handled by a switch statement
with the same indentation cost as an if:
switch (family) {
case RTL8365MB_FAMILY_D:
...
break;
case RTL8365MB_FAMILY_C:
...
}
And, by omitting the default clause, when we add a future FAMILY_E and
forget to add a case for it, the compiler will warn us of the missing
check. You can even use a (commented) fallthrough when a family
incrementally expands a feature.
quoted hunk ↗ jump to hunk
+ /* Speed is 3 bits on family D: bits[1:0] go into FORCE_SPEED, + * bit[2] goes into FORCE_SPEED2 (bit 12); only 2500M sets it, + * bit 13 of the field is unused. + */ + val |= FIELD_PREP(RTL8365MB_D_DIGITAL_INTERFACE_FORCE_SPEED2_MASK, + r_speed >> + RTL8365MB_DIGITAL_INTERFACE_FORCE_SPEED_WIDTH);
Here is where FIELD_GET() with a HI/LO mask avoids the manual shift and makes the WIDTH definition unnecessary. Best regards, Luiz