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

[Sander Vanheule]

Hi Alex,

On Tue, 2022-10-18 at 15:23 -0500, Alex G. wrote:
> On 10/16/22 16:58, Sander Vanheule wrote:
> > Hi Alex,
> Hi
> 
> [snip]
> 
> > > > +
> > > > +       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
> > 
> > This behaviour is intentional, but I am open to suggestions on improving it,
> > as
> > I agree it isn't very nice.
> > 
> > When using the "netdev" trigger, the sysfs configuration files are only
> > exposed
> > when the trigger is selected. This makes sense if each LED can be configured
> > independently of the others.
> > 
> > The hardware trigger on RTL838x requires an LED to be part of a group (or
> > "set"
> > in the SDK). As such, an LED itself cannot be configured to trigger on a
> > certain
> > hardware state, but the group it belongs to must be configured. That is why
> > I
> > opted to let the user set a HW trigger condition before activating the HW
> > trigger, since activation means assigning the LED to one of the limited
> > number
> > of groups.
> > 
> > One alternative would be to only expose the rtl_hw_trigger file once the
> > "realtek-switchport" trigger is selected, but that would mean I have to
> > either
> >   * ignore configurations that cannot currently be mapped to any active
> > group,
> >   * change the behaviour of other LEDs belonging to an already existing
> > group, if
> >     the HW trigger is overwritten.
> > 
> > The latter may work on RTL838x, where there is only one HW trigger setting
> > for
> > any given LED. On RTL839x this is not possible though, because there is no
> > way
> > to decide which of the 4 available groups should be used if there aren't any
> > available.
> 
> This is modeled in sysfs such that each LED appears to have its own 
> "rtl_hw_trigger". Instead, why not model one "rtl_hw_trigger" per LED 
> group? For example, symlink "rtl_hw_trigger" to a sysfs entry in the parent.
> 
> You can make the symlink appear or disappear when "realtek-switchport" 
> trigger is selected, or just leave it always on. When you change the 
> setting for one LED, it changes it for all the LEDs in the group. I 
> think it's more intuitive because it aligns more to how the LED 
> controller actually works.
> 
> Thus, "realtek-switchport" controls led{0,1,2}_sw_p_en_ctrl, and 
> "rtl_hw_trigger" only controls led_mode_ctrl. No need to intermingle 
> them with complicated logic.

On RTL838x the group mapping is static. On RTL839x (and newer) the group mapping
is runtime-configurable. I don't want to provide 4 symlinks + group selection
per LED, or dynamically update a symlink to the currently selected group; the
latter sounding pretty fragile.

The main issue is again that RTL838x is different from the other platforms, but
I don't want to ask developers/users to learn 2/3/4 subtly different LED
interfaces. A 1:1 interface between userspace and the device is probably the
easiest initially, but this interface is also a bit of an exercise to develop an
interface for offloaded netdev triggering.

> 
> 
> > > 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.
> > > 
> > 
> > Did you run this after rebooting? I'm not able to reproduce this on my
> > (mainline) image. When only one port has trigger flags "13", this results in
> > log
> > messages in my quick test.
> 
> Yes, that's right after reboot (rtl838x-based TL-SG2008P). I see the 
> dmesg messages now. Still, I would expect the 'tee' or 'echo' command to 
> show an error. It's not obvious that the answer lies in dmesg.

To help with setting up the LEDs, I've added a new DT property in the v3 of this
series. The trigger flags are now also documented in a dt-bindings header, which
should help avoid confusion between the trigger setting and the raw register
value.

Best,
Sander