Re: [PATCH 4/7] ARM: dts: Enable N950 keyboard sleep leds by default

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2197 bytes --]

Hi,

On Wed, Mar 30, 2016 at 12:35:21PM -0700, Tony Lindgren wrote:
> > > I must say I've seen it on N900, and yes, it is useful, but no, I
> > > don't think this is right.
> > >
> > > This is not a LED. This is a interface that changes meaning of two
> > > other LEDs. I guess it should go to debugfs somewhere.
> 
> Eh that sounds like a GPIO LED to me :) And it already has a
> /sys/class/leds/debug interface.
> 
> The two LEDs this GPIO controls are hardwired to sys_clkreq and
> sys_off_mode pins that are the control signals between the SoC
> and PMIC.
> 
> In theory you could steal the sys_clkreq and sys_off_mode pins
> from the PMIC at the cost of breaking PM. But I doubt anybody
> wants to do that considering it's a battery operated device.
> 
> > I don't think we should diverge N900 and N950 userspace
> > APIs in this regard.
> > 
> > Actually the correct way would be a custom trigger
> > for the leds IMHO. I don't know if the led framework
> > supports per led custom triggers, though.
> 
> Sure, there are something like 10 triggers already. And
> you can already change them using:
> 
> echo none > /sys/class/leds/debug::sleep/trigger
> 
> That still does not change the fact that the LEDs trigger
> based on the state of sys_clkreq and sys_off_mode.
> 
> Maybe I'm not following what you guys are trying to achieve
> here though :) If so, please let me know.

If I understand the situation correctly there are two different
control methods for the same LED. I was wondering if the trigger
from lp5523:kb0 could enable the gpio. For example like this:

echo cpu-sleep-state > /sys/class/leds/lp5523:kb0/trigger

Thinking about it again supporting this requires quite some effort
with N9xx being the only affected platform. Apart from that (as
Pavel mentioned) two LEDs are affected by the GPIO, so even the
trigger is not a clean option.

So I'm fine with the current solution. IMHO moving it to debugfs
is not recommendable while keeping it default enabled (which makes
sense IMHO). At the end the correct solution is fixing the power
management. If the cpu sleeps the impact of this should be marginal.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]