Re: [ibm-acpi-devel] [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger
Hi! > >>Triggers are not limited to periodic blinking or reporting cpu > >>activity. There is also oneshot trigger that can be used e.g. when > >>user touches the screen, as Pali mentioned. > > > >Using oneshot trigger for this would be pretty strange. > > It was only an example to mention other than periodic triggers. > You could have a trigger that just turns the LED permanently on > after user touches the screen. Well.. triggers kind of assume they control the LED. They were not prepared to deal with hardware changing the brightness behind their back. > >Notice that in > >some cases (thinkpad battery led, for example) we either have firmware > >controls the LED (but then software can't control it) or we have > >software controlling the LED (but then we don't know what firmware > >would put there). Maybe keyboard backlight can be controlled > >"simultaneously" by both software and firmware, but there are > >certainly LEDs that can't handle that, and IMO it would be nice to > >have same interface. > > > >>>Well.. actually... I think this is a little bit over complex and > >>>probably unneccessary. I'd let Hans implement whatever he thinks is > >>>easiest. > >> > >>I'd say this is the trigger approach which is a bit convoluted. > > > >In my eyes trigger approach is neccessary at least for some hardware, > >and things it pretty clear: trigger on == LED changes without > >userspace involvement. trigger off == userspace controls the LED. > > It is likely that it would break many existing users. Can you elaborate on that? I just tried with leds on thinkpad root@duo:/sys/class/leds/tpacpi::power# echo 1 > brightness root@duo:/sys/class/leds/tpacpi::power# echo 0 > brightness root@duo:/sys/class/leds/tpacpi::power# cat trigger [none] bluetooth-power kbd-scrollock kbd-numlock kbd-capslock kbd-kanalock kbd-shiftlock kbd-altgrlock kbd-ctrllock kbd-altlock kbd-shiftllock kbd-shiftrlock kbd-ctrlllock kbd-ctrlrlock AC-online BAT0-charging-or-full BAT0-charging BAT0-full BAT0-charging-blink-full-solid rfkill0 phy0rx phy0tx phy0assoc phy0radio phy0tpt mmc0 timer pattern rfkill1 hci0-power rfkill74 heartbeat root@duo:/sys/class/leds/tpacpi::power# I can control the LED from userspace, but then there's no way to put the LED back to firmware control. That's just broken. Do you have a proposal how to handle that? > >So I'd do the trigger here. It is same way we can handle LEDs on > >thinkpad. Yes, it means you won't be able to do oneshot trigger on > >backlight. I don't think that's a huge problem. > > There have been voices in this discussion claiming the opposite. :-) Well, lets ignore those voices until the voices understand the current design :-). Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature -- ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH] thinkpad_acpi: Fix old style declaration GCC warning
On Fri, 25 Nov 2016, Tobias Klauser wrote:
> Fix an [-Wold-style-declaration] GCC warning by moving the inline
> keyword before the return type.
>
> Signed-off-by: Tobias Klauser
Acked-by: Henrique de Moraes Holschuh
> ---
> drivers/platform/x86/thinkpad_acpi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c
> b/drivers/platform/x86/thinkpad_acpi.c
> index b65ce7519411..c24ce1adc577 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -7716,7 +7716,7 @@ static struct ibm_struct volume_driver_data = {
>
> #define alsa_card NULL
>
> -static void inline volume_alsa_notify_change(void)
> +static inline void volume_alsa_notify_change(void)
> {
> }
>
--
Henrique Holschuh
--
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger
Hi! > >Lets keep it simple. Yes, monitoring backlight state while hardware > >updates it is useful. But doing the monitor when some kind of blinking > >from the kernel is active is just a unneccessary complexity... > > Triggers are not limited to periodic blinking or reporting cpu > activity. There is also oneshot trigger that can be used e.g. when > user touches the screen, as Pali mentioned. Using oneshot trigger for this would be pretty strange. Notice that in some cases (thinkpad battery led, for example) we either have firmware controls the LED (but then software can't control it) or we have software controlling the LED (but then we don't know what firmware would put there). Maybe keyboard backlight can be controlled "simultaneously" by both software and firmware, but there are certainly LEDs that can't handle that, and IMO it would be nice to have same interface. > >Well.. actually... I think this is a little bit over complex and > >probably unneccessary. I'd let Hans implement whatever he thinks is > >easiest. > > I'd say this is the trigger approach which is a bit convoluted. In my eyes trigger approach is neccessary at least for some hardware, and things it pretty clear: trigger on == LED changes without userspace involvement. trigger off == userspace controls the LED. So I'd do the trigger here. It is same way we can handle LEDs on thinkpad. Yes, it means you won't be able to do oneshot trigger on backlight. I don't think that's a huge problem. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature -- ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
[ibm-acpi-devel] [PATCH] thinkpad_acpi: Fix old style declaration GCC warning
Fix an [-Wold-style-declaration] GCC warning by moving the inline
keyword before the return type.
Signed-off-by: Tobias Klauser
---
drivers/platform/x86/thinkpad_acpi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c
b/drivers/platform/x86/thinkpad_acpi.c
index b65ce7519411..c24ce1adc577 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -7716,7 +7716,7 @@ static struct ibm_struct volume_driver_data = {
#define alsa_card NULL
-static void inline volume_alsa_notify_change(void)
+static inline void volume_alsa_notify_change(void)
{
}
--
2.11.0.rc0.7.gbe5a750
--
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger
Hi! > > As for the modeling how the hotkey controls the LED as a trigger, > > although I do like this from one pov, I can see Jacek's point that > > this is confusing as there really is nothing to configure here, > > where as normally a user could do "echo none > trigger" to break > > the link. So I think that is best (cleanest /minimal non confusing > > API) with just the hw_brightness_change sysfs-attribute and not > > model this as a trigger. > > I can accept with this solution (no trigger, event on new sysfs file > which returns current/actual brightness state, new sysfs file only for > devices which can report brightness state). > > But I'm not sure if it is really fixing that original problem with high > power usage... Yes, it is fixing that problem. -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature -- ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger
On Friday 25 November 2016 12:14:56 Hans de Goede wrote: > Hi, > > On 25-11-16 11:01, Pavel Machek wrote: > > Hi! > > > >> In view of the above we could report hw brightness changes with > >> POLLPRI on brightness file, but unfortunately we can't because it > >> is impossible to guarantee that readout of brightness file will > >> return the brightness the POLLPRI was meant to notify about. > > > > Agreed here. > > > >> That's why a separate read only file seems to be the only proper > >> solution. > > > > Yes please. And lets make self-changing leds into a trigger, as > > proposed, and as Hans' patch should be already doing. > > > >> Moreover, the file should return the brightness from the time > >> of last POLLPRI. > > > > Not sure I agree here. Normally, kernel returns current state for > > variables, does not track "old" state. > > Agreed, storing the last state just unnecessarily complicates things. > > So do we have a consensus on implementing a new hw_brightness_change > sysfs attribute now, which only some LEDs will have, can be polled > to detect changed done autonomously by the hardware and returns > the current / actual LED brightness when read ? > > As for the modeling how the hotkey controls the LED as a trigger, > although I do like this from one pov, I can see Jacek's point that > this is confusing as there really is nothing to configure here, > where as normally a user could do "echo none > trigger" to break > the link. So I think that is best (cleanest /minimal non confusing > API) with just the hw_brightness_change sysfs-attribute and not > model this as a trigger. I can accept with this solution (no trigger, event on new sysfs file which returns current/actual brightness state, new sysfs file only for devices which can report brightness state). But I'm not sure if it is really fixing that original problem with high power usage... As wrote in some previous emails, consider "actual_brightness" sysfs name which is already used for this purpose by backlight subsystem -- because for consistency. backlight devices have: actual_brightness, brightness, max_brightness. > That, or fall back to my latest patch-set as posted, I still like > that one the most. > > Regards, > > Hans -- Pali Rohár pali.ro...@gmail.com signature.asc Description: This is a digitally signed message part. -- ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger
Hi, On 25-11-16 11:01, Pavel Machek wrote: > Hi! > >> In view of the above we could report hw brightness changes with POLLPRI >> on brightness file, but unfortunately we can't because it is impossible >> to guarantee that readout of brightness file will return the brightness >> the POLLPRI was meant to notify about. > > Agreed here. > >> That's why a separate read only file seems to be the only proper >> solution. > > Yes please. And lets make self-changing leds into a trigger, as > proposed, and as Hans' patch should be already doing. > >> Moreover, the file should return the brightness from the time >> of last POLLPRI. > > Not sure I agree here. Normally, kernel returns current state for > variables, does not track "old" state. Agreed, storing the last state just unnecessarily complicates things. So do we have a consensus on implementing a new hw_brightness_change sysfs attribute now, which only some LEDs will have, can be polled to detect changed done autonomously by the hardware and returns the current / actual LED brightness when read ? As for the modeling how the hotkey controls the LED as a trigger, although I do like this from one pov, I can see Jacek's point that this is confusing as there really is nothing to configure here, where as normally a user could do "echo none > trigger" to break the link. So I think that is best (cleanest /minimal non confusing API) with just the hw_brightness_change sysfs-attribute and not model this as a trigger. That, or fall back to my latest patch-set as posted, I still like that one the most. Regards, Hans -- ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger
Hi! > >>In view of the above we could report hw brightness changes with POLLPRI > >>on brightness file, but unfortunately we can't because it is impossible > >>to guarantee that readout of brightness file will return the brightness > >>the POLLPRI was meant to notify about. > > > >Agreed here. > > > >>That's why a separate read only file seems to be the only proper > >>solution. > > > >Yes please. And lets make self-changing leds into a trigger, as > >proposed, and as Hans' patch should be already doing. > > We can set one trigger at a time. In this case it will be impossible > to have hw brightness change notifications while other trigger is > active. And that is a _good_ thing. We don't want to deal with "echo heartbeat > kbd_backlight_trigger" and then asking for hardware brightness changes. Lets keep it simple. Yes, monitoring backlight state while hardware updates it is useful. But doing the monitor when some kind of blinking from the kernel is active is just a unneccessary complexity... > >>Moreover, the file should return the brightness from the time > >>of last POLLPRI. > > > >Not sure I agree here. Normally, kernel returns current state for > >variables, does not track "old" state. > > It would report current state. The file called hw_brightness_change > should report last brightness change originating from hardware. > The most recent hw brightness change is still current state from > this perspective. It will be valid until next hw brightness change > occurs. > > Current state does not mean current brightness in this case. Well.. actually... I think this is a little bit over complex and probably unneccessary. I'd let Hans implement whatever he thinks is easiest. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature -- ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger
On Mon 2016-11-21 10:31:33, Hans de Goede wrote: > Hi, > > On 21-11-16 09:35, Jacek Anaszewski wrote: > >On 11/20/2016 04:05 PM, Pali Rohár wrote: > >>On Saturday 19 November 2016 16:44:09 Jacek Anaszewski wrote: > >>>Hi, > >>> > >>>On 11/18/2016 07:47 PM, Hans de Goede wrote: > HI, > > On 18-11-16 17:03, Jacek Anaszewski wrote: > >Hi, > > > >On 11/18/2016 10:07 AM, Hans de Goede wrote: > >>Hi, > >> > >>On 18-11-16 09:55, Jacek Anaszewski wrote: > >>>Hi Hans, > >>> > >>>Thanks for the patch. > >>> > >>>I think we need less generic trigger name. > >>>With present name we pretend that all kbd-backlight controllers > >>>can change LED brightness autonomously. > >>> > >>>How about kbd-backlight-pollable ? > >> > >>This is a trigger to control kbd-backlights, in the > >>current use-case the brightness change is already done > >>by the firmware, hence the set_brightness argument to > >>ledtrig_kbd_backlight(), so that we can avoid setting > >>it again. > >> > >>But I can see future cases where we do want to have some > >>event (e.g. a wmi hotkey event on a laptop where the firmware > >>does not do the adjustment automatically) which does > >>lead to actually updating the brightness. > >> > >>So I decided to go with a generic kbd-backlight trigger, > >>which in the future can also be used to directly control > >>kbd-backlight brightness; and not just to make ot > >>poll-able. > > > >I thought that kbd-backlight stands for "keyboard backlight", > > It does. > > >that's why I assessed it is too generic. > > The whole purpose of the trigger as implemented is to be > generic, as it seems senseless to implement a one off > trigger for just the dell / thinkpad case. > > >It seems however to be > >the other way round - if kbd-backlight means that this is > >a trigger only for use with dell-laptop keyboard driver > >(I see kbd namespacing prefix in the driver functions) than it is > >not generic at all. > > The trigger as implemented is generic, if you think > otherwise, please let me know which part is not generic > according to you. > >>> > >>>I think I was too meticulous here. In the end of the previous > >>>message I mentioned that we cannot guarantee that all keyboard > >>>backlight controllers can adjust brightness autonomously. > >>>Nonetheless, for the ones that cannot do that it would make no sense > >>>to have a trigger. In view of that this trigger is generic enough. > >>> > >>>I'll wait for Pavel's opinion before merging the patch set > >>>as he was also involved in this whole thread. > >> > >>Do you mean me? Or Pavel Machek (CCed) who was involved in LEDs for input > >>devices? > > > >I meant Pavel Machek, I haven't known that Pali is an equivalent of > >Pavel :-) > >Your opinion is very much appreciated though, thanks. > > > >>As pointed in other email, we do not know if HW really controls keyboard > >>backlight, > >>so adding "fake" trigger on machines without HW control is not a good idea. > >> > > > >Yes, I had also similar doubts, but got almost convinced due to > >no objections. Now it becomes clear that we need to improve this > >feature. > > But we are not adding such a fake trigger. We are only setting up the > kbd-backlight trigger on systems where there actually is hw-control. > > Sure someone can do echo "kbd-backlight" > trigger to enable it, > but the same is true for e.g. "mtd" or "nand-disk" on systems > without an mtd-device / a nand-disk, and the result is the same, > the LED gets coupled to the trigger, but nothing ever triggers > the trigger. We can do that, yes. Alternatively, we could do some magic so that kbd-backlight trigger is not available for other leds, and that _only_ kbd-backlight trigger is available for leds that are always controlled by the hardware. But we can do that in future patch... > I really believe that we have the right design now (I was skeptical > about the trigger approach at first, but it has turned out really > well) and unless Pavel Machek has any objections I would really like > patches 1-2 of this series merged. I certainly like the interface from thedescription. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature -- ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger
Hi! > In view of the above we could report hw brightness changes with POLLPRI > on brightness file, but unfortunately we can't because it is impossible > to guarantee that readout of brightness file will return the brightness > the POLLPRI was meant to notify about. Agreed here. > That's why a separate read only file seems to be the only proper > solution. Yes please. And lets make self-changing leds into a trigger, as proposed, and as Hans' patch should be already doing. > Moreover, the file should return the brightness from the time > of last POLLPRI. Not sure I agree here. Normally, kernel returns current state for variables, does not track "old" state. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature -- ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger
On Thu 2016-11-24 16:36:51, Pali Rohár wrote: > On Thursday 24 November 2016 16:32:06 Jacek Anaszewski wrote: > > Since it has been reported that POLLPRI notifications on brightness > > file can lead to increased power consumption, and having my above > > statement I don't think that it is a good idea to use brightness > > file for this. > > How is brightness file different from others that it cannot issue > POLLPRI notification? Reading "brightness" may or may not give you current brightness of the led, depending on hardware limitations -- and you can't know. (It may give you maximum brightness of trigger) Reading "hw_current_brightness" file (if present) will give you current brightness. In addition, it is pollable. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature -- ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger
On Thu 2016-11-24 10:15:25, Pali Rohár wrote:
> On Wednesday 23 November 2016 12:01:02 Jacek Anaszewski wrote:
> > I would also appreciate your opinion on the other solution to the
> > problem of notifying brightness changes originating from hardware,
> > i.e. hw_brightness_change{_ro} file, that would support POLLPRI events,
> > and reading brightness.
>
> Another idea:
>
> If no trigger is active then led subsystem will invoke POLLPRI on
> "brightness" sysfs file.
>
> And if there is active trigger then only trigger code could invoke
> POLLPRI on "brightness" file.
That would mess up interface even more. Sorry.
> Do not know if this is really enough for your situation, it is just and
> another idea.
We don't need another ideas at the moment, unless they are better than
proposed solution.
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
--
___
ibm-acpi-devel mailing list
ibm-acpi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
Re: [ibm-acpi-devel] [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger
Hi! > As pointed in other email, we do not know if HW really controls keyboard > backlight, > so adding "fake" trigger on machines without HW control is not a good > idea. > >>> > >>>Well, if we know that hardware will not change the brightness on its > >>>own, yes, I'd avoid the trigger. If we don't know (as is common on > >>>ACPI machines, I'd keep the trigger). > >> > >>I'd drop the trigger approach due to the mess it can make in peoples' > >>minds due to the fact that LED class device handles trigger events > >>generated by itself. > > > >We can teach people. IMO the LED that changes itself is special, and > >trigger explains that nicely to the userspace. Plus, it allows us to > >keep this functionality out of the core. > > Please refer to the downsides of this appraoch: > > - lack of information if given LED class device supports hw > generated POLLPRI events Userspace can plainly see that a trigger is active, and knows to expect > - impossible to apply other trigger while polling That's a good thing. We _don't_ want polling to be active when trigger such as "CPU active" is active. We want userspace to monitor hardware events but not software ones. > >>I'd add a file hw_brightness_change or async_brightness or something > >>similar and make it only readable/pollable. current_brightness is > >>ambiguous and questionable. > > > >Well, exact name is not too important... > > The name should clearly explain the file purpose. I bet that we would > see many questions once the file appeared in the mainline. > Also, I'm afraid that I wouldn't be able to explain this name in > few simple words, without daunting the listener, or even triggering > the discussion on brightness shortcomings we've already gone through. Well, feel free to suggest non-confusing name. Unfortunately, yes, we'll need to do some explaining, as existing "brightness" behaviour is already pretty tricky / confusing / counterintuitive. Lets at least make sure new additions are clean and simple. Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature -- ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
