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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 11/21/2016 12:41 PM, Pavel Machek wrote:
Hi!

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.

We can teach people. IMO the LED that changes itself is special, and
trigger explains that nicely to the userspace. Plus, it allows us to
keep this functionality out of the core.

Please refer to the downsides of this appraoch:

- lack of information if given LED class device supports hw
  generated POLLPRI events
- impossible to apply other trigger while polling
- circular trigger event path (if set_brightness parameter of
  ledtrig_kbd_backlight() is true)

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.

Well, exact name is not too important...

The name should clearly explain the file purpose. I bet that we would
see many questions once the file appeared in the mainline.
Also, I'm afraid that I wouldn't be able to explain this name in
few simple words, without daunting the listener, or even triggering
the discussion on brightness shortcomings we've already gone through.

--
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux