diff mbox series

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

Message ID 20240427-realtek-led-v2-3-5abaddc32cf6@gmail.com (mailing list archive)
State Accepted
Commit 32d617005475a71ebcc4ec8b2791e8d1481e9a10
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: realtek: fix LED support for rtl8366 | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
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: 926 this patch: 926
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang success Errors and warnings before: 937 this patch: 937
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: 937 this patch: 937
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 362 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 fail Was 0 now: 1
netdev/contest fail net-next-2024-04-29--12-00 (tests: 1000)

Commit Message

Luiz Angelo Daros de Luca April 27, 2024, 5:11 a.m. UTC
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(-)

Comments

Jakub Kicinski April 29, 2024, 1:39 p.m. UTC | #1
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;" ?
Luiz Angelo Daros de Luca May 16, 2024, 5:30 p.m. UTC | #2
> 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
Linus Walleij May 16, 2024, 9:05 p.m. UTC | #3
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
Linus Walleij May 16, 2024, 9:16 p.m. UTC | #4
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
diff mbox series

Patch

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);