[PATCH v2 05/16] realtek: add switch port LED driver

[Alex G.]

On 10/3/22 15:52, Sander Vanheule wrote:

>    3. Port type index (0 for RJ45/primary, 1 for SFP/secondary)
> 
> The driver refers to the RJ45 and SFP LEDs as "primary" and "secondary",
> since SFP LED values are always output from the peripheral after RJ45
> LED values. Note that on Maple it is not possible to control these
> secondary LEDs from userspace. Nevertheless, if a combo port has one LED
> per physical medium, both need to be specified in the devicetree.

I am quite confused by this "port type" on rtl838x. Is this a capability 
of the LED controller? Is it applicable to rtl838x ?

> 
> Signed-off-by: Sander Vanheule <sander at svanheule.net>
Tested-by: Alexandru Gagniuc <mr.nuke.me at gmail.com>

I vote to get it merged, then update interface later.

> +/*
> + * Maple/RTL838x has two static groups:
> + *   - group 0: ports 0-23
> + *   - group 1: ports 24-27 (high combo ports)
> + *
> + * When both groups need the same setting, the generic implementation would
> + * always return the first group. However, high ports can only be controlled
> + * via the second group, so we need an override of the generic implementation.
> + */
> +static struct led_port_group *rtl838x_port_led_map_group(struct switch_port_led *led, u32 trigger)
> +{
> +	int rtl_trigger = rtl838x_port_trigger_xlate(led, trigger);
> +	struct switch_port_led_ctrl *ctrl = led->ctrl;
> +	struct led_port_group *group;
> +	u32 current_trigger;
> +	int err;
> +
> +	if (rtl_trigger < 0)
> +		return ERR_PTR(rtl_trigger);
> +
> +	if (led->port < RTL838X_PORT_COMBO_HIGH)
> +		group = switch_port_led_get_group(led, 0);
> +	else
> +		group = switch_port_led_get_group(led, 1);
> +
> +	err = regmap_field_read(group->setting, &current_trigger);
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	if (current_trigger != rtl_trigger && !bitmap_empty(group->ports, group->size)) {
> +		dev_warn(ctrl->dev, "cannot map (%d,%d) to group %d: 0x%02x != 0x%02x\n",
> +			 led->port, led->index, group->index, current_trigger, rtl_trigger);
> +		return ERR_PTR(-ENOSPC);
> +	}

It seems that the rtl_hw_trigger cannot be changed once any of the LEDs 
are switched to "realtek-switchport".

# echo realtek-switchport | tee /sys/class/leds/green:lan*_1000/trigger
# echo 13 | tee /sys/class/leds/green:lan*_1000/rtl_hw_trigger
tee: /sys/class/leds/green:lan1_1000/rtl_hw_trigger: I/O error


[  852.470000] realtek-switch-port-led realtek-switchcore-port-leds.0: 
cannot map (15,1) to group 0: 0x1f != 0x0a
[  852.480000] realtek-switch-port-led realtek-switchcore-port-leds.0: 
cannot map (14,1) to group 0: 0x1f != 0x0a
...


However, doing things in opposite order works (after reboot):
-------------------------------------------------------------

# echo 13 | tee /sys/class/leds/green:lan*_1000/rtl_hw_trigger
# echo realtek-switchport | tee /sys/class/leds/green:lan*_1000/trigger
# grep . /sys/kernel/debug/rtl838x/led/led*
/sys/kernel/debug/rtl838x/led/led1_sw_p_en_ctrl:0x0fff00ff
/sys/kernel/debug/rtl838x/led/led_mode_ctrl:0x00838147


But then there's yet another confusing point:
---------------------------------------------

# echo 13 > /sys/class/leds/green:lan1_1000/rtl_hw_trigger
# cat /sys/class/leds/green:lan1_1000/rtl_hw_trigger
13
# echo realtek-switchport | tee /sys/class/leds/green:lan*_1000/trigger
# grep . /sys/class/leds/green:lan*_1000/trigger
/sys/class/leds/green:lan1_1000/trigger:none timer heartbeat default-on 
netdev [realtek-switchport]
/sys/class/leds/green:lan2_1000/trigger:[none] timer heartbeat 
default-on netdev realtek-switchport
...

The other ports don't get switched to realtek-switchport, and there is 
no noticeable indication of an error.


> +static ssize_t rtl_hw_trigger_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *cdev = dev_get_drvdata(dev);
> +	struct switch_port_led *pled = to_switch_port_led(cdev);
> +
> +	return sprintf(buf, "%x\n", pled->trigger_flags);
> +}


# grep . /sys/class/leds/green:lan*_1000/*rigger
rtl_hw_trigger:13
trigger:none timer heartbeat default-on netdev [realtek-switchport]

The "trigger flags" number is meaningless for sysfs. I suggest this 
should show a text field with a selected choice, similar to how 
"trigger" behaves:

rtl_hw_trigger:none link_10-100 [act_10-100] link_1g act_1g ...