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

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

[Pali Rohár]

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@gmail.com>
---
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

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

[Arcadiy Ivanov]

I've tested the patch over 4.10.10 on Fedora 25 and it works both on and 
off AC power on Dell Precision 7510.
Thanks a lot!

On 2017-04-23 15:40, Pali Rohár 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@gmail.com>
> ---
> 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:


-- 
Arcadiy Ivanov
arcadiy@ivanov.biz | @arcivanov | https://ivanov.biz
https://github.com/arcivanov

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

[Mario.Limonciello]

> -----Original Message-----
> From: Arcadiy Ivanov [mailto:arcadiy@ivanov.biz]
> Sent: Sunday, April 23, 2017 3:30 PM
> To: Pali Rohár <pali.rohar@gmail.com>; Matthew Garrett <mjg59@srcf.ucam.org>;
> Darren Hart <dvhart@infradead.org>; Andy Shevchenko <andy@infradead.org>;
> Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] dell-laptop: Adds support for keyboard backlight timeout AC
> settings
> 
> I've tested the patch over 4.10.10 on Fedora 25 and it works both on and
> off AC power on Dell Precision 7510.
> Thanks a lot!
> 
> On 2017-04-23 15:40, Pali Rohár 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@gmail.com>
> > ---
> > 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:
> 
> 
> --
> Arcadiy Ivanov
> arcadiy@ivanov.biz | @arcivanov | https://ivanov.biz
> https://github.com/arcivanov
> 

Arcadiy, Pali, thanks for this quick turnaround!
This looks good to me.  I've passed it to my internal team
for any other feedback.

Acked-by: Mario Limonciello <mario.limonciello@dell.com>

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@gmail.com> 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@gmail.com>

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

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

[Pali Rohár]

On Tuesday 25 April 2017 15:36:56 Andy Shevchenko wrote:
> On Sun, Apr 23, 2017 at 10:40 PM, Pali Rohár <pali.rohar@gmail.com> 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@gmail.com>
> 
> What base did you use for that?

Current linus master at the time of sending patch.

> This patch misses few others that were applied to our testing / for-next.

Ah... sorry for that :-( And I was in impression that there were fixes
for that code which use mutexes... Now I know why I have not seen them!

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

Can you send a link to your fix?

> > ---
> > 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
> >
> 
> 
> 

-- 
Pali Rohár
pali.rohar@gmail.com

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

[Andy Shevchenko]

On Tue, Apr 25, 2017 at 3:54 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Tuesday 25 April 2017 15:36:56 Andy Shevchenko wrote:
>> On Sun, Apr 23, 2017 at 10:40 PM, Pali Rohár <pali.rohar@gmail.com> wrote:

>> This patch misses few others that were applied to our testing / for-next.
>
> Ah... sorry for that :-( And I was in impression that there were fixes
> for that code which use mutexes... Now I know why I have not seen them!
>
>> I have applied it to testing with corrected conflict (easy to fix),
>> though check if everything still as supposed to be.
>
> Can you send a link to your fix?

Sure, here it is:
http://git.infradead.org/linux-platform-drivers-x86.git/commit/bcf7d8a30e2888f78a19778a16d3dd8c10b4b0ad

-- 
With Best Regards,
Andy Shevchenko

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

[Pali Rohár]

On Tuesday 25 April 2017 16:32:36 Andy Shevchenko wrote:
> On Tue, Apr 25, 2017 at 3:54 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > On Tuesday 25 April 2017 15:36:56 Andy Shevchenko wrote:
> >> On Sun, Apr 23, 2017 at 10:40 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> 
> >> This patch misses few others that were applied to our testing / for-next.
> >
> > Ah... sorry for that :-( And I was in impression that there were fixes
> > for that code which use mutexes... Now I know why I have not seen them!
> >
> >> I have applied it to testing with corrected conflict (easy to fix),
> >> though check if everything still as supposed to be.
> >
> > Can you send a link to your fix?
> 
> Sure, here it is:
> http://git.infradead.org/linux-platform-drivers-x86.git/commit/bcf7d8a30e2888f78a19778a16d3dd8c10b4b0ad

It is OK.

-- 
Pali Rohár
pali.rohar@gmail.com

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

[Mario.Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Tuesday, April 25, 2017 8:41 AM
> To: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>; Darren Hart
> <dvhart@infradead.org>; Andy Shevchenko <andy@infradead.org>; Arcadiy Ivanov
> <arcadiy@ivanov.biz>; Limonciello, Mario <Mario_Limonciello@Dell.com>;
> Platform Driver <platform-driver-x86@vger.kernel.org>; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] dell-laptop: Adds support for keyboard backlight timeout AC
> settings
> 
> On Tuesday 25 April 2017 16:32:36 Andy Shevchenko wrote:
> > On Tue, Apr 25, 2017 at 3:54 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > > On Tuesday 25 April 2017 15:36:56 Andy Shevchenko wrote:
> > >> On Sun, Apr 23, 2017 at 10:40 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> >
> > >> This patch misses few others that were applied to our testing / for-next.
> > >
> > > Ah... sorry for that :-( And I was in impression that there were fixes
> > > for that code which use mutexes... Now I know why I have not seen them!
> > >
> > >> I have applied it to testing with corrected conflict (easy to fix),
> > >> though check if everything still as supposed to be.
> > >
> > > Can you send a link to your fix?
> >
> > Sure, here it is:
> > http://git.infradead.org/linux-platform-drivers-
> x86.git/commit/bcf7d8a30e2888f78a19778a16d3dd8c10b4b0ad
> 
> It is OK.
> 

Pali,

Considering this is negatively affecting folks on stable kernel releases too
when using this newer HW, would you consider to send to @stable too?

Thanks,

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

[Darren Hart]

On Tue, Apr 25, 2017 at 05:50:57PM -0400, Arcadiy Ivanov wrote:
> On 2017-04-25 09:49, Mario.Limonciello@dell.com wrote:
> > > -----Original Message-----
> > > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > > Sent: Tuesday, April 25, 2017 8:41 AM
> > > To: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Cc: Matthew Garrett <mjg59@srcf.ucam.org>; Darren Hart
> > > <dvhart@infradead.org>; Andy Shevchenko <andy@infradead.org>; Arcadiy Ivanov
> > > <arcadiy@ivanov.biz>; Limonciello, Mario <Mario_Limonciello@Dell.com>;
> > > Platform Driver <platform-driver-x86@vger.kernel.org>; linux-
> > > kernel@vger.kernel.org
> > > Subject: Re: [PATCH] dell-laptop: Adds support for keyboard backlight timeout AC
> > > settings
> > > 
> > > On Tuesday 25 April 2017 16:32:36 Andy Shevchenko wrote:
> > > > On Tue, Apr 25, 2017 at 3:54 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > On Tuesday 25 April 2017 15:36:56 Andy Shevchenko wrote:
> > > > > > On Sun, Apr 23, 2017 at 10:40 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > > This patch misses few others that were applied to our testing / for-next.
> > > > > Ah... sorry for that :-( And I was in impression that there were fixes
> > > > > for that code which use mutexes... Now I know why I have not seen them!
> > > > > 
> > > > > > I have applied it to testing with corrected conflict (easy to fix),
> > > > > > though check if everything still as supposed to be.
> > > > > Can you send a link to your fix?
> > > > Sure, here it is:
> > > > http://git.infradead.org/linux-platform-drivers-
> > > x86.git/commit/bcf7d8a30e2888f78a19778a16d3dd8c10b4b0ad
> > > 
> > > It is OK.
> > > 
> > Pali,
> > 
> > Considering this is negatively affecting folks on stable kernel releases too
> > when using this newer HW, would you consider to send to @stable too?
> > 
> > Thanks,
> 
> I second that. Thanks!
> 

In order for this to go back to stable, we will need to include the list of
dependencies per stable-kernel-rules.txt. This can be done after it is merged,
but if you can provide the set of Cc: stable lines needed, we can look at
getting this into testing now so no further action will be required.

...

Upon closer inspection, this does fail the 100 lines including context stable
rule by an additional 55 lines. This is a process issue I've been thinking about
for a while and trying to find the best way to express this problem and what a
viable solution might look like. As it stands, fixes such as these are
effectively limited to current and future kernel versions. I do think there is a
need for vendors to be able to update their drivers upstream to work with current
hardware without requiring a bleeding edge kernel. Perhaps "leaf node drivers"
should have a looser set of stable rules, but that is not currently the way the
upstream stable kernel rules work (at least as far as I understand them).

+Greg KH who has the ultimate say on this. Greg, have I misrepresented anything
here?

-- 
Darren Hart
VMware Open Source Technology Center

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

[Pali Rohár]

On Wednesday 26 April 2017 15:40:49 Darren Hart wrote:
> On Tue, Apr 25, 2017 at 05:50:57PM -0400, Arcadiy Ivanov wrote:
> > On 2017-04-25 09:49, Mario.Limonciello@dell.com wrote:
> > > > -----Original Message-----
> > > > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > > > Sent: Tuesday, April 25, 2017 8:41 AM
> > > > To: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > Cc: Matthew Garrett <mjg59@srcf.ucam.org>; Darren Hart
> > > > <dvhart@infradead.org>; Andy Shevchenko <andy@infradead.org>; Arcadiy Ivanov
> > > > <arcadiy@ivanov.biz>; Limonciello, Mario <Mario_Limonciello@Dell.com>;
> > > > Platform Driver <platform-driver-x86@vger.kernel.org>; linux-
> > > > kernel@vger.kernel.org
> > > > Subject: Re: [PATCH] dell-laptop: Adds support for keyboard backlight timeout AC
> > > > settings
> > > > 
> > > > On Tuesday 25 April 2017 16:32:36 Andy Shevchenko wrote:
> > > > > On Tue, Apr 25, 2017 at 3:54 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > > On Tuesday 25 April 2017 15:36:56 Andy Shevchenko wrote:
> > > > > > > On Sun, Apr 23, 2017 at 10:40 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > > > This patch misses few others that were applied to our testing / for-next.
> > > > > > Ah... sorry for that :-( And I was in impression that there were fixes
> > > > > > for that code which use mutexes... Now I know why I have not seen them!
> > > > > > 
> > > > > > > I have applied it to testing with corrected conflict (easy to fix),
> > > > > > > though check if everything still as supposed to be.
> > > > > > Can you send a link to your fix?
> > > > > Sure, here it is:
> > > > > http://git.infradead.org/linux-platform-drivers-
> > > > x86.git/commit/bcf7d8a30e2888f78a19778a16d3dd8c10b4b0ad
> > > > 
> > > > It is OK.
> > > > 
> > > Pali,
> > > 
> > > Considering this is negatively affecting folks on stable kernel releases too
> > > when using this newer HW, would you consider to send to @stable too?
> > > 
> > > Thanks,

I'm fine with it, if Greg or stable maintainers approve this change.

> > I second that. Thanks!
> > 
> 
> In order for this to go back to stable, we will need to include the list of
> dependencies per stable-kernel-rules.txt. This can be done after it is merged,
> but if you can provide the set of Cc: stable lines needed, we can look at
> getting this into testing now so no further action will be required.
> 
> ...
> 
> Upon closer inspection, this does fail the 100 lines including context stable
> rule by an additional 55 lines. This is a process issue I've been thinking about
> for a while and trying to find the best way to express this problem and what a
> viable solution might look like. As it stands, fixes such as these are
> effectively limited to current and future kernel versions. I do think there is a
> need for vendors to be able to update their drivers upstream to work with current
> hardware without requiring a bleeding edge kernel. Perhaps "leaf node drivers"
> should have a looser set of stable rules, but that is not currently the way the
> upstream stable kernel rules work (at least as far as I understand them).
> 
> +Greg KH who has the ultimate say on this. Greg, have I misrepresented anything
> here?
> 

-- 
Pali Rohár
pali.rohar@gmail.com

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

[Greg Kroah-Hartman]

On Wed, Apr 26, 2017 at 03:40:49PM -0700, Darren Hart wrote:
> On Tue, Apr 25, 2017 at 05:50:57PM -0400, Arcadiy Ivanov wrote:
> > On 2017-04-25 09:49, Mario.Limonciello@dell.com wrote:
> > > > -----Original Message-----
> > > > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > > > Sent: Tuesday, April 25, 2017 8:41 AM
> > > > To: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > Cc: Matthew Garrett <mjg59@srcf.ucam.org>; Darren Hart
> > > > <dvhart@infradead.org>; Andy Shevchenko <andy@infradead.org>; Arcadiy Ivanov
> > > > <arcadiy@ivanov.biz>; Limonciello, Mario <Mario_Limonciello@Dell.com>;
> > > > Platform Driver <platform-driver-x86@vger.kernel.org>; linux-
> > > > kernel@vger.kernel.org
> > > > Subject: Re: [PATCH] dell-laptop: Adds support for keyboard backlight timeout AC
> > > > settings
> > > > 
> > > > On Tuesday 25 April 2017 16:32:36 Andy Shevchenko wrote:
> > > > > On Tue, Apr 25, 2017 at 3:54 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > > On Tuesday 25 April 2017 15:36:56 Andy Shevchenko wrote:
> > > > > > > On Sun, Apr 23, 2017 at 10:40 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > > > This patch misses few others that were applied to our testing / for-next.
> > > > > > Ah... sorry for that :-( And I was in impression that there were fixes
> > > > > > for that code which use mutexes... Now I know why I have not seen them!
> > > > > > 
> > > > > > > I have applied it to testing with corrected conflict (easy to fix),
> > > > > > > though check if everything still as supposed to be.
> > > > > > Can you send a link to your fix?
> > > > > Sure, here it is:
> > > > > http://git.infradead.org/linux-platform-drivers-
> > > > x86.git/commit/bcf7d8a30e2888f78a19778a16d3dd8c10b4b0ad
> > > > 
> > > > It is OK.
> > > > 
> > > Pali,
> > > 
> > > Considering this is negatively affecting folks on stable kernel releases too
> > > when using this newer HW, would you consider to send to @stable too?
> > > 
> > > Thanks,
> > 
> > I second that. Thanks!
> > 
> 
> In order for this to go back to stable, we will need to include the list of
> dependencies per stable-kernel-rules.txt. This can be done after it is merged,
> but if you can provide the set of Cc: stable lines needed, we can look at
> getting this into testing now so no further action will be required.
> 
> ...
> 
> Upon closer inspection, this does fail the 100 lines including context stable
> rule by an additional 55 lines. This is a process issue I've been thinking about
> for a while and trying to find the best way to express this problem and what a
> viable solution might look like. As it stands, fixes such as these are
> effectively limited to current and future kernel versions. I do think there is a
> need for vendors to be able to update their drivers upstream to work with current
> hardware without requiring a bleeding edge kernel. Perhaps "leaf node drivers"
> should have a looser set of stable rules, but that is not currently the way the
> upstream stable kernel rules work (at least as far as I understand them).
> 
> +Greg KH who has the ultimate say on this. Greg, have I misrepresented anything
> here?

Nope, I usually defer to the maintainer of the subsystem to see if the
patch should be a valid stable patch or not.  So if you think it should
be applied, and it's a "bit" larger than normal, that's fine, no
objection from me.

thanks,

greg k-h