Re: [PATCH net-next v3 1/9] dt-bindings: net: dsa: realtek: add RTL8367S-VB

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch adds RTL8367S-VB to the list of models under the
  realtek,rtl8365mb compatible in…

--

dt-bindings: net: dsa: realtek: add RTL8367S-VB

This patch adds RTL8367S-VB to the list of models in the description of
the realtek,rtl8365mb compatible in the realtek DSA binding. Only the
description text changes. The schema constraints stay the same.

> The RTL8367S-VB (chip ID 0x6642) is supported by the rtl8365mb driver.

[Severity: Low]
Is this statement accurate at this point in the series?

At this commit, every entry in rtl8365mb_chip_infos[] uses chip_id 0x6367.
rtl8365mb_detect() also needs an exact match:

drivers/net/dsa/realtek/rtl8365mb_main.c:rtl8365mb_detect() {
    ...
	if (ci->chip_id == chip_id && ci->chip_ver == chip_ver) {
    ...
	if (!mb->chip_info) {
		dev_err(priv->dev, "unrecognized switch (id=0x%04x, ver=0x%04x)", chip_id, chip_ver);
		return -ENODEV;
	}
    ...
}

So a switch with chip ID 0x6642 still fails to probe with -ENODEV until a
later patch in the series, "net: dsa: realtek: rtl8365mb: add support for
RTL8367S-VB", adds the table entry:

	.name = "RTL8367S-VB",
	.chip_id = 0x6642,
	.chip_ver = 0x0010,
	.family = RTL8365MB_FAMILY_D,

With the whole series applied, the binding text and the driver agree, and
nothing regresses at runtime. Could the commit message say that this series
adds the support, rather than describing it as already present?

> Like the other models in the family, it is identified by its chip ID
> register, so it does not need a compatible string of its own.

A small wording point too. Later in the series, rtl8365mb_main.c gets this
header comment:

 * The RTL8367S-VB (chip ID 0x6642) is not part of the family listed above. It
 * belongs to a newer generation, which this driver calls family D.

Should "the other models in the family" be reworded to match that comment?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/e84d76ee-03df-49b0-8c9a-b289dfae8728%40yahoo.com