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.
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.
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.
--
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
