Date: Sat, 6 Jan 2024 16:47:10 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: ansuelsmth@...il.com
Cc: linus.walleij@...aro.org, alsi@...g-olufsen.dk, andrew@...n.ch, 
	f.fainelli@...il.com, olteanv@...il.com, davem@...emloft.net, 
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	arinc.unal@...nc9.com, netdev@...r.kernel.org
Subject: Re: [RFC net-next 0/2] net: dsa: realtek: fix LED support for rtl8366rb

> The rtl8366rb switch family has 4 LED groups, with one LED from each
> group for each of its 6 ports. LEDs in this family can be controlled
> manually using a bitmap or triggered by hardware. It's important to note
> that hardware triggers are configured at the LED group level, meaning
> all LEDs in the same group share the same hardware triggers settings.
>
> The first part of this series involves dropping most of the existing
> code, as, except for disabling the LEDs, it was not working as expected.
> If not disabled, the LEDs will retain their default settings after a
> switch reset, which may be sufficient for many devices.
>
> The second part introduces the LED driver to control the switch LEDs
> from sysfs or device-tree. This driver still allows the LEDs to retain
> their default settings, but it will shift to the software-based OS LED
> triggers if any configuration is changed. Subsequently, the LEDs will
> operate as normal LEDs until the switch undergoes another reset.
>
> Netdev LED trigger supports offloading to hardware triggers.
> Unfortunately, this isn't possible with the current LED API for this
> switch family. When the hardware trigger is enabled, it applies to all
> LEDs in the LED group while the LED API decides to offload based on only
> the state of a single LED. To avoid inconsistency between LEDs,
> offloading would need to check if all LEDs in the group share the same
> compatible settings and atomically enable offload for all LEDs.

Hi Christian,

I tried to implement something close to your work with qca8k and LED
hw control. However, I couldn't find a solution that would work with
the existing API. The HW led configuration in realtek switches is
shared with all LEDs in a group. Before activating the hw control, all
LEDs in the same group must share the same netdev trigger config, use
the correct device and also use a compatible netdev trigger settings.
In order to check that, I would need to expose some internal netdev
trigger info that is only available through sysfs (and I believe sysfs
is not suitable to be used from the kernel). Even if I got all LEDs
with the correct settings, I would need to atomicly switch all LEDs to
use the hw control or, at least, I would need to stop all update jobs
because if the OS changes a LED brightness, it might be interpreted as
the OS disabling the hw control:

/*
...
* Deactivate hardware blink control by setting brightness to LED_OFF via
* the brightness_set() callback.
*
...
*/
int (*hw_control_set)(struct led_classdev *led_cdev,
 unsigned long flags);

Do you have any idea how to implement it?

BTW, during my tests with a single LED, ignoring the LED group
situation, I noticed that the OS was sending a brightness_set(LED_OFF)
after I changed the trigger to netdev, a moment after hw_control_set
was called. It doesn't make sense to enable hw control just to disable
it afterwards. The call came from set_brightness_delayed(). Maybe it
is because my test device is pretty slow and the previous trigger
event always gets queued. Touching any settings after that worked as
expected without the spurious brightness_set(LED_OFF). Did you see
something like this?

Regards,

Luiz

Powered by blists - more mailing lists