Re: [PATCH net-next v2 3/4] net: phy: micrel: Replace hardcoded pages with defines

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Thu, Jul 24, 2025 at 10:08:25PM +0200, Horatiu Vultur wrote:
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index b04c471c11a4a..d20f028106b7d 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -2788,6 +2788,13 @@ static int ksz886x_cable_test_get_status(struct phy_device *phydev,
>  	return ret;
>  }
>  
> +#define LAN_EXT_PAGE_0					0
> +#define LAN_EXT_PAGE_1					1
> +#define LAN_EXT_PAGE_2					2
> +#define LAN_EXT_PAGE_4					4
> +#define LAN_EXT_PAGE_5					5
> +#define LAN_EXT_PAGE_31					31

I don't see the point of this change. This is almost as bad as:

#define ZERO 0
#define ONE 1
#define TWO 2
#define THREE 3
...
#define ONE_HUNDRED_AND_FIFTY_FIVE 155
etc

It doesn't give us any new information, and just adds extra clutter,
making the code less readable.

The point of using register definitions is to describe the purpose
of the number, giving the number a meaning, not to just hide the
number because we don't want to see such things in C code.

I'm sorry if you were asked to do this in v1, but I think if you
were asked to do it, it would've been assuming that the definitions
could be more meaningful.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!