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

[Alex G.]

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.


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

>>
>>> +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 ...
> 
> On RTL838x/RTL839x (i.e. the current patches), ca. 16 modes are documented.
> These can be represented as an enum [1], which would be text-reprentable.
> On RTL930x/RTL931x however, there are 13 bits that can be set independently [2].
> Not all these bits are orthogonal though, but this would still result in >8000
> combinations. I don't think anybody can process that kind of enumeration.
> 
> Maybe the flags could just be represented as text, but I'm not sure how common
> that is for a sysfs file (instead of the typical enumeration of mutually
> exclusive options).

I definitely wouldn't want 8000 options to represent a 13 bit bitfield. 
Looking through sysfs, bitmasks are represented as integers. I agree the 
enum idea only makes sense for rtl838x.

Alex

> [1] https://svanheule.net/realtek/maple/register/led_mode_ctrl
> [2] https://svanheule.net/realtek/longan/register/led_set0_0_ctrl
> 
> Best,
> Sander