[net-next,PATCH v2] net: phy: realtek: Add support for PHY LEDs on RTL8211F

[net-next,PATCH v2] net: phy: realtek: Add support for PHY LEDs on RTL8211F

[Marek Vasut]

Realtek RTL8211F Ethernet PHY supports 3 LED pins which are used to
indicate link status and activity. Add minimal LED controller driver
supporting the most common uses with the 'netdev' trigger.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Christophe Roullier <christophe.roullier@foss.st.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: kernel@dh-electronics.com
Cc: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org
---
V2: - RX and TX are not differentiated, either both are set or not set,
      filter this in rtl8211f_led_hw_is_supported()
---
 drivers/net/phy/realtek.c | 106 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 106 insertions(+)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 2174893c974f3..bed839237fb55 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -32,6 +32,15 @@
 #define RTL8211F_PHYCR2				0x19
 #define RTL8211F_INSR				0x1d
 
+#define RTL8211F_LEDCR				0x10
+#define RTL8211F_LEDCR_MODE			BIT(15)
+#define RTL8211F_LEDCR_ACT_TXRX			BIT(4)
+#define RTL8211F_LEDCR_LINK_1000		BIT(3)
+#define RTL8211F_LEDCR_LINK_100			BIT(1)
+#define RTL8211F_LEDCR_LINK_10			BIT(0)
+#define RTL8211F_LEDCR_MASK			GENMASK(4, 0)
+#define RTL8211F_LEDCR_SHIFT			5
+
 #define RTL8211F_TX_DELAY			BIT(8)
 #define RTL8211F_RX_DELAY			BIT(3)
 
@@ -87,6 +96,8 @@
 #define RTL_8221B_VN_CG				0x001cc84a
 #define RTL_8251B				0x001cc862
 
+#define RTL8211F_LED_COUNT			3
+
 MODULE_DESCRIPTION("Realtek PHY driver");
 MODULE_AUTHOR("Johnson Leung");
 MODULE_LICENSE("GPL");
@@ -476,6 +487,98 @@ static int rtl821x_resume(struct phy_device *phydev)
 	return 0;
 }
 
+static int rtl8211f_led_hw_is_supported(struct phy_device *phydev, u8 index,
+					unsigned long rules)
+{
+	const unsigned long mask = BIT(TRIGGER_NETDEV_LINK_10) |
+				   BIT(TRIGGER_NETDEV_LINK_100) |
+				   BIT(TRIGGER_NETDEV_LINK_1000) |
+				   BIT(TRIGGER_NETDEV_RX) |
+				   BIT(TRIGGER_NETDEV_TX);
+
+	/* The RTL8211F PHY supports these LED settings on up to three LEDs:
+	 * - Link: Configurable subset of 10/100/1000 link rates
+	 * - Active: Blink on activity, RX or TX is not differentiated
+	 * The Active option has two modes, A and B:
+	 * - A: Link and Active indication at configurable, but matching,
+	 *      subset of 10/100/1000 link rates
+	 * - B: Link indication at configurable subset of 10/100/1000 link
+	 *      rates and Active indication always at all three 10+100+1000
+	 *      link rates.
+	 * This code currently uses mode B only.
+	 */
+
+	if (index >= RTL8211F_LED_COUNT)
+		return -EINVAL;
+
+	/* Filter out any other unsupported triggers. */
+	if (rules & ~mask)
+		return -EOPNOTSUPP;
+
+	/* RX and TX are not differentiated, either both are set or not set. */
+	if (!(rules & BIT(TRIGGER_NETDEV_RX)) ^ !(rules & BIT(TRIGGER_NETDEV_TX)))
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+
+static int rtl8211f_led_hw_control_get(struct phy_device *phydev, u8 index,
+				       unsigned long *rules)
+{
+	int val;
+
+	val = phy_read_paged(phydev, 0xd04, RTL8211F_LEDCR);
+	if (val < 0)
+		return val;
+
+	val >>= RTL8211F_LEDCR_SHIFT * index;
+	val &= RTL8211F_LEDCR_MASK;
+
+	if (val & RTL8211F_LEDCR_LINK_10)
+		set_bit(TRIGGER_NETDEV_LINK_10, rules);
+
+	if (val & RTL8211F_LEDCR_LINK_100)
+		set_bit(TRIGGER_NETDEV_LINK_100, rules);
+
+	if (val & RTL8211F_LEDCR_LINK_1000)
+		set_bit(TRIGGER_NETDEV_LINK_1000, rules);
+
+	if (val & RTL8211F_LEDCR_ACT_TXRX) {
+		set_bit(TRIGGER_NETDEV_RX, rules);
+		set_bit(TRIGGER_NETDEV_TX, rules);
+	}
+
+	return 0;
+}
+
+static int rtl8211f_led_hw_control_set(struct phy_device *phydev, u8 index,
+				       unsigned long rules)
+{
+	const u16 mask = RTL8211F_LEDCR_MASK << (RTL8211F_LEDCR_SHIFT * index);
+	u16 reg = RTL8211F_LEDCR_MODE;	/* Mode B */
+
+	if (index >= RTL8211F_LED_COUNT)
+		return -EINVAL;
+
+	if (test_bit(TRIGGER_NETDEV_LINK_10, &rules))
+		reg |= RTL8211F_LEDCR_LINK_10;
+
+	if (test_bit(TRIGGER_NETDEV_LINK_100, &rules))
+		reg |= RTL8211F_LEDCR_LINK_100;
+
+	if (test_bit(TRIGGER_NETDEV_LINK_1000, &rules))
+		reg |= RTL8211F_LEDCR_LINK_1000;
+
+	if (test_bit(TRIGGER_NETDEV_RX, &rules) ||
+	    test_bit(TRIGGER_NETDEV_TX, &rules)) {
+		reg |= RTL8211F_LEDCR_ACT_TXRX;
+	}
+
+	reg <<= RTL8211F_LEDCR_SHIFT * index;
+
+	return phy_modify_paged(phydev, 0xd04, RTL8211F_LEDCR, mask, reg);
+}
+
 static int rtl8211e_config_init(struct phy_device *phydev)
 {
 	int ret = 0, oldpage;
@@ -1192,6 +1295,9 @@ 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_control_get = rtl8211f_led_hw_control_get,
+		.led_hw_control_set = rtl8211f_led_hw_control_set,
 	}, {
 		PHY_ID_MATCH_EXACT(RTL_8211FVD_PHYID),
 		.name		= "RTL8211F-VD Gigabit Ethernet",
-- 
2.43.0

Re: [net-next,PATCH v2] net: phy: realtek: Add support for PHY LEDs on RTL8211F

[Simon Horman]

On Tue, Jun 25, 2024 at 10:42:17PM +0200, Marek Vasut wrote:
> Realtek RTL8211F Ethernet PHY supports 3 LED pins which are used to
> indicate link status and activity. Add minimal LED controller driver
> supporting the most common uses with the 'netdev' trigger.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Christophe Roullier <christophe.roullier@foss.st.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: kernel@dh-electronics.com
> Cc: linux-kernel@vger.kernel.org
> Cc: netdev@vger.kernel.org
> ---
> V2: - RX and TX are not differentiated, either both are set or not set,
>       filter this in rtl8211f_led_hw_is_supported()
> ---
>  drivers/net/phy/realtek.c | 106 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 106 insertions(+)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 2174893c974f3..bed839237fb55 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -32,6 +32,15 @@
>  #define RTL8211F_PHYCR2				0x19
>  #define RTL8211F_INSR				0x1d
>  
> +#define RTL8211F_LEDCR				0x10
> +#define RTL8211F_LEDCR_MODE			BIT(15)
> +#define RTL8211F_LEDCR_ACT_TXRX			BIT(4)
> +#define RTL8211F_LEDCR_LINK_1000		BIT(3)
> +#define RTL8211F_LEDCR_LINK_100			BIT(1)
> +#define RTL8211F_LEDCR_LINK_10			BIT(0)
> +#define RTL8211F_LEDCR_MASK			GENMASK(4, 0)
> +#define RTL8211F_LEDCR_SHIFT			5
> +

Hi Marek,

FWIIW, I think that if you use FIELD_PREP and FIELD_GET then
RTL8211F_LEDCR_SHIFT can be removed.

...

Re: [net-next,PATCH v2] net: phy: realtek: Add support for PHY LEDs on RTL8211F

[Marek Vasut]

On 6/28/24 4:27 PM, Simon Horman wrote:
> On Tue, Jun 25, 2024 at 10:42:17PM +0200, Marek Vasut wrote:
>> Realtek RTL8211F Ethernet PHY supports 3 LED pins which are used to
>> indicate link status and activity. Add minimal LED controller driver
>> supporting the most common uses with the 'netdev' trigger.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
>> Cc: Andrew Lunn <andrew@lunn.ch>
>> Cc: Christophe Roullier <christophe.roullier@foss.st.com>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Eric Dumazet <edumazet@google.com>
>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>> Cc: Jakub Kicinski <kuba@kernel.org>
>> Cc: Paolo Abeni <pabeni@redhat.com>
>> Cc: Russell King <linux@armlinux.org.uk>
>> Cc: kernel@dh-electronics.com
>> Cc: linux-kernel@vger.kernel.org
>> Cc: netdev@vger.kernel.org
>> ---
>> V2: - RX and TX are not differentiated, either both are set or not set,
>>        filter this in rtl8211f_led_hw_is_supported()
>> ---
>>   drivers/net/phy/realtek.c | 106 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 106 insertions(+)
>>
>> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
>> index 2174893c974f3..bed839237fb55 100644
>> --- a/drivers/net/phy/realtek.c
>> +++ b/drivers/net/phy/realtek.c
>> @@ -32,6 +32,15 @@
>>   #define RTL8211F_PHYCR2				0x19
>>   #define RTL8211F_INSR				0x1d
>>   
>> +#define RTL8211F_LEDCR				0x10
>> +#define RTL8211F_LEDCR_MODE			BIT(15)
>> +#define RTL8211F_LEDCR_ACT_TXRX			BIT(4)
>> +#define RTL8211F_LEDCR_LINK_1000		BIT(3)
>> +#define RTL8211F_LEDCR_LINK_100			BIT(1)
>> +#define RTL8211F_LEDCR_LINK_10			BIT(0)
>> +#define RTL8211F_LEDCR_MASK			GENMASK(4, 0)
>> +#define RTL8211F_LEDCR_SHIFT			5
>> +
> 
> Hi Marek,
> 
> FWIIW, I think that if you use FIELD_PREP and FIELD_GET then
> RTL8211F_LEDCR_SHIFT can be removed.

FIELD_PREP/FIELD_GET only works for constant mask, in this case the mask 
is not constant but shifted by SHIFT*index .

Other drivers introduce workarounds like this for exactly this issue:

drivers/clk/at91/pmc.h:#define field_prep(_mask, _val) (((_val) << 
(ffs(_mask) - 1)) & (_mask))

I don't think it is worth perpetuating that.

Re: [net-next,PATCH v2] net: phy: realtek: Add support for PHY LEDs on RTL8211F

[Simon Horman]

On Fri, Jun 28, 2024 at 08:58:51PM +0200, Marek Vasut wrote:
> On 6/28/24 4:27 PM, Simon Horman wrote:
> > On Tue, Jun 25, 2024 at 10:42:17PM +0200, Marek Vasut wrote:
> > > Realtek RTL8211F Ethernet PHY supports 3 LED pins which are used to
> > > indicate link status and activity. Add minimal LED controller driver
> > > supporting the most common uses with the 'netdev' trigger.
> > > 
> > > Signed-off-by: Marek Vasut <marex@denx.de>
> > > ---
> > > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> > > Cc: Andrew Lunn <andrew@lunn.ch>
> > > Cc: Christophe Roullier <christophe.roullier@foss.st.com>
> > > Cc: David S. Miller <davem@davemloft.net>
> > > Cc: Eric Dumazet <edumazet@google.com>
> > > Cc: Heiner Kallweit <hkallweit1@gmail.com>
> > > Cc: Jakub Kicinski <kuba@kernel.org>
> > > Cc: Paolo Abeni <pabeni@redhat.com>
> > > Cc: Russell King <linux@armlinux.org.uk>
> > > Cc: kernel@dh-electronics.com
> > > Cc: linux-kernel@vger.kernel.org
> > > Cc: netdev@vger.kernel.org
> > > ---
> > > V2: - RX and TX are not differentiated, either both are set or not set,
> > >        filter this in rtl8211f_led_hw_is_supported()
> > > ---
> > >   drivers/net/phy/realtek.c | 106 ++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 106 insertions(+)
> > > 
> > > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > > index 2174893c974f3..bed839237fb55 100644
> > > --- a/drivers/net/phy/realtek.c
> > > +++ b/drivers/net/phy/realtek.c
> > > @@ -32,6 +32,15 @@
> > >   #define RTL8211F_PHYCR2				0x19
> > >   #define RTL8211F_INSR				0x1d
> > > +#define RTL8211F_LEDCR				0x10
> > > +#define RTL8211F_LEDCR_MODE			BIT(15)
> > > +#define RTL8211F_LEDCR_ACT_TXRX			BIT(4)
> > > +#define RTL8211F_LEDCR_LINK_1000		BIT(3)
> > > +#define RTL8211F_LEDCR_LINK_100			BIT(1)
> > > +#define RTL8211F_LEDCR_LINK_10			BIT(0)
> > > +#define RTL8211F_LEDCR_MASK			GENMASK(4, 0)
> > > +#define RTL8211F_LEDCR_SHIFT			5
> > > +
> > 
> > Hi Marek,
> > 
> > FWIIW, I think that if you use FIELD_PREP and FIELD_GET then
> > RTL8211F_LEDCR_SHIFT can be removed.
> 
> FIELD_PREP/FIELD_GET only works for constant mask, in this case the mask is
> not constant but shifted by SHIFT*index .
> 
> Other drivers introduce workarounds like this for exactly this issue:
> 
> drivers/clk/at91/pmc.h:#define field_prep(_mask, _val) (((_val) <<
> (ffs(_mask) - 1)) & (_mask))
> 
> I don't think it is worth perpetuating that.

Thanks Marek,

Sorry for missing that the mask is not constant.
And in that case I agree with the approach you have taken in this patch.

Re: [net-next,PATCH v2] net: phy: realtek: Add support for PHY LEDs on RTL8211F

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Tue, 25 Jun 2024 22:42:17 +0200 you wrote:
> Realtek RTL8211F Ethernet PHY supports 3 LED pins which are used to
> indicate link status and activity. Add minimal LED controller driver
> supporting the most common uses with the 'netdev' trigger.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Christophe Roullier <christophe.roullier@foss.st.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: kernel@dh-electronics.com
> Cc: linux-kernel@vger.kernel.org
> Cc: netdev@vger.kernel.org
> 
> [...]

Here is the summary with links:
  - [net-next,v2] net: phy: realtek: Add support for PHY LEDs on RTL8211F
    https://git.kernel.org/netdev/net-next/c/17784801d888

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html