Messages in this thread | | | Date | Wed, 18 Oct 2017 21:14:39 +0200 | From | Pali Rohár <> | Subject | Re: [PATCH v2] dell-laptop: Fix keyboard led max_brightness property for Dell Latitude E6410 |
| |
On Wednesday 18 October 2017 22:09:41 Andy Shevchenko wrote: > On Wed, Oct 18, 2017 at 9:06 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > > This machine reports number of keyboard backlight led levels, instead of > > value of the last led level index. Therefore max_brightness properly needs > > to be subtracted by 1 to match led max_brightness API. > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > > Reported-by: Gabriel M. Elder <gabriel@tekgnowsys.com> > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=196913 > > > + u8 kbd_led_num_of_levels_instead_of_last_index; > > Sorry for last minute review comments. > > First of all, can it be just boolean?
Yes, touchpad_led and needs_kbd_timeouts are booleans too.
> Naming... Yes, too hard as always was, is and will be. > > What about just > > kbd_led_use_levels?
If you code:
if (quirks->kbd_led_use_levels) info->levels--;
Then I think it does not help reader what that quirk represent.
> Also, does it make sense to create a quirks as an unsigned long and > put there corresponding bits with definitions?
Problem is that part of quirk structure is variable length array kbd_timeouts. So one unsigned long does not help.
> /* Comment what is this for */ > #define QUIRK_KBD_LED_USE_LEVELS 0 > > unsigned long quirks; > > ? > > > + if (quirks && quirks->kbd_led_num_of_levels_instead_of_last_index && info->levels) > > + info->levels--; > > With last suggestion if becomes something like > > if (quirks & BIT(QUIRK_KBD_LED_USE_LEVELS) && info->levels) > >
-- Pali Rohár pali.rohar@gmail.com
| |