Message-ID: <659b1106.050a0220.66c7.9f80@mx.google.com>
Date: Sun, 7 Jan 2024 21:51:32 +0100
From: Christian Marangi <ansuelsmth@...il.com>
To: Luiz Angelo Daros de Luca <luizluca@...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

On Sat, Jan 06, 2024 at 04:47:10PM -0300, Luiz Angelo Daros de Luca wrote:
> > 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:
> 

Saddly we still don't have the concept of LED groups, but from what I
notice 99% of the time switch have limitation of HW control but single
LED can still be controlled separately.

With this limitation you can use the is_supported function and some priv
struct to enforce and reject unsupported configuration.
netdev trigger will then fallback to software in this case. (I assume on
real world scenario to have all the LED in the group to be set to the
common rule set resulting in is_supported never rejecting it)

Also consider this situation, it's the first LED touched that enables HW
control that drive everything. LED configuration are not enabled all at
once. You can totally introduce a priv struct that cache the current
modes and on the other LEDs make sure the requested mode match the cache
one.

And I guess this limitation should be printed and documented in DT.

> /*
> ...
> * 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?
>

Consider that brightness_set is called whatever a trigger is changed,
the logic is in the generic LED handling. Setting OFF and then enabling
hw control should not change a thing. In other driver tho I notice an
extra measure is needed to reset any HW control rule already applied by
default.

-- 
	Ansuel

Powered by blists - more mailing lists