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.
>
> Current LED subsystem triggers are generic - e.g. disk, mtd,
> backlight (it registers on video fb notifications).
Right and I tried to follow that model, the trigger as
implemented is generic. Any event which wishes to trigger
a keyboard backlight brightness change can call the added
ledtrig_kbd_backlight() function, just like any mtd
event can call the mtd trigger, etc.
If you think anything about this trigger is not generic,
please explain which bits are not generic according to
you.
Regards,
Hans
> Driver specific trigger should be implemented inside the driver.
>
> Last but not least - generic keyboard backlight trigger can't assume
> that all devices of this type can adjust backlight brightness.
>
> Best Regards,
> Jacek Anaszewski
>
>>> On 11/17/2016 11:24 PM, Hans de Goede wrote:
>>>> Add a trigger to control keyboard backlight LED devices. Note that in
>>>> some cases the keyboard backlight control is hardwired (taken care of
>>>> in firmware outside of the kernels control), in that case this triggers
>>>> main purpose is to allow userspace to monitor these changes.
>>>>
>>>> The ledtrig_kbd_backlight function has a set_brightness parameter to
>>>> differentiate between full backlight control through the trigger
>>>> (set_brightness set to true) or change notification only (false).
>>>>
>>>> Note the Kconfig option for this is a bool because the code is so
>>>> small that it is not worth the overhead of being a module.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>>>> ---
>>>> Changes in v5:
>>>> -This is a new patch in v5 of this patch-set (replacing earlier attempts
>>>> at similar functionality)
>>>> ---
>>>> drivers/leds/trigger/Kconfig | 10 ++++++++
>>>> drivers/leds/trigger/Makefile | 1 +
>>>> drivers/leds/trigger/ledtrig-kbd-backlight.c | 38
>>>> ++++++++++++++++++++++++++++
>>>> include/linux/leds.h | 8 ++++++
>>>> 4 files changed, 57 insertions(+)
>>>> create mode 100644 drivers/leds/trigger/ledtrig-kbd-backlight.c
>>>>
>>>> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
>>>> index 3f9ddb9..350e2c7 100644
>>>> --- a/drivers/leds/trigger/Kconfig
>>>> +++ b/drivers/leds/trigger/Kconfig
>>>> @@ -67,6 +67,16 @@ config LEDS_TRIGGER_BACKLIGHT
>>>>
>>>> If unsure, say N.
>>>>
>>>> +config LEDS_TRIGGER_KBD_BACKLIGHT
>>>> + bool "LED keyboard backlight Trigger"
>>>> + depends on LEDS_TRIGGERS
>>>> + help
>>>> + This trigger can control keyboard backlight LED devices,
>>>> + it also allows user-space to monitor keyboard backlight
>>>> brightness
>>>> + changes done through e.g. hotkeys on some laptops.
>>>> +
>>>> + If unsure, say Y.
>>>> +
>>>> config LEDS_TRIGGER_CPU
>>>> bool "LED CPU Trigger"
>>>> depends on LEDS_TRIGGERS
>>>> diff --git a/drivers/leds/trigger/Makefile
>>>> b/drivers/leds/trigger/Makefile
>>>> index a72c43c..be6b249 100644
>>>> --- a/drivers/leds/trigger/Makefile
>>>> +++ b/drivers/leds/trigger/Makefile
>>>> @@ -4,6 +4,7 @@ obj-$(CONFIG_LEDS_TRIGGER_DISK) += ledtrig-disk.o
>>>> obj-$(CONFIG_LEDS_TRIGGER_MTD) += ledtrig-mtd.o
>>>> obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT) += ledtrig-heartbeat.o
>>>> obj-$(CONFIG_LEDS_TRIGGER_BACKLIGHT) += ledtrig-backlight.o
>>>> +obj-$(CONFIG_LEDS_TRIGGER_KBD_BACKLIGHT) += ledtrig-kbd-backlight.o
>>>> obj-$(CONFIG_LEDS_TRIGGER_GPIO) += ledtrig-gpio.o
>>>> obj-$(CONFIG_LEDS_TRIGGER_CPU) += ledtrig-cpu.o
>>>> obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON) += ledtrig-default-on.o
>>>> diff --git a/drivers/leds/trigger/ledtrig-kbd-backlight.c
>>>> b/drivers/leds/trigger/ledtrig-kbd-backlight.c
>>>> new file mode 100644
>>>> index 0000000..353ee92
>>>> --- /dev/null
>>>> +++ b/drivers/leds/trigger/ledtrig-kbd-backlight.c
>>>> @@ -0,0 +1,38 @@
>>>> +/*
>>>> + * LED Trigger for keyboard backlight control
>>>> + *
>>>> + * Copyright 2016, Hans de Goede <hdegoede@xxxxxxxxxx>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + *
>>>> + */
>>>> +
>>>> +#include <linux/module.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/init.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/leds.h>
>>>> +#include "../leds.h"
>>>> +
>>>> +static struct led_trigger kbd_backlight_trigger = {
>>>> + .name = "kbd-backlight",
>>>> + .activate = led_trigger_add_current_brightness,
>>>> + .deactivate = led_trigger_remove_current_brightness,
>>>> +};
>>>> +
>>>> +void ledtrig_kbd_backlight(bool set_brightness, enum led_brightness
>>>> brightness)
>>>> +{
>>>> + if (set_brightness)
>>>> + led_trigger_event(&kbd_backlight_trigger, brightness);
>>>> +
>>>> +
>>>> led_trigger_notify_current_brightness_change(&kbd_backlight_trigger);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(ledtrig_kbd_backlight);
>>>> +
>>>> +static int __init kbd_backlight_trig_init(void)
>>>> +{
>>>> + return led_trigger_register(&kbd_backlight_trigger);
>>>> +}
>>>> +device_initcall(kbd_backlight_trig_init);
>>>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>>>> index d3eb992..870b8c2 100644
>>>> --- a/include/linux/leds.h
>>>> +++ b/include/linux/leds.h
>>>> @@ -353,6 +353,14 @@ static inline void ledtrig_flash_ctrl(bool on) {}
>>>> static inline void ledtrig_torch_ctrl(bool on) {}
>>>> #endif
>>>>
>>>> +#ifdef CONFIG_LEDS_TRIGGER_KBD_BACKLIGHT
>>>> +extern void ledtrig_kbd_backlight(bool set_brightness,
>>>> + enum led_brightness brightness);
>>>> +#else
>>>> +static inline void ledtrig_kbd_backlight(bool set_brightness,
>>>> + enum led_brightness brightness) {}
>>>> +#endif
>>>> +
>>>> /*
>>>> * Generic LED platform data for describing LED names and default
>>>> triggers.
>>>> */
>>>>
>>>
>>>
>>
>>
>>
>
------------------------------------------------------------------------------
_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
