From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S976312AbdDXTHi (ORCPT ); Mon, 24 Apr 2017 15:07:38 -0400 Received: from esa7.dell-outbound.iphmx.com ([68.232.153.96]:37863 "EHLO esa7.dell-outbound.iphmx.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S975378AbdDXTH0 (ORCPT ); Mon, 24 Apr 2017 15:07:26 -0400 From: DomainKey-Signature: s=smtpout; d=dell.com; c=nofws; q=dns; h=X-LoopCount0:X-IronPort-AV:From:To:CC:Subject: Thread-Topic:Thread-Index:Date:Message-ID:References: In-Reply-To:Accept-Language:Content-Language: X-MS-Has-Attach:X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader:x-originating-ip: Content-Type:Content-Transfer-Encoding:MIME-Version: Return-Path; b=H0uTG3YTXBlS7ati1ucwBk7r3Tnfyt/Pb9dKdeayJBx4ZqJ366noXxRP r0+FzhwGaXZNj/lEweTwgTtn9oFX/thhtInj9fZKO4XjnW4eEduv0OT0K h2qK6iew2tneBdQhyatGiyYQzR+lskfU90vQDMhzGTSb0II/jH+ZS7bqo I=; X-LoopCount0: from 10.175.216.251 X-IronPort-AV: E=Sophos;i="5.37,245,1488866400"; d="scan'208";a="12080110" To: , , , , CC: , Subject: RE: [PATCH] dell-laptop: Adds support for keyboard backlight timeout AC settings Thread-Topic: [PATCH] dell-laptop: Adds support for keyboard backlight timeout AC settings Thread-Index: AQHSvGmVbigPMd42cUuxnvgN61PQvKHTu9KAgAEm49A= Date: Mon, 24 Apr 2017 19:07:20 +0000 Message-ID: <2ba95415387849d0b65cff4b9bbd5808@ausx13mpc120.AMER.DELL.COM> References: <1492976447-10339-1-git-send-email-pali.rohar@gmail.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.208.86.26] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id v3OJ7i4c020993 > -----Original Message----- > From: Arcadiy Ivanov [mailto:arcadiy@ivanov.biz] > Sent: Sunday, April 23, 2017 3:30 PM > To: Pali Rohár ; Matthew Garrett ; > Darren Hart ; Andy Shevchenko ; > Limonciello, Mario > 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 > > --- > > 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