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

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

[Michael Klein]

Like the RTL8211F, the RTL8211E PHY supports up to three LEDs.
Add netdev trigger support for them, too.

Signed-off-by: Michael Klein <michael@fossekall.de>
---
 drivers/net/phy/realtek.c | 120 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 114 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 166f6a728373..906640d6dca5 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -52,6 +52,18 @@
 #define RTL8211E_TX_DELAY			BIT(12)
 #define RTL8211E_RX_DELAY			BIT(11)
 
+#define RTL8211E_LEDCR1				0x1a
+#define RTL8211E_LEDCR1_ACT_TXRX		BIT(4)
+#define RTL8211E_LEDCR1_MASK			BIT(4)
+#define RTL8211E_LEDCR1_SHIFT			1
+
+#define RTL8211E_LEDCR2				0x1c
+#define RTL8211E_LEDCR2_LINK_1000		BIT(2)
+#define RTL8211E_LEDCR2_LINK_100		BIT(1)
+#define RTL8211E_LEDCR2_LINK_10			BIT(0)
+#define RTL8211E_LEDCR2_MASK			GENMASK(2, 0)
+#define RTL8211E_LEDCR2_SHIFT			4
+
 #define RTL8211F_CLKOUT_EN			BIT(0)
 
 #define RTL8201F_ISR				0x1e
@@ -96,7 +108,8 @@
 #define RTL_8221B_VN_CG				0x001cc84a
 #define RTL_8251B				0x001cc862
 
-#define RTL8211F_LED_COUNT			3
+/* RTL8211E and RTL8211F support up to three LEDs */
+#define RTL8211x_LED_COUNT			3
 
 MODULE_DESCRIPTION("Realtek PHY driver");
 MODULE_AUTHOR("Johnson Leung");
@@ -487,7 +500,7 @@ static int rtl821x_resume(struct phy_device *phydev)
 	return 0;
 }
 
-static int rtl8211f_led_hw_is_supported(struct phy_device *phydev, u8 index,
+static int rtl8211x_led_hw_is_supported(struct phy_device *phydev, u8 index,
 					unsigned long rules)
 {
 	const unsigned long mask = BIT(TRIGGER_NETDEV_LINK_10) |
@@ -506,9 +519,11 @@ static int rtl8211f_led_hw_is_supported(struct phy_device *phydev, u8 index,
 	 *      rates and Active indication always at all three 10+100+1000
 	 *      link rates.
 	 * This code currently uses mode B only.
+	 *
+	 * RTL8211E PHY LED has one mode, which works like RTL8211F * mode B.
 	 */
 
-	if (index >= RTL8211F_LED_COUNT)
+	if (index >= RTL8211x_LED_COUNT)
 		return -EINVAL;
 
 	/* Filter out any other unsupported triggers. */
@@ -527,7 +542,7 @@ static int rtl8211f_led_hw_control_get(struct phy_device *phydev, u8 index,
 {
 	int val;
 
-	if (index >= RTL8211F_LED_COUNT)
+	if (index >= RTL8211x_LED_COUNT)
 		return -EINVAL;
 
 	val = phy_read_paged(phydev, 0xd04, RTL8211F_LEDCR);
@@ -560,7 +575,7 @@ static int rtl8211f_led_hw_control_set(struct phy_device *phydev, u8 index,
 	const u16 mask = RTL8211F_LEDCR_MASK << (RTL8211F_LEDCR_SHIFT * index);
 	u16 reg = 0;
 
-	if (index >= RTL8211F_LED_COUNT)
+	if (index >= RTL8211x_LED_COUNT)
 		return -EINVAL;
 
 	if (test_bit(TRIGGER_NETDEV_LINK_10, &rules))
@@ -583,6 +598,96 @@ static int rtl8211f_led_hw_control_set(struct phy_device *phydev, u8 index,
 	return phy_modify_paged(phydev, 0xd04, RTL8211F_LEDCR, mask, reg);
 }
 
+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;
+
+	cr1 = ret = __phy_read(phydev, RTL8211E_LEDCR1);
+	if (ret < 0)
+		goto err_restore_page;
+
+	cr1 >>= RTL8211E_LEDCR1_SHIFT * index;
+	if (cr1 & RTL8211E_LEDCR1_ACT_TXRX) {
+		set_bit(TRIGGER_NETDEV_RX, rules);
+		set_bit(TRIGGER_NETDEV_TX, rules);
+	}
+
+	cr2 = ret = __phy_read(phydev, RTL8211E_LEDCR2);
+	if (ret < 0)
+		goto err_restore_page;
+
+	cr2 >>= RTL8211E_LEDCR2_SHIFT * index;
+	if (cr2 & RTL8211E_LEDCR2_LINK_10)
+		set_bit(TRIGGER_NETDEV_LINK_10, rules);
+
+	if (cr2 & RTL8211E_LEDCR2_LINK_100)
+		set_bit(TRIGGER_NETDEV_LINK_100, rules);
+
+	if (cr2 & RTL8211E_LEDCR2_LINK_1000)
+		set_bit(TRIGGER_NETDEV_LINK_1000, rules);
+
+err_restore_page:
+	return phy_restore_page(phydev, oldpage, ret);
+}
+
+static int rtl8211e_led_hw_control_set(struct phy_device *phydev, u8 index,
+				       unsigned long rules)
+{
+	const u16 cr1mask = RTL8211E_LEDCR1_MASK << (RTL8211E_LEDCR1_SHIFT * index);
+	const u16 cr2mask = RTL8211E_LEDCR2_MASK << (RTL8211E_LEDCR2_SHIFT * index);
+	u16 cr1 = 0, cr2 = 0;
+	int oldpage, ret;
+
+	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;
+
+	if (test_bit(TRIGGER_NETDEV_RX, &rules) ||
+	    test_bit(TRIGGER_NETDEV_TX, &rules)) {
+		cr1 |= RTL8211E_LEDCR1_ACT_TXRX;
+	}
+
+	cr1 <<= RTL8211E_LEDCR1_SHIFT * index;
+	ret = __phy_modify(phydev, RTL8211E_LEDCR1, cr1mask, cr1);
+	if (ret < 0)
+		goto err_restore_page;
+
+	if (test_bit(TRIGGER_NETDEV_LINK_10, &rules))
+		cr2 |= RTL8211E_LEDCR2_LINK_10;
+
+	if (test_bit(TRIGGER_NETDEV_LINK_100, &rules))
+		cr2 |= RTL8211E_LEDCR2_LINK_100;
+
+	if (test_bit(TRIGGER_NETDEV_LINK_1000, &rules))
+		cr2 |= RTL8211E_LEDCR2_LINK_1000;
+
+	cr2 <<= RTL8211E_LEDCR2_SHIFT * index;
+	ret = __phy_modify(phydev, RTL8211E_LEDCR2, cr2mask, cr2);
+
+err_restore_page:
+	return phy_restore_page(phydev, oldpage, ret);
+}
+
 static int rtl8211e_config_init(struct phy_device *phydev)
 {
 	int ret = 0, oldpage;
@@ -1296,6 +1401,9 @@ static struct phy_driver realtek_drvs[] = {
 		.resume		= genphy_resume,
 		.read_page	= rtl821x_read_page,
 		.write_page	= rtl821x_write_page,
+		.led_hw_is_supported = rtl8211x_led_hw_is_supported,
+		.led_hw_control_get = rtl8211e_led_hw_control_get,
+		.led_hw_control_set = rtl8211e_led_hw_control_set,
 	}, {
 		PHY_ID_MATCH_EXACT(0x001cc916),
 		.name		= "RTL8211F Gigabit Ethernet",
@@ -1309,7 +1417,7 @@ static struct phy_driver realtek_drvs[] = {
 		.read_page	= rtl821x_read_page,
 		.write_page	= rtl821x_write_page,
 		.flags		= PHY_ALWAYS_CALL_SUSPEND,
-		.led_hw_is_supported = rtl8211f_led_hw_is_supported,
+		.led_hw_is_supported = rtl8211x_led_hw_is_supported,
 		.led_hw_control_get = rtl8211f_led_hw_control_get,
 		.led_hw_control_set = rtl8211f_led_hw_control_set,
 	}, {
-- 
2.39.5

[PATCH 2/2] ARM: dts: bananapi: add support for PHY LEDs

[Michael Klein]

The Bananapi M1 has three LEDs connected to the RTL8211E ethernet PHY.
Add the corresponding nodes to the device tree.

Signed-off-by: Michael Klein <michael@fossekall.de>
---
 .../boot/dts/allwinner/sun7i-a20-bananapi.dts | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/arch/arm/boot/dts/allwinner/sun7i-a20-bananapi.dts b/arch/arm/boot/dts/allwinner/sun7i-a20-bananapi.dts
index 46ecf9db2324..4976453ed192 100644
--- a/arch/arm/boot/dts/allwinner/sun7i-a20-bananapi.dts
+++ b/arch/arm/boot/dts/allwinner/sun7i-a20-bananapi.dts
@@ -48,6 +48,7 @@
 
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/leds/common.h>
 
 / {
 	model = "LeMaker Banana Pi";
@@ -169,6 +170,32 @@ &ir0 {
 &gmac_mdio {
 	phy1: ethernet-phy@1 {
 		reg = <1>;
+
+		leds {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			led@0 {
+				reg = <0>;
+				color = <LED_COLOR_ID_GREEN>;
+				default-state = "keep";
+				linux,default-trigger = "netdev";
+			};
+
+			led@1 {
+				reg = <1>;
+				color = <LED_COLOR_ID_AMBER>;
+				default-state = "keep";
+				linux,default-trigger = "netdev";
+			};
+
+			led@2 {
+				reg = <2>;
+				color = <LED_COLOR_ID_BLUE>;
+				default-state = "keep";
+				linux,default-trigger = "netdev";
+			};
+		};
 	};
 };
 
-- 
2.39.5

Re: [PATCH 2/2] ARM: dts: bananapi: add support for PHY LEDs

[Jernej Škrabec]

Hi,

Dne sreda, 12. marec 2025 ob 20:36:28 Srednjeevropski standardni čas je Michael Klein napisal(a):
> The Bananapi M1 has three LEDs connected to the RTL8211E ethernet PHY.
> Add the corresponding nodes to the device tree.
> 
> Signed-off-by: Michael Klein <michael@fossekall.de>

This is patch 2/2. Which one is patch 1/2? I got only one.

Best regards,
Jernej

> ---
>  .../boot/dts/allwinner/sun7i-a20-bananapi.dts | 27 +++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/allwinner/sun7i-a20-bananapi.dts b/arch/arm/boot/dts/allwinner/sun7i-a20-bananapi.dts
> index 46ecf9db2324..4976453ed192 100644
> --- a/arch/arm/boot/dts/allwinner/sun7i-a20-bananapi.dts
> +++ b/arch/arm/boot/dts/allwinner/sun7i-a20-bananapi.dts
> @@ -48,6 +48,7 @@
>  
>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/leds/common.h>
>  
>  / {
>  	model = "LeMaker Banana Pi";
> @@ -169,6 +170,32 @@ &ir0 {
>  &gmac_mdio {
>  	phy1: ethernet-phy@1 {
>  		reg = <1>;
> +
> +		leds {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			led@0 {
> +				reg = <0>;
> +				color = <LED_COLOR_ID_GREEN>;
> +				default-state = "keep";
> +				linux,default-trigger = "netdev";
> +			};
> +
> +			led@1 {
> +				reg = <1>;
> +				color = <LED_COLOR_ID_AMBER>;
> +				default-state = "keep";
> +				linux,default-trigger = "netdev";
> +			};
> +
> +			led@2 {
> +				reg = <2>;
> +				color = <LED_COLOR_ID_BLUE>;
> +				default-state = "keep";
> +				linux,default-trigger = "netdev";
> +			};
> +		};
>  	};
>  };
>  
> 

Re: [PATCH 2/2] ARM: dts: bananapi: add support for PHY LEDs

[Michael Klein]

Hi,

On Thu, Mar 13, 2025 at 07:07:24AM +0100, Jernej Škrabec wrote:
>Dne sreda, 12. marec 2025 ob 20:36:28 Srednjeevropski standardni čas je Michael Klein napisal(a):
>> The Bananapi M1 has three LEDs connected to the RTL8211E ethernet PHY.
>> Add the corresponding nodes to the device tree.
>>
>> Signed-off-by: Michael Klein <michael@fossekall.de>
>
>This is patch 2/2. Which one is patch 1/2? I got only one.

https://patchwork.kernel.org/project/netdevbpf/patch/20250312193629.85417-1-michael@fossekall.de/

Sorry for any inconvenience in case I messed up the patch submission.

I made two commits for this change and submitted them via `git send-email 
HEAD^^`. The first patch went to netdev@vger.kernel.org, the second 
to linux-arm-kernel@lists.infradead.org, which seems logical. Have I 
done something wrong?

>Best regards,
>Jernej

Michael

>
>> ---
>>  .../boot/dts/allwinner/sun7i-a20-bananapi.dts | 27 +++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/allwinner/sun7i-a20-bananapi.dts b/arch/arm/boot/dts/allwinner/sun7i-a20-bananapi.dts
>> index 46ecf9db2324..4976453ed192 100644
>> --- a/arch/arm/boot/dts/allwinner/sun7i-a20-bananapi.dts
>> +++ b/arch/arm/boot/dts/allwinner/sun7i-a20-bananapi.dts
>> @@ -48,6 +48,7 @@
>>
>>  #include <dt-bindings/gpio/gpio.h>
>>  #include <dt-bindings/interrupt-controller/irq.h>
>> +#include <dt-bindings/leds/common.h>
>>
>>  / {
>>  	model = "LeMaker Banana Pi";
>> @@ -169,6 +170,32 @@ &ir0 {
>>  &gmac_mdio {
>>  	phy1: ethernet-phy@1 {
>>  		reg = <1>;
>> +
>> +		leds {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +
>> +			led@0 {
>> +				reg = <0>;
>> +				color = <LED_COLOR_ID_GREEN>;
>> +				default-state = "keep";
>> +				linux,default-trigger = "netdev";
>> +			};
>> +
>> +			led@1 {
>> +				reg = <1>;
>> +				color = <LED_COLOR_ID_AMBER>;
>> +				default-state = "keep";
>> +				linux,default-trigger = "netdev";
>> +			};
>> +
>> +			led@2 {
>> +				reg = <2>;
>> +				color = <LED_COLOR_ID_BLUE>;
>> +				default-state = "keep";
>> +				linux,default-trigger = "netdev";
>> +			};
>> +		};
>>  	};
>>  };
>>
>>
>
>
>
>
>

-- 

Re: [PATCH 2/2] ARM: dts: bananapi: add support for PHY LEDs

[Andre Przywara]

On Thu, 13 Mar 2025 09:34:50 +0100
Michael Klein <michael@fossekall.de> wrote:

Hi,

> On Thu, Mar 13, 2025 at 07:07:24AM +0100, Jernej Škrabec wrote:
> >Dne sreda, 12. marec 2025 ob 20:36:28 Srednjeevropski standardni čas je Michael Klein napisal(a):  
> >> The Bananapi M1 has three LEDs connected to the RTL8211E ethernet PHY.
> >> Add the corresponding nodes to the device tree.
> >>
> >> Signed-off-by: Michael Klein <michael@fossekall.de>  
> >
> >This is patch 2/2. Which one is patch 1/2? I got only one.  
> 
> https://patchwork.kernel.org/project/netdevbpf/patch/20250312193629.85417-1-michael@fossekall.de/
> 
> Sorry for any inconvenience in case I messed up the patch submission.
> 
> I made two commits for this change and submitted them via `git send-email 
> HEAD^^`. The first patch went to netdev@vger.kernel.org, the second 
> to linux-arm-kernel@lists.infradead.org, which seems logical. Have I 
> done something wrong?

Well, for those really small "series" it's probably better to send all
patches to everyone, especially if the first patch gives some context,
without which the second leaves people (like me) scratching their head.
Also in this case a cover letter would help
(git format-patch --cover-letter), to give some idea of what you are after.

Cheers,
Andre


> 
> >Best regards,
> >Jernej  
> 
> Michael
> 
> >  
> >> ---
> >>  .../boot/dts/allwinner/sun7i-a20-bananapi.dts | 27 +++++++++++++++++++
> >>  1 file changed, 27 insertions(+)
> >>
> >> diff --git a/arch/arm/boot/dts/allwinner/sun7i-a20-bananapi.dts b/arch/arm/boot/dts/allwinner/sun7i-a20-bananapi.dts
> >> index 46ecf9db2324..4976453ed192 100644
> >> --- a/arch/arm/boot/dts/allwinner/sun7i-a20-bananapi.dts
> >> +++ b/arch/arm/boot/dts/allwinner/sun7i-a20-bananapi.dts
> >> @@ -48,6 +48,7 @@
> >>
> >>  #include <dt-bindings/gpio/gpio.h>
> >>  #include <dt-bindings/interrupt-controller/irq.h>
> >> +#include <dt-bindings/leds/common.h>
> >>
> >>  / {
> >>  	model = "LeMaker Banana Pi";
> >> @@ -169,6 +170,32 @@ &ir0 {
> >>  &gmac_mdio {
> >>  	phy1: ethernet-phy@1 {
> >>  		reg = <1>;
> >> +
> >> +		leds {
> >> +			#address-cells = <1>;
> >> +			#size-cells = <0>;
> >> +
> >> +			led@0 {
> >> +				reg = <0>;
> >> +				color = <LED_COLOR_ID_GREEN>;
> >> +				default-state = "keep";
> >> +				linux,default-trigger = "netdev";
> >> +			};
> >> +
> >> +			led@1 {
> >> +				reg = <1>;
> >> +				color = <LED_COLOR_ID_AMBER>;
> >> +				default-state = "keep";
> >> +				linux,default-trigger = "netdev";
> >> +			};
> >> +
> >> +			led@2 {
> >> +				reg = <2>;
> >> +				color = <LED_COLOR_ID_BLUE>;
> >> +				default-state = "keep";
> >> +				linux,default-trigger = "netdev";
> >> +			};
> >> +		};
> >>  	};
> >>  };
> >>
> >>  
> >
> >
> >
> >
> >  
> 

Re: [PATCH 2/2] ARM: dts: bananapi: add support for PHY LEDs

[Andrew Lunn]

On Thu, Mar 13, 2025 at 10:40:42AM +0000, Andre Przywara wrote:
> On Thu, 13 Mar 2025 09:34:50 +0100
> Michael Klein <michael@fossekall.de> wrote:
> 
> Hi,
> 
> > On Thu, Mar 13, 2025 at 07:07:24AM +0100, Jernej Škrabec wrote:
> > >Dne sreda, 12. marec 2025 ob 20:36:28 Srednjeevropski standardni čas je Michael Klein napisal(a):  
> > >> The Bananapi M1 has three LEDs connected to the RTL8211E ethernet PHY.
> > >> Add the corresponding nodes to the device tree.
> > >>
> > >> Signed-off-by: Michael Klein <michael@fossekall.de>  
> > >
> > >This is patch 2/2. Which one is patch 1/2? I got only one.  
> > 
> > https://patchwork.kernel.org/project/netdevbpf/patch/20250312193629.85417-1-michael@fossekall.de/
> > 
> > Sorry for any inconvenience in case I messed up the patch submission.
> > 
> > I made two commits for this change and submitted them via `git send-email 
> > HEAD^^`. The first patch went to netdev@vger.kernel.org, the second 
> > to linux-arm-kernel@lists.infradead.org, which seems logical. Have I 
> > done something wrong?
> 
> Well, for those really small "series" it's probably better to send all
> patches to everyone, especially if the first patch gives some context,
> without which the second leaves people (like me) scratching their head.

However, netdev does not like pathchsets which contain patches which
should not be applied to the netdev tree. DT patches generally go
through a different Maintainer to driver changes implementing the
binding.

So for your DT patch, you could add to the commit message something
like:

The RTL8211E ethernet PHY driver has recently gained support for
controlling PHY LEDs via /sys/class/leds. The Bananapi M1 has three
LEDs connected to the RTL8211E PHY.  Add the corresponding nodes to
the device tree.

	Andrew

Re: [PATCH 2/2] ARM: dts: bananapi: add support for PHY LEDs

[Michael Klein]

On Thu, Mar 13, 2025 at 02:17:27PM +0100, Andrew Lunn wrote:
>On Thu, Mar 13, 2025 at 10:40:42AM +0000, Andre Przywara wrote:
>> On Thu, 13 Mar 2025 09:34:50 +0100
>> Michael Klein <michael@fossekall.de> wrote:
>> > On Thu, Mar 13, 2025 at 07:07:24AM +0100, Jernej Škrabec wrote:
>> > >Dne sreda, 12. marec 2025 ob 20:36:28 Srednjeevropski standardni čas je Michael Klein napisal(a):
>> > >> The Bananapi M1 has three LEDs connected to the RTL8211E ethernet PHY.
>> > >> Add the corresponding nodes to the device tree.
>> > >>
>> > >> Signed-off-by: Michael Klein <michael@fossekall.de>
>> > >
>> > >This is patch 2/2. Which one is patch 1/2? I got only one.
>> >
>> > https://patchwork.kernel.org/project/netdevbpf/patch/20250312193629.85417-1-michael@fossekall.de/
>> >
>> > Sorry for any inconvenience in case I messed up the patch submission.
>> >
>> > I made two commits for this change and submitted them via `git send-email
>> > HEAD^^`. The first patch went to netdev@vger.kernel.org, the second
>> > to linux-arm-kernel@lists.infradead.org, which seems logical. Have I
>> > done something wrong?
>>
>> Well, for those really small "series" it's probably better to send all
>> patches to everyone, especially if the first patch gives some context,
>> without which the second leaves people (like me) scratching their head.
>
>However, netdev does not like pathchsets which contain patches which
>should not be applied to the netdev tree. DT patches generally go
>through a different Maintainer to driver changes implementing the
>binding.
>
>So for your DT patch, you could add to the commit message something
>like:
>
>The RTL8211E ethernet PHY driver has recently gained support for
>controlling PHY LEDs via /sys/class/leds. The Bananapi M1 has three
>LEDs connected to the RTL8211E PHY.  Add the corresponding nodes to
>the device tree.

Thanks, this totally makes sense. I'll split the patch series then and 
follow-up here with the updated commit message when the netdev change is 
accepted.

-- 
Michael

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

[Andrew Lunn]

On Wed, Mar 12, 2025 at 08:36:27PM +0100, Michael Klein wrote:
> Like the RTL8211F, the RTL8211E PHY supports up to three LEDs.
> Add netdev trigger support for them, too.
> 
> Signed-off-by: Michael Klein <michael@fossekall.de>
> ---
>  drivers/net/phy/realtek.c | 120 ++++++++++++++++++++++++++++++++++++--

What tree is this based on?

ommit 1416a9b2ba710d31954131c06d46f298e340aa2c
Author: Heiner Kallweit <hkallweit1@gmail.com>
Date:   Sat Jan 11 21:50:19 2025 +0100

    net: phy: move realtek PHY driver to its own subdirectory
    
    In preparation of adding a source file with hwmon support, move the
    Realtek PHY driver to its own subdirectory and rename realtek.c to
    realtek_main.c.


https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

> +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?

       Andrew

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

[Michael Klein]

On Thu, Mar 13, 2025 at 05:45:05PM +0100, Andrew Lunn wrote:
>On Wed, Mar 12, 2025 at 08:36:27PM +0100, Michael Klein wrote:
>> Like the RTL8211F, the RTL8211E PHY supports up to three LEDs.
>> Add netdev trigger support for them, too.
>>
>> Signed-off-by: Michael Klein <michael@fossekall.de>
>> ---
>>  drivers/net/phy/realtek.c | 120 ++++++++++++++++++++++++++++++++++++--
>
>What tree is this based on?

This was based on mainline, will be addressed in the next version.

>> +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

-- 
Michael

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

[Andrew Lunn]

> > > +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