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

[Alvin Šipraga]

On Wed, Nov 04, 2026 at 10:40:11AM +0100, Mieczyslaw Nalewaj wrote:
> On Wed, Nov 04, 2026, Alvin Šipraga wrote:
> > I haven't yet had the time to look closely at all the patches [...].
> > But I'll try and take a closer look tomorrow and point out where else
> > we could improve in this direction. I hope it's possible to remove
> > most of the if (family is x) { do something } constructs and keep
> > things flat.
> 
> Hi Alvin,
> 
> While waiting for your comments I went through the series myself with
> that goal. Per-family constants and the L2 serializers are the cleanest
> case, so I'm taking them one step further than a num_ports member.
> Instead of adding more members to struct rtl8365mb_chip_info one by
> one, I'm grouping everything that depends only on the family into a
> single struct that chip_info points to:
> 
>   struct rtl8365mb_family_info {
>     enum rtl8365mb_family family;
>     unsigned int num_ports;
>     const struct rtl8365mb_jam_tbl_entry *jam_table;
>     size_t jam_size;
>     size_t l2_entry_size;
>     void (*l2_data_to_uc)(const u16 *data, struct rtl8365mb_l2_uc *uc);
>     void (*l2_uc_to_data)(const struct rtl8365mb_l2_uc *uc, u16 *data);
>     void (*l2_data_to_mc)(const u16 *data, struct rtl8365mb_l2_mc *mc);
>     void (*l2_mc_to_data)(const struct rtl8365mb_l2_mc *mc, u16 *data);
>   };
> 
> There is one instance for family C and one for family D, and the chip
> table entries only say:
> 
>     .family_info = &rtl8365mb_family_d,

Yes, sounds reasonable. Starts to resemble how mv88e6xxx does it.

> 
> > 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
> 
> For the L2 code this removes the construct entirely: the four
> rtl8365mb_l2_*() dispatchers and rtl8365mb_l2_entry_size() are gone,
> and the callers use fi->l2_uc_to_data(), fi->l2_entry_size and so on.
> The same goes for num_ports in detect() and the jam table in
> switch_init().
> 
> Some places differ in sequence and not only in constants, e.g. the
> SerDes configuration, the CVLAN entry packing and the PVID handling.
> I'd rather not hide those behind callbacks without a clear gain, but
> I'll go through them again and convert what reads better as
> per-family data or ops. If you see specific spots when you review the
> rest of the series, please point them out and I'll look at those
> first.

It seemed to me that VLAN would also benefit from being split
out somehow. I already tried to structure the VLAN/L2 APIs into somewhat
high- and low-level implementations, the former being accessed by
_main.c and the latter implemented in _{vlan,l2}.c. In the cases where
the binary format/address the chip expects is different, it seems
logical to then have different low-level implementations depending on
family. There might be some code duplication, but to the extent that
it's just incidental (e.g. 50% of the stuff is the same), it can still
improve the maintainability of the code IMO.

But if there's also extra steps in the sequence, then perhaps
it's better to avoid an if (family D) { do this extra thing }, and
instead implement a whole different operation with a little code
duplication. It's a matter of taste though.

I would encourage you not to send a v9 straight away but to discuss some
things on the list first. Otherwise it's easy to lose track of the
conversation.

Kind regards,
Alvin