Re: [PATCH net-next v3 5/8] net: dsa: motorcomm: Introduce yt921x_speed

[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] The new `enum yt921x_speed` in drivers/net/dsa/motorcomm/chip.h
  (lines 116-123) has no explicit enumerator values and no comment, yet…
- [Low] The new driver-private helper added in
  drivers/net/dsa/motorcomm/chip.h is named `ethtool_speed_to_yt921x()`,…

--

net: dsa: motorcomm: Introduce yt921x_speed

This replaces the three per-register speed switch statements in
yt921x_port_up() with a single conversion helper and a new enum, since
FIELD_PREP() results cannot be used as case labels.  The old
YT921X_{PORT,SERDES,MDIO_POLLING}_SPEED_* constants are dropped in favour
of feeding the enum value into the corresponding FIELD_PREP() wrappers.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 5049f4be8aa6b..c99be2044fa20 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -3771,29 +3771,15 @@ yt921x_port_up(struct yt921x_priv *priv, int port, unsigned int mode,
>  {
>  	const struct yt921x_info *info = priv->info;
>  	struct yt921x_port *pp = &priv->ports[port];
> +	int ps = ethtool_speed_to_yt921x(speed);

[Severity: Low]
This isn't a bug, but should this helper carry the driver prefix?  Every
other symbol reachable from chip.h uses yt921x_/YT921X_, while this new
static inline takes a name in the core ethtool_ namespace, so the call
site here reads as if the DSA driver is calling core ethtool API.

Would something like yt921x_speed_from_ethtool() be preferable, both here
and at the definition in chip.h?  It would also avoid a collision should a
generic ethtool_speed_to_* helper or macro ever appear in
include/linux/ethtool.h.

>  	u32 mask;
>  	u32 ctrl;
>  	int res;
>  
> -	switch (speed) {
> -	case SPEED_10:
> -		ctrl = YT921X_PORT_SPEED_10;
> -		break;
> -	case SPEED_100:
> -		ctrl = YT921X_PORT_SPEED_100;
> -		break;
> -	case SPEED_1000:
> -		ctrl = YT921X_PORT_SPEED_1000;
> -		break;
> -	case SPEED_2500:
> -		ctrl = YT921X_PORT_SPEED_2500;
> -		break;
> -	case SPEED_10000:
> -		ctrl = YT921X_PORT_SPEED_10000;
> -		break;
> -	default:
> +	if (ps == YT921X_SPEED_NUM)
>  		return -EINVAL;
> -	}
> +
> +	ctrl = YT921X_PORT_SPEED(ps);

[ ... ]

> @@ -3810,25 +3796,7 @@ yt921x_port_up(struct yt921x_priv *priv, int port, unsigned int mode,
>  
>  	if (pp->serdes) {
>  		mask = YT921X_SERDES_SPEED_M;
> -		switch (speed) {
[ ... ]
> -		}
> +		ctrl = YT921X_SERDES_SPEED(ps);

[ ... ]

> @@ -3850,25 +3818,7 @@ yt921x_port_up(struct yt921x_priv *priv, int port, unsigned int mode,
>  		if (res)
>  			return res;
>  
> -		switch (speed) {
[ ... ]
> -		}
> +		ctrl = YT921X_MDIO_POLLING_SPEED(ps);

> diff --git a/drivers/net/dsa/motorcomm/chip.h b/drivers/net/dsa/motorcomm/chip.h
> index 3fb0c6a69dd8c..b3670f1e14276 100644
> --- a/drivers/net/dsa/motorcomm/chip.h
> +++ b/drivers/net/dsa/motorcomm/chip.h
> @@ -89,11 +89,6 @@
>  #define  YT921X_SERDES_DUPLEX_FULL		BIT(3)
>  #define  YT921X_SERDES_SPEED_M			GENMASK(2, 0)
>  #define   YT921X_SERDES_SPEED(x)			FIELD_PREP(YT921X_SERDES_SPEED_M, (x))
> -#define   YT921X_SERDES_SPEED_10			YT921X_SERDES_SPEED(0)
> -#define   YT921X_SERDES_SPEED_100			YT921X_SERDES_SPEED(1)
> -#define   YT921X_SERDES_SPEED_1000			YT921X_SERDES_SPEED(2)
> -#define   YT921X_SERDES_SPEED_10000			YT921X_SERDES_SPEED(3)
> -#define   YT921X_SERDES_SPEED_2500			YT921X_SERDES_SPEED(4)

[ ... ]

> @@ -107,11 +102,6 @@
>  #define  YT921X_PORT_SPEED_M			GENMASK(2, 0)
>  #define   YT921X_PORT_SPEED(x)				FIELD_PREP(YT921X_PORT_SPEED_M, (x))
> -#define   YT921X_PORT_SPEED_10				YT921X_PORT_SPEED(0)
> -#define   YT921X_PORT_SPEED_100				YT921X_PORT_SPEED(1)
> -#define   YT921X_PORT_SPEED_1000			YT921X_PORT_SPEED(2)
> -#define   YT921X_PORT_SPEED_10000			YT921X_PORT_SPEED(3)
> -#define   YT921X_PORT_SPEED_2500			YT921X_PORT_SPEED(4)

[ ... ]

> @@ -122,11 +112,16 @@
>  #define  YT921X_MDIO_POLLING_SPEED_M		GENMASK(2, 0)
>  #define   YT921X_MDIO_POLLING_SPEED(x)			FIELD_PREP(YT921X_MDIO_POLLING_SPEED_M, (x))
> -#define   YT921X_MDIO_POLLING_SPEED_10			YT921X_MDIO_POLLING_SPEED(0)
> -#define   YT921X_MDIO_POLLING_SPEED_100			YT921X_MDIO_POLLING_SPEED(1)
> -#define   YT921X_MDIO_POLLING_SPEED_1000		YT921X_MDIO_POLLING_SPEED(2)
> -#define   YT921X_MDIO_POLLING_SPEED_10000		YT921X_MDIO_POLLING_SPEED(3)
> -#define   YT921X_MDIO_POLLING_SPEED_2500		YT921X_MDIO_POLLING_SPEED(4)
> +
> +enum yt921x_speed {
> +	YT921X_SPEED_10,
> +	YT921X_SPEED_100,
> +	YT921X_SPEED_1000,
> +	YT921X_SPEED_10000,
> +	YT921X_SPEED_2500,
> +	YT921X_SPEED_NUM
> +};

[Severity: Low]
Should these enumerators carry explicit values?  The ordinals here are the
raw hardware field encodings, which yt921x_port_up() writes straight into
three register fields:

	ctrl = YT921X_PORT_SPEED(ps);
	...
	ctrl = YT921X_SERDES_SPEED(ps);
	...
	ctrl = YT921X_MDIO_POLLING_SPEED(ps);

The encoding is not monotonic in speed (10G is 3, 2.5G is 4).  Before this
patch that was visible in the constants being removed above:

	#define   YT921X_PORT_SPEED_10000	YT921X_PORT_SPEED(3)
	#define   YT921X_PORT_SPEED_2500	YT921X_PORT_SPEED(4)

Afterwards the only record of it is the textual order of the enumerators.

Would a later "sort these by speed" cleanup then silently program 2.5G as 3
and 10G as 4, with no compile-time or runtime diagnostic?  Assigning
YT921X_SPEED_10000 = 3 and YT921X_SPEED_2500 = 4 explicitly, and/or noting
in a comment that these are hardware field encodings, would keep that
intact.

The values themselves are unchanged by this patch (10 -> 0, 100 -> 1,
1000 -> 2, 10000 -> 3, 2500 -> 4 all match the removed macros), and the
enum still has no explicit values at the end of the series.

[ ... ]

> @@ -1004,4 +999,22 @@ struct yt921x_priv {
>  	struct yt921x_acl_blk *acl_blks[YT921X_ACL_BLK_NUM];
>  };
>  
> +static inline int ethtool_speed_to_yt921x(int speed)
     ^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Low]
This is the definition referred to above; renaming it to something like
yt921x_speed_from_ethtool() would keep the driver's namespace consistent.

> +{
> +	switch (speed) {
> +	case SPEED_10:
> +		return YT921X_SPEED_10;
[ ... ]
> +	default:
> +		return YT921X_SPEED_NUM;
> +	}
> +}
> +
>  #endif

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917185010.3189199-1-mmyangfl%40gmail.com