Re: [PATCH] dell-laptop: Adds support for keyboard backlight timeout AC settings

[Andy Shevchenko]

On Sun, Apr 23, 2017 at 10:40 PM, Pali RohÃr <pali.rohar@xxxxxxxxx> wrote:
> When changing keyboard backlight state on new Dell laptops, firmware
> expects a new timeout AC value filled in Set New State SMBIOS call.
>
> Without it any change of keyboard backlight state on new Dell laptops
> fails. And user can see following error message in dmesg:
>
>   dell_laptop: Setting old previous keyboard state failed
>   leds dell::kbd_backlight: Setting an LED's brightness failed (-6)
>
> This patch adds support for retrieving current timeout AC values and also
> updating them. Current timeout value in sysfs is displayed based on current
> AC status, like current display brightness value.
>
> Detection if Dell laptop supports or not new timeout AC settings is done by
> checking existence of Keyboard Backlight with AC SMBIOS token (0x0451).
>
> Signed-off-by: Pali RohÃr <pali.rohar@xxxxxxxxx>

What base did you use for that?
This patch misses few others that were applied to our testing / for-next.

I have applied it to testing with corrected conflict (easy to fix),
though check if everything still as supposed to be.

> ---
> I have not tested this patch yet as I do not have affected machine. I would
> appreciate some testing of this patch on more new Dell laptops. Without
> this patch changing keyboard backlight is not possible on affected machines.
> ---
>  drivers/platform/x86/dell-laptop.c |   59 ++++++++++++++++++++++++++++++++----
>  1 file changed, 53 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index f57dd28..cddf3c2 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -42,6 +42,7 @@
>  #define KBD_LED_AUTO_50_TOKEN 0x02EB
>  #define KBD_LED_AUTO_75_TOKEN 0x02EC
>  #define KBD_LED_AUTO_100_TOKEN 0x02F6
> +#define KBD_LED_AC_TOKEN 0x0451
>
>  struct quirk_entry {
>         u8 touchpad_led;
> @@ -1024,7 +1025,7 @@ static void touchpad_led_exit(void)
>   *     bit 2     Pointing stick
>   *     bit 3     Any mouse
>   *     bits 4-7  Reserved for future use
> - *  cbRES2, byte3  Current Timeout
> + *  cbRES2, byte3  Current Timeout on battery
>   *     bits 7:6  Timeout units indicator:
>   *     00b       Seconds
>   *     01b       Minutes
> @@ -1036,6 +1037,15 @@ static void touchpad_led_exit(void)
>   *  cbRES3, byte0  Current setting of ALS value that turns the light on or off.
>   *  cbRES3, byte1  Current ALS reading
>   *  cbRES3, byte2  Current keyboard light level.
> + *  cbRES3, byte3  Current timeout on AC Power
> + *     bits 7:6  Timeout units indicator:
> + *     00b       Seconds
> + *     01b       Minutes
> + *     10b       Hours
> + *     11b       Days
> + *     Bits 5:0  Timeout value (0-63) in sec/min/hr/day
> + *     NOTE: A value of 0 means always on (no timeout) if any bits of RES3 byte2
> + *     are set upon return from the upon return from the [Get Feature information] call.
>   *
>   * cbArg1 0x2 = Set New State
>   *  cbRES1         Standard return codes (0, -1, -2)
> @@ -1058,7 +1068,7 @@ static void touchpad_led_exit(void)
>   *     bit 2     Pointing stick
>   *     bit 3     Any mouse
>   *     bits 4-7  Reserved for future use
> - *  cbArg2, byte3  Desired Timeout
> + *  cbArg2, byte3  Desired Timeout on battery
>   *     bits 7:6  Timeout units indicator:
>   *     00b       Seconds
>   *     01b       Minutes
> @@ -1067,6 +1077,13 @@ static void touchpad_led_exit(void)
>   *     bits 5:0  Timeout value (0-63) in sec/min/hr/day
>   *  cbArg3, byte0  Desired setting of ALS value that turns the light on or off.
>   *  cbArg3, byte2  Desired keyboard light level.
> + *  cbArg3, byte3  Desired Timeout on AC power
> + *     bits 7:6  Timeout units indicator:
> + *     00b       Seconds
> + *     01b       Minutes
> + *     10b       Hours
> + *     11b       Days
> + *     bits 5:0  Timeout value (0-63) in sec/min/hr/day
>   */
>
>
> @@ -1112,6 +1129,8 @@ struct kbd_state {
>         u8 triggers;
>         u8 timeout_value;
>         u8 timeout_unit;
> +       u8 timeout_value_ac;
> +       u8 timeout_unit_ac;
>         u8 als_setting;
>         u8 als_value;
>         u8 level;
> @@ -1131,6 +1150,7 @@ struct kbd_state {
>  static struct kbd_info kbd_info;
>  static bool kbd_als_supported;
>  static bool kbd_triggers_supported;
> +static bool kbd_timeout_ac_supported;
>
>  static u8 kbd_mode_levels[16];
>  static int kbd_mode_levels_count;
> @@ -1269,6 +1289,8 @@ static int kbd_get_state(struct kbd_state *state)
>         state->als_setting = buffer->output[2] & 0xFF;
>         state->als_value = (buffer->output[2] >> 8) & 0xFF;
>         state->level = (buffer->output[2] >> 16) & 0xFF;
> +       state->timeout_value_ac = (buffer->output[2] >> 24) & 0x3F;
> +       state->timeout_unit_ac = (buffer->output[2] >> 30) & 0x3;
>
>   out:
>         dell_smbios_release_buffer();
> @@ -1288,6 +1310,8 @@ static int kbd_set_state(struct kbd_state *state)
>         buffer->input[1] |= (state->timeout_unit & 0x3) << 30;
>         buffer->input[2] = state->als_setting & 0xFF;
>         buffer->input[2] |= (state->level & 0xFF) << 16;
> +       buffer->input[2] |= (state->timeout_value_ac & 0x3F) << 24;
> +       buffer->input[2] |= (state->timeout_unit_ac & 0x3) << 30;
>         dell_smbios_send_request(4, 11);
>         ret = buffer->output[0];
>         dell_smbios_release_buffer();
> @@ -1394,6 +1418,13 @@ static inline int kbd_init_info(void)
>         if (ret)
>                 return ret;
>
> +       /* NOTE: Old models without KBD_LED_AC_TOKEN token supports only one
> +        *       timeout value which is shared for both battery and AC power
> +        *       settings. So do not try to set AC values on old models.
> +        */
> +       if (dell_smbios_find_token(KBD_LED_AC_TOKEN))
> +               kbd_timeout_ac_supported = true;
> +
>         kbd_get_state(&state);
>
>         /* NOTE: timeout value is stored in 6 bits so max value is 63 */
> @@ -1573,8 +1604,14 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
>                 return ret;
>
>         new_state = state;
> -       new_state.timeout_value = value;
> -       new_state.timeout_unit = unit;
> +
> +       if (kbd_timeout_ac_supported && power_supply_is_system_supplied() > 0) {
> +               new_state.timeout_value_ac = value;
> +               new_state.timeout_unit_ac = unit;
> +       } else {
> +               new_state.timeout_value = value;
> +               new_state.timeout_unit = unit;
> +       }
>
>         ret = kbd_set_state_safe(&new_state, &state);
>         if (ret)
> @@ -1587,16 +1624,26 @@ static ssize_t kbd_led_timeout_show(struct device *dev,
>                                     struct device_attribute *attr, char *buf)
>  {
>         struct kbd_state state;
> +       int value;
>         int ret;
>         int len;
> +       u8 unit;
>
>         ret = kbd_get_state(&state);
>         if (ret)
>                 return ret;
>
> -       len = sprintf(buf, "%d", state.timeout_value);
> +       if (kbd_timeout_ac_supported && power_supply_is_system_supplied() > 0) {
> +               value = state.timeout_value_ac;
> +               unit = state.timeout_unit_ac;
> +       } else {
> +               value = state.timeout_value;
> +               unit = state.timeout_unit;
> +       }
> +
> +       len = sprintf(buf, "%d", value);
>
> -       switch (state.timeout_unit) {
> +       switch (unit) {
>         case KBD_TIMEOUT_SECONDS:
>                 return len + sprintf(buf+len, "s\n");
>         case KBD_TIMEOUT_MINUTES:
> --
> 1.7.9.5
>



-- 
With Best Regards,
Andy Shevchenko