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 <[email protected]>
Tested-by: Alexandru Gagniuc <[email protected]>

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 ...

_______________________________________________
openwrt-devel mailing list
[email protected]
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Reply via email to