linux-kernel - Re: [PATCH 1/2] net: phy: realtek: Add support for PHY LEDs on RTL8211E

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <e3a36d27-5f7b-4c86-a6b9-2b37d3d16ee8@lunn.ch>
Date: Thu, 13 Mar 2025 20:39:56 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Michael Klein <michael@...sekall.de>
Cc: Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] net: phy: realtek: Add support for PHY LEDs on
 RTL8211E

> > > +static int rtl8211e_led_hw_control_get(struct phy_device *phydev, u8 index,
> > > +				       unsigned long *rules)
> > > +{
> > > +	int oldpage, ret;
> > > +	u16 cr1, cr2;
> > > +
> > > +	if (index >= RTL8211x_LED_COUNT)
> > > +		return -EINVAL;
> > > +
> > > +	oldpage = phy_select_page(phydev, 0x7);
> > > +	if (oldpage < 0)
> > > +		goto err_restore_page;
> > > +
> > > +	ret = __phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0x2c);
> > > +	if (ret)
> > > +		goto err_restore_page;
> > 
> > What is happening here? You select page 0x7, and then use
> > RTL821x_EXT_PAGE_SELECT to select 0x2c? Does this hardware have pages
> > within pages?
> 
> Kind of; this is from the datasheet:
> 
> 	6.9.5.  Access to Extension Page (ExtPage)
> 	
> 	Set MDIO commands as shown below to switch to the Extension Page (ExtPage) 0xXY (in Hex).
> 	1. Set Register 31 Data=0x0007 (set to Extension Page)
> 	2. Set Register 30 Data=0x00XY (Extension Page XY)
> 	3. Set the target Register Data
> 	4. Set Register 31 Data=0x0000 (switch to Page 0)
> 
> Register 30 is RTL821x_EXT_PAGE_SELECT, LED config registers are on
> extension page 0x2c

O.K. So it would be good to turn this into a patch series doing some
cleanup and then add LED support at the end.

Please add a #define for 0x07 page number. It is used in a few places,
and it would be good to replace the magic number with some sort of
name taken from the datasheet. Add other #defines as you need them, if
the datasheet gives them a name.

Add a helper something like:

rtl8211e_modify_ext_page(struct phy_device *phydev, u16 ext_page, u32 regnum,
                         u16 mask, u16 set)

and use it in rtl8211e_config_init()

Add helpers

rtl8211e_read_ext_page(struct phy_device *phydev, u16 ext_page, u32 regnum)
rtl8211e_write_ext_page(struct phy_device *phydev, u16 ext_page, u32 regnum, u16 val)

and then add LED support using these helpers. That should help
separate the LED code itself from this odd page in page code.

    Andrew

---
pw-bot: cr