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

[Tony Lindgren]

* Pavel Machek <pavel@xxxxxx> [160401 05:46]:
> Hi!
> 
> > > On Tue, Mar 29, 2016 at 12:51:28PM +0200, Pavel Machek wrote:
> > > > For 1-3 in the series, Acked-by: Pavel Machek <pavel@xxxxxx>
> > > > 
> > > > > Like the Nokia N900, the N950 has leds to show
> > > > > the state of sys_clkreq and sys_off_mode pins.
> > > > > 
> > > > > A detailed description for the LEDs and
> > > > > OMAP's sleep states can be found in Tony's
> > > > > commit for the Nokia N900:
> > > > > 
> > > > > c1be2032f66df9e1238bd5bc4ca666de88a62abc
> > > > 
> > > > 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.
> 
> No, not on N900. On N900, these LEDs are normally used for keyboard
> backlight.

Oh I see. I've totally forgotten that as I always keep the debug
option enabled :) The extra battery consumption by those is quite
minimal and warns you if you have something hogging the CPU.

> > 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.
> 
> No. It seems that on N900, you can select whether sys_clkreq and
> sys_off_mode is binary-or'ed with normal control of two keyboard
> backlight LEDs.

That might be doable by reconfiguring the I2C LED driver.

For the GPIO, the "default-on" just keeps the I2C LED driver
powered.

> > > 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.
> 
> ...and this is what this GPIO does. So it is not exactly a LED. You
> can turn it on, but than, _two_ LEDs will start blinking. You can't
> control them with the brightess control. "Heartbeat" trigger is going
> to be very confusing on debug::sleep.

And it occured to me that adding any other policy than
"default-on" to the GPIO LED will only work when the device
is active.

Sounds like the thing to do is to just configure the I2C LED
controller in the dts file if we don't already have that. And
assuming it has a Linux driver.

Regards,

Tony