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

[Sander Vanheule]

Hi Alex,

On Sun, 2022-10-16 at 12:54 -0500, Alex G. wrote:
> 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 ?

By port type, do you mean the distinction that is made between RJ45 and SFP? The
LED controller on RTL838x does support combo ports. I suppose the active port is
provided by some other part of the switch core, but the LEDs need to be
configured separately.

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

Thank you for testing!

> 
> > +/*
> > + * 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

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.

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

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

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

Best,
Sander