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

[Andrew Lunn <andrew@lunn.ch>]

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