[PATCH net-next v2 0/3] net: dsa: realtek: fix LED support for rtl8366

[PATCH net-next v2 0/3] net: dsa: realtek: fix LED support for rtl8366

[Luiz Angelo Daros de Luca]

This series fixes the LED support for rtl8366. The existing code was not
tested in a device with switch LEDs and it was using a flawed logic.

The driver now keeps the default LED configuration if nothing requests a
different behavior. This may be enough for most devices. This can be
achieved either by omitting the LED from the device-tree or configuring
all LEDs in a group with the default state set to "keep".

The hardware trigger for LEDs in Realtek switches is shared among all
LEDs in a group. This behavior doesn't align well with the Linux LED
API, which controls LEDs individually. Once the OS changes the
brightness of a LED in a group still triggered by the hardware, the
entire group switches to software-controlled LEDs, even for those not
metioned in the device-tree. This shared behavior also prevents
offloading the trigger to the hardware as it would require an
orchestration between LEDs in a group, not currently present in the LED
API.

The assertion of device hardware reset during driver removal was removed
because it was causing an issue with the LED release code. Devres
devices are released after the driver's removal is executed. Asserting
the reset at that point was causing timeout errors during LED release
when it attempted to turn off the LED.
 

To: Linus Walleij <linus.walleij@linaro.org>
To: Alvin Šipraga <alsi@bang-olufsen.dk>
To: Andrew Lunn <andrew@lunn.ch>
To: Florian Fainelli <f.fainelli@gmail.com>
To: Vladimir Oltean <olteanv@gmail.com>
To: David S. Miller <davem@davemloft.net>
To: Eric Dumazet <edumazet@google.com>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: Rob Herring <robh+dt@kernel.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
To: Conor Dooley <conor+dt@kernel.org>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>

Changes in v2:
- Fixed commit message formatting
- Added GROUP to LED group enum values. With that, moved the code that
  disables LED into a new function to keep 80-collumn limit. 
- Dropped unused enable argument in rb8366rb_get_port_led()
- Fixed variable order in rtl8366rb_setup_led()
- Removed redundant led group test in rb8366rb_{g,s}et_port_led()
- Initialize ret as 0 in rtl8366rb_setup_leds()
- Updated comments related to LED blinking and setup
- Link to v1: https://lore.kernel.org/r/20240310-realtek-led-v1-0-4d9813ce938e@gmail.com

Changes in v1:
- Rebased on new relatek DSA drivers
- Improved commit messages
- Added commit to remove the reset assert during .remove
- Link to RFC: https://lore.kernel.org/r/20240106184651.3665-1-luizluca@gmail.com

---
Luiz Angelo Daros de Luca (3):
      net: dsa: realtek: keep default LED state in rtl8366rb
      net: dsa: realtek: do not assert reset on remove
      net: dsa: realtek: add LED drivers for rtl8366rb

 drivers/net/dsa/realtek/rtl8366rb.c | 363 +++++++++++++++++++++++++++---------
 drivers/net/dsa/realtek/rtl83xx.c   |   7 +-
 2 files changed, 273 insertions(+), 97 deletions(-)
---
base-commit: 5c4c0edca68a5841a8d53ccd49596fe199c8334c
change-id: 20240212-realtek-led-a665ae39a9c5

Best regards,
-- 
Luiz Angelo Daros de Luca <luizluca@gmail.com>

[PATCH net-next v2 1/3] net: dsa: realtek: keep default LED state in rtl8366rb

[Luiz Angelo Daros de Luca]

This switch family supports four LEDs for each of its six ports. Each
LED group is composed of one of these four LEDs from all six ports. LED
groups can be configured to display hardware information, such as link
activity, or manually controlled through a bitmap in registers
RTL8366RB_LED_0_1_CTRL_REG and RTL8366RB_LED_2_3_CTRL_REG.

After a reset, the default LED group configuration for groups 0 to 3
indicates, respectively, link activity, link at 1000M, 100M, and 10M, or
RTL8366RB_LED_CTRL_REG as 0x5432. These configurations are commonly used
for LED indications. However, the driver was replacing that
configuration to use manually controlled LEDs (RTL8366RB_LED_FORCE)
without providing a way for the OS to control them. The default
configuration is deemed more useful than fixed, uncontrollable turned-on
LEDs.

The driver was enabling/disabling LEDs during port_enable/disable.
However, these events occur when the port is administratively controlled
(up or down) and are not related to link presence. Additionally, when a
port N was disabled, the driver was turning off all LEDs for group N,
not only the corresponding LED for port N in any of those 4 groups. In
such cases, if port 0 was brought down, the LEDs for all ports in LED
group 0 would be turned off. As another side effect, the driver was
wrongly warning that port 5 didn't have an LED ("no LED for port 5").
Since showing the administrative state of ports is not an orthodox way
to use LEDs, it was not worth it to fix it and all this code was
dropped.

The code to disable LEDs was simplified only changing each LED group to
the RTL8366RB_LED_OFF state. Registers RTL8366RB_LED_0_1_CTRL_REG and
RTL8366RB_LED_2_3_CTRL_REG are only used when the corresponding LED
group is configured with RTL8366RB_LED_FORCE and they don't need to be
cleaned. The code still references an LED controlled by
RTL8366RB_INTERRUPT_CONTROL_REG, but as of now, no test device has
actually used it. Also, some magic numbers were replaced by macros.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/dsa/realtek/rtl8366rb.c | 87 +++++++++----------------------------
 1 file changed, 20 insertions(+), 67 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index e10ae94cf771..5ccb1a3a149d 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -185,7 +185,12 @@
 #define RTL8366RB_LED_BLINKRATE_222MS		0x0004
 #define RTL8366RB_LED_BLINKRATE_446MS		0x0005
 
+/* LED trigger event for each group */
 #define RTL8366RB_LED_CTRL_REG			0x0431
+#define RTL8366RB_LED_CTRL_OFFSET(led_group)	\
+	(4 * (led_group))
+#define RTL8366RB_LED_CTRL_MASK(led_group)	\
+	(0xf << RTL8366RB_LED_CTRL_OFFSET(led_group))
 #define RTL8366RB_LED_OFF			0x0
 #define RTL8366RB_LED_DUP_COL			0x1
 #define RTL8366RB_LED_LINK_ACT			0x2
@@ -202,6 +207,11 @@
 #define RTL8366RB_LED_LINK_TX			0xd
 #define RTL8366RB_LED_MASTER			0xe
 #define RTL8366RB_LED_FORCE			0xf
+
+/* The RTL8366RB_LED_X_X registers are used to manually set the LED state only
+ * when the corresponding LED group in RTL8366RB_LED_CTRL_REG is
+ * RTL8366RB_LED_FORCE. Otherwise, it is ignored.
+ */
 #define RTL8366RB_LED_0_1_CTRL_REG		0x0432
 #define RTL8366RB_LED_1_OFFSET			6
 #define RTL8366RB_LED_2_3_CTRL_REG		0x0433
@@ -1001,28 +1011,20 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 	 */
 	if (priv->leds_disabled) {
 		/* Turn everything off */
-		regmap_update_bits(priv->map,
-				   RTL8366RB_LED_0_1_CTRL_REG,
-				   0x0FFF, 0);
-		regmap_update_bits(priv->map,
-				   RTL8366RB_LED_2_3_CTRL_REG,
-				   0x0FFF, 0);
 		regmap_update_bits(priv->map,
 				   RTL8366RB_INTERRUPT_CONTROL_REG,
 				   RTL8366RB_P4_RGMII_LED,
 				   0);
-		val = RTL8366RB_LED_OFF;
-	} else {
-		/* TODO: make this configurable per LED */
-		val = RTL8366RB_LED_FORCE;
-	}
-	for (i = 0; i < 4; i++) {
-		ret = regmap_update_bits(priv->map,
-					 RTL8366RB_LED_CTRL_REG,
-					 0xf << (i * 4),
-					 val << (i * 4));
-		if (ret)
-			return ret;
+
+		for (i = 0; i < RTL8366RB_NUM_LEDGROUPS; i++) {
+			val = RTL8366RB_LED_OFF << RTL8366RB_LED_CTRL_OFFSET(i);
+			ret = regmap_update_bits(priv->map,
+						 RTL8366RB_LED_CTRL_REG,
+						 RTL8366RB_LED_CTRL_MASK(i),
+						 val);
+			if (ret)
+				return ret;
+		}
 	}
 
 	ret = rtl8366_reset_vlan(priv);
@@ -1167,52 +1169,6 @@ rtl8366rb_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode,
 	}
 }
 
-static void rb8366rb_set_port_led(struct realtek_priv *priv,
-				  int port, bool enable)
-{
-	u16 val = enable ? 0x3f : 0;
-	int ret;
-
-	if (priv->leds_disabled)
-		return;
-
-	switch (port) {
-	case 0:
-		ret = regmap_update_bits(priv->map,
-					 RTL8366RB_LED_0_1_CTRL_REG,
-					 0x3F, val);
-		break;
-	case 1:
-		ret = regmap_update_bits(priv->map,
-					 RTL8366RB_LED_0_1_CTRL_REG,
-					 0x3F << RTL8366RB_LED_1_OFFSET,
-					 val << RTL8366RB_LED_1_OFFSET);
-		break;
-	case 2:
-		ret = regmap_update_bits(priv->map,
-					 RTL8366RB_LED_2_3_CTRL_REG,
-					 0x3F, val);
-		break;
-	case 3:
-		ret = regmap_update_bits(priv->map,
-					 RTL8366RB_LED_2_3_CTRL_REG,
-					 0x3F << RTL8366RB_LED_3_OFFSET,
-					 val << RTL8366RB_LED_3_OFFSET);
-		break;
-	case 4:
-		ret = regmap_update_bits(priv->map,
-					 RTL8366RB_INTERRUPT_CONTROL_REG,
-					 RTL8366RB_P4_RGMII_LED,
-					 enable ? RTL8366RB_P4_RGMII_LED : 0);
-		break;
-	default:
-		dev_err(priv->dev, "no LED for port %d\n", port);
-		return;
-	}
-	if (ret)
-		dev_err(priv->dev, "error updating LED on port %d\n", port);
-}
-
 static int
 rtl8366rb_port_enable(struct dsa_switch *ds, int port,
 		      struct phy_device *phy)
@@ -1226,7 +1182,6 @@ rtl8366rb_port_enable(struct dsa_switch *ds, int port,
 	if (ret)
 		return ret;
 
-	rb8366rb_set_port_led(priv, port, true);
 	return 0;
 }
 
@@ -1241,8 +1196,6 @@ rtl8366rb_port_disable(struct dsa_switch *ds, int port)
 				 BIT(port));
 	if (ret)
 		return;
-
-	rb8366rb_set_port_led(priv, port, false);
 }
 
 static int

-- 
2.44.0

[PATCH net-next v2 2/3] net: dsa: realtek: do not assert reset on remove

[Luiz Angelo Daros de Luca]

The necessity of asserting the reset on removal was previously
questioned, as DSA's own cleanup methods should suffice to prevent
traffic leakage[1].

When a driver has subdrivers controlled by devres, they will be
unregistered after the main driver's .remove is executed. If it asserts
a reset, the subdrivers will be unable to communicate with the hardware
during their cleanup. For LEDs, this means that they will fail to turn
off, resulting in a timeout error.

[1] https://lore.kernel.org/r/20240123215606.26716-9-luizluca@gmail.com/

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/dsa/realtek/rtl83xx.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
index d2e876805393..a9c1702431ef 100644
--- a/drivers/net/dsa/realtek/rtl83xx.c
+++ b/drivers/net/dsa/realtek/rtl83xx.c
@@ -290,16 +290,13 @@ EXPORT_SYMBOL_NS_GPL(rtl83xx_shutdown, REALTEK_DSA);
  * rtl83xx_remove() - Cleanup a realtek switch driver
  * @priv: realtek_priv pointer
  *
- * If a method is provided, this function asserts the hard reset of the switch
- * in order to avoid leaking traffic when the driver is gone.
+ * Placehold for common cleanup procedures.
  *
- * Context: Might sleep if priv->gdev->chip->can_sleep.
+ * Context: Any
  * Return: nothing
  */
 void rtl83xx_remove(struct realtek_priv *priv)
 {
-	/* leave the device reset asserted */
-	rtl83xx_reset_assert(priv);
 }
 EXPORT_SYMBOL_NS_GPL(rtl83xx_remove, REALTEK_DSA);
 

-- 
2.44.0

[PATCH net-next v2 3/3] net: dsa: realtek: add LED drivers for rtl8366rb

[Luiz Angelo Daros de Luca]

This commit introduces LED drivers for rtl8366rb, enabling LEDs to be
described in the device tree using the same format as qca8k. Each port
can configure up to 4 LEDs.

If all LEDs in a group use the default state "keep", they will use the
default behavior after a reset. Changing the brightness of one LED,
either manually or by a trigger, will disable the default hardware
trigger and switch the entire LED group to manually controlled LEDs.
Once in this mode, there is no way to revert to hardware-controlled LEDs
(except by resetting the switch).

Software triggers function as expected with manually controlled LEDs.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/dsa/realtek/rtl8366rb.c | 304 +++++++++++++++++++++++++++++++-----
 1 file changed, 265 insertions(+), 39 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c
index 5ccb1a3a149d..9244c63e8289 100644
--- a/drivers/net/dsa/realtek/rtl8366rb.c
+++ b/drivers/net/dsa/realtek/rtl8366rb.c
@@ -176,6 +176,7 @@
 #define RTL8366RB_VLAN_INGRESS_CTRL2_REG	0x037f
 
 /* LED control registers */
+/* The LED blink rate is global; it is used by all triggers in all groups. */
 #define RTL8366RB_LED_BLINKRATE_REG		0x0430
 #define RTL8366RB_LED_BLINKRATE_MASK		0x0007
 #define RTL8366RB_LED_BLINKRATE_28MS		0x0000
@@ -191,31 +192,21 @@
 	(4 * (led_group))
 #define RTL8366RB_LED_CTRL_MASK(led_group)	\
 	(0xf << RTL8366RB_LED_CTRL_OFFSET(led_group))
-#define RTL8366RB_LED_OFF			0x0
-#define RTL8366RB_LED_DUP_COL			0x1
-#define RTL8366RB_LED_LINK_ACT			0x2
-#define RTL8366RB_LED_SPD1000			0x3
-#define RTL8366RB_LED_SPD100			0x4
-#define RTL8366RB_LED_SPD10			0x5
-#define RTL8366RB_LED_SPD1000_ACT		0x6
-#define RTL8366RB_LED_SPD100_ACT		0x7
-#define RTL8366RB_LED_SPD10_ACT			0x8
-#define RTL8366RB_LED_SPD100_10_ACT		0x9
-#define RTL8366RB_LED_FIBER			0xa
-#define RTL8366RB_LED_AN_FAULT			0xb
-#define RTL8366RB_LED_LINK_RX			0xc
-#define RTL8366RB_LED_LINK_TX			0xd
-#define RTL8366RB_LED_MASTER			0xe
-#define RTL8366RB_LED_FORCE			0xf
 
 /* The RTL8366RB_LED_X_X registers are used to manually set the LED state only
  * when the corresponding LED group in RTL8366RB_LED_CTRL_REG is
- * RTL8366RB_LED_FORCE. Otherwise, it is ignored.
+ * RTL8366RB_LEDGROUP_FORCE. Otherwise, it is ignored.
  */
 #define RTL8366RB_LED_0_1_CTRL_REG		0x0432
-#define RTL8366RB_LED_1_OFFSET			6
 #define RTL8366RB_LED_2_3_CTRL_REG		0x0433
-#define RTL8366RB_LED_3_OFFSET			6
+#define RTL8366RB_LED_X_X_CTRL_REG(led_group)	\
+	((led_group) <= 1 ? \
+		RTL8366RB_LED_0_1_CTRL_REG : \
+		RTL8366RB_LED_2_3_CTRL_REG)
+#define RTL8366RB_LED_0_X_CTRL_MASK		GENMASK(5, 0)
+#define RTL8366RB_LED_X_1_CTRL_MASK		GENMASK(11, 6)
+#define RTL8366RB_LED_2_X_CTRL_MASK		GENMASK(5, 0)
+#define RTL8366RB_LED_X_3_CTRL_MASK		GENMASK(11, 6)
 
 #define RTL8366RB_MIB_COUNT			33
 #define RTL8366RB_GLOBAL_MIB_COUNT		1
@@ -359,14 +350,44 @@
 #define RTL8366RB_GREEN_FEATURE_TX	BIT(0)
 #define RTL8366RB_GREEN_FEATURE_RX	BIT(2)
 
+enum rtl8366_ledgroup_mode {
+	RTL8366RB_LEDGROUP_OFF			= 0x0,
+	RTL8366RB_LEDGROUP_DUP_COL		= 0x1,
+	RTL8366RB_LEDGROUP_LINK_ACT		= 0x2,
+	RTL8366RB_LEDGROUP_SPD1000		= 0x3,
+	RTL8366RB_LEDGROUP_SPD100		= 0x4,
+	RTL8366RB_LEDGROUP_SPD10		= 0x5,
+	RTL8366RB_LEDGROUP_SPD1000_ACT		= 0x6,
+	RTL8366RB_LEDGROUP_SPD100_ACT		= 0x7,
+	RTL8366RB_LEDGROUP_SPD10_ACT		= 0x8,
+	RTL8366RB_LEDGROUP_SPD100_10_ACT	= 0x9,
+	RTL8366RB_LEDGROUP_FIBER		= 0xa,
+	RTL8366RB_LEDGROUP_AN_FAULT		= 0xb,
+	RTL8366RB_LEDGROUP_LINK_RX		= 0xc,
+	RTL8366RB_LEDGROUP_LINK_TX		= 0xd,
+	RTL8366RB_LEDGROUP_MASTER		= 0xe,
+	RTL8366RB_LEDGROUP_FORCE		= 0xf,
+
+	__RTL8366RB_LEDGROUP_MODE_MAX
+};
+
+struct rtl8366rb_led {
+	u8 port_num;
+	u8 led_group;
+	struct realtek_priv *priv;
+	struct led_classdev cdev;
+};
+
 /**
  * struct rtl8366rb - RTL8366RB-specific data
  * @max_mtu: per-port max MTU setting
  * @pvid_enabled: if PVID is set for respective port
+ * @leds: per-port and per-ledgroup led info
  */
 struct rtl8366rb {
 	unsigned int max_mtu[RTL8366RB_NUM_PORTS];
 	bool pvid_enabled[RTL8366RB_NUM_PORTS];
+	struct rtl8366rb_led leds[RTL8366RB_NUM_PORTS][RTL8366RB_NUM_LEDGROUPS];
 };
 
 static struct rtl8366_mib_counter rtl8366rb_mib_counters[] = {
@@ -809,6 +830,217 @@ static int rtl8366rb_jam_table(const struct rtl8366rb_jam_tbl_entry *jam_table,
 	return 0;
 }
 
+static int rb8366rb_set_ledgroup_mode(struct realtek_priv *priv,
+				      u8 led_group,
+				      enum rtl8366_ledgroup_mode mode)
+{
+	int ret;
+	u32 val;
+
+	val = mode << RTL8366RB_LED_CTRL_OFFSET(led_group);
+
+	ret = regmap_update_bits(priv->map,
+				 RTL8366RB_LED_CTRL_REG,
+				 RTL8366RB_LED_CTRL_MASK(led_group),
+				 val);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static inline u32 rtl8366rb_led_group_port_mask(u8 led_group, u8 port)
+{
+	switch (led_group) {
+	case 0:
+		return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
+	case 1:
+		return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
+	case 2:
+		return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
+	case 3:
+		return FIELD_PREP(RTL8366RB_LED_0_X_CTRL_MASK, BIT(port));
+	default:
+		return 0;
+	}
+}
+
+static int rb8366rb_get_port_led(struct rtl8366rb_led *led)
+{
+	struct realtek_priv *priv = led->priv;
+	u8 led_group = led->led_group;
+	u8 port_num = led->port_num;
+	int ret;
+	u32 val;
+
+	ret = regmap_read(priv->map, RTL8366RB_LED_X_X_CTRL_REG(led_group),
+			  &val);
+	if (ret) {
+		dev_err(priv->dev, "error reading LED on port %d group %d\n",
+			led_group, port_num);
+		return ret;
+	}
+
+	return !!(val & rtl8366rb_led_group_port_mask(led_group, port_num));
+}
+
+static int rb8366rb_set_port_led(struct rtl8366rb_led *led, bool enable)
+{
+	struct realtek_priv *priv = led->priv;
+	u8 led_group = led->led_group;
+	u8 port_num = led->port_num;
+	int ret;
+
+	ret = regmap_update_bits(priv->map,
+				 RTL8366RB_LED_X_X_CTRL_REG(led_group),
+				 rtl8366rb_led_group_port_mask(led_group,
+							       port_num),
+				 enable ? 0xffff : 0);
+	if (ret) {
+		dev_err(priv->dev, "error updating LED on port %d group %d\n",
+			led_group, port_num);
+		return ret;
+	}
+
+	/* Change the LED group to manual controlled LEDs if required */
+	ret = rb8366rb_set_ledgroup_mode(priv, led_group,
+					 RTL8366RB_LEDGROUP_FORCE);
+
+	if (ret) {
+		dev_err(priv->dev, "error updating LED GROUP group %d\n",
+			led_group);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int
+rtl8366rb_cled_brightness_set_blocking(struct led_classdev *ldev,
+				       enum led_brightness brightness)
+{
+	struct rtl8366rb_led *led = container_of(ldev, struct rtl8366rb_led,
+						 cdev);
+
+	return rb8366rb_set_port_led(led, brightness == LED_ON);
+}
+
+static int rtl8366rb_setup_led(struct realtek_priv *priv, struct dsa_port *dp,
+			       struct fwnode_handle *led_fwnode)
+{
+	struct rtl8366rb *rb = priv->chip_data;
+	struct led_init_data init_data = { };
+	enum led_default_state state;
+	struct rtl8366rb_led *led;
+	u32 led_group;
+	int ret;
+
+	ret = fwnode_property_read_u32(led_fwnode, "reg", &led_group);
+	if (ret)
+		return ret;
+
+	if (led_group >= RTL8366RB_NUM_LEDGROUPS) {
+		dev_warn(priv->dev, "Invalid LED reg %d defined for port %d",
+			 led_group, dp->index);
+		return -EINVAL;
+	}
+
+	led = &rb->leds[dp->index][led_group];
+	led->port_num = dp->index;
+	led->led_group = led_group;
+	led->priv = priv;
+
+	state = led_init_default_state_get(led_fwnode);
+	switch (state) {
+	case LEDS_DEFSTATE_ON:
+		led->cdev.brightness = 1;
+		rb8366rb_set_port_led(led, 1);
+		break;
+	case LEDS_DEFSTATE_KEEP:
+		led->cdev.brightness =
+			rb8366rb_get_port_led(led);
+		break;
+	case LEDS_DEFSTATE_OFF:
+	default:
+		led->cdev.brightness = 0;
+		rb8366rb_set_port_led(led, 0);
+	}
+
+	led->cdev.max_brightness = 1;
+	led->cdev.brightness_set_blocking =
+		rtl8366rb_cled_brightness_set_blocking;
+	init_data.fwnode = led_fwnode;
+	init_data.devname_mandatory = true;
+
+	init_data.devicename = kasprintf(GFP_KERNEL, "Realtek-%d:0%d:%d",
+					 dp->ds->index, dp->index, led_group);
+	if (!init_data.devicename)
+		return -ENOMEM;
+
+	ret = devm_led_classdev_register_ext(priv->dev, &led->cdev, &init_data);
+	if (ret) {
+		dev_warn(priv->dev, "Failed to init LED %d for port %d",
+			 led_group, dp->index);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int rtl8366rb_setup_all_leds_off(struct realtek_priv *priv)
+{
+	int ret = 0;
+	int i;
+
+	regmap_update_bits(priv->map,
+			   RTL8366RB_INTERRUPT_CONTROL_REG,
+			   RTL8366RB_P4_RGMII_LED,
+			   0);
+
+	for (i = 0; i < RTL8366RB_NUM_LEDGROUPS; i++) {
+		ret = rb8366rb_set_ledgroup_mode(priv, i,
+						 RTL8366RB_LEDGROUP_OFF);
+		if (ret)
+			return ret;
+	}
+
+	return ret;
+}
+
+static int rtl8366rb_setup_leds(struct realtek_priv *priv)
+{
+	struct device_node *leds_np, *led_np;
+	struct dsa_switch *ds = &priv->ds;
+	struct dsa_port *dp;
+	int ret = 0;
+
+	dsa_switch_for_each_port(dp, ds) {
+		if (!dp->dn)
+			continue;
+
+		leds_np = of_get_child_by_name(dp->dn, "leds");
+		if (!leds_np) {
+			dev_dbg(priv->dev, "No leds defined for port %d",
+				dp->index);
+			continue;
+		}
+
+		for_each_child_of_node(leds_np, led_np) {
+			ret = rtl8366rb_setup_led(priv, dp,
+						  of_fwnode_handle(led_np));
+			if (ret) {
+				of_node_put(led_np);
+				break;
+			}
+		}
+
+		of_node_put(leds_np);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
 static int rtl8366rb_setup(struct dsa_switch *ds)
 {
 	struct realtek_priv *priv = ds->priv;
@@ -817,7 +1049,6 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 	u32 chip_ver = 0;
 	u32 chip_id = 0;
 	int jam_size;
-	u32 val;
 	int ret;
 	int i;
 
@@ -997,7 +1228,9 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
-	/* Set blinking, TODO: make this configurable */
+	/* Set blinking, used by all LED groups using HW triggers.
+	 * TODO: make this configurable
+	 */
 	ret = regmap_update_bits(priv->map, RTL8366RB_LED_BLINKRATE_REG,
 				 RTL8366RB_LED_BLINKRATE_MASK,
 				 RTL8366RB_LED_BLINKRATE_56MS);
@@ -1005,26 +1238,19 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 		return ret;
 
 	/* Set up LED activity:
-	 * Each port has 4 LEDs, we configure all ports to the same
-	 * behaviour (no individual config) but we can set up each
-	 * LED separately.
+	 * Each port has 4 LEDs on fixed groups. Each group shares the same
+	 * hardware trigger across all ports. LEDs can only be indiviually
+	 * controlled setting the LED group to fixed mode and using the driver
+	 * to toggle them LEDs on/off.
 	 */
 	if (priv->leds_disabled) {
-		/* Turn everything off */
-		regmap_update_bits(priv->map,
-				   RTL8366RB_INTERRUPT_CONTROL_REG,
-				   RTL8366RB_P4_RGMII_LED,
-				   0);
-
-		for (i = 0; i < RTL8366RB_NUM_LEDGROUPS; i++) {
-			val = RTL8366RB_LED_OFF << RTL8366RB_LED_CTRL_OFFSET(i);
-			ret = regmap_update_bits(priv->map,
-						 RTL8366RB_LED_CTRL_REG,
-						 RTL8366RB_LED_CTRL_MASK(i),
-						 val);
-			if (ret)
-				return ret;
-		}
+		ret = rtl8366rb_setup_all_leds_off(priv);
+		if (ret)
+			return ret;
+	} else {
+		ret = rtl8366rb_setup_leds(priv);
+		if (ret)
+			return ret;
 	}
 
 	ret = rtl8366_reset_vlan(priv);

-- 
2.44.0

Re: [PATCH net-next v2 3/3] net: dsa: realtek: add LED drivers for rtl8366rb

[Jakub Kicinski]

On Sat, 27 Apr 2024 02:11:30 -0300 Luiz Angelo Daros de Luca wrote:
> +static int rtl8366rb_setup_leds(struct realtek_priv *priv)
> +{
> +	struct device_node *leds_np, *led_np;
> +	struct dsa_switch *ds = &priv->ds;
> +	struct dsa_port *dp;
> +	int ret = 0;
> +
> +	dsa_switch_for_each_port(dp, ds) {
> +		if (!dp->dn)
> +			continue;
> +
> +		leds_np = of_get_child_by_name(dp->dn, "leds");
> +		if (!leds_np) {
> +			dev_dbg(priv->dev, "No leds defined for port %d",
> +				dp->index);
> +			continue;
> +		}
> +
> +		for_each_child_of_node(leds_np, led_np) {
> +			ret = rtl8366rb_setup_led(priv, dp,
> +						  of_fwnode_handle(led_np));
> +			if (ret) {
> +				of_node_put(led_np);
> +				break;
> +			}
> +		}
> +
> +		of_node_put(leds_np);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}

coccicheck generates this warning:

drivers/net/dsa/realtek/rtl8366rb.c:1032:4-15: ERROR: probable double put.

I think it's a false positive. But aren't you missing a put(dp) before
"return ret;" ?

Re: [PATCH net-next v2 3/3] net: dsa: realtek: add LED drivers for rtl8366rb

[Luiz Angelo Daros de Luca]

> On Sat, 27 Apr 2024 02:11:30 -0300 Luiz Angelo Daros de Luca wrote:
> > +static int rtl8366rb_setup_leds(struct realtek_priv *priv)
> > +{
> > +     struct device_node *leds_np, *led_np;
> > +     struct dsa_switch *ds = &priv->ds;
> > +     struct dsa_port *dp;
> > +     int ret = 0;
> > +
> > +     dsa_switch_for_each_port(dp, ds) {
> > +             if (!dp->dn)
> > +                     continue;
> > +
> > +             leds_np = of_get_child_by_name(dp->dn, "leds");
> > +             if (!leds_np) {
> > +                     dev_dbg(priv->dev, "No leds defined for port %d",
> > +                             dp->index);
> > +                     continue;
> > +             }
> > +
> > +             for_each_child_of_node(leds_np, led_np) {
> > +                     ret = rtl8366rb_setup_led(priv, dp,
> > +                                               of_fwnode_handle(led_np));
> > +                     if (ret) {
> > +                             of_node_put(led_np);
> > +                             break;
> > +                     }
> > +             }
> > +
> > +             of_node_put(leds_np);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +     return 0;
> > +}
>
> coccicheck generates this warning:
>
> drivers/net/dsa/realtek/rtl8366rb.c:1032:4-15: ERROR: probable double put.
>
> I think it's a false positive.

Me too. I don't think it is a double put. The put for led_np is called
in the increment code inside the for_each_child_of_node macro. With a
break, we skip that part and we need to put it before leaving. I don't
know coccicheck but maybe it got confused by the double for.

> But aren't you missing a put(dp) before
> "return ret;" ?

dsa_switch_for_each_port doesn't get nodes. So, no put required.

Regards,

Luiz

Re: [PATCH net-next v2 3/3] net: dsa: realtek: add LED drivers for rtl8366rb

[Linus Walleij]

On Thu, May 16, 2024 at 7:30 PM Luiz Angelo Daros de Luca
<luizluca@gmail.com> wrote:
> > On Sat, 27 Apr 2024 02:11:30 -0300 Luiz Angelo Daros de Luca wrote:
> > > +static int rtl8366rb_setup_leds(struct realtek_priv *priv)
> > > +{
> > > +     struct device_node *leds_np, *led_np;
> > > +     struct dsa_switch *ds = &priv->ds;
> > > +     struct dsa_port *dp;
> > > +     int ret = 0;
> > > +
> > > +     dsa_switch_for_each_port(dp, ds) {
> > > +             if (!dp->dn)
> > > +                     continue;
> > > +
> > > +             leds_np = of_get_child_by_name(dp->dn, "leds");
> > > +             if (!leds_np) {
> > > +                     dev_dbg(priv->dev, "No leds defined for port %d",
> > > +                             dp->index);
> > > +                     continue;
> > > +             }
> > > +
> > > +             for_each_child_of_node(leds_np, led_np) {
> > > +                     ret = rtl8366rb_setup_led(priv, dp,
> > > +                                               of_fwnode_handle(led_np));
> > > +                     if (ret) {
> > > +                             of_node_put(led_np);
> > > +                             break;
> > > +                     }
> > > +             }
> > > +
> > > +             of_node_put(leds_np);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +     return 0;
> > > +}
> >
> > coccicheck generates this warning:
> >
> > drivers/net/dsa/realtek/rtl8366rb.c:1032:4-15: ERROR: probable double put.
> >
> > I think it's a false positive.
>
> Me too. I don't think it is a double put. The put for led_np is called
> in the increment code inside the for_each_child_of_node macro. With a
> break, we skip that part and we need to put it before leaving. I don't
> know coccicheck but maybe it got confused by the double for.

Maybe I can use for_each_child_of_node_scoped() and
get the handling for free? The checkers should learn about
*_scoped now.

(I'm still working on the patch, I'm just slow.)

Yours,
Linus Walleij

Re: [PATCH net-next v2 3/3] net: dsa: realtek: add LED drivers for rtl8366rb

[Linus Walleij]

On Thu, May 16, 2024 at 11:05 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, May 16, 2024 at 7:30 PM Luiz Angelo Daros de Luca
> <luizluca@gmail.com> wrote:
> > > On Sat, 27 Apr 2024 02:11:30 -0300 Luiz Angelo Daros de Luca wrote:
> > > > +static int rtl8366rb_setup_leds(struct realtek_priv *priv)
> > > > +{
> > > > +     struct device_node *leds_np, *led_np;
> > > > +     struct dsa_switch *ds = &priv->ds;
> > > > +     struct dsa_port *dp;
> > > > +     int ret = 0;
> > > > +
> > > > +     dsa_switch_for_each_port(dp, ds) {
> > > > +             if (!dp->dn)
> > > > +                     continue;
> > > > +
> > > > +             leds_np = of_get_child_by_name(dp->dn, "leds");
> > > > +             if (!leds_np) {
> > > > +                     dev_dbg(priv->dev, "No leds defined for port %d",
> > > > +                             dp->index);
> > > > +                     continue;
> > > > +             }
> > > > +
> > > > +             for_each_child_of_node(leds_np, led_np) {
> > > > +                     ret = rtl8366rb_setup_led(priv, dp,
> > > > +                                               of_fwnode_handle(led_np));
> > > > +                     if (ret) {
> > > > +                             of_node_put(led_np);
> > > > +                             break;
> > > > +                     }
> > > > +             }
> > > > +
> > > > +             of_node_put(leds_np);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +     }
> > > > +     return 0;
> > > > +}
> > >
> > > coccicheck generates this warning:
> > >
> > > drivers/net/dsa/realtek/rtl8366rb.c:1032:4-15: ERROR: probable double put.
> > >
> > > I think it's a false positive.
> >
> > Me too. I don't think it is a double put. The put for led_np is called
> > in the increment code inside the for_each_child_of_node macro. With a
> > break, we skip that part and we need to put it before leaving. I don't
> > know coccicheck but maybe it got confused by the double for.
>
> Maybe I can use for_each_child_of_node_scoped() and
> get the handling for free? The checkers should learn about
> *_scoped now.
>
> (I'm still working on the patch, I'm just slow.)

Referring to my Marvell LED patch, confusing it for this one,
damn I need to get to sleep :D

Luiz: try to use for_each_child_of_node_scoped()!

Yours,
Linus Walleij

Re: [PATCH net-next v2 0/3] net: dsa: realtek: fix LED support for rtl8366

[patchwork-bot+netdevbpf]

Hello:

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

On Sat, 27 Apr 2024 02:11:27 -0300 you wrote:
> This series fixes the LED support for rtl8366. The existing code was not
> tested in a device with switch LEDs and it was using a flawed logic.
> 
> The driver now keeps the default LED configuration if nothing requests a
> different behavior. This may be enough for most devices. This can be
> achieved either by omitting the LED from the device-tree or configuring
> all LEDs in a group with the default state set to "keep".
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/3] net: dsa: realtek: keep default LED state in rtl8366rb
    https://git.kernel.org/netdev/net-next/c/5edc6585aafe
  - [net-next,v2,2/3] net: dsa: realtek: do not assert reset on remove
    https://git.kernel.org/netdev/net-next/c/4f580e9aced1
  - [net-next,v2,3/3] net: dsa: realtek: add LED drivers for rtl8366rb
    https://git.kernel.org/netdev/net-next/c/32d617005475

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