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@xxxxxxxxx> 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@xxxxxxxxx> > > Reported-by: Gabriel M. Elder <gabriel@xxxxxxxxxxxxxx> > > 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@xxxxxxxxx
