Re: [PATCH net-next v6 3/9] net: dsa: realtek: rtl8365mb: set speed for family D

[Luiz Angelo Daros de Luca]

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

>  /* 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.

> +/* 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;

[...]

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

> +               /* 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