diff mbox series

[net-next] r8169: add LED support for RTL8125/RTL8126

Message ID f982602c-9de3-4ca6-85a3-2c1d118dcb15@gmail.com (mailing list archive)
State Accepted
Commit be51ed104ba9929c741afb718ef7198dbcecef94
Delegated to: Netdev Maintainers
Headers show
Series [net-next] r8169: add LED support for RTL8125/RTL8126 | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 989 this patch: 989
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 1006 this patch: 1006
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1006 this patch: 1006
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 212 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-02-13--21-00 (tests: 1437)

Commit Message

Heiner Kallweit Feb. 12, 2024, 6:44 p.m. UTC
This adds LED support for RTL8125/RTL8126.

Note: Due to missing datasheets changing the 5Gbps link mode isn't
supported for RTL8126.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169.h      |   3 +
 drivers/net/ethernet/realtek/r8169_leds.c | 106 ++++++++++++++++++++++
 drivers/net/ethernet/realtek/r8169_main.c |  61 ++++++++++++-
 3 files changed, 166 insertions(+), 4 deletions(-)

Comments

Andrew Lunn Feb. 13, 2024, 3:11 a.m. UTC | #1
> +static int rtl8125_get_led_reg(int index)
> +{
> +	static const int led_regs[] = { LEDSEL0, LEDSEL1, LEDSEL2, LEDSEL3 };
> +
> +	return led_regs[index];
> +}
> +
> +int rtl8125_set_led_mode(struct rtl8169_private *tp, int index, u16 mode)
> +{
> +	int reg = rtl8125_get_led_reg(index);
> +	struct device *dev = tp_to_dev(tp);
> +	int ret;
> +	u16 val;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	mutex_lock(&tp->led_lock);
> +	val = RTL_R16(tp, reg) & ~LEDSEL_MASK_8125;
> +	RTL_W16(tp, reg, val | mode);
> +	mutex_unlock(&tp->led_lock);

I'm wondering if this mutex is actually needed. Each LED has its own
register. So you don't need to worry about setting two different LEDs
in parallel. Its just a question of, can the LED core act on one LED
in parallel? I don't know the answer to this, but it does use delayed
work for some things, and that should not run in parallel.

Maybe you can look into this and see if its really needed. Otherwise,
lets keep it, it does no real harm.

     Andrew
Heiner Kallweit Feb. 13, 2024, 7:19 a.m. UTC | #2
On 13.02.2024 04:11, Andrew Lunn wrote:
>> +static int rtl8125_get_led_reg(int index)
>> +{
>> +	static const int led_regs[] = { LEDSEL0, LEDSEL1, LEDSEL2, LEDSEL3 };
>> +
>> +	return led_regs[index];
>> +}
>> +
>> +int rtl8125_set_led_mode(struct rtl8169_private *tp, int index, u16 mode)
>> +{
>> +	int reg = rtl8125_get_led_reg(index);
>> +	struct device *dev = tp_to_dev(tp);
>> +	int ret;
>> +	u16 val;
>> +
>> +	ret = pm_runtime_resume_and_get(dev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	mutex_lock(&tp->led_lock);
>> +	val = RTL_R16(tp, reg) & ~LEDSEL_MASK_8125;
>> +	RTL_W16(tp, reg, val | mode);
>> +	mutex_unlock(&tp->led_lock);
> 
> I'm wondering if this mutex is actually needed. Each LED has its own
> register. So you don't need to worry about setting two different LEDs
> in parallel. Its just a question of, can the LED core act on one LED
> in parallel? I don't know the answer to this, but it does use delayed
> work for some things, and that should not run in parallel.
> 
This is applicable for non-atomic set_brightness ops.
For sysfs changes of the hw-controlled mode, the call chain is:
netdev_led_attr_store()
  set_baseline_state()
    rtl8125_led_hw_control_set()
      rtl8125_set_led_mode()
        mutex_lock()

So I think we need an own serialization.

> Maybe you can look into this and see if its really needed. Otherwise,
> lets keep it, it does no real harm.
> 
>      Andrew

Heiner
Andrew Lunn Feb. 13, 2024, 12:52 p.m. UTC | #3
On Mon, Feb 12, 2024 at 07:44:11PM +0100, Heiner Kallweit wrote:
> This adds LED support for RTL8125/RTL8126.
> 
> Note: Due to missing datasheets changing the 5Gbps link mode isn't
> supported for RTL8126.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
patchwork-bot+netdevbpf@kernel.org Feb. 14, 2024, 3 a.m. UTC | #4
Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 12 Feb 2024 19:44:11 +0100 you wrote:
> This adds LED support for RTL8125/RTL8126.
> 
> Note: Due to missing datasheets changing the 5Gbps link mode isn't
> supported for RTL8126.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> [...]

Here is the summary with links:
  - [net-next] r8169: add LED support for RTL8125/RTL8126
    https://git.kernel.org/netdev/net-next/c/be51ed104ba9

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/net/ethernet/realtek/r8169.h b/drivers/net/ethernet/realtek/r8169.h
index c921456ed..4c0430521 100644
--- a/drivers/net/ethernet/realtek/r8169.h
+++ b/drivers/net/ethernet/realtek/r8169.h
@@ -85,3 +85,6 @@  void r8169_get_led_name(struct rtl8169_private *tp, int idx,
 int rtl8168_get_led_mode(struct rtl8169_private *tp);
 int rtl8168_led_mod_ctrl(struct rtl8169_private *tp, u16 mask, u16 val);
 void rtl8168_init_leds(struct net_device *ndev);
+int rtl8125_get_led_mode(struct rtl8169_private *tp, int index);
+int rtl8125_set_led_mode(struct rtl8169_private *tp, int index, u16 mode);
+void rtl8125_init_leds(struct net_device *ndev);
diff --git a/drivers/net/ethernet/realtek/r8169_leds.c b/drivers/net/ethernet/realtek/r8169_leds.c
index 78078eca3..7c5dc9d0d 100644
--- a/drivers/net/ethernet/realtek/r8169_leds.c
+++ b/drivers/net/ethernet/realtek/r8169_leds.c
@@ -18,7 +18,14 @@ 
 #define RTL8168_LED_CTRL_LINK_100	BIT(1)
 #define RTL8168_LED_CTRL_LINK_10	BIT(0)
 
+#define RTL8125_LED_CTRL_ACT		BIT(9)
+#define RTL8125_LED_CTRL_LINK_2500	BIT(5)
+#define RTL8125_LED_CTRL_LINK_1000	BIT(3)
+#define RTL8125_LED_CTRL_LINK_100	BIT(1)
+#define RTL8125_LED_CTRL_LINK_10	BIT(0)
+
 #define RTL8168_NUM_LEDS		3
+#define RTL8125_NUM_LEDS		4
 
 struct r8169_led_classdev {
 	struct led_classdev led;
@@ -156,3 +163,102 @@  void rtl8168_init_leds(struct net_device *ndev)
 	for (i = 0; i < RTL8168_NUM_LEDS; i++)
 		rtl8168_setup_ldev(leds + i, ndev, i);
 }
+
+static int rtl8125_led_hw_control_is_supported(struct led_classdev *led_cdev,
+					       unsigned long flags)
+{
+	struct r8169_led_classdev *ldev = lcdev_to_r8169_ldev(led_cdev);
+	struct rtl8169_private *tp = netdev_priv(ldev->ndev);
+
+	if (!r8169_trigger_mode_is_valid(flags)) {
+		/* Switch LED off to indicate that mode isn't supported */
+		rtl8125_set_led_mode(tp, ldev->index, 0);
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int rtl8125_led_hw_control_set(struct led_classdev *led_cdev,
+				      unsigned long flags)
+{
+	struct r8169_led_classdev *ldev = lcdev_to_r8169_ldev(led_cdev);
+	struct rtl8169_private *tp = netdev_priv(ldev->ndev);
+	u16 mode = 0;
+
+	if (flags & BIT(TRIGGER_NETDEV_LINK_10))
+		mode |= RTL8125_LED_CTRL_LINK_10;
+	if (flags & BIT(TRIGGER_NETDEV_LINK_100))
+		mode |= RTL8125_LED_CTRL_LINK_100;
+	if (flags & BIT(TRIGGER_NETDEV_LINK_1000))
+		mode |= RTL8125_LED_CTRL_LINK_1000;
+	if (flags & BIT(TRIGGER_NETDEV_LINK_2500))
+		mode |= RTL8125_LED_CTRL_LINK_2500;
+	if (flags & (BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX)))
+		mode |= RTL8125_LED_CTRL_ACT;
+
+	return rtl8125_set_led_mode(tp, ldev->index, mode);
+}
+
+static int rtl8125_led_hw_control_get(struct led_classdev *led_cdev,
+				      unsigned long *flags)
+{
+	struct r8169_led_classdev *ldev = lcdev_to_r8169_ldev(led_cdev);
+	struct rtl8169_private *tp = netdev_priv(ldev->ndev);
+	int mode;
+
+	mode = rtl8125_get_led_mode(tp, ldev->index);
+	if (mode < 0)
+		return mode;
+
+	if (mode & RTL8125_LED_CTRL_LINK_10)
+		*flags |= BIT(TRIGGER_NETDEV_LINK_10);
+	if (mode & RTL8125_LED_CTRL_LINK_100)
+		*flags |= BIT(TRIGGER_NETDEV_LINK_100);
+	if (mode & RTL8125_LED_CTRL_LINK_1000)
+		*flags |= BIT(TRIGGER_NETDEV_LINK_1000);
+	if (mode & RTL8125_LED_CTRL_LINK_2500)
+		*flags |= BIT(TRIGGER_NETDEV_LINK_2500);
+	if (mode & RTL8125_LED_CTRL_ACT)
+		*flags |= BIT(TRIGGER_NETDEV_TX) | BIT(TRIGGER_NETDEV_RX);
+
+	return 0;
+}
+
+static void rtl8125_setup_led_ldev(struct r8169_led_classdev *ldev,
+				   struct net_device *ndev, int index)
+{
+	struct rtl8169_private *tp = netdev_priv(ndev);
+	struct led_classdev *led_cdev = &ldev->led;
+	char led_name[LED_MAX_NAME_SIZE];
+
+	ldev->ndev = ndev;
+	ldev->index = index;
+
+	r8169_get_led_name(tp, index, led_name, LED_MAX_NAME_SIZE);
+	led_cdev->name = led_name;
+	led_cdev->hw_control_trigger = "netdev";
+	led_cdev->flags |= LED_RETAIN_AT_SHUTDOWN;
+	led_cdev->hw_control_is_supported = rtl8125_led_hw_control_is_supported;
+	led_cdev->hw_control_set = rtl8125_led_hw_control_set;
+	led_cdev->hw_control_get = rtl8125_led_hw_control_get;
+	led_cdev->hw_control_get_device = r8169_led_hw_control_get_device;
+
+	/* ignore errors */
+	devm_led_classdev_register(&ndev->dev, led_cdev);
+}
+
+void rtl8125_init_leds(struct net_device *ndev)
+{
+	/* bind resource mgmt to netdev */
+	struct device *dev = &ndev->dev;
+	struct r8169_led_classdev *leds;
+	int i;
+
+	leds = devm_kcalloc(dev, RTL8125_NUM_LEDS, sizeof(*leds), GFP_KERNEL);
+	if (!leds)
+		return;
+
+	for (i = 0; i < RTL8125_NUM_LEDS; i++)
+		rtl8125_setup_led_ldev(leds + i, ndev, i);
+}
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index b43db3c49..9fdc9b0f3 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -330,17 +330,23 @@  enum rtl8168_registers {
 };
 
 enum rtl8125_registers {
+	LEDSEL0			= 0x18,
 	INT_CFG0_8125		= 0x34,
 #define INT_CFG0_ENABLE_8125		BIT(0)
 #define INT_CFG0_CLKREQEN		BIT(3)
 	IntrMask_8125		= 0x38,
 	IntrStatus_8125		= 0x3c,
 	INT_CFG1_8125		= 0x7a,
+	LEDSEL2			= 0x84,
+	LEDSEL1			= 0x86,
 	TxPoll_8125		= 0x90,
+	LEDSEL3			= 0x96,
 	MAC0_BKP		= 0x19e0,
 	EEE_TXIDLE_TIMER_8125	= 0x6048,
 };
 
+#define LEDSEL_MASK_8125	0x23f
+
 #define RX_VLAN_INNER_8125	BIT(22)
 #define RX_VLAN_OUTER_8125	BIT(23)
 #define RX_VLAN_8125		(RX_VLAN_INNER_8125 | RX_VLAN_OUTER_8125)
@@ -830,6 +836,51 @@  int rtl8168_get_led_mode(struct rtl8169_private *tp)
 	return ret;
 }
 
+static int rtl8125_get_led_reg(int index)
+{
+	static const int led_regs[] = { LEDSEL0, LEDSEL1, LEDSEL2, LEDSEL3 };
+
+	return led_regs[index];
+}
+
+int rtl8125_set_led_mode(struct rtl8169_private *tp, int index, u16 mode)
+{
+	int reg = rtl8125_get_led_reg(index);
+	struct device *dev = tp_to_dev(tp);
+	int ret;
+	u16 val;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0)
+		return ret;
+
+	mutex_lock(&tp->led_lock);
+	val = RTL_R16(tp, reg) & ~LEDSEL_MASK_8125;
+	RTL_W16(tp, reg, val | mode);
+	mutex_unlock(&tp->led_lock);
+
+	pm_runtime_put_sync(dev);
+
+	return 0;
+}
+
+int rtl8125_get_led_mode(struct rtl8169_private *tp, int index)
+{
+	int reg = rtl8125_get_led_reg(index);
+	struct device *dev = tp_to_dev(tp);
+	int ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0)
+		return ret;
+
+	ret = RTL_R16(tp, reg);
+
+	pm_runtime_put_sync(dev);
+
+	return ret;
+}
+
 void r8169_get_led_name(struct rtl8169_private *tp, int idx,
 			char *buf, int buf_len)
 {
@@ -5388,10 +5439,12 @@  static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (rc)
 		return rc;
 
-	if (IS_ENABLED(CONFIG_R8169_LEDS) &&
-	    tp->mac_version > RTL_GIGA_MAC_VER_06 &&
-	    tp->mac_version < RTL_GIGA_MAC_VER_61)
-		rtl8168_init_leds(dev);
+	if (IS_ENABLED(CONFIG_R8169_LEDS)) {
+		if (rtl_is_8125(tp))
+			rtl8125_init_leds(dev);
+		else if (tp->mac_version > RTL_GIGA_MAC_VER_06)
+			rtl8168_init_leds(dev);
+	}
 
 	netdev_info(dev, "%s, %pM, XID %03x, IRQ %d\n",
 		    rtl_chip_infos[chipset].name, dev->dev_addr, xid, tp->irq);