[PATCH net-next] net: dsa: mv88e6xxx: Enable LED control on 88E6171

[PATCH net-next] net: dsa: mv88e6xxx: Enable LED control on 88E6171

[Aleksander Jan Bajkowski]

Enable LED control on the Marvell 88E6171. This switch uses a register
layout similar to the 6352 family, but has 4 LEDs instead of 2. Hardware
triggers are not supported at this moment.

Signed-off-by: Aleksander Jan Bajkowski <olek2@wp.pl>
---
 drivers/net/dsa/mv88e6xxx/chip.c |  10 ++
 drivers/net/dsa/mv88e6xxx/chip.h |  16 ++++
 drivers/net/dsa/mv88e6xxx/leds.c | 155 ++++++++++++++++++++++++++-----
 drivers/net/dsa/mv88e6xxx/port.h |  35 +++++++
 4 files changed, 191 insertions(+), 25 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 425fd2d22c28..ac25a6711ac6 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -4609,6 +4609,7 @@ static const struct mv88e6xxx_ops mv88e6171_ops = {
 	.port_pause_limit = mv88e6097_port_pause_limit,
 	.port_disable_learn_limit = mv88e6xxx_port_disable_learn_limit,
 	.port_disable_pri_override = mv88e6xxx_port_disable_pri_override,
+	.port_setup_leds = mv88e6xxx_port_setup_leds,
 	.port_get_cmode = mv88e6352_port_get_cmode,
 	.port_setup_message_port = mv88e6xxx_setup_message_port,
 	.stats_snapshot = mv88e6xxx_g1_stats_snapshot,
@@ -6014,6 +6015,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_macs = 8192,
 		.num_ports = 7,
 		.num_internal_phys = 5,
+		.num_leds = 4,
 		.max_vid = 4095,
 		.max_sid = 63,
 		.port_base_addr = 0x10,
@@ -6040,6 +6042,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_ports = 7,
 		.num_internal_phys = 5,
 		.num_gpio = 15,
+		.num_leds = 2,
 		.max_vid = 4095,
 		.max_sid = 63,
 		.port_base_addr = 0x10,
@@ -6052,6 +6055,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.stats_type = STATS_TYPE_BANK0 | STATS_TYPE_PORT,
 		.atu_move_port_mask = 0xf,
 		.pvt = true,
+		.led_hw_triggers = true,
 		.multi_chip = true,
 		.edsa_support = MV88E6XXX_EDSA_SUPPORTED,
 		.ops = &mv88e6172_ops,
@@ -6091,6 +6095,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_ports = 7,
 		.num_internal_phys = 5,
 		.num_gpio = 15,
+		.num_leds = 2,
 		.max_vid = 4095,
 		.max_sid = 63,
 		.port_base_addr = 0x10,
@@ -6103,6 +6108,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.stats_type = STATS_TYPE_BANK0 | STATS_TYPE_PORT,
 		.atu_move_port_mask = 0xf,
 		.pvt = true,
+		.led_hw_triggers = true,
 		.multi_chip = true,
 		.edsa_support = MV88E6XXX_EDSA_SUPPORTED,
 		.ops = &mv88e6176_ops,
@@ -6291,6 +6297,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_ports = 7,
 		.num_internal_phys = 5,
 		.num_gpio = 15,
+		.num_leds = 2,
 		.max_vid = 4095,
 		.max_sid = 63,
 		.port_base_addr = 0x10,
@@ -6303,6 +6310,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.stats_type = STATS_TYPE_BANK0 | STATS_TYPE_PORT,
 		.atu_move_port_mask = 0xf,
 		.pvt = true,
+		.led_hw_triggers = true,
 		.multi_chip = true,
 		.edsa_support = MV88E6XXX_EDSA_SUPPORTED,
 		.ptp_support = true,
@@ -6500,6 +6508,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.num_ports = 7,
 		.num_internal_phys = 5,
 		.num_gpio = 15,
+		.num_leds = 2,
 		.max_vid = 4095,
 		.max_sid = 63,
 		.port_base_addr = 0x10,
@@ -6512,6 +6521,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
 		.stats_type = STATS_TYPE_BANK0 | STATS_TYPE_PORT,
 		.atu_move_port_mask = 0xf,
 		.pvt = true,
+		.led_hw_triggers = true,
 		.multi_chip = true,
 		.edsa_support = MV88E6XXX_EDSA_SUPPORTED,
 		.ptp_support = true,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index e966e7c4cc5d..deb32fe46954 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -135,6 +135,7 @@ struct mv88e6xxx_info {
 	unsigned int num_ports;
 	unsigned int num_internal_phys;
 	unsigned int num_gpio;
+	unsigned int num_leds;
 	unsigned int num_tcam_entries;
 	unsigned int max_vid;
 	unsigned int max_sid;
@@ -149,6 +150,9 @@ struct mv88e6xxx_info {
 	int stats_type;
 	bool pvt;
 
+	/* Support hardware LED triggers */
+	bool led_hw_triggers;
+
 	/* Mark certain ports as invalid. This is required for example for the
 	 * MV88E6220 (which is in general a MV88E6250 with 7 ports) but the
 	 * ports 2-4 are not routet to pins.
@@ -301,6 +305,8 @@ struct mv88e6xxx_port {
 	bool fiber;
 	struct led_classdev led0;
 	struct led_classdev led1;
+	struct led_classdev led2;
+	struct led_classdev led3;
 
 	/* MacAuth Bypass control flag */
 	bool mab;
@@ -810,6 +816,11 @@ static inline bool mv88e6xxx_has_pvt(struct mv88e6xxx_chip *chip)
 	return chip->info->pvt;
 }
 
+static inline bool mv88e6xxx_has_led_hw_triggers(struct mv88e6xxx_chip *chip)
+{
+	return chip->info->led_hw_triggers;
+}
+
 static inline bool mv88e6xxx_has_lag(struct mv88e6xxx_chip *chip)
 {
 	return !!chip->info->global2_addr;
@@ -855,6 +866,11 @@ static inline unsigned int mv88e6xxx_num_gpio(struct mv88e6xxx_chip *chip)
 	return chip->info->num_gpio;
 }
 
+static inline unsigned int mv88e6xxx_num_leds(struct mv88e6xxx_chip *chip)
+{
+	return chip->info->num_leds;
+}
+
 static inline bool mv88e6xxx_is_invalid_port(struct mv88e6xxx_chip *chip, int port)
 {
 	return (chip->info->invalid_port_mask & BIT(port)) != 0;
diff --git a/drivers/net/dsa/mv88e6xxx/leds.c b/drivers/net/dsa/mv88e6xxx/leds.c
index ab3bc645da56..925c120afba3 100644
--- a/drivers/net/dsa/mv88e6xxx/leds.c
+++ b/drivers/net/dsa/mv88e6xxx/leds.c
@@ -37,32 +37,53 @@ static int mv88e6xxx_led_brightness_set(struct mv88e6xxx_port *p, int led,
 	u16 reg;
 	int err;
 
-	err = mv88e6xxx_port_led_read(p->chip, p->port,
-				      MV88E6XXX_PORT_LED_CONTROL_POINTER_LED01_CTRL,
-				      &reg);
+	if (led == 2 || led == 3)
+		err = mv88e6xxx_port_led_read(p->chip, p->port,
+					      MV88E6XXX_PORT_LED_CONTROL_POINTER_LED23_CTRL,
+					      &reg);
+	else
+		err = mv88e6xxx_port_led_read(p->chip, p->port,
+					      MV88E6XXX_PORT_LED_CONTROL_POINTER_LED01_CTRL,
+					      &reg);
+
 	if (err)
 		return err;
 
-	if (led == 1)
+	if (led == 3)
+		reg &= ~MV88E6XXX_PORT_LED_CONTROL_LED3_SEL_MASK;
+	else if (led == 2)
+		reg &= ~MV88E6XXX_PORT_LED_CONTROL_LED2_SEL_MASK;
+	else if (led == 1)
 		reg &= ~MV88E6XXX_PORT_LED_CONTROL_LED1_SEL_MASK;
 	else
 		reg &= ~MV88E6XXX_PORT_LED_CONTROL_LED0_SEL_MASK;
 
 	if (brightness) {
 		/* Selector 0x0f == Force LED ON */
-		if (led == 1)
+		if (led == 3)
+			reg |= MV88E6XXX_PORT_LED_CONTROL_LED3_SELF;
+		else if (led == 2)
+			reg |= MV88E6XXX_PORT_LED_CONTROL_LED2_SELF;
+		else if (led == 1)
 			reg |= MV88E6XXX_PORT_LED_CONTROL_LED1_SELF;
 		else
 			reg |= MV88E6XXX_PORT_LED_CONTROL_LED0_SELF;
 	} else {
 		/* Selector 0x0e == Force LED OFF */
-		if (led == 1)
+		if (led == 3)
+			reg |= MV88E6XXX_PORT_LED_CONTROL_LED3_SELE;
+		else if (led == 2)
+			reg |= MV88E6XXX_PORT_LED_CONTROL_LED2_SELE;
+		else if (led == 1)
 			reg |= MV88E6XXX_PORT_LED_CONTROL_LED1_SELE;
 		else
 			reg |= MV88E6XXX_PORT_LED_CONTROL_LED0_SELE;
 	}
 
-	reg |= MV88E6XXX_PORT_LED_CONTROL_POINTER_LED01_CTRL;
+	if (led == 2 || led == 3)
+		reg |= MV88E6XXX_PORT_LED_CONTROL_POINTER_LED23_CTRL;
+	else
+		reg |= MV88E6XXX_PORT_LED_CONTROL_POINTER_LED01_CTRL;
 
 	return mv88e6xxx_port_led_write(p->chip, p->port, reg);
 }
@@ -93,6 +114,32 @@ static int mv88e6xxx_led1_brightness_set_blocking(struct led_classdev *ldev,
 	return err;
 }
 
+static int mv88e6xxx_led2_brightness_set_blocking(struct led_classdev *ldev,
+						  enum led_brightness brightness)
+{
+	struct mv88e6xxx_port *p = container_of(ldev, struct mv88e6xxx_port, led2);
+	int err;
+
+	mv88e6xxx_reg_lock(p->chip);
+	err = mv88e6xxx_led_brightness_set(p, 2, brightness);
+	mv88e6xxx_reg_unlock(p->chip);
+
+	return err;
+}
+
+static int mv88e6xxx_led3_brightness_set_blocking(struct led_classdev *ldev,
+						  enum led_brightness brightness)
+{
+	struct mv88e6xxx_port *p = container_of(ldev, struct mv88e6xxx_port, led3);
+	int err;
+
+	mv88e6xxx_reg_lock(p->chip);
+	err = mv88e6xxx_led_brightness_set(p, 3, brightness);
+	mv88e6xxx_reg_unlock(p->chip);
+
+	return err;
+}
+
 struct mv88e6xxx_led_hwconfig {
 	int led;
 	u8 portmask;
@@ -526,24 +573,40 @@ static int mv88e6xxx_led_blink_set(struct mv88e6xxx_port *p, int led,
 	if (err)
 		return err;
 
-	err = mv88e6xxx_port_led_read(p->chip, p->port,
-				      MV88E6XXX_PORT_LED_CONTROL_POINTER_LED01_CTRL,
-				      &reg);
+	if (led == 2 || led == 3)
+		err = mv88e6xxx_port_led_read(p->chip, p->port,
+					      MV88E6XXX_PORT_LED_CONTROL_POINTER_LED23_CTRL,
+					      &reg);
+	else
+		err = mv88e6xxx_port_led_read(p->chip, p->port,
+					      MV88E6XXX_PORT_LED_CONTROL_POINTER_LED01_CTRL,
+					      &reg);
 	if (err)
 		return err;
 
-	if (led == 1)
+	if (led == 3)
+		reg &= ~MV88E6XXX_PORT_LED_CONTROL_LED3_SEL_MASK;
+	else if (led == 2)
+		reg &= ~MV88E6XXX_PORT_LED_CONTROL_LED2_SEL_MASK;
+	else if (led == 1)
 		reg &= ~MV88E6XXX_PORT_LED_CONTROL_LED1_SEL_MASK;
 	else
 		reg &= ~MV88E6XXX_PORT_LED_CONTROL_LED0_SEL_MASK;
 
 	/* This will select the forced blinking status */
-	if (led == 1)
+	if (led == 3)
+		reg |= MV88E6XXX_PORT_LED_CONTROL_LED3_SELD;
+	else if (led == 2)
+		reg |= MV88E6XXX_PORT_LED_CONTROL_LED2_SELD;
+	else if (led == 1)
 		reg |= MV88E6XXX_PORT_LED_CONTROL_LED1_SELD;
 	else
 		reg |= MV88E6XXX_PORT_LED_CONTROL_LED0_SELD;
 
-	reg |= MV88E6XXX_PORT_LED_CONTROL_POINTER_LED01_CTRL;
+	if (led == 2 || led == 3)
+		reg |= MV88E6XXX_PORT_LED_CONTROL_POINTER_LED23_CTRL;
+	else
+		reg |= MV88E6XXX_PORT_LED_CONTROL_POINTER_LED01_CTRL;
 
 	return mv88e6xxx_port_led_write(p->chip, p->port, reg);
 }
@@ -576,6 +639,34 @@ static int mv88e6xxx_led1_blink_set(struct led_classdev *ldev,
 	return err;
 }
 
+static int mv88e6xxx_led2_blink_set(struct led_classdev *ldev,
+				    unsigned long *delay_on,
+				    unsigned long *delay_off)
+{
+	struct mv88e6xxx_port *p = container_of(ldev, struct mv88e6xxx_port, led2);
+	int err;
+
+	mv88e6xxx_reg_lock(p->chip);
+	err = mv88e6xxx_led_blink_set(p, 2, delay_on, delay_off);
+	mv88e6xxx_reg_unlock(p->chip);
+
+	return err;
+}
+
+static int mv88e6xxx_led3_blink_set(struct led_classdev *ldev,
+				    unsigned long *delay_on,
+				    unsigned long *delay_off)
+{
+	struct mv88e6xxx_port *p = container_of(ldev, struct mv88e6xxx_port, led3);
+	int err;
+
+	mv88e6xxx_reg_lock(p->chip);
+	err = mv88e6xxx_led_blink_set(p, 3, delay_on, delay_off);
+	mv88e6xxx_reg_unlock(p->chip);
+
+	return err;
+}
+
 static int
 mv88e6xxx_led0_hw_control_is_supported(struct led_classdev *ldev, unsigned long rules)
 {
@@ -771,13 +862,13 @@ int mv88e6xxx_port_setup_leds(struct mv88e6xxx_chip *chip, int port)
 	}
 
 	fwnode_for_each_child_node(leds, led) {
-		/* Reg represent the led number of the port, max 2
+		/* Reg represent the led number of the port, max 4
 		 * LEDs can be connected to each port, in some designs
 		 * only one LED is connected.
 		 */
 		if (fwnode_property_read_u32(led, "reg", &led_num))
 			continue;
-		if (led_num > 1) {
+		if (led_num >= mv88e6xxx_num_leds(chip)) {
 			dev_err(dev, "invalid LED specified port %d\n", port);
 			ret = -EINVAL;
 			goto err_put_led;
@@ -785,8 +876,12 @@ int mv88e6xxx_port_setup_leds(struct mv88e6xxx_chip *chip, int port)
 
 		if (led_num == 0)
 			l = &p->led0;
-		else
+		else if (led_num == 1)
 			l = &p->led1;
+		else if (led_num == 2)
+			l = &p->led2;
+		else
+			l = &p->led3;
 
 		state = led_init_default_state_get(led);
 		switch (state) {
@@ -805,17 +900,27 @@ int mv88e6xxx_port_setup_leds(struct mv88e6xxx_chip *chip, int port)
 		if (led_num == 0) {
 			l->brightness_set_blocking = mv88e6xxx_led0_brightness_set_blocking;
 			l->blink_set = mv88e6xxx_led0_blink_set;
-			l->hw_control_is_supported = mv88e6xxx_led0_hw_control_is_supported;
-			l->hw_control_set = mv88e6xxx_led0_hw_control_set;
-			l->hw_control_get = mv88e6xxx_led0_hw_control_get;
-			l->hw_control_get_device = mv88e6xxx_led0_hw_control_get_device;
-		} else {
+			if (mv88e6xxx_has_led_hw_triggers(chip)) {
+				l->hw_control_is_supported = mv88e6xxx_led0_hw_control_is_supported;
+				l->hw_control_set = mv88e6xxx_led0_hw_control_set;
+				l->hw_control_get = mv88e6xxx_led0_hw_control_get;
+				l->hw_control_get_device = mv88e6xxx_led0_hw_control_get_device;
+			}
+		} else if (led_num == 1) {
 			l->brightness_set_blocking = mv88e6xxx_led1_brightness_set_blocking;
 			l->blink_set = mv88e6xxx_led1_blink_set;
-			l->hw_control_is_supported = mv88e6xxx_led1_hw_control_is_supported;
-			l->hw_control_set = mv88e6xxx_led1_hw_control_set;
-			l->hw_control_get = mv88e6xxx_led1_hw_control_get;
-			l->hw_control_get_device = mv88e6xxx_led1_hw_control_get_device;
+			if (mv88e6xxx_has_led_hw_triggers(chip)) {
+				l->hw_control_is_supported = mv88e6xxx_led1_hw_control_is_supported;
+				l->hw_control_set = mv88e6xxx_led1_hw_control_set;
+				l->hw_control_get = mv88e6xxx_led1_hw_control_get;
+				l->hw_control_get_device = mv88e6xxx_led1_hw_control_get_device;
+			}
+		} else if (led_num == 2) {
+			l->brightness_set_blocking = mv88e6xxx_led2_brightness_set_blocking;
+			l->blink_set = mv88e6xxx_led2_blink_set;
+		} else {
+			l->brightness_set_blocking = mv88e6xxx_led3_brightness_set_blocking;
+			l->blink_set = mv88e6xxx_led3_blink_set;
 		}
 		l->hw_control_trigger = "netdev";
 
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index 5b6cde9f7406..45d062af50ea 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -317,12 +317,15 @@
 #define MV88E6XXX_PORT_LED_CONTROL_UPDATE			BIT(15)
 #define MV88E6XXX_PORT_LED_CONTROL_POINTER_MASK			GENMASK(14, 12)
 #define MV88E6XXX_PORT_LED_CONTROL_POINTER_LED01_CTRL		(0x00 << 12) /* Control for LED 0 and 1 */
+#define MV88E6XXX_PORT_LED_CONTROL_POINTER_LED23_CTRL		(0x01 << 12) /* Control for LED 2 and 3 */
 #define MV88E6XXX_PORT_LED_CONTROL_POINTER_STRETCH_BLINK	(0x06 << 12) /* Stetch and Blink Rate */
 #define MV88E6XXX_PORT_LED_CONTROL_POINTER_CNTL_SPECIAL		(0x07 << 12) /* Control for the Port's Special LED */
 #define MV88E6XXX_PORT_LED_CONTROL_DATA_MASK			GENMASK(10, 0)
 /* Selection masks valid for either port 1,2,3,4 or 5 */
 #define MV88E6XXX_PORT_LED_CONTROL_LED0_SEL_MASK		GENMASK(3, 0)
 #define MV88E6XXX_PORT_LED_CONTROL_LED1_SEL_MASK		GENMASK(7, 4)
+#define MV88E6XXX_PORT_LED_CONTROL_LED2_SEL_MASK		GENMASK(3, 0)
+#define MV88E6XXX_PORT_LED_CONTROL_LED3_SEL_MASK		GENMASK(7, 4)
 /* Selection control for LED 0 and 1, ports 5 and 6 only has LED 0
  * Bits  Function
  * 0..3  LED 0 control selector on ports 1-5
@@ -415,6 +418,38 @@
 #define MV88E6XXX_PORT_LED_CONTROL_LED1_SELD			(0xd << 4)
 #define MV88E6XXX_PORT_LED_CONTROL_LED1_SELE			(0xe << 4)
 #define MV88E6XXX_PORT_LED_CONTROL_LED1_SELF			(0xf << 4)
+#define MV88E6XXX_PORT_LED_CONTROL_LED2_SEL0			0x0
+#define MV88E6XXX_PORT_LED_CONTROL_LED2_SEL1			0x1
+#define MV88E6XXX_PORT_LED_CONTROL_LED2_SEL2			0x2
+#define MV88E6XXX_PORT_LED_CONTROL_LED2_SEL3			0x3
+#define MV88E6XXX_PORT_LED_CONTROL_LED2_SEL4			0x4
+#define MV88E6XXX_PORT_LED_CONTROL_LED2_SEL5			0x5
+#define MV88E6XXX_PORT_LED_CONTROL_LED2_SEL6			0x6
+#define MV88E6XXX_PORT_LED_CONTROL_LED2_SEL7			0x7
+#define MV88E6XXX_PORT_LED_CONTROL_LED2_SEL8			0x8
+#define MV88E6XXX_PORT_LED_CONTROL_LED2_SEL9			0x9
+#define MV88E6XXX_PORT_LED_CONTROL_LED2_SELA			0xa
+#define MV88E6XXX_PORT_LED_CONTROL_LED2_SELB			0xb
+#define MV88E6XXX_PORT_LED_CONTROL_LED2_SELC			0xc
+#define MV88E6XXX_PORT_LED_CONTROL_LED2_SELD			0xd
+#define MV88E6XXX_PORT_LED_CONTROL_LED2_SELE			0xe
+#define MV88E6XXX_PORT_LED_CONTROL_LED2_SELF			0xf
+#define MV88E6XXX_PORT_LED_CONTROL_LED3_SEL0			(0x0 << 4)
+#define MV88E6XXX_PORT_LED_CONTROL_LED3_SEL1			(0x1 << 4)
+#define MV88E6XXX_PORT_LED_CONTROL_LED3_SEL2			(0x2 << 4)
+#define MV88E6XXX_PORT_LED_CONTROL_LED3_SEL3			(0x3 << 4)
+#define MV88E6XXX_PORT_LED_CONTROL_LED3_SEL4			(0x4 << 4)
+#define MV88E6XXX_PORT_LED_CONTROL_LED3_SEL5			(0x5 << 4)
+#define MV88E6XXX_PORT_LED_CONTROL_LED3_SEL6			(0x6 << 4)
+#define MV88E6XXX_PORT_LED_CONTROL_LED3_SEL7			(0x7 << 4)
+#define MV88E6XXX_PORT_LED_CONTROL_LED3_SEL8			(0x8 << 4)
+#define MV88E6XXX_PORT_LED_CONTROL_LED3_SEL9			(0x9 << 4)
+#define MV88E6XXX_PORT_LED_CONTROL_LED3_SELA			(0xa << 4)
+#define MV88E6XXX_PORT_LED_CONTROL_LED3_SELB			(0xb << 4)
+#define MV88E6XXX_PORT_LED_CONTROL_LED3_SELC			(0xc << 4)
+#define MV88E6XXX_PORT_LED_CONTROL_LED3_SELD			(0xd << 4)
+#define MV88E6XXX_PORT_LED_CONTROL_LED3_SELE			(0xe << 4)
+#define MV88E6XXX_PORT_LED_CONTROL_LED3_SELF			(0xf << 4)
 /* Stretch and Blink Rate Control (Index 0x06 of LED Control) */
 /* Pulse Stretch Selection for all LED's on this port */
 #define MV88E6XXX_PORT_LED_CONTROL_0x06_PULSE_STRETCH_NONE	(0 << 4)
-- 
2.55.0

Re: [PATCH net-next] net: dsa: mv88e6xxx: Enable LED control on 88E6171

[Andrew Lunn]

On Sun, Nov 15, 2026 at 07:12:36PM +0100, Aleksander Jan Bajkowski wrote:
> Enable LED control on the Marvell 88E6171. This switch uses a register
> layout similar to the 6352 family, but has 4 LEDs instead of 2. Hardware
> triggers are not supported at this moment.

Please break this patch up.

.num_leds makes a lot of sense, but please introduce it as a patch.

.led_hw_triggers i'm not sure about. But please add it as a patch of
its own.

> @@ -4609,6 +4609,7 @@ static const struct mv88e6xxx_ops mv88e6171_ops = {
>  	.port_pause_limit = mv88e6097_port_pause_limit,
>  	.port_disable_learn_limit = mv88e6xxx_port_disable_learn_limit,
>  	.port_disable_pri_override = mv88e6xxx_port_disable_pri_override,
> +	.port_setup_leds = mv88e6xxx_port_setup_leds,
>  	.port_get_cmode = mv88e6352_port_get_cmode,
>  	.port_setup_message_port = mv88e6xxx_setup_message_port,
>  	.stats_snapshot = mv88e6xxx_g1_stats_snapshot,

static const struct mv88e6xxx_ops mv88e6171_ops = {
	/* MV88E6XXX_FAMILY_6351

I assume these changes work for all members of the family?

> @@ -301,6 +305,8 @@ struct mv88e6xxx_port {
>  	bool fiber;
>  	struct led_classdev led0;
>  	struct led_classdev led1;
> +	struct led_classdev led2;
> +	struct led_classdev led3;

Maybe turn it into an array?

> -	if (led == 1)
> +	if (led == 3)
> +		reg &= ~MV88E6XXX_PORT_LED_CONTROL_LED3_SEL_MASK;
> +	else if (led == 2)
> +		reg &= ~MV88E6XXX_PORT_LED_CONTROL_LED2_SEL_MASK;
> +	else if (led == 1)
>  		reg &= ~MV88E6XXX_PORT_LED_CONTROL_LED1_SEL_MASK;
>  	else
>  		reg &= ~MV88E6XXX_PORT_LED_CONTROL_LED0_SEL_MASK;

Seems like this should be turned into a switch statement? Or can this
be simplified with some simple arithmetic or FIELD_PREP?

> +static int mv88e6xxx_led2_brightness_set_blocking(struct led_classdev *ldev,
> +						  enum led_brightness brightness)
> +{
> +	struct mv88e6xxx_port *p = container_of(ldev, struct mv88e6xxx_port, led2);
> +	int err;
> +
> +	mv88e6xxx_reg_lock(p->chip);
> +	err = mv88e6xxx_led_brightness_set(p, 2, brightness);
> +	mv88e6xxx_reg_unlock(p->chip);
> +
> +	return err;
> +}
> +
> +static int mv88e6xxx_led3_brightness_set_blocking(struct led_classdev *ldev,
> +						  enum led_brightness brightness)
> +{
> +	struct mv88e6xxx_port *p = container_of(ldev, struct mv88e6xxx_port, led3);
> +	int err;
> +
> +	mv88e6xxx_reg_lock(p->chip);
> +	err = mv88e6xxx_led_brightness_set(p, 3, brightness);
> +	mv88e6xxx_reg_unlock(p->chip);
> +
> +	return err;
> +}

Maybe mv88e6xxx_ledx_brightness_set_blocking(), and 4 one line
wrappers around it?

The current code is not scaling well, so please look at refactoring it
make it scale.

Lots of small patches, which are obviously correct, and good commit
messages.

     Andrew