Re: [PATCH net-next v9 02/11] net: dsa: realtek: rtl8365mb: add chip family infrastructure

[netdev-bot+sashiko]

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 leaves a stale comment in rtl8365mb_switch_init()
  (drivers/net/dsa/realtek/rtl8365mb_main.c).

--

net: dsa: realtek: rtl8365mb: add chip family infrastructure

This commit adds enum rtl8365mb_family and struct rtl8365mb_family_info.
The family id, port count and init jam table now live in the family
info, which struct rtl8365mb_chip_info points to, and all existing chips
use the RTL8365MB_FAMILY_C instance. rtl8365mb_detect() now takes
num_ports from the family info.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index da06f952a053d..b500c8404c24b 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c

[ ... ]

> @@ -702,14 +705,20 @@ struct rtl8365mb_extint {
>  	unsigned int supported_interfaces;
>  };
>  
> +static const struct rtl8365mb_family_info rtl8365mb_family_c = {
> +	.family = RTL8365MB_FAMILY_C,
> +	.num_ports = RTL8365MB_C_MAX_NUM_PORTS,
> +	.jam_table = rtl8365mb_init_jam_8365mb_vc,
> +	.jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc),
> +};
> +
>  /**
>   * struct rtl8365mb_chip_info - static chip-specific info
>   * @name: human-readable chip name
>   * @chip_id: chip identifier
>   * @chip_ver: chip silicon revision
> + * @family_info: data shared by all chips of the same family
>   * @extints: available external interfaces
> - * @jam_table: chip-specific initialization jam table
> - * @jam_size: size of the chip's jam table

[ ... ]

> @@ -3036,17 +3048,17 @@ static int rtl8365mb_change_tag_protocol(struct dsa_switch *ds,
>  static int rtl8365mb_switch_init(struct realtek_priv *priv)
>  {
>  	struct rtl8365mb *mb = priv->chip_data;
> -	const struct rtl8365mb_chip_info *ci;
> +	const struct rtl8365mb_family_info *fi;
>  	int ret;
>  	int i;
>  
> -	ci = mb->chip_info;
> +	fi = mb->chip_info->family_info;
>  
>  	/* Do any chip-specific init jam before getting to the common stuff */
> -	if (ci->jam_table) {
> -		for (i = 0; i < ci->jam_size; i++) {
> -			ret = regmap_write(priv->map, ci->jam_table[i].reg,
> -					   ci->jam_table[i].val);
> +	if (fi->jam_table) {
> +		for (i = 0; i < fi->jam_size; i++) {
> +			ret = regmap_write(priv->map, fi->jam_table[i].reg,
> +					   fi->jam_table[i].val);

[Severity: Low]
This isn't a bug, but should this comment in rtl8365mb_switch_init() be
updated?

The jam table no longer comes from struct rtl8365mb_chip_info. It now
comes from mb->chip_info->family_info, and the new kernel-doc describes
@family_info as "data shared by all chips of the same family".

The comment still says "chip-specific init jam", though. No later patch in
the series changes it. At the end of the series both rtl8365mb_family_c
and rtl8365mb_family_d point to the same rtl8365mb_init_jam_8365mb_vc
table.

Would something like "family-specific init jam" fit better?

>  			if (ret)
>  				return ret;
>  		}

[ ... ]

<br>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/9b73858d-fc09-4988-8bab-fd7b88db9412%40yahoo.com