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. > - circular trigger event path That is not true either. 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. Regards, Hans ------------------------------------------------------------------------------ _______________________________________________ ibm-acpi-devel mailing list ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
