Re: [PATCH net-next v7 09/10] net: dsa: realtek: rtl8365mb: make L2/LUT access work on family D

[Mieczyslaw Nalewaj <namiltd@yahoo.com>]

On Mon, Nov 02, 2026 at 03:44:36PM +0100, Mieczyslaw Nalewaj wrote:
> +static bool rtl8365mb_l2_is_d(struct realtek_priv *priv)
> +{
> +	return rtl8365mb_get_family(priv) == RTL8365MB_FAMILY_D;
> +}

On Tue, Nov 03, 2026, Alvin wrote:
> Shouldn't this be a top-level function, like rtl8365mb_is_d()?

Yes. For v8 I'll add a single rtl8365mb_is_d() static inline to
rtl8365mb.h (in the patch that introduces the family infrastructure)
and the L2, table and main code now use it. The l2-local helper will
gone.

> +static void rtl8365mb_l2_mac_to_data(const u8 *m, u16 *data)
> +static void rtl8365mb_l2_data_to_mac(const u16 *data, u8 *m)
[...]
> From the diff it would seem that this is specific to the D family, since
> you only use it in those functions. But looking over my crappy original
> code for the C family, it's quite the same is it not? Can't you update
> the C family functions to use these helpers too?

Good catch, the packing is identical. For v8 I'll add a preparatory
patch before this one that moves the two MAC helpers above the C
serialisers and converts the C functions to use them. It is meant to
be a no-op refactor, so it can be reviewed on its own.

> +static void rtl8365mb_l2_data_to_uc_any(struct realtek_priv *priv,
> +					const u16 *data,
> +					struct rtl8365mb_l2_uc *uc)
> +{
> +	if (rtl8365mb_l2_is_d(priv))
> +		rtl8365mb_d_l2_data_to_uc(data, uc);
> +	else
> +		rtl8365mb_l2_data_to_uc(data, uc);
> +}
>
> Can't we have:
>
> rtl8365mb_l2_data_to_uc() # same as before at all callsites
>
> call into either:
>
> rtl8365mb_c_l2_data_to_uc()
> rtl8365mb_d_l2_data_to_uc()
>
> instead?
> [...]
> The above comment applies to all these _any() functions.

Agreed. In v8 the existing C serialisers are renamed to
rtl8365mb_c_l2_*() (in the same preparatory patch, so this patch
stays purely additive), and rtl8365mb_l2_{data_to_uc,uc_to_data,
data_to_mc,mc_to_data}() become the per-family dispatchers calling
the c_ or d_ variant. The _any() wrappers are dropped. The
dispatchers need to know the family, so the callsites only gain the
priv argument.

> Even though family C is the originally supported family, I guess D is an
> equal citizen, so it's good to keep things in this driver symmetric.

Agreed, D should be a peer of C rather than a special case bolted on.

Thanks for the review,
Mieczyslaw