Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1366157 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 4/7] ARM: dts: Enable N950 keyboard sleep leds by default Pavel Machek <pavel@ucw.cz> - 2016-03-29 13:00 +0200
    Re: [PATCH 4/7] ARM: dts: Enable N950 keyboard sleep leds by default Sebastian Reichel <sre@kernel.org> - 2016-03-29 17:00 +0200
      Re: [PATCH 4/7] ARM: dts: Enable N950 keyboard sleep leds by default Pavel Machek <pavel@ucw.cz> - 2016-03-29 20:00 +0200
      Re: [PATCH 4/7] ARM: dts: Enable N950 keyboard sleep leds by default Tony Lindgren <tony@atomide.com> - 2016-03-30 21:40 +0200
        Re: [PATCH 4/7] ARM: dts: Enable N950 keyboard sleep leds by default Sebastian Reichel <sre@kernel.org> - 2016-03-31 02:50 +0200
        Re: [PATCH 4/7] ARM: dts: Enable N950 keyboard sleep leds by default Pavel Machek <pavel@ucw.cz> - 2016-04-01 14:50 +0200
          Re: [PATCH 4/7] ARM: dts: Enable N950 keyboard sleep leds by default Tony Lindgren <tony@atomide.com> - 2016-04-01 20:40 +0200
            Re: [PATCH 4/7] ARM: dts: Enable N950 keyboard sleep leds by default Pavel Machek <pavel@ucw.cz> - 2016-04-02 08:50 +0200

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

Hi!

For 1-3 in the series, Acked-by: Pavel Machek <pavel@ucw.cz>

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

Best regards,
								Pavel
								
> Signed-off-by: Sebastian Reichel <sre@kernel.org>

> +++ b/arch/arm/boot/dts/omap3-n950-n9.dtsi
> @@ -31,9 +31,27 @@
>  		startup-delay-us = <150>;
>  		enable-active-high;
>  	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +
> +		heartbeat {
> +			label = "debug::sleep";
> +			gpios = <&gpio3 28 GPIO_ACTIVE_HIGH>;  /* gpio92 */
> +			linux,default-trigger = "default-on";
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&debug_leds>;
> +		};
> +	};
>  };
>  
>  &omap3_pmx_core {
> +	debug_leds: pinmux_debug_led_pins {
> +		pinctrl-single,pins = <
> +			OMAP3_CORE1_IOPAD(0x2108, PIN_OUTPUT | MUX_MODE4)       /* dss_data22.gpio_92 */
> +		>;
> +	};
> +
>  	mmc2_pins: pinmux_mmc2_pins {
>  		pinctrl-single,pins = <
>  			OMAP3_CORE1_IOPAD(0x2158, PIN_INPUT_PULLUP | MUX_MODE0) /* sdmmc2_clk */

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [next] | [standalone]

#1366355

[Multipart message — attachments visible in raw view] — view raw

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@ucw.cz>
> 
> > 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.

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.

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1366520

On Tue 2016-03-29 16:52:09, Sebastian Reichel wrote:
> 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@ucw.cz>
> > 
> > > 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.
> 
> I don't think we should diverge N900 and N950 userspace
> APIs in this regard.

That's a good argument.

But maybe we should move both to debugfs somewhere before we get too
much userspace that relies on it...

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

Well, not even custom trigger would make this easy: AFAIK you need to
enable this on two LEDs or not at all, so you'd have one trigger _that
would need to be shared over two LEDs_.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1367536

* Sebastian Reichel <sre@kernel.org> [160329 07:53]:
> 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@ucw.cz>
> > 
> > > 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.

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.

Regards,

Tony

[toc] | [prev] | [next] | [standalone]

#1367722

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1369263

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@ucw.cz>
> > > 
> > > > 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.

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

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

Best regards,
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1369514

* Pavel Machek <pavel@ucw.cz> [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@ucw.cz>
> > > > 
> > > > > 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

[toc] | [prev] | [next] | [standalone]

#1369883

Hi!

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

Yes, I like it.

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

I don't see what you mean here. If you want to always keep the debug
leds on... that may be a bit confusing for the users (and Pali wants
to keep kernel usable for mere mortals it seems).

Anyway, current solution is not too horrible (its wrong but it does
not hurt that much), so...

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web