Re: [ibm-acpi-devel] [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger
Hi, On 21-11-16 12:24, Jacek Anaszewski wrote: > On 11/21/2016 11:42 AM, Hans de Goede wrote: >> Hi, >> >> On 21-11-16 11:24, Jacek Anaszewski wrote: >>> Hi, >>> >>> On 11/20/2016 05:21 PM, Pavel Machek wrote: Hi! >> 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. If we have a keyboard backlight that may be changed automatically, I'd go for trigger. If we know for sure that hardware will not change brightnes "on its own", I'd not put a trigger there (and polling makes no sense). If we don't know... I guess I'd go for trigger. We can do various white/blacklists if we really want to.. > 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. >> >> That is actually not true. I believe that Pavel Machek was entirely >> right that we should model this as a trigger. Take e.g. Dell laptops, >> there are 2 different drivers there on for the Dell smbios ACPI >> interface, which is the one registering the led_classdev to query / >> control the kbd backlight and then there is a completely separate >> driver for receiving WMI events, the dell-wmi driver (both can be >> build and insmod-ed completely separate from one another), which >> actually gets events that the brightness was changed by the hotkey. >> >> Before the trigger approach we had a custom dell_laptop notifier >> chain in a dell-common module to propagate events from one to >> the other, with the trigger approach this hack is all gone. > > Well, in this particular case it turned out to be beneficial. > >>> I have an impression that we're trying to abuse trigger mechanism, >>> e.g. the need for set_brightness parameter to ledtrig_kbd_backlight(), >>> which actually prevents setting brightness, and its only task is >>> to generate brightness change notification. >> >> In the dell / thinkpad case yes, but maybe we will get another >> laptop where we do actually need to actually update the brightness >> ourselves on some event, in that case we will also want to notify >> userspace. >> >>> 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. >>> >>> This is quite specific hardware
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. > 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... 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, On 21-11-16 11:24, Jacek Anaszewski wrote: > Hi, > > On 11/20/2016 05:21 PM, Pavel Machek wrote: >> Hi! >> 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. >> >> If we have a keyboard backlight that may be changed automatically, I'd >> go for trigger. If we know for sure that hardware will not change >> brightnes "on its own", I'd not put a trigger there (and polling makes >> no sense). If we don't know... I guess I'd go for trigger. >> >> We can do various white/blacklists if we really want to.. >> >>> 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. That is actually not true. I believe that Pavel Machek was entirely right that we should model this as a trigger. Take e.g. Dell laptops, there are 2 different drivers there on for the Dell smbios ACPI interface, which is the one registering the led_classdev to query / control the kbd backlight and then there is a completely separate driver for receiving WMI events, the dell-wmi driver (both can be build and insmod-ed completely separate from one another), which actually gets events that the brightness was changed by the hotkey. Before the trigger approach we had a custom dell_laptop notifier chain in a dell-common module to propagate events from one to the other, with the trigger approach this hack is all gone. > I have an impression that we're trying to abuse trigger mechanism, > e.g. the need for set_brightness parameter to ledtrig_kbd_backlight(), > which actually prevents setting brightness, and its only task is > to generate brightness change notification. In the dell / thinkpad case yes, but maybe we will get another laptop where we do actually need to actually update the brightness ourselves on some event, in that case we will also want to notify userspace. > 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. > > This is quite specific hardware feature so it needs specific handling > and a separate sysfs file. We could add polling on brightness and > apply some event filtering as proposed by Pali, by it could result > in losing crucial brightness changes in some use cases. > > Therefore a separate file for this specific feature is needed. > There still remains objection raised by Hans related to polling > a sysfs file to detect changes
Re: [ibm-acpi-devel] [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger
Hi, On 21-11-16 11:12, Pali Rohár wrote: > On Monday 21 November 2016 10:31:33 Hans de Goede wrote: >> Pali, I'm sorry that you don't like the LED side design, but there >> has been a long discussion about this (which you apparently missed) >> and this really is the best way forward. > > Yea, I thought that I should have missed something as I was not able to > find all needed information about it in my mailbox. > > Is that discussion somewhere logged/available? That could help to > describe and understand all problems hidden behind. > >> Have you looked at what the new design means for the platform/x86 >> patches ? Gone is the ugly dell_laptop_notifier as the event >> forwarding between dell-wmi and dell-laptop is now handles by >> the led-trigger subsys. > > Yes, I saw that ugly dell_lpatop notifier is not there. If there is some > more information about decision and pro/coins about this approach I > would like to read it before. It is discussed in this thread: https://www.spinics.net/lists/linux-leds/msg07049.html Unfortunately the starter of the thread dropped the Cc: platform-driver-...@vger.kernel.org the original patch that thread is about had. 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
On Monday 21 November 2016 10:31:33 Hans de Goede wrote: > Pali, I'm sorry that you don't like the LED side design, but there > has been a long discussion about this (which you apparently missed) > and this really is the best way forward. Yea, I thought that I should have missed something as I was not able to find all needed information about it in my mailbox. Is that discussion somewhere logged/available? That could help to describe and understand all problems hidden behind. > Have you looked at what the new design means for the platform/x86 > patches ? Gone is the ugly dell_laptop_notifier as the event > forwarding between dell-wmi and dell-laptop is now handles by > the led-trigger subsys. Yes, I saw that ugly dell_lpatop notifier is not there. If there is some more information about decision and pro/coins about this approach I would like to read it before. -- Pali Rohár pali.ro...@gmail.com -- ___ 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 1/6] leds: triggers: Add current_brightness trigger parameter
On Monday 21 November 2016 00:48:42 Pavel Machek wrote: > On Mon 2016-11-21 00:07:35, Pali Rohár wrote: > > On Thursday 17 November 2016 23:24:36 Hans de Goede wrote: > > > In some cases it may be desirable for userspace to be notified when > > > a trigger event happens. This commit adds support for a poll-able > > > current_brightness trigger specific sysfs attribute which triggers > > > may register: > > > > > > What: /sys/class/leds//current_brightness > > > > What about name actual_brightness? That name is already used by > > /sys/class/backlight/... > > Is that a problem? Is not better to have same name? -- Pali Rohár pali.ro...@gmail.com -- ___ 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 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. 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. Regards, Hans p.s. Pali, I'm sorry that you don't like the LED side design, but there has been a long discussion about this (which you apparently missed) and this really is the best way forward. Have you looked at what the new design means for the platform/x86 patches ? Gone is the ugly dell_laptop_notifier as the event forwarding between dell-wmi and dell-laptop is now handles by the led-trigger subsys. -- ___ ibm-acpi-devel mailing list ibm-acpi-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
