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

The 07/24/2025 21:45, Russell King (Oracle) wrote:

On Thu, Jul 24, 2025 at 10:08:25PM +0200, Horatiu Vultur wrote:

quoted

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

Hi Russell,

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.

You are right, I have been ask to change this in version 1:
https://lkml.org/lkml/2025/7/23/672

I have mentioned it that the extended pages don't have any meaningfull
names also in the register description document. But Oleksij says he
will be fine with xxxx_EXT_PAGE_0, so maybe I have missunderstood Oleksij
in what he proposed.

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

-- 
/Horatiu