Message-ID: <20250728065646.c7bozpk4pxkqrswz@DEN-DL-M31836.microchip.com>
Date: Mon, 28 Jul 2025 08:56:46 +0200
From: Horatiu Vultur <horatiu.vultur@...rochip.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>
CC: "Russell King (Oracle)" <linux@...linux.org.uk>, <andrew@...n.ch>,
	<hkallweit1@...il.com>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <richardcochran@...il.com>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v2 3/4] net: phy: micrel: Replace hardcoded
 pages with defines

The 07/26/2025 08:21, Oleksij Rempel wrote:

Hi,

> 
> On Fri, Jul 25, 2025 at 08:48:39AM +0200, Horatiu Vultur wrote:
> > The 07/24/2025 21:45, Russell King (Oracle) wrote:
> > >
> > > 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;
> > e >  }
> > > >
> > > > +#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
> 
> Hi,
> 
> I requested these defines because it's much easier to search for a specific
> define than for a raw number - especially when debugging or comparing with
> datasheets. Even if the names are generic, it helps track down usage when
> documentation becomes available or evolves.
> 
> To improve the situation, I reviewed the LAN8814 documentation and observed how
> the existing driver and patches (for LAN8842) use these extended pages.
> Based on that, I suggest following names:
> 
> Documented Extended Pages:
> 
> These are described in the LAN8814 documentation:
> 
> /**
>  * LAN8814_PAGE_COMMON_REGS - Selects Extended Page 4.
>  *
>  * This page contains device-common registers that affect the entire chip.
>  * It includes controls for chip-level resets, strap status, GPIO,
>  * QSGMII, the shared 1588 PTP block, and the PVT monitor.
>  */
> #define LAN8814_PAGE_COMMON_REGS 4
> 
> /**
>  * LAN8814_PAGE_PORT_REGS - Selects Extended Page 5.
>  *
>  * This page contains port-specific registers that must be accessed
>  * on a per-port basis. It includes controls for port LEDs, QSGMII PCS,
>  * rate adaptation FIFOs, and the per-port 1588 TSU block.
>  */
> #define LAN8814_PAGE_PORT_REGS 5
> 
> Undocumented Pages (based on driver and patch analysis):
> 
> These pages are not officially documented, but their use is visible in the
> driver and LAN8842 patch:
> 
> /**
>  * LAN8814_PAGE_AFE_PMA - Selects Extended Page 1.
>  *
>  * This page appears to control the Analog Front-End (AFE) and Physical
>  * Medium Attachment (PMA) layers. It is used to access registers like
>  * LAN8814_PD_CONTROLS and LAN8814_LINK_QUALITY.
>  */
> #define LAN8814_PAGE_AFE_PMA 1
> 
> /**
>  * LAN8814_PAGE_PCS_DIGITAL - Selects Extended Page 2.
>  *
>  * This page seems dedicated to the Physical Coding Sublayer (PCS) and other
>  * digital logic. It is used for MDI-X alignment (LAN8814_ALIGN_SWAP) and EEE
>  * state (LAN8814_EEE_STATE) in the LAN8814, and is repurposed for statistics
>  * and self-test counters in the LAN8842.
>  */
> #define LAN8814_PAGE_PCS_DIGITAL 2
> 
> /**
>  * LAN8814_PAGE_SYSTEM_CTRL - Selects Extended Page 31.
>  *
>  * This page appears to hold fundamental system or global controls. In the
>  * driver, it is used by the related LAN8804 to access the
>  * LAN8814_CLOCK_MANAGEMENT register.
>  */
> #define LAN8814_PAGE_SYSTEM_CTRL 31
> 
> While these names are not official, they still give useful hints and make the
> code more readable. I doubt the LAN8842 has an identical layout, but it looks
> similar enough to reuse these patterns for now.

I think the name of the defines matches pretty good with the
functionality of those pages.

> 
> Are there any plans to make the LAN8842 register documentation public? That
> would help clarify this further and improve upstream support.

I don't know exactly but I have started to ask around when and if they will
make the lan8842 register description public.

> 
> Best regards,
> Oleksij
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

-- 
/Horatiu

Powered by blists - more mailing lists