Re: [PATCH net-next v7 10/10] net: dsa: realtek: rtl8365mb: add support for RTL8367S-VB

[Mieczyslaw Nalewaj]

On Tue, Nov 03, 2026 at 11:52:24PM +0100, Alvin Šipraga wrote:
> Hi Mieczyslaw,
>
> First of all, apologies for such a late review! I hope you appreciate
> the feedback anyways.

Hi Alvin,

Thanks for the review, no need to apologise. Your comments crossed
with the v8 I had just sent, so they are not included there. I will
address them in v9.

> This is OK, but in the same spirit as my comment to your other patch, I
> think we want to make the C and D families equal. So why not:
>
> -#define RTL8365MB_MAX_NUM_PORTS	11
> +#define RTL8365MB_C_MAX_NUM_PORTS	11
> +#define RTL8365MB_D_MAX_NUM_PORTS	8
>
> and add a num_ports member to struct rtl8365mb_chip_info. Then below:
[...]
> The above becomes something like
>
> priv->num_ports = mb->chip_info->num_ports;

Agreed. In v9 I'll rename the constants as you suggest, add num_ports
to struct rtl8365mb_chip_info and use it in rtl8365mb_detect().

> Another thing I noticed in the L2 patch was that you added a macro:
>
> #define RTL8365MB_D_L2_MAX_PORT 7
[...]
> Which I wonder, can you even get there if the num_ports is 8? We didn't
> need that protection for family C. I think you can drop the macro and
> this check.

Right, with num_ports capped at 8 the core never passes a port above 7,
same as on family C. I'll drop the macro and the related checks in v9.

> The reason I'm saying this is that I saw quite a few places where you
> have a construct of the form:
>
> if (family is d)
>   // use family d constant
> else
>   // use family c constant
>
> Which can become a bit hard to follow/maintain.
[...]
> I hope it's possible to remove most of the if (family is x) { do
> something } constructs and keep things flat.

Understood. I'll wait for the rest of your comments on v8 and then
move per-family constants into chip_info where it makes sense, rather
than branching in the code.

Thanks,
Mieczyslaw