| Message ID | 1457827580-16919-5-git-send-email-sre@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 */
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
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
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
* Pavel Machek <pavel@ucw.cz> [160401 23:48]: > > 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). I think to make the LEDs ignore the hardware idle lines requires reconfiguring the I2C LED controller. > Anyway, current solution is not too horrible (its wrong but it does > not hurt that much), so... Yeah further changes can be done later. Applying the whole series to omap-for-v4.7/dt with your ack on patches 5 - 7. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/boot/dts/omap3-n950-n9.dtsi b/arch/arm/boot/dts/omap3-n950-n9.dtsi index 3c7f1d2deb2f..c42e8fc846b9 100644 --- a/arch/arm/boot/dts/omap3-n950-n9.dtsi +++ 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 */
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 Signed-off-by: Sebastian Reichel <sre@kernel.org> --- arch/arm/boot/dts/omap3-n950-n9.dtsi | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)