Re: [PATCH net-next v2 6/8] net: dsa: mt7530: convert to use field accessor macros

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: mt7530: convert to use field accessor macros

This patch converts the mt7530 DSA driver to use FIELD_GET and FIELD_PREP
instead of open-coded shift/mask constructs, and replaces a 0x1f literal
in the PHY address helper with (PHY_MAX_ADDR - 1).

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 4168adca949f..dcf72ab0cd66 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c

[ ... ]

> @@ -1555,7 +1555,7 @@ mt7530_vlan_cmd(struct mt7530_priv *priv, enum mt7530_vlan_cmd cmd, u16 vid)
>  	u32 val;
>  	int ret;
>  
> -	val = VTCR_BUSY | VTCR_FUNC(cmd) | vid;
> +	val = VTCR_BUSY | VTCR_FUNC(cmd) | VTCR_VID(vid);

[Low]
The commit message describes this change as a refactor:
"Use FIELD_GET and FIELD_PREP instead of open-coding register fields."

The mt7530_vlan_cmd() hunk is not quite a pure conversion though.  The
previous code OR'd in vid directly:

    val = VTCR_BUSY | VTCR_FUNC(cmd) | vid;

while the new code applies a GENMASK(11, 0) truncation through VTCR_VID():

    val = VTCR_BUSY | VTCR_FUNC(cmd) | VTCR_VID(vid);

Spec-conformant VIDs always fit in 12 bits, so the runtime result is the
same, but the value is now masked where it previously was not.

A related observation is that the prior VTCR_VID and ATC_HASH macros
referenced an undeclared x:

    #define  ATC_HASH                       (((x) & 0xfff) << 16)
    #define  VTCR_VID                       ((x) & 0xfff)

so they could not have been used by any caller before this patch.  Adding
the (x) parameter and a GENMASK is effectively a fix for those macros.

Would it be worth calling out the VTCR_VID() truncation and the macro
parameter fix in the commit message, or splitting them into a separate
patch so the conversion remains a no-op refactor?

> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h

[ ... ]

> @@ -171,32 +176,29 @@ enum mt7530_fdb_cmd {
>  
>  /* Registers for table search read address */
>  #define MT7530_TSRA1			0x84

[ ... ]

>  #define MT7530_ATRD			0x8C
> -#define	 AGE_TIMER			24
> -#define  AGE_TIMER_MASK			0xff
> -#define  PORT_MAP			4
> -#define  PORT_MAP_MASK			0xff
> -#define  ENT_STATUS			2
> -#define  ENT_STATUS_MASK		0x3
> +#define  AGE_TIMER_RD_MASK		GENMASK(31, 24)
> +#define  PORT_MAP_MASK			GENMASK(11, 4)
> +#define  ENT_STATUS_MASK		GENMASK(3, 2)

[Low]
The mask formerly named AGE_TIMER_MASK is renamed to AGE_TIMER_RD_MASK,
but the same mask is consumed on both the read and write paths
(mt7530_fdb_read() and mt7530_fdb_write()), so the _RD_ infix reads as
"read-side only".

Its sibling fields in the same MT7530_ATRD register keep plain names
(PORT_MAP_MASK, ENT_STATUS_MASK), and the related ageing counters in
MT7530_AAC use AGE_CNT_MASK / AGE_UNIT_MASK rather than an _RD_
disambiguation.

Was the _RD_ infix intentional, and if so, what does it disambiguate
from?

[ ... ]

> @@ -731,38 +745,48 @@ enum mt7531_xtal_fsel {
>  #define  MT7531_PHY_PLL_OFF		BIT(5)
>  #define  MT7531_PHY_PLL_BYPASS_MODE	BIT(4)
>  
> -#define MT753X_CTRL_PHY_ADDR(addr)	((addr + 1) & 0x1f)
> +#define MT753X_CTRL_PHY_ADDR(addr)	(((addr) + 1) & (PHY_MAX_ADDR - 1))

[Low]
The replacement evaluates to the same value today, since PHY_MAX_ADDR is
32 in include/linux/phy.h and PHY_MAX_ADDR - 1 is 0x1f, and both forms
ultimately derive from the IEEE 802.3 5-bit MDIO PHY-address field.

The original 0x1f represented the width of the hardware register field,
while PHY_MAX_ADDR - 1 represents the maximum software PHY address minus
one and additionally relies on PHY_MAX_ADDR being a power of two, which
phy.h does not formally guarantee.

Given the rest of the patch introduces explicit GENMASK based field
masks, would something like:

    #define MT753X_CTRL_PHY_ADDR_MASK GENMASK(4, 0)
    #define MT753X_CTRL_PHY_ADDR(addr) FIELD_PREP(MT753X_CTRL_PHY_ADDR_MASK, (addr) + 1)

be more consistent and make the hardware-field intent clearer?