Re: [PATCH net-next v11 3/7] net: dsa: motorcomm: use fixed-width GENMASK_U*() helpers

[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 commit message says what the patch does but not why, and its
  wording claims more than the patch does.

--

net: dsa: motorcomm: use fixed-width GENMASK_U*() helpers

This commit switches the GENMASK() call sites in chip.c and mdio_bus.c to
the fixed-width GENMASK_U16() and GENMASK_U32() helpers. The converted
values are unchanged, so the patch has no runtime effect.

> Replace the plain GENMASK() uses with the fixed-width GENMASK_U16()
> and GENMASK_U32() variants.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 79422def16ffb..13fc286d4194b 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c
> @@ -101,19 +101,19 @@ static const struct yt921x_mib_desc yt921x_mib_descs[] = {
>  static const struct yt921x_info yt921x_infos[] = {
>  	{
>  		"YT9215SC", YT9215_MAJOR, 1, 0,
> -		GENMASK(4, 0),
> +		GENMASK_U16(4, 0),
>  		BIT(9),
>  		BIT(8) | BIT(9),
>  	},

[ ... ]

[Severity: Low]
This isn't a bug, but does the commit message match what the patch does?

"Replace the plain GENMASK() uses" reads as if every plain GENMASK() in
the driver is converted. Only six call sites in chip.c and mdio_bus.c
change.

After this patch, drivers/net/dsa/motorcomm/ still has 153 plain GENMASK()
uses: 133 in chip.h, 16 in leds.h and 4 in mdio_bus.h. The mdio_bus.h ones
are the field masks for the mdio_bus.c file this patch edits:

drivers/net/dsa/motorcomm/mdio_bus.h:
#define  YT921X_MBUS_CTRL_PORT_M		GENMASK(25, 21)
#define  YT921X_MBUS_CTRL_REG_M			GENMASK(20, 16)
...

Even inside yt921x_infos[], only the GENMASK() values for the u16
internal_mask field are converted. The BIT() values for xmii_mask and
serdes_mask, which are also u16, stay as unsigned long BIT() constants.
So does internal_mask for YT9214NB and YT9213NB:

	{
		"YT9214NB", YT9215_MAJOR, 3, 2,
		BIT(1) | BIT(3),
		BIT(9),
		BIT(8),
	},

include/linux/bits.h already has BIT_U16() and BIT_U32() for these.

The later YT922x patch in this series ("net: dsa: motorcomm: Add support
for Motorcomm YT922x") also adds new plain GENMASK() defines to chip.h,
such as YT922X_PORT_SPEED_M and YT922X_SERDES_MODE_M. That means the
series never finishes the conversion.

Could the commit message explain why this conversion is wanted and why it
stops at these call sites? Could it also say that there is no functional
change?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002173706.1225502-1-kyle.switch%40motor-comm.com