Hi, On 11/21/2016 12:56 PM, Hans de Goede wrote: > 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 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 > > That is not true, current_brightness presence implies POLL_PRI support. Right, it should be: - lack of information if given LED class device can generate hardware triggered POLLPRI events > >> - circular trigger event path > > That is not true either. Not true only if set_brightness argument is false. > Anyways I'm fine with using a new (optional) hw_brightness_change sysfs > file which is poll-able, but, BUT can you please make a decision and then > stick with it ? All this lets do it this way, I spend time writing and > testing a patch and then you changing your mind is becoming very tiresome. Kernel development relies on lazy consensus, so it is always possible that someone will add something to the discussion that changes the way how the newly proposed feature is being assessed. Until it is merged to mainline it is always possible that someone will point out some crucial problem. Once an API is added to kernel it is carved in stone so we have to be very careful, as the case of existing brightness file shows. Let's wait until every involved part agrees (Pavel, Pali). I'm open to hear strong arguments against my approach. -- 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
