Re: [PATCH v5 2/6] leds: triggers: Add a keyboard backlight trigger

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Hi,

On 11/18/2016 07:47 PM, Hans de Goede wrote:
> 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.

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.

-- 
Best regards,
Jacek Anaszewski