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 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 on the other sysfs file, but in either >> case we need to make some workaround, be it circular trigger or >> inconsistent polling. The advantage of the latter is that it explicitly >> advertises additional LED class device feature. >> >> The file and corresponding op should be compiled only when turned on in >> kernel config, and LED class devices which need that feature should >> select in the kernel config. > > I do not understand why you're changing your mind on this. > > The current_brightness approach with a trigger works well and is > generic, other triggers (which don't fire too often) can easily > also add current_brightness to allow userspace to monitor for changes, LED class drivers could also easily add a hw_brightness_change attr. > which is why I added shared current_brightness code to led-triggers.c, > so that this code can be re-used. I don't like the name current_brightness. It is ambiguous in the relation to existing brightness file. > Pavel Machek is completely right that this should be modeled as > a trigger and I think that doing some one-off special sysfs > file called hw_brightness_change for this case is bad design when > we've a better generic solution. Let's sum up pros and cons of both approaches: hw_brightness_change/async_brightness: Pros: - explicit declaration of an additional LED class device feature in the sysfs - ability to receive hw brightness change notifications even when having other trigger active Cons: - polling one file to detect changes on the other (not entirely true as the polled file will also report updated brightness) kbd-backlight trigger: Pros: - simplification of specific driver design Cons: - lack of information if given LED class device supports POLLPRI events - impossible to apply other trigger while polling - circular trigger event path -- Best regards, Jacek Anaszewski ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot _______________________________________________ ibm-acpi-devel mailing list ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
