Re: [PATCH net-next v7 05/10] net: dsa: realtek: rtl8365mb: set and get vlan 4k for family D

[Luiz Angelo Daros de Luca]

> Don't you think it's better to just split out into family C/D variants
> for these ops? If you do so, then I think relabelling the old macros for
> the C family with RTL8365MB_C_ prefixes will also be appropriate.

Adding a `C` prefix is a clean way to organize the code, and it avoids
scaling issues with `is_d` checks as more families are added.

Splitting the logic into subfunctions handled via a family enum switch
(or eventually function pointers) is much more maintainable. Using a
switch statement also has the advantage of compiler warnings if a new
family is added and forgotten.

Here is a revised version of your response, keeping it professional,
concise, and clear:

---

> Don't you think it's better to just split out into family C/D variants
> for these ops? If you do so, then I think relabelling the old macros for
> the C family with RTL8365MB_C_ prefixes will also be appropriate.

Adding a `*_C_` prefix is a good way to organize it. It will get a bit
messier when I submit the RTL8367R support, which I arbitrarily named
Family A, but extra macros won't increase the final binary size. We
might need to duplicate some macros once we have a register used by
more than one but not all families.

As Alvin mentioned, splitting into subfunctions when each family has a
distinct implementation is much cleaner. In my WIP tree
(https://github.com/torvalds/linux/compare/master...luizluca:linux:realtek_families_wip),
I handle it like this:

static void rtl8365mb_l2_data_to_uc(struct realtek_priv *priv, const u16 *data,
                                    struct rtl8365mb_l2_uc *uc)
{
        switch (rtl8365mb_get_family(priv)) {
        case RTL8365MB_FAMILY_A:
                rtl8365mb_a_l2_data_to_uc(data, uc);
                break;
        case RTL8365MB_FAMILY_C:
                rtl8365mb_c_l2_data_to_uc(data, uc);
                break;
        }
}

This is how I'm structuring it now, though it might migrate to
function pointers later. A family check inside a shared function only
makes sense for very localized differences. Since family A is shaping
up well and family B sits conceptually between A and C, `is_d` doesn't
scale well for future variants. Using an enum with a switch statement
also ensures the compiler warns you if a new family case is missed.

Best regards,

Luiz