[PATCH] platform: x86: dell-laptop: Add support for keyboard backlight

[PATCH] platform: x86: dell-laptop: Add support for keyboard backlight

[Pali Rohár]

This patch adds support for configuring keyboard backlight settings on supported
Dell laptops. It exports kernel leds interface and uses Dell SMBIOS tokens or
keyboard class interface.

With this patch it is possible to set:
* keyboard backlight level
* timeout after which will be backlight automatically turned off
* input activity triggers (keyboard, touchpad, mouse) which enable backlight
* ambient light settings

Settings are exported via sysfs:
/sys/class/leds/dell::kbd_backlight/

Code is based on newly released documentation by Dell in libsmbios project.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
---
 drivers/platform/x86/dell-laptop.c | 1001 +++++++++++++++++++++++++++++++++++-
 1 file changed, 997 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 233d2ee..bf5b1cc 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -2,9 +2,11 @@
  *  Driver for Dell laptop extras
  *
  *  Copyright (c) Red Hat <mjg@redhat.com>
+ *  Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@gmail.com>
+ *  Copyright (c) 2014 Pali Rohár <pali.rohar@gmail.com>
  *
- *  Based on documentation in the libsmbios package, Copyright (C) 2005 Dell
- *  Inc.
+ *  Based on documentation in the libsmbios package:
+ *  Copyright (C) 2005-2014 Dell Inc.
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License version 2 as
@@ -32,6 +34,13 @@
 #include "../../firmware/dcdbas.h"
 
 #define BRIGHTNESS_TOKEN 0x7d
+#define KBD_LED_OFF_TOKEN 0x01E1
+#define KBD_LED_ON_TOKEN 0x01E2
+#define KBD_LED_AUTO_TOKEN 0x01E3
+#define KBD_LED_AUTO_25_TOKEN 0x02EA
+#define KBD_LED_AUTO_50_TOKEN 0x02EB
+#define KBD_LED_AUTO_75_TOKEN 0x02EC
+#define KBD_LED_AUTO_100_TOKEN 0x02F6
 
 /* This structure will be modified by the firmware when we enter
  * system management mode, hence the volatiles */
@@ -62,6 +71,10 @@ struct calling_interface_structure {
 
 struct quirk_entry {
 	u8 touchpad_led;
+
+	/* Ordered list of timeouts expressed in seconds.
+	 * The list must end with -1 */
+	int kbd_timeouts[];
 };
 
 static struct quirk_entry *quirks;
@@ -76,6 +89,10 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
 	return 1;
 }
 
+static struct quirk_entry quirk_dell_xps13_9333 = {
+	.kbd_timeouts = { 0, 5, 15, 60, 5*60, 15*60, -1 },
+};
+
 static int da_command_address;
 static int da_command_code;
 static int da_num_tokens;
@@ -268,6 +285,15 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
 		},
 		.driver_data = &quirk_dell_vostro_v130,
 	},
+	{
+		.callback = dmi_matched,
+		.ident = "Dell XPS13 9333",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "XPS13 9333"),
+		},
+		.driver_data = &quirk_dell_xps13_9333,
+	},
 	{ }
 };
 
@@ -332,17 +358,29 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
 	}
 }
 
-static int find_token_location(int tokenid)
+static int find_token_id(int tokenid)
 {
 	int i;
+
 	for (i = 0; i < da_num_tokens; i++) {
 		if (da_tokens[i].tokenID == tokenid)
-			return da_tokens[i].location;
+			return i;
 	}
 
 	return -1;
 }
 
+static int find_token_location(int tokenid)
+{
+	int id;
+
+	id = find_token_id(tokenid);
+	if (id == -1)
+		return -1;
+
+	return da_tokens[id].location;
+}
+
 static struct calling_interface_buffer *
 dell_send_request(struct calling_interface_buffer *buffer, int class,
 		  int select)
@@ -790,6 +828,956 @@ static void touchpad_led_exit(void)
 	led_classdev_unregister(&touchpad_led);
 }
 
+/* Derived from information in smbios-keyboard-ctl:
+
+ cbClass 4
+ cbSelect 11
+ Keyboar illumination
+ cbArg1 determines the function to be performed
+
+ cbArg1 0x0 = Get Feature Information
+  cbRES1         Standard return codes (0, -1, -2)
+  cbRES2, word0  Bitmap of user-selectable modes
+     bit 0     Always off (All systems)
+     bit 1     Always on (Travis ATG, Siberia)
+     bit 2     Auto: ALS-based On; ALS-based Off (Travis ATG)
+     bit 3     Auto: ALS- and input-activity-based On; input-activity based Off
+     bit 4     Auto: Input-activity-based On; input-activity based Off
+     bit 5     Auto: Input-activity-based On (illumination level 25%); input-activity based Off
+     bit 6     Auto: Input-activity-based On (illumination level 50%); input-activity based Off
+     bit 7     Auto: Input-activity-based On (illumination level 75%); input-activity based Off
+     bit 8     Auto: Input-activity-based On (illumination level 100%); input-activity based Off
+     bits 9-15 Reserved for future use
+  cbRES2, byte2  Reserved for future use
+  cbRES2, byte3  Keyboard illumination type
+     0         Reserved
+     1         Tasklight
+     2         Backlight
+     3-255     Reserved for future use
+  cbRES3, byte0  Supported auto keyboard illumination trigger bitmap.
+     bit 0     Any keystroke
+     bit 1     Touchpad activity
+     bit 2     Pointing stick
+     bit 3     Any mouse
+     bits 4-7  Reserved for future use
+  cbRES3, byte1  Supported timeout unit bitmap
+     bit 0     Seconds
+     bit 1     Minutes
+     bit 2     Hours
+     bit 3     Days
+     bits 4-7  Reserved for future use
+  cbRES3, byte2  Number of keyboard light brightness levels
+  cbRES4, byte0  Maximum acceptable seconds value (0 if seconds not supported).
+  cbRES4, byte1  Maximum acceptable minutes value (0 if minutes not supported).
+  cbRES4, byte2  Maximum acceptable hours value (0 if hours not supported).
+  cbRES4, byte3  Maximum acceptable days value (0 if days not supported)
+
+ cbArg1 0x1 = Get Current State
+  cbRES1         Standard return codes (0, -1, -2)
+  cbRES2, word0  Bitmap of current mode state
+     bit 0     Always off (All systems)
+     bit 1     Always on (Travis ATG, Siberia)
+     bit 2     Auto: ALS-based On; ALS-based Off (Travis ATG)
+     bit 3     Auto: ALS- and input-activity-based On; input-activity based Off
+     bit 4     Auto: Input-activity-based On; input-activity based Off
+     bit 5     Auto: Input-activity-based On (illumination level 25%); input-activity based Off
+     bit 6     Auto: Input-activity-based On (illumination level 50%); input-activity based Off
+     bit 7     Auto: Input-activity-based On (illumination level 75%); input-activity based Off
+     bit 8     Auto: Input-activity-based On (illumination level 100%); input-activity based Off
+     bits 9-15 Reserved for future use
+     Note: Only One bit can be set
+  cbRES2, byte2  Currently active auto keyboard illumination triggers.
+     bit 0     Any keystroke
+     bit 1     Touchpad activity
+     bit 2     Pointing stick
+     bit 3     Any mouse
+     bits 4-7  Reserved for future use
+  cbRES2, byte3  Current Timeout
+     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 byte
+     are set upon return from the [Get feature information] call.
+  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.
+
+ cbArg1 0x2 = Set New State
+  cbRES1         Standard return codes (0, -1, -2)
+  cbArg2, word0  Bitmap of current mode state
+     bit 0     Always off (All systems)
+     bit 1     Always on (Travis ATG, Siberia)
+     bit 2     Auto: ALS-based On; ALS-based Off (Travis ATG)
+     bit 3     Auto: ALS- and input-activity-based On; input-activity based Off
+     bit 4     Auto: Input-activity-based On; input-activity based Off
+     bit 5     Auto: Input-activity-based On (illumination level 25%); input-activity based Off
+     bit 6     Auto: Input-activity-based On (illumination level 50%); input-activity based Off
+     bit 7     Auto: Input-activity-based On (illumination level 75%); input-activity based Off
+     bit 8     Auto: Input-activity-based On (illumination level 100%); input-activity based Off
+     bits 9-15 Reserved for future use
+     Note: Only One bit can be set
+  cbArg2, byte2  Desired auto keyboard illumination triggers. Must remain inactive to allow
+                 keyboard to turn off automatically.
+     bit 0     Any keystroke
+     bit 1     Touchpad activity
+     bit 2     Pointing stick
+     bit 3     Any mouse
+     bits 4-7  Reserved for future use
+  cbArg2, byte3  Desired Timeout
+     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
+  cbArg3, byte0  Desired setting of ALS value that turns the light on or off.
+  cbArg3, byte2  Desired keyboard light level.
+*/
+
+
+enum kbd_timeout_unit {
+	KBD_TIMEOUT_SECONDS = 0,
+	KBD_TIMEOUT_MINUTES,
+	KBD_TIMEOUT_HOURS,
+	KBD_TIMEOUT_DAYS,
+};
+
+enum kbd_mode_bit {
+	KBD_MODE_BIT_OFF = 0,
+	KBD_MODE_BIT_ON,
+	KBD_MODE_BIT_ALS,
+	KBD_MODE_BIT_TRIGGER_ALS,
+	KBD_MODE_BIT_TRIGGER,
+	KBD_MODE_BIT_TRIGGER_25,
+	KBD_MODE_BIT_TRIGGER_50,
+	KBD_MODE_BIT_TRIGGER_75,
+	KBD_MODE_BIT_TRIGGER_100,
+};
+
+#define kbd_is_als_mode_bit(bit) \
+	((bit) == KBD_MODE_BIT_ALS || (bit) == KBD_MODE_BIT_TRIGGER_ALS)
+#define kbd_is_trigger_mode_bit(bit) \
+	((bit) >= KBD_MODE_BIT_TRIGGER_ALS && (bit) <= KBD_MODE_BIT_TRIGGER_100)
+#define kbd_is_level_mode_bit(bit) \
+	((bit) >= KBD_MODE_BIT_TRIGGER_25 && (bit) <= KBD_MODE_BIT_TRIGGER_100)
+
+struct kbd_info {
+	u16 modes;
+	u8 type;
+	u8 triggers;
+	u8 levels;
+	u8 seconds;
+	u8 minutes;
+	u8 hours;
+	u8 days;
+};
+
+struct kbd_state {
+	u8 mode_bit;
+	u8 triggers;
+	u8 timeout_value;
+	u8 timeout_unit;
+	u8 als_setting;
+	u8 als_value;
+	u8 level;
+};
+
+static const int kbd_tokens[] = {
+	KBD_LED_OFF_TOKEN,
+	KBD_LED_AUTO_25_TOKEN,
+	KBD_LED_AUTO_50_TOKEN,
+	KBD_LED_AUTO_75_TOKEN,
+	KBD_LED_AUTO_100_TOKEN,
+	KBD_LED_ON_TOKEN,
+};
+
+static u16 kbd_token_bits;
+
+static struct kbd_info kbd_info;
+static bool kbd_als_supported;
+static bool kbd_triggers_supported;
+
+static u8 kbd_mode_levels[16];
+static int kbd_mode_levels_count;
+
+static u8 kbd_previous_level;
+static u8 kbd_previous_mode_bit;
+
+static bool kbd_led_present;
+
+static int kbd_get_info(struct kbd_info *info)
+{
+	u8 units;
+	int ret;
+
+	get_buffer();
+
+	buffer->input[0] = 0x0;
+	dell_send_request(buffer, 4, 11);
+	ret = buffer->output[0];
+
+	if (ret == 0) {
+		info->modes = buffer->output[1] & 0xFFFF;
+		info->type = (buffer->output[1] >> 24) & 0xFF;
+		info->triggers = buffer->output[2] & 0xFF;
+		units = (buffer->output[2] >> 8) & 0xFF;
+		info->levels = (buffer->output[2] >> 16) & 0xFF;
+		if (units & BIT(0))
+			info->seconds = (buffer->output[3] >> 0) & 0xFF;
+		if (units & BIT(1))
+			info->minutes = (buffer->output[3] >> 8) & 0xFF;
+		if (units & BIT(2))
+			info->hours = (buffer->output[3] >> 16) & 0xFF;
+		if (units & BIT(3))
+			info->days = (buffer->output[3] >> 24) & 0xFF;
+	}
+
+	release_buffer();
+
+	if (ret)
+		return -EINVAL;
+
+	return 0;
+}
+
+static unsigned int kbd_get_max_level(void)
+{
+	if (kbd_info.levels != 0)
+		return kbd_info.levels;
+	if (kbd_mode_levels_count > 0)
+		return kbd_mode_levels_count - 1;
+	return 0;
+}
+
+static int kbd_get_level(struct kbd_state *state)
+{
+	int i;
+
+	if (kbd_info.levels != 0)
+		return state->level;
+
+	if (kbd_mode_levels_count > 0) {
+		for (i = 0; i < kbd_mode_levels_count; ++i)
+			if (kbd_mode_levels[i] == state->mode_bit)
+				return i;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int kbd_set_level(struct kbd_state *state, u8 level)
+{
+	if (kbd_info.levels != 0) {
+		if (level != 0)
+			kbd_previous_level = level;
+		if (state->level == level)
+			return 0;
+		state->level = level;
+		if (level != 0 && state->mode_bit == KBD_MODE_BIT_OFF)
+			state->mode_bit = kbd_previous_mode_bit;
+		else if (level == 0 && state->mode_bit != KBD_MODE_BIT_OFF) {
+			kbd_previous_mode_bit = state->mode_bit;
+			state->mode_bit = KBD_MODE_BIT_OFF;
+		}
+		return 0;
+	}
+
+	if (kbd_mode_levels_count > 0 && level < kbd_mode_levels_count) {
+		if (level != 0)
+			kbd_previous_level = level;
+		state->mode_bit = kbd_mode_levels[level];
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int kbd_get_state(struct kbd_state *state)
+{
+	int ret;
+
+	get_buffer();
+
+	buffer->input[0] = 0x1;
+	dell_send_request(buffer, 4, 11);
+	ret = buffer->output[0];
+
+	if (ret == 0) {
+		state->mode_bit = ffs(buffer->output[1] & 0xFFFF);
+		if (state->mode_bit != 0)
+			state->mode_bit--;
+		state->triggers = (buffer->output[1] >> 16) & 0xFF;
+		state->timeout_value = (buffer->output[1] >> 24) & 0x3F;
+		state->timeout_unit = (buffer->output[1] >> 30) & 0x3;
+		state->als_setting = buffer->output[2] & 0xFF;
+		state->als_value = (buffer->output[2] >> 8) & 0xFF;
+		state->level = (buffer->output[2] >> 16) & 0xFF;
+	}
+
+	release_buffer();
+
+	if (ret)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int kbd_set_state(struct kbd_state *state)
+{
+	int ret;
+
+	get_buffer();
+	buffer->input[0] = 0x2;
+	buffer->input[1] = BIT(state->mode_bit) & 0xFFFF;
+	buffer->input[1] |= (state->triggers & 0xFF) << 16;
+	buffer->input[1] |= (state->timeout_value & 0x3F) << 24;
+	buffer->input[1] |= (state->timeout_unit & 0x3) << 30;
+	buffer->input[2] = state->als_setting & 0xFF;
+	buffer->input[2] |= (state->level & 0xFF) << 16;
+	dell_send_request(buffer, 4, 11);
+	ret = buffer->output[0];
+	release_buffer();
+
+	if (ret)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int kbd_set_state_safe(struct kbd_state *state, struct kbd_state *old)
+{
+	int ret;
+
+	ret = kbd_set_state(state);
+	if (ret == 0)
+		return 0;
+
+	if (kbd_set_state(old))
+		pr_err("Setting old previous keyboard state failed\n");
+
+	return ret;
+}
+
+static int kbd_set_token_bit(u8 bit)
+{
+	int id;
+	int ret;
+
+	if (bit >= ARRAY_SIZE(kbd_tokens))
+		return -EINVAL;
+
+	id = find_token_id(kbd_tokens[bit]);
+	if (id == -1)
+		return -EINVAL;
+
+	get_buffer();
+	buffer->input[0] = da_tokens[id].location;
+	buffer->input[1] = da_tokens[id].value;
+	dell_send_request(buffer, 1, 0);
+	ret = buffer->output[0];
+	release_buffer();
+
+	if (ret)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int kbd_get_token_bit(u8 bit)
+{
+	int id;
+	int ret;
+	int val;
+
+	if (bit >= ARRAY_SIZE(kbd_tokens))
+		return -EINVAL;
+
+	id = find_token_id(kbd_tokens[bit]);
+	if (id == -1)
+		return -EINVAL;
+
+	get_buffer();
+	buffer->input[0] = da_tokens[id].location;
+	dell_send_request(buffer, 0, 0);
+	ret = buffer->output[0];
+	val = buffer->output[1];
+	release_buffer();
+
+	if (ret)
+		return -EINVAL;
+
+	return (val == da_tokens[id].value);
+}
+
+static int kbd_get_first_active_token_bit(void)
+{
+	int i;
+	int ret;
+
+	for (i = 0; i < ARRAY_SIZE(kbd_tokens); ++i) {
+		ret = kbd_get_token_bit(i);
+		if (ret == 1)
+			return i;
+	}
+
+	return ret;
+}
+
+static int kbd_get_valid_token_counts(void)
+{
+	return hweight16(kbd_token_bits);
+}
+
+static void kbd_init(void)
+{
+	struct kbd_state state;
+	int ret;
+	int i;
+
+	ret = kbd_get_info(&kbd_info);
+
+	if (ret == 0) {
+
+		kbd_get_state(&state);
+
+		/* NOTE: timeout value is stored in 6 bits so max value is 63 */
+		if (kbd_info.seconds > 63)
+			kbd_info.seconds = 63;
+		if (kbd_info.minutes > 63)
+			kbd_info.minutes = 63;
+		if (kbd_info.hours > 63)
+			kbd_info.hours = 63;
+		if (kbd_info.days > 63)
+			kbd_info.days = 63;
+
+		/* NOTE: On tested machines ON mode did not work and caused
+		 *       problems (turned backlight off) so do not use it
+		 */
+		kbd_info.modes &= ~BIT(KBD_MODE_BIT_ON);
+
+		kbd_previous_level = kbd_get_level(&state);
+		kbd_previous_mode_bit = state.mode_bit;
+
+		if (kbd_previous_level == 0) {
+			if (kbd_get_max_level() != 0)
+				kbd_previous_level = 1;
+		}
+
+		if (kbd_previous_mode_bit == KBD_MODE_BIT_OFF) {
+			kbd_previous_mode_bit =
+				ffs(kbd_info.modes & ~BIT(KBD_MODE_BIT_OFF));
+			if (kbd_previous_mode_bit != 0)
+				kbd_previous_mode_bit--;
+		}
+
+		if (kbd_info.modes & (BIT(KBD_MODE_BIT_ALS) |
+				      BIT(KBD_MODE_BIT_TRIGGER_ALS)))
+			kbd_als_supported = true;
+
+		if (kbd_info.modes & (
+		    BIT(KBD_MODE_BIT_TRIGGER_ALS) | BIT(KBD_MODE_BIT_TRIGGER) |
+		    BIT(KBD_MODE_BIT_TRIGGER_25) | BIT(KBD_MODE_BIT_TRIGGER_50) |
+		    BIT(KBD_MODE_BIT_TRIGGER_75) | BIT(KBD_MODE_BIT_TRIGGER_100)
+		   ))
+			kbd_triggers_supported = true;
+
+		for (i = 0; i < 16; ++i)
+			if (kbd_is_level_mode_bit(i) &&
+			    (BIT(i) & kbd_info.modes))
+				kbd_mode_levels[1+kbd_mode_levels_count++] = i;
+
+		if (kbd_mode_levels_count > 0) {
+			for (i = 0; i < 16; ++i) {
+				if (BIT(i) & kbd_info.modes) {
+					kbd_mode_levels[0] = i;
+					break;
+				}
+			}
+			kbd_mode_levels_count++;
+		}
+
+	}
+
+	for (i = 0; i < ARRAY_SIZE(kbd_tokens); ++i)
+		if (find_token_id(kbd_tokens[i]) != -1)
+			kbd_token_bits |= BIT(i);
+
+	if (kbd_token_bits != 0 || ret == 0)
+		kbd_led_present = true;
+}
+
+static ssize_t kbd_led_timeout_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct kbd_state state;
+	struct kbd_state new_state;
+	int ret;
+	bool convert;
+	char ch;
+	u8 unit;
+	int value;
+	int i;
+
+	ret = sscanf(buf, "%d %c", &value, &ch);
+	if (ret < 1)
+		return -EINVAL;
+	else if (ret == 1)
+		ch = 's';
+
+	if (value < 0)
+		return -EINVAL;
+
+	convert = false;
+
+	switch (ch) {
+	case 's':
+		if (value > kbd_info.seconds)
+			convert = true;
+		unit = KBD_TIMEOUT_SECONDS;
+		break;
+	case 'm':
+		if (value > kbd_info.minutes)
+			convert = true;
+		unit = KBD_TIMEOUT_MINUTES;
+		break;
+	case 'h':
+		if (value > kbd_info.hours)
+			convert = true;
+		unit = KBD_TIMEOUT_HOURS;
+		break;
+	case 'd':
+		if (value > kbd_info.days)
+			convert = true;
+		unit = KBD_TIMEOUT_DAYS;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (quirks && quirks->kbd_timeouts)
+		convert = true;
+
+	if (convert) {
+		/* NOTE: this switch fall down */
+		switch (unit) {
+		case KBD_TIMEOUT_DAYS:
+			value *= 24;
+		case KBD_TIMEOUT_HOURS:
+			value *= 60;
+		case KBD_TIMEOUT_MINUTES:
+			value *= 60;
+			unit = KBD_TIMEOUT_SECONDS;
+		}
+
+		if (quirks && quirks->kbd_timeouts) {
+			for (i = 0; quirks->kbd_timeouts[i] != -1; i++) {
+				if (value <= quirks->kbd_timeouts[i]) {
+					value = quirks->kbd_timeouts[i];
+					break;
+				}
+			}
+		}
+
+		if (value <= kbd_info.seconds && kbd_info.seconds) {
+			unit = KBD_TIMEOUT_SECONDS;
+		} else if (value/60 <= kbd_info.minutes && kbd_info.minutes) {
+			value /= 60;
+			unit = KBD_TIMEOUT_MINUTES;
+		} else if (value/(60*60) <= kbd_info.hours && kbd_info.hours) {
+			value /= (60*60);
+			unit = KBD_TIMEOUT_HOURS;
+		} else if (value/(60*60*24) <= kbd_info.days && kbd_info.days) {
+			value /= (60*60*24);
+			unit = KBD_TIMEOUT_DAYS;
+		} else {
+			return -EINVAL;
+		}
+	}
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	new_state = state;
+	new_state.timeout_value = value;
+	new_state.timeout_unit = unit;
+
+	ret = kbd_set_state_safe(&new_state, &state);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t kbd_led_timeout_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct kbd_state state;
+	int ret;
+	int len;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	len = sprintf(buf, "%d", state.timeout_value);
+
+	switch (state.timeout_unit) {
+	case KBD_TIMEOUT_SECONDS:
+		return len + sprintf(buf+len, "s\n");
+	case KBD_TIMEOUT_MINUTES:
+		return len + sprintf(buf+len, "m\n");
+	case KBD_TIMEOUT_HOURS:
+		return len + sprintf(buf+len, "h\n");
+	case KBD_TIMEOUT_DAYS:
+		return len + sprintf(buf+len, "d\n");
+	default:
+		return -EINVAL;
+	}
+
+	return len;
+}
+
+static DEVICE_ATTR(stop_timeout, S_IRUGO | S_IWUSR,
+		   kbd_led_timeout_show, kbd_led_timeout_store);
+
+static const char * const kbd_led_triggers[] = {
+	"keyboard",
+	"touchpad",
+	/*"trackstick"*/ NULL, /* NOTE: trackstick is just alias for touchpad */
+	"mouse",
+};
+
+static ssize_t kbd_led_triggers_store(struct device *dev,
+				      struct device_attribute *attr,
+				      const char *buf, size_t count)
+{
+	struct kbd_state state;
+	struct kbd_state new_state;
+	char trigger[21];
+	bool als_enabled;
+	bool triggers_enabled;
+	bool enable_als;
+	bool disable_als;
+	int trigger_bit;
+	int i;
+	int ret;
+
+	ret = sscanf(buf, "%20s", trigger);
+	if (ret != 1)
+		return -EINVAL;
+
+	if (trigger[0] != '+' && trigger[0] != '-')
+		return -EINVAL;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	if (kbd_als_supported)
+		als_enabled = kbd_is_als_mode_bit(state.mode_bit);
+	else
+		als_enabled = false;
+
+	if (kbd_triggers_supported)
+		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
+	else
+		triggers_enabled = false;
+
+	enable_als = false;
+	disable_als = false;
+
+	if (kbd_als_supported) {
+		if (strcmp(trigger, "+als") == 0) {
+			if (als_enabled)
+				return count;
+			enable_als = true;
+		} else if (strcmp(trigger, "-als") == 0) {
+			if (!als_enabled)
+				return count;
+			disable_als = true;
+		}
+	}
+
+	if (enable_als || disable_als) {
+		new_state = state;
+		if (enable_als) {
+			if (triggers_enabled)
+				new_state.mode_bit = KBD_MODE_BIT_TRIGGER_ALS;
+			else
+				new_state.mode_bit = KBD_MODE_BIT_ALS;
+		} else {
+			if (triggers_enabled) {
+				new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
+				kbd_set_level(&new_state, kbd_previous_level);
+			} else
+				new_state.mode_bit = KBD_MODE_BIT_ON;
+		}
+		if (!(kbd_info.modes & BIT(new_state.mode_bit)))
+			return -EINVAL;
+		ret = kbd_set_state_safe(&new_state, &state);
+		if (ret)
+			return ret;
+		kbd_previous_mode_bit = new_state.mode_bit;
+		return count;
+	}
+
+	trigger_bit = -1;
+
+	if (kbd_triggers_supported) {
+		for (i = 0; i < ARRAY_SIZE(kbd_led_triggers); ++i) {
+			if (!(kbd_info.triggers & BIT(i)))
+				continue;
+			if (!kbd_led_triggers[i])
+				continue;
+			if (strcmp(trigger+1, kbd_led_triggers[i]) != 0)
+				continue;
+			if (trigger[0] == '+' &&
+			    triggers_enabled && (state.triggers & BIT(i)))
+				return count;
+			if (trigger[0] == '-' &&
+			    (!triggers_enabled || !(state.triggers & BIT(i))))
+				return count;
+			trigger_bit = i;
+			break;
+		}
+	}
+
+	if (trigger_bit != -1) {
+		new_state = state;
+		if (trigger[0] == '+')
+			new_state.triggers |= BIT(trigger_bit);
+		else {
+			new_state.triggers &= ~BIT(trigger_bit);
+			/* NOTE: trackstick bit (2) must be disabled when
+			 *       disabling touchpad bit (1), otherwise touchpad
+			 *       bit (1) will not be disabled */
+			if (trigger_bit == 1)
+				new_state.triggers &= ~BIT(2);
+		}
+		if ((kbd_info.triggers & new_state.triggers) !=
+		    new_state.triggers)
+			return -EINVAL;
+		if (new_state.triggers && !triggers_enabled) {
+			if (als_enabled)
+				new_state.mode_bit = KBD_MODE_BIT_TRIGGER_ALS;
+			else {
+				new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
+				kbd_set_level(&new_state, kbd_previous_level);
+			}
+		} else if (new_state.triggers == 0) {
+			if (als_enabled)
+				new_state.mode_bit = KBD_MODE_BIT_ALS;
+			else
+				kbd_set_level(&new_state, 0);
+		}
+		if (!(kbd_info.modes & BIT(new_state.mode_bit)))
+			return -EINVAL;
+		ret = kbd_set_state_safe(&new_state, &state);
+		if (ret)
+			return ret;
+		if (new_state.mode_bit != KBD_MODE_BIT_OFF)
+			kbd_previous_mode_bit = new_state.mode_bit;
+		return count;
+	}
+
+	return -EINVAL;
+}
+
+static ssize_t kbd_led_triggers_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct kbd_state state;
+	bool triggers_enabled;
+	int level;
+	int i;
+	int ret;
+	int len;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	len = 0;
+
+	if (kbd_triggers_supported) {
+		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
+		level = kbd_get_level(&state);
+		for (i = 0; i < ARRAY_SIZE(kbd_led_triggers); ++i) {
+			if (!(kbd_info.triggers & BIT(i)))
+				continue;
+			if (!kbd_led_triggers[i])
+				continue;
+			if ((triggers_enabled || level <= 0) &&
+			    (state.triggers & BIT(i)))
+				buf[len++] = '+';
+			else
+				buf[len++] = '-';
+			len += sprintf(buf+len, "%s ", kbd_led_triggers[i]);
+		}
+	}
+
+	if (kbd_als_supported) {
+		if (kbd_is_als_mode_bit(state.mode_bit))
+			len += sprintf(buf+len, "+als ");
+		else
+			len += sprintf(buf+len, "-als ");
+	}
+
+	if (len)
+		buf[len - 1] = '\n';
+
+	return len;
+}
+
+static DEVICE_ATTR(start_triggers, S_IRUGO | S_IWUSR,
+		   kbd_led_triggers_show, kbd_led_triggers_store);
+
+static ssize_t kbd_led_als_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct kbd_state state;
+	struct kbd_state new_state;
+	u8 setting;
+	int ret;
+
+	ret = kstrtou8(buf, 10, &setting);
+	if (ret)
+		return ret;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	new_state = state;
+	new_state.als_setting = setting;
+
+	ret = kbd_set_state_safe(&new_state, &state);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t kbd_led_als_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct kbd_state state;
+	int ret;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "%d\n", state.als_setting);
+}
+
+static DEVICE_ATTR(als_setting, S_IRUGO | S_IWUSR,
+		   kbd_led_als_show, kbd_led_als_store);
+
+static struct attribute *kbd_led_attrs[] = {
+	&dev_attr_stop_timeout.attr,
+	&dev_attr_start_triggers.attr,
+	&dev_attr_als_setting.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(kbd_led);
+
+static enum led_brightness kbd_led_level_get(struct led_classdev *led_cdev)
+{
+	int ret;
+	u16 num;
+	struct kbd_state state;
+
+	if (kbd_get_max_level()) {
+		ret = kbd_get_state(&state);
+		if (ret)
+			return 0;
+		ret = kbd_get_level(&state);
+		if (ret < 0)
+			return 0;
+		return ret;
+	} else if (kbd_get_valid_token_counts()) {
+		ret = kbd_get_first_active_token_bit();
+		if (ret < 0)
+			return 0;
+		for (num = kbd_token_bits; num != 0 && ret > 0; --ret)
+			num &= num - 1; /* clear the first bit set */
+		if (num == 0)
+			return 0;
+		return ffs(num) - 1;
+	} else {
+		pr_warn("Keyboard brightness level control not supported\n");
+		return 0;
+	}
+}
+
+static void kbd_led_level_set(struct led_classdev *led_cdev,
+			      enum led_brightness value)
+{
+	struct kbd_state state;
+	struct kbd_state new_state;
+	u16 num;
+
+	if (kbd_get_max_level()) {
+		if (kbd_get_state(&state))
+			return;
+		new_state = state;
+		if (kbd_set_level(&new_state, value))
+			return;
+		kbd_set_state_safe(&new_state, &state);
+	} else if (kbd_get_valid_token_counts()) {
+		for (num = kbd_token_bits; num != 0 && value > 0; --value)
+			num &= num - 1; /* clear the first bit set */
+		if (num == 0)
+			return;
+		kbd_set_token_bit(ffs(num) - 1);
+	} else {
+		pr_warn("Keyboard brightness level control not supported\n");
+	}
+}
+
+static struct led_classdev kbd_led = {
+	.name           = "dell::kbd_backlight",
+	.brightness_set = kbd_led_level_set,
+	.brightness_get = kbd_led_level_get,
+	.groups         = kbd_led_groups,
+};
+
+static int __init kbd_led_init(struct device *dev)
+{
+	kbd_init();
+	if (!kbd_led_present)
+		return -ENODEV;
+	kbd_led.max_brightness = kbd_get_max_level();
+	if (!kbd_led.max_brightness) {
+		kbd_led.max_brightness = kbd_get_valid_token_counts();
+		if (kbd_led.max_brightness)
+			kbd_led.max_brightness--;
+	}
+	return led_classdev_register(dev, &kbd_led);
+}
+
+static void brightness_set_exit(struct led_classdev *led_cdev,
+				enum led_brightness value)
+{
+	/* Don't change backlight level on exit */
+};
+
+static void kbd_led_exit(void)
+{
+	if (!kbd_led_present)
+		return;
+	kbd_led.brightness_set = brightness_set_exit;
+	led_classdev_unregister(&kbd_led);
+}
+
 static int __init dell_init(void)
 {
 	int max_intensity = 0;
@@ -842,6 +1830,8 @@ static int __init dell_init(void)
 	if (quirks && quirks->touchpad_led)
 		touchpad_led_init(&platform_device->dev);
 
+	kbd_led_init(&platform_device->dev);
+
 	dell_laptop_dir = debugfs_create_dir("dell_laptop", NULL);
 	if (dell_laptop_dir != NULL)
 		debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
@@ -909,6 +1899,7 @@ static void __exit dell_exit(void)
 	debugfs_remove_recursive(dell_laptop_dir);
 	if (quirks && quirks->touchpad_led)
 		touchpad_led_exit();
+	kbd_led_exit();
 	i8042_remove_filter(dell_laptop_i8042_filter);
 	cancel_delayed_work_sync(&dell_rfkill_work);
 	backlight_device_unregister(dell_backlight_device);
@@ -925,5 +1916,7 @@ module_init(dell_init);
 module_exit(dell_exit);
 
 MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
+MODULE_AUTHOR("Gabriele Mazzotta <gabriele.mzt@gmail.com>");
+MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
 MODULE_DESCRIPTION("Dell laptop driver");
 MODULE_LICENSE("GPL");
-- 
1.7.9.5

Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight

[Darren Hart]

On Fri, Nov 14, 2014 at 01:23:33PM +0100, Pali Rohár wrote:
> This patch adds support for configuring keyboard backlight settings on supported
> Dell laptops. It exports kernel leds interface and uses Dell SMBIOS tokens or
> keyboard class interface.
> 
> With this patch it is possible to set:
> * keyboard backlight level
> * timeout after which will be backlight automatically turned off
> * input activity triggers (keyboard, touchpad, mouse) which enable backlight
> * ambient light settings
> 
> Settings are exported via sysfs:
> /sys/class/leds/dell::kbd_backlight/
> 
> Code is based on newly released documentation by Dell in libsmbios project.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>

Hi Pali,

Apologies for the delay.

I'm somewhat concerned that this patch doubles the size of this driver. When
we're adding this much code, I have to ask - does it make sense to grow this
driver rather than create a new one?

There is no ACPI backlight driver on these systems? We need a platform driver?

Matthew, I'd appreciate your thoughts as you're listed as the module author.

My main commentary throughout is surrounding large indented logic blocks which
could be less nested by checking for errors and returning/jumping rather than
using an if (success) block for the main logic. There are various places where
some logic reduction would result in some significantly tighter code.

Specific comments below.

> ---
>  drivers/platform/x86/dell-laptop.c | 1001 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 997 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index 233d2ee..bf5b1cc 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -2,9 +2,11 @@
>   *  Driver for Dell laptop extras
>   *
>   *  Copyright (c) Red Hat <mjg@redhat.com>
> + *  Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@gmail.com>
> + *  Copyright (c) 2014 Pali Rohár <pali.rohar@gmail.com>
>   *
> - *  Based on documentation in the libsmbios package, Copyright (C) 2005 Dell
> - *  Inc.
> + *  Based on documentation in the libsmbios package:
> + *  Copyright (C) 2005-2014 Dell Inc.
>   *
>   *  This program is free software; you can redistribute it and/or modify
>   *  it under the terms of the GNU General Public License version 2 as
> @@ -32,6 +34,13 @@
>  #include "../../firmware/dcdbas.h"
>  
>  #define BRIGHTNESS_TOKEN 0x7d
> +#define KBD_LED_OFF_TOKEN 0x01E1
> +#define KBD_LED_ON_TOKEN 0x01E2
> +#define KBD_LED_AUTO_TOKEN 0x01E3
> +#define KBD_LED_AUTO_25_TOKEN 0x02EA
> +#define KBD_LED_AUTO_50_TOKEN 0x02EB
> +#define KBD_LED_AUTO_75_TOKEN 0x02EC
> +#define KBD_LED_AUTO_100_TOKEN 0x02F6
>  
>  /* This structure will be modified by the firmware when we enter
>   * system management mode, hence the volatiles */
> @@ -62,6 +71,10 @@ struct calling_interface_structure {
>  
>  struct quirk_entry {
>  	u8 touchpad_led;
> +
> +	/* Ordered list of timeouts expressed in seconds.
> +	 * The list must end with -1 */
> +	int kbd_timeouts[];
>  };
>  
>  static struct quirk_entry *quirks;
> @@ -76,6 +89,10 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
>  	return 1;
>  }
>  
> +static struct quirk_entry quirk_dell_xps13_9333 = {
> +	.kbd_timeouts = { 0, 5, 15, 60, 5*60, 15*60, -1 },
> +};
> +
>  static int da_command_address;
>  static int da_command_code;
>  static int da_num_tokens;
> @@ -268,6 +285,15 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
>  		},
>  		.driver_data = &quirk_dell_vostro_v130,
>  	},
> +	{
> +		.callback = dmi_matched,
> +		.ident = "Dell XPS13 9333",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS13 9333"),
> +		},
> +		.driver_data = &quirk_dell_xps13_9333,
> +	},
>  	{ }
>  };
>  
> @@ -332,17 +358,29 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
>  	}
>  }
>  
> -static int find_token_location(int tokenid)
> +static int find_token_id(int tokenid)
>  {
>  	int i;
> +
>  	for (i = 0; i < da_num_tokens; i++) {
>  		if (da_tokens[i].tokenID == tokenid)
> -			return da_tokens[i].location;
> +			return i;
>  	}
>  
>  	return -1;
>  }
>  
> +static int find_token_location(int tokenid)
> +{
> +	int id;
> +
> +	id = find_token_id(tokenid);
> +	if (id == -1)
> +		return -1;
> +
> +	return da_tokens[id].location;
> +}
> +
>  static struct calling_interface_buffer *
>  dell_send_request(struct calling_interface_buffer *buffer, int class,
>  		  int select)
> @@ -790,6 +828,956 @@ static void touchpad_led_exit(void)
>  	led_classdev_unregister(&touchpad_led);
>  }
>  
> +/* Derived from information in smbios-keyboard-ctl:
> +
> + cbClass 4
> + cbSelect 11
> + Keyboar illumination
> + cbArg1 determines the function to be performed
> +
> + cbArg1 0x0 = Get Feature Information
> +  cbRES1         Standard return codes (0, -1, -2)
> +  cbRES2, word0  Bitmap of user-selectable modes
> +     bit 0     Always off (All systems)
> +     bit 1     Always on (Travis ATG, Siberia)
> +     bit 2     Auto: ALS-based On; ALS-based Off (Travis ATG)
> +     bit 3     Auto: ALS- and input-activity-based On; input-activity based Off
> +     bit 4     Auto: Input-activity-based On; input-activity based Off
> +     bit 5     Auto: Input-activity-based On (illumination level 25%); input-activity based Off
> +     bit 6     Auto: Input-activity-based On (illumination level 50%); input-activity based Off
> +     bit 7     Auto: Input-activity-based On (illumination level 75%); input-activity based Off
> +     bit 8     Auto: Input-activity-based On (illumination level 100%); input-activity based Off
> +     bits 9-15 Reserved for future use
> +  cbRES2, byte2  Reserved for future use
> +  cbRES2, byte3  Keyboard illumination type
> +     0         Reserved
> +     1         Tasklight
> +     2         Backlight
> +     3-255     Reserved for future use
> +  cbRES3, byte0  Supported auto keyboard illumination trigger bitmap.
> +     bit 0     Any keystroke
> +     bit 1     Touchpad activity
> +     bit 2     Pointing stick
> +     bit 3     Any mouse
> +     bits 4-7  Reserved for future use
> +  cbRES3, byte1  Supported timeout unit bitmap
> +     bit 0     Seconds
> +     bit 1     Minutes
> +     bit 2     Hours
> +     bit 3     Days
> +     bits 4-7  Reserved for future use
> +  cbRES3, byte2  Number of keyboard light brightness levels
> +  cbRES4, byte0  Maximum acceptable seconds value (0 if seconds not supported).
> +  cbRES4, byte1  Maximum acceptable minutes value (0 if minutes not supported).
> +  cbRES4, byte2  Maximum acceptable hours value (0 if hours not supported).
> +  cbRES4, byte3  Maximum acceptable days value (0 if days not supported)
> +
> + cbArg1 0x1 = Get Current State
> +  cbRES1         Standard return codes (0, -1, -2)
> +  cbRES2, word0  Bitmap of current mode state
> +     bit 0     Always off (All systems)
> +     bit 1     Always on (Travis ATG, Siberia)
> +     bit 2     Auto: ALS-based On; ALS-based Off (Travis ATG)
> +     bit 3     Auto: ALS- and input-activity-based On; input-activity based Off
> +     bit 4     Auto: Input-activity-based On; input-activity based Off
> +     bit 5     Auto: Input-activity-based On (illumination level 25%); input-activity based Off
> +     bit 6     Auto: Input-activity-based On (illumination level 50%); input-activity based Off
> +     bit 7     Auto: Input-activity-based On (illumination level 75%); input-activity based Off
> +     bit 8     Auto: Input-activity-based On (illumination level 100%); input-activity based Off
> +     bits 9-15 Reserved for future use
> +     Note: Only One bit can be set
> +  cbRES2, byte2  Currently active auto keyboard illumination triggers.
> +     bit 0     Any keystroke
> +     bit 1     Touchpad activity
> +     bit 2     Pointing stick
> +     bit 3     Any mouse
> +     bits 4-7  Reserved for future use
> +  cbRES2, byte3  Current Timeout
> +     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 byte
> +     are set upon return from the [Get feature information] call.
> +  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.
> +
> + cbArg1 0x2 = Set New State
> +  cbRES1         Standard return codes (0, -1, -2)
> +  cbArg2, word0  Bitmap of current mode state
> +     bit 0     Always off (All systems)
> +     bit 1     Always on (Travis ATG, Siberia)
> +     bit 2     Auto: ALS-based On; ALS-based Off (Travis ATG)
> +     bit 3     Auto: ALS- and input-activity-based On; input-activity based Off
> +     bit 4     Auto: Input-activity-based On; input-activity based Off
> +     bit 5     Auto: Input-activity-based On (illumination level 25%); input-activity based Off
> +     bit 6     Auto: Input-activity-based On (illumination level 50%); input-activity based Off
> +     bit 7     Auto: Input-activity-based On (illumination level 75%); input-activity based Off
> +     bit 8     Auto: Input-activity-based On (illumination level 100%); input-activity based Off
> +     bits 9-15 Reserved for future use
> +     Note: Only One bit can be set
> +  cbArg2, byte2  Desired auto keyboard illumination triggers. Must remain inactive to allow
> +                 keyboard to turn off automatically.
> +     bit 0     Any keystroke
> +     bit 1     Touchpad activity
> +     bit 2     Pointing stick
> +     bit 3     Any mouse
> +     bits 4-7  Reserved for future use
> +  cbArg2, byte3  Desired Timeout
> +     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
> +  cbArg3, byte0  Desired setting of ALS value that turns the light on or off.
> +  cbArg3, byte2  Desired keyboard light level.
> +*/
> +
> +
> +enum kbd_timeout_unit {
> +	KBD_TIMEOUT_SECONDS = 0,
> +	KBD_TIMEOUT_MINUTES,
> +	KBD_TIMEOUT_HOURS,
> +	KBD_TIMEOUT_DAYS,
> +};
> +
> +enum kbd_mode_bit {
> +	KBD_MODE_BIT_OFF = 0,
> +	KBD_MODE_BIT_ON,
> +	KBD_MODE_BIT_ALS,
> +	KBD_MODE_BIT_TRIGGER_ALS,
> +	KBD_MODE_BIT_TRIGGER,
> +	KBD_MODE_BIT_TRIGGER_25,
> +	KBD_MODE_BIT_TRIGGER_50,
> +	KBD_MODE_BIT_TRIGGER_75,
> +	KBD_MODE_BIT_TRIGGER_100,
> +};
> +
> +#define kbd_is_als_mode_bit(bit) \
> +	((bit) == KBD_MODE_BIT_ALS || (bit) == KBD_MODE_BIT_TRIGGER_ALS)
> +#define kbd_is_trigger_mode_bit(bit) \
> +	((bit) >= KBD_MODE_BIT_TRIGGER_ALS && (bit) <= KBD_MODE_BIT_TRIGGER_100)
> +#define kbd_is_level_mode_bit(bit) \
> +	((bit) >= KBD_MODE_BIT_TRIGGER_25 && (bit) <= KBD_MODE_BIT_TRIGGER_100)
> +
> +struct kbd_info {
> +	u16 modes;
> +	u8 type;
> +	u8 triggers;
> +	u8 levels;
> +	u8 seconds;
> +	u8 minutes;
> +	u8 hours;
> +	u8 days;
> +};
> +
> +struct kbd_state {
> +	u8 mode_bit;
> +	u8 triggers;
> +	u8 timeout_value;
> +	u8 timeout_unit;
> +	u8 als_setting;
> +	u8 als_value;
> +	u8 level;
> +};
> +
> +static const int kbd_tokens[] = {
> +	KBD_LED_OFF_TOKEN,
> +	KBD_LED_AUTO_25_TOKEN,
> +	KBD_LED_AUTO_50_TOKEN,
> +	KBD_LED_AUTO_75_TOKEN,
> +	KBD_LED_AUTO_100_TOKEN,
> +	KBD_LED_ON_TOKEN,
> +};
> +
> +static u16 kbd_token_bits;
> +
> +static struct kbd_info kbd_info;
> +static bool kbd_als_supported;
> +static bool kbd_triggers_supported;
> +
> +static u8 kbd_mode_levels[16];
> +static int kbd_mode_levels_count;
> +
> +static u8 kbd_previous_level;
> +static u8 kbd_previous_mode_bit;
> +
> +static bool kbd_led_present;
> +
> +static int kbd_get_info(struct kbd_info *info)
> +{
> +	u8 units;
> +	int ret;
> +
> +	get_buffer();
> +
> +	buffer->input[0] = 0x0;
> +	dell_send_request(buffer, 4, 11);
> +	ret = buffer->output[0];
> +
> +	if (ret == 0) {

Generally speaking, check for error to keep the main logic from getting
indented.

	if (buffer->output[0]) {
		ret = -EINVAL;
		goto out;
	}

> +		info->modes = buffer->output[1] & 0xFFFF;
> +		info->type = (buffer->output[1] >> 24) & 0xFF;
> +		info->triggers = buffer->output[2] & 0xFF;
> +		units = (buffer->output[2] >> 8) & 0xFF;
> +		info->levels = (buffer->output[2] >> 16) & 0xFF;
> +		if (units & BIT(0))
> +			info->seconds = (buffer->output[3] >> 0) & 0xFF;
> +		if (units & BIT(1))
> +			info->minutes = (buffer->output[3] >> 8) & 0xFF;
> +		if (units & BIT(2))
> +			info->hours = (buffer->output[3] >> 16) & 0xFF;
> +		if (units & BIT(3))
> +			info->days = (buffer->output[3] >> 24) & 0xFF;
> +	}
> +

 out:

> +	release_buffer();
> +
> +	if (ret)
> +		return -EINVAL;

Drop this.

> +	return 0;

return ret;

In this particular case, it might be shorter by a line or two to drop the ret
variable and simply release_buffer() and return -EINVAL in the error check above
and just return 0 here.

> +}
> +
> +static unsigned int kbd_get_max_level(void)
> +{
> +	if (kbd_info.levels != 0)
> +		return kbd_info.levels;

This test.... does nothing? if it is 0, you still return 0 below :-)

> +	if (kbd_mode_levels_count > 0)
> +		return kbd_mode_levels_count - 1;
> +	return 0;

So the function is:

return kbd_mode_levels_count > 0 ? kbd_mode_levels_count - 1 : kbd_info.levels;

The if blocks are more legible, so that's fine, but the first one doesn't seem
to do anything and you can replace the final return with return kbd_info.levels.

> +}
> +
> +static int kbd_get_level(struct kbd_state *state)
> +{
> +	int i;
> +
> +	if (kbd_info.levels != 0)
> +		return state->level;
> +
> +	if (kbd_mode_levels_count > 0) {

The if test is redundant, the for loop will simply run 0 iterations in this
case.

> +		for (i = 0; i < kbd_mode_levels_count; ++i)
> +			if (kbd_mode_levels[i] == state->mode_bit)
> +				return i;
> +		return 0;
> +	}
> +
> +	return -EINVAL;

Ah, so you're testing for an invalid kbd_mode_levels_count? Is that possible?
Should that perhaps be an unsigned integer, then we only have to worry about 0.
Can we test for that at driver init? How do we get to here with a bad
kbd_mode_levels_count?

> +}
> +
> +static int kbd_set_level(struct kbd_state *state, u8 level)
> +{
> +	if (kbd_info.levels != 0) {

Again, generally speaking, test for errors and indent those, rather than the
core logic.

> +		if (level != 0)
> +			kbd_previous_level = level;
> +		if (state->level == level)
> +			return 0;
> +		state->level = level;
> +		if (level != 0 && state->mode_bit == KBD_MODE_BIT_OFF)
> +			state->mode_bit = kbd_previous_mode_bit;
> +		else if (level == 0 && state->mode_bit != KBD_MODE_BIT_OFF) {
> +			kbd_previous_mode_bit = state->mode_bit;
> +			state->mode_bit = KBD_MODE_BIT_OFF;
> +		}
> +		return 0;
> +	}
> +
> +	if (kbd_mode_levels_count > 0 && level < kbd_mode_levels_count) {
> +		if (level != 0)
> +			kbd_previous_level = level;
> +		state->mode_bit = kbd_mode_levels[level];
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int kbd_get_state(struct kbd_state *state)
> +{
> +	int ret;
> +
> +	get_buffer();
> +
> +	buffer->input[0] = 0x1;
> +	dell_send_request(buffer, 4, 11);
> +	ret = buffer->output[0];
> +
> +	if (ret == 0) {
> +		state->mode_bit = ffs(buffer->output[1] & 0xFFFF);
> +		if (state->mode_bit != 0)
> +			state->mode_bit--;
> +		state->triggers = (buffer->output[1] >> 16) & 0xFF;
> +		state->timeout_value = (buffer->output[1] >> 24) & 0x3F;
> +		state->timeout_unit = (buffer->output[1] >> 30) & 0x3;
> +		state->als_setting = buffer->output[2] & 0xFF;
> +		state->als_value = (buffer->output[2] >> 8) & 0xFF;
> +		state->level = (buffer->output[2] >> 16) & 0xFF;
> +	}
> +
> +	release_buffer();
> +
> +	if (ret)
> +		return -EINVAL;
> +
> +	return 0;

Same comment about error checking approach and indentation.

> +}
> +
> +static int kbd_set_state(struct kbd_state *state)
> +{
> +	int ret;
> +
> +	get_buffer();
> +	buffer->input[0] = 0x2;
> +	buffer->input[1] = BIT(state->mode_bit) & 0xFFFF;
> +	buffer->input[1] |= (state->triggers & 0xFF) << 16;
> +	buffer->input[1] |= (state->timeout_value & 0x3F) << 24;
> +	buffer->input[1] |= (state->timeout_unit & 0x3) << 30;
> +	buffer->input[2] = state->als_setting & 0xFF;
> +	buffer->input[2] |= (state->level & 0xFF) << 16;
> +	dell_send_request(buffer, 4, 11);
> +	ret = buffer->output[0];
> +	release_buffer();
> +
> +	if (ret)
> +		return -EINVAL;

Also, is EINVAL right here and elsewhere? Or did something fail? EXIO?

> +
> +	return 0;
> +}
> +
> +static int kbd_set_state_safe(struct kbd_state *state, struct kbd_state *old)
> +{
> +	int ret;
> +
> +	ret = kbd_set_state(state);
> +	if (ret == 0)
> +		return 0;
> +
> +	if (kbd_set_state(old))
> +		pr_err("Setting old previous keyboard state failed\n");

And if we got an error and kbd_set_state(old) doesn't return !0, what's the
state of things? Still a failure a presume?

> +
> +	return ret;
> +}
> +
> +static int kbd_set_token_bit(u8 bit)
> +{
> +	int id;
> +	int ret;
> +
> +	if (bit >= ARRAY_SIZE(kbd_tokens))
> +		return -EINVAL;
> +
> +	id = find_token_id(kbd_tokens[bit]);
> +	if (id == -1)
> +		return -EINVAL;
> +
> +	get_buffer();
> +	buffer->input[0] = da_tokens[id].location;
> +	buffer->input[1] = da_tokens[id].value;
> +	dell_send_request(buffer, 1, 0);
> +	ret = buffer->output[0];
> +	release_buffer();
> +
> +	if (ret)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int kbd_get_token_bit(u8 bit)
> +{
> +	int id;
> +	int ret;
> +	int val;
> +
> +	if (bit >= ARRAY_SIZE(kbd_tokens))
> +		return -EINVAL;
> +
> +	id = find_token_id(kbd_tokens[bit]);
> +	if (id == -1)
> +		return -EINVAL;
> +
> +	get_buffer();
> +	buffer->input[0] = da_tokens[id].location;
> +	dell_send_request(buffer, 0, 0);
> +	ret = buffer->output[0];
> +	val = buffer->output[1];
> +	release_buffer();
> +
> +	if (ret)
> +		return -EINVAL;
> +
> +	return (val == da_tokens[id].value);
> +}
> +
> +static int kbd_get_first_active_token_bit(void)
> +{
> +	int i;
> +	int ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(kbd_tokens); ++i) {
> +		ret = kbd_get_token_bit(i);
> +		if (ret == 1)
> +			return i;
> +	}
> +
> +	return ret;
> +}
> +
> +static int kbd_get_valid_token_counts(void)
> +{
> +	return hweight16(kbd_token_bits);
> +}
> +
> +static void kbd_init(void)
> +{
> +	struct kbd_state state;
> +	int ret;
> +	int i;
> +
> +	ret = kbd_get_info(&kbd_info);
> +
> +	if (ret == 0) {
> +

Checking for success, let's invert and avoid the indentation here too.

> +		kbd_get_state(&state);
> +
> +		/* NOTE: timeout value is stored in 6 bits so max value is 63 */
> +		if (kbd_info.seconds > 63)
> +			kbd_info.seconds = 63;
> +		if (kbd_info.minutes > 63)
> +			kbd_info.minutes = 63;
> +		if (kbd_info.hours > 63)
> +			kbd_info.hours = 63;
> +		if (kbd_info.days > 63)
> +			kbd_info.days = 63;
> +
> +		/* NOTE: On tested machines ON mode did not work and caused
> +		 *       problems (turned backlight off) so do not use it
> +		 */
> +		kbd_info.modes &= ~BIT(KBD_MODE_BIT_ON);
> +
> +		kbd_previous_level = kbd_get_level(&state);
> +		kbd_previous_mode_bit = state.mode_bit;
> +
> +		if (kbd_previous_level == 0) {
> +			if (kbd_get_max_level() != 0)
> +				kbd_previous_level = 1;
> +		}
> +
> +		if (kbd_previous_mode_bit == KBD_MODE_BIT_OFF) {
> +			kbd_previous_mode_bit =
> +				ffs(kbd_info.modes & ~BIT(KBD_MODE_BIT_OFF));
> +			if (kbd_previous_mode_bit != 0)
> +				kbd_previous_mode_bit--;
> +		}
> +
> +		if (kbd_info.modes & (BIT(KBD_MODE_BIT_ALS) |
> +				      BIT(KBD_MODE_BIT_TRIGGER_ALS)))
> +			kbd_als_supported = true;
> +
> +		if (kbd_info.modes & (
> +		    BIT(KBD_MODE_BIT_TRIGGER_ALS) | BIT(KBD_MODE_BIT_TRIGGER) |
> +		    BIT(KBD_MODE_BIT_TRIGGER_25) | BIT(KBD_MODE_BIT_TRIGGER_50) |
> +		    BIT(KBD_MODE_BIT_TRIGGER_75) | BIT(KBD_MODE_BIT_TRIGGER_100)
> +		   ))
> +			kbd_triggers_supported = true;
> +
> +		for (i = 0; i < 16; ++i)
> +			if (kbd_is_level_mode_bit(i) &&
> +			    (BIT(i) & kbd_info.modes))
> +				kbd_mode_levels[1+kbd_mode_levels_count++] = i;
> +
> +		if (kbd_mode_levels_count > 0) {
> +			for (i = 0; i < 16; ++i) {
> +				if (BIT(i) & kbd_info.modes) {
> +					kbd_mode_levels[0] = i;
> +					break;
> +				}
> +			}
> +			kbd_mode_levels_count++;
> +		}
> +
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(kbd_tokens); ++i)
> +		if (find_token_id(kbd_tokens[i]) != -1)
> +			kbd_token_bits |= BIT(i);
> +
> +	if (kbd_token_bits != 0 || ret == 0)
> +		kbd_led_present = true;
> +}
> +
> +static ssize_t kbd_led_timeout_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	struct kbd_state state;
> +	struct kbd_state new_state;
> +	int ret;
> +	bool convert;
> +	char ch;
> +	u8 unit;
> +	int value;
> +	int i;

Decreasing line lenth please.

> +
> +	ret = sscanf(buf, "%d %c", &value, &ch);
> +	if (ret < 1)
> +		return -EINVAL;
> +	else if (ret == 1)
> +		ch = 's';
> +
> +	if (value < 0)
> +		return -EINVAL;
> +
> +	convert = false;
> +
> +	switch (ch) {
> +	case 's':
> +		if (value > kbd_info.seconds)
> +			convert = true;
> +		unit = KBD_TIMEOUT_SECONDS;
> +		break;
> +	case 'm':
> +		if (value > kbd_info.minutes)
> +			convert = true;
> +		unit = KBD_TIMEOUT_MINUTES;
> +		break;
> +	case 'h':
> +		if (value > kbd_info.hours)
> +			convert = true;
> +		unit = KBD_TIMEOUT_HOURS;
> +		break;
> +	case 'd':
> +		if (value > kbd_info.days)
> +			convert = true;
> +		unit = KBD_TIMEOUT_DAYS;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (quirks && quirks->kbd_timeouts)
> +		convert = true;
> +
> +	if (convert) {
> +		/* NOTE: this switch fall down */

"fall down" ? As in, it intentionally doesn't have breaks?

> +		switch (unit) {
> +		case KBD_TIMEOUT_DAYS:
> +			value *= 24;
> +		case KBD_TIMEOUT_HOURS:
> +			value *= 60;
> +		case KBD_TIMEOUT_MINUTES:
> +			value *= 60;
> +			unit = KBD_TIMEOUT_SECONDS;
> +	 	}
> +
> +		if (quirks && quirks->kbd_timeouts) {
> +			for (i = 0; quirks->kbd_timeouts[i] != -1; i++) {
> +				if (value <= quirks->kbd_timeouts[i]) {
> +					value = quirks->kbd_timeouts[i];
> +					break;
> +				}
> +			}
> +		}
> +
> +		if (value <= kbd_info.seconds && kbd_info.seconds) {
> +			unit = KBD_TIMEOUT_SECONDS;
> +		} else if (value/60 <= kbd_info.minutes && kbd_info.minutes) {

One space around operators like / and * please, applies throughout.

> +			value /= 60;
> +			unit = KBD_TIMEOUT_MINUTES;
> +		} else if (value/(60*60) <= kbd_info.hours && kbd_info.hours) {
> +			value /= (60*60);
> +			unit = KBD_TIMEOUT_HOURS;
> +		} else if (value/(60*60*24) <= kbd_info.days && kbd_info.days) {
> +			value /= (60*60*24);
> +			unit = KBD_TIMEOUT_DAYS;
> +		} else {
> +			return -EINVAL;
> +		}
> +	}
> +
> +	ret = kbd_get_state(&state);
> +	if (ret)
> +		return ret;
> +
> +	new_state = state;
> +	new_state.timeout_value = value;
> +	new_state.timeout_unit = unit;
> +
> +	ret = kbd_set_state_safe(&new_state, &state);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static ssize_t kbd_led_timeout_show(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	struct kbd_state state;
> +	int ret;
> +	int len;
> +
> +	ret = kbd_get_state(&state);
> +	if (ret)
> +		return ret;
> +
> +	len = sprintf(buf, "%d", state.timeout_value);
> +
> +	switch (state.timeout_unit) {
> +	case KBD_TIMEOUT_SECONDS:
> +		return len + sprintf(buf+len, "s\n");
> +	case KBD_TIMEOUT_MINUTES:
> +		return len + sprintf(buf+len, "m\n");
> +	case KBD_TIMEOUT_HOURS:
> +		return len + sprintf(buf+len, "h\n");
> +	case KBD_TIMEOUT_DAYS:
> +		return len + sprintf(buf+len, "d\n");
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return len;
> +}
> +
> +static DEVICE_ATTR(stop_timeout, S_IRUGO | S_IWUSR,
> +		   kbd_led_timeout_show, kbd_led_timeout_store);
> +
> +static const char * const kbd_led_triggers[] = {
> +	"keyboard",
> +	"touchpad",
> +	/*"trackstick"*/ NULL, /* NOTE: trackstick is just alias for touchpad */
> +	"mouse",
> +};
> +
> +static ssize_t kbd_led_triggers_store(struct device *dev,
> +				      struct device_attribute *attr,
> +				      const char *buf, size_t count)
> +{
> +	struct kbd_state state;
> +	struct kbd_state new_state;
> +	char trigger[21];
> +	bool als_enabled;
> +	bool triggers_enabled;
> +	bool enable_als;
> +	bool disable_als;
> +	int trigger_bit;
> +	int i;
> +	int ret;
> +
> +	ret = sscanf(buf, "%20s", trigger);
> +	if (ret != 1)
> +		return -EINVAL;
> +
> +	if (trigger[0] != '+' && trigger[0] != '-')
> +		return -EINVAL;
> +
> +	ret = kbd_get_state(&state);
> +	if (ret)
> +		return ret;
> +
> +	if (kbd_als_supported)
> +		als_enabled = kbd_is_als_mode_bit(state.mode_bit);
> +	else
> +		als_enabled = false;
> +
> +	if (kbd_triggers_supported)
> +		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
> +	else
> +		triggers_enabled = false;

Could skip the else blocks with initial assignments.

> +
> +	enable_als = false;
> +	disable_als = false;

Can skip these too.

> +
> +	if (kbd_als_supported) {
> +		if (strcmp(trigger, "+als") == 0) {
> +			if (als_enabled)
> +				return count;
> +			enable_als = true;
> +		} else if (strcmp(trigger, "-als") == 0) {
> +			if (!als_enabled)
> +				return count;
> +			disable_als = true;
> +		}
> +	}
> +
> +	if (enable_als || disable_als) {
> +		new_state = state;
> +		if (enable_als) {
> +			if (triggers_enabled)
> +				new_state.mode_bit = KBD_MODE_BIT_TRIGGER_ALS;
> +			else
> +				new_state.mode_bit = KBD_MODE_BIT_ALS;
> +		} else {
> +			if (triggers_enabled) {
> +				new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
> +				kbd_set_level(&new_state, kbd_previous_level);
> +			} else
> +				new_state.mode_bit = KBD_MODE_BIT_ON;

if one block needs braces, use them throughout.
Apply throughout.

> +		}
> +		if (!(kbd_info.modes & BIT(new_state.mode_bit)))
> +			return -EINVAL;
> +		ret = kbd_set_state_safe(&new_state, &state);
> +		if (ret)
> +			return ret;
> +		kbd_previous_mode_bit = new_state.mode_bit;
> +		return count;
> +	}
> +
> +	trigger_bit = -1;
> +
> +	if (kbd_triggers_supported) {
> +		for (i = 0; i < ARRAY_SIZE(kbd_led_triggers); ++i) {
> +			if (!(kbd_info.triggers & BIT(i)))
> +				continue;
> +			if (!kbd_led_triggers[i])
> +				continue;
> +			if (strcmp(trigger+1, kbd_led_triggers[i]) != 0)
> +				continue;
> +			if (trigger[0] == '+' &&
> +			    triggers_enabled && (state.triggers & BIT(i)))
> +				return count;
> +			if (trigger[0] == '-' &&
> +			    (!triggers_enabled || !(state.triggers & BIT(i))))
> +				return count;
> +			trigger_bit = i;
> +			break;
> +		}
> +	}
> +
> +	if (trigger_bit != -1) {
> +		new_state = state;
> +		if (trigger[0] == '+')
> +			new_state.triggers |= BIT(trigger_bit);
> +		else {
> +			new_state.triggers &= ~BIT(trigger_bit);
> +			/* NOTE: trackstick bit (2) must be disabled when
> +			 *       disabling touchpad bit (1), otherwise touchpad
> +			 *       bit (1) will not be disabled */
> +			if (trigger_bit == 1)
> +				new_state.triggers &= ~BIT(2);
> +		}
> +		if ((kbd_info.triggers & new_state.triggers) !=
> +		    new_state.triggers)
> +			return -EINVAL;
> +		if (new_state.triggers && !triggers_enabled) {
> +			if (als_enabled)
> +				new_state.mode_bit = KBD_MODE_BIT_TRIGGER_ALS;
> +			else {
> +				new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
> +				kbd_set_level(&new_state, kbd_previous_level);
> +			}
> +		} else if (new_state.triggers == 0) {
> +			if (als_enabled)
> +				new_state.mode_bit = KBD_MODE_BIT_ALS;
> +			else
> +				kbd_set_level(&new_state, 0);
> +		}
> +		if (!(kbd_info.modes & BIT(new_state.mode_bit)))
> +			return -EINVAL;
> +		ret = kbd_set_state_safe(&new_state, &state);
> +		if (ret)
> +			return ret;
> +		if (new_state.mode_bit != KBD_MODE_BIT_OFF)
> +			kbd_previous_mode_bit = new_state.mode_bit;
> +		return count;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static ssize_t kbd_led_triggers_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	struct kbd_state state;
> +	bool triggers_enabled;
> +	int level;
> +	int i;
> +	int ret;
> +	int len;
> +
> +	ret = kbd_get_state(&state);
> +	if (ret)
> +		return ret;
> +
> +	len = 0;
> +
> +	if (kbd_triggers_supported) {
> +		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
> +		level = kbd_get_level(&state);
> +		for (i = 0; i < ARRAY_SIZE(kbd_led_triggers); ++i) {
> +			if (!(kbd_info.triggers & BIT(i)))
> +				continue;
> +			if (!kbd_led_triggers[i])
> +				continue;
> +			if ((triggers_enabled || level <= 0) &&
> +			    (state.triggers & BIT(i)))
> +				buf[len++] = '+';
> +			else
> +				buf[len++] = '-';
> +			len += sprintf(buf+len, "%s ", kbd_led_triggers[i]);
> +		}
> +	}
> +
> +	if (kbd_als_supported) {
> +		if (kbd_is_als_mode_bit(state.mode_bit))
> +			len += sprintf(buf+len, "+als ");
> +		else
> +			len += sprintf(buf+len, "-als ");
> +	}
> +
> +	if (len)
> +		buf[len - 1] = '\n';
> +
> +	return len;
> +}
> +
> +static DEVICE_ATTR(start_triggers, S_IRUGO | S_IWUSR,
> +		   kbd_led_triggers_show, kbd_led_triggers_store);
> +
> +static ssize_t kbd_led_als_store(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	struct kbd_state state;
> +	struct kbd_state new_state;
> +	u8 setting;
> +	int ret;
> +
> +	ret = kstrtou8(buf, 10, &setting);
> +	if (ret)
> +		return ret;
> +
> +	ret = kbd_get_state(&state);
> +	if (ret)
> +		return ret;
> +
> +	new_state = state;
> +	new_state.als_setting = setting;
> +
> +	ret = kbd_set_state_safe(&new_state, &state);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static ssize_t kbd_led_als_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct kbd_state state;
> +	int ret;
> +
> +	ret = kbd_get_state(&state);
> +	if (ret)
> +		return ret;
> +
> +	return sprintf(buf, "%d\n", state.als_setting);
> +}
> +
> +static DEVICE_ATTR(als_setting, S_IRUGO | S_IWUSR,
> +		   kbd_led_als_show, kbd_led_als_store);
> +
> +static struct attribute *kbd_led_attrs[] = {
> +	&dev_attr_stop_timeout.attr,
> +	&dev_attr_start_triggers.attr,
> +	&dev_attr_als_setting.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(kbd_led);
> +
> +static enum led_brightness kbd_led_level_get(struct led_classdev *led_cdev)
> +{
> +	int ret;
> +	u16 num;
> +	struct kbd_state state;
> +
> +	if (kbd_get_max_level()) {
> +		ret = kbd_get_state(&state);
> +		if (ret)
> +			return 0;
> +		ret = kbd_get_level(&state);
> +		if (ret < 0)
> +			return 0;
> +		return ret;
> +	} else if (kbd_get_valid_token_counts()) {
> +		ret = kbd_get_first_active_token_bit();
> +		if (ret < 0)
> +			return 0;
> +		for (num = kbd_token_bits; num != 0 && ret > 0; --ret)
> +			num &= num - 1; /* clear the first bit set */
> +		if (num == 0)
> +			return 0;
> +		return ffs(num) - 1;
> +	} else {
> +		pr_warn("Keyboard brightness level control not supported\n");
> +		return 0;
> +	}

You can drop the else blocks from the above as every case returns explicitly.

if (condA)
	return retA;
if (condB)
	return retB
return ret

(checkpatch.pl catches this)

> +}
> +
> +static void kbd_led_level_set(struct led_classdev *led_cdev,
> +			      enum led_brightness value)
> +{
> +	struct kbd_state state;
> +	struct kbd_state new_state;
> +	u16 num;
> +
> +	if (kbd_get_max_level()) {
> +		if (kbd_get_state(&state))
> +			return;
> +		new_state = state;
> +		if (kbd_set_level(&new_state, value))
> +			return;
> +		kbd_set_state_safe(&new_state, &state);
> +	} else if (kbd_get_valid_token_counts()) {
> +		for (num = kbd_token_bits; num != 0 && value > 0; --value)
> +			num &= num - 1; /* clear the first bit set */
> +		if (num == 0)
> +			return;
> +		kbd_set_token_bit(ffs(num) - 1);
> +	} else {
> +		pr_warn("Keyboard brightness level control not supported\n");
> +	}
> +}
> +
> +static struct led_classdev kbd_led = {
> +	.name           = "dell::kbd_backlight",
> +	.brightness_set = kbd_led_level_set,
> +	.brightness_get = kbd_led_level_get,
> +	.groups         = kbd_led_groups,
> +};
> +
> +static int __init kbd_led_init(struct device *dev)
> +{
> +	kbd_init();
> +	if (!kbd_led_present)
> +		return -ENODEV;
> +	kbd_led.max_brightness = kbd_get_max_level();
> +	if (!kbd_led.max_brightness) {
> +		kbd_led.max_brightness = kbd_get_valid_token_counts();
> +		if (kbd_led.max_brightness)
> +			kbd_led.max_brightness--;
> +	}
> +	return led_classdev_register(dev, &kbd_led);
> +}
> +
> +static void brightness_set_exit(struct led_classdev *led_cdev,
> +				enum led_brightness value)
> +{
> +	/* Don't change backlight level on exit */
> +};
> +
> +static void kbd_led_exit(void)
> +{
> +	if (!kbd_led_present)
> +		return;
> +	kbd_led.brightness_set = brightness_set_exit;
> +	led_classdev_unregister(&kbd_led);
> +}
> +
>  static int __init dell_init(void)
>  {
>  	int max_intensity = 0;
> @@ -842,6 +1830,8 @@ static int __init dell_init(void)
>  	if (quirks && quirks->touchpad_led)
>  		touchpad_led_init(&platform_device->dev);
>  
> +	kbd_led_init(&platform_device->dev);
> +
>  	dell_laptop_dir = debugfs_create_dir("dell_laptop", NULL);
>  	if (dell_laptop_dir != NULL)
>  		debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
> @@ -909,6 +1899,7 @@ static void __exit dell_exit(void)
>  	debugfs_remove_recursive(dell_laptop_dir);
>  	if (quirks && quirks->touchpad_led)
>  		touchpad_led_exit();
> +	kbd_led_exit();
>  	i8042_remove_filter(dell_laptop_i8042_filter);
>  	cancel_delayed_work_sync(&dell_rfkill_work);
>  	backlight_device_unregister(dell_backlight_device);
> @@ -925,5 +1916,7 @@ module_init(dell_init);
>  module_exit(dell_exit);
>  
>  MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
> +MODULE_AUTHOR("Gabriele Mazzotta <gabriele.mzt@gmail.com>");
> +MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
>  MODULE_DESCRIPTION("Dell laptop driver");
>  MODULE_LICENSE("GPL");
> -- 
> 1.7.9.5
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight

[Matthew Garrett]

On Wed, Nov 19, 2014 at 10:34:16AM -0800, Darren Hart wrote:

> I'm somewhat concerned that this patch doubles the size of this driver. When
> we're adding this much code, I have to ask - does it make sense to grow this
> driver rather than create a new one?

There'd be a fair amount of code duplication in splitting it. 

> There is no ACPI backlight driver on these systems? We need a platform driver?

ACPI doesn't specify keyboard backlight control, so this ends up being 
very vendor specific.
 
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 960 bytes --]

On Wednesday 19 November 2014 20:23:36 Matthew Garrett wrote:
> On Wed, Nov 19, 2014 at 10:34:16AM -0800, Darren Hart wrote:
> > I'm somewhat concerned that this patch doubles the size of
> > this driver. When we're adding this much code, I have to
> > ask - does it make sense to grow this driver rather than
> > create a new one?
> 
> There'd be a fair amount of code duplication in splitting it.
> 

Yes. dell-laptop.ko implements functions for doing Dell SMBIOS 
calls which are needed for keyboard backlight.

> > There is no ACPI backlight driver on these systems? We need
> > a platform driver?
> 
> ACPI doesn't specify keyboard backlight control, so this ends
> up being very vendor specific.

dell-laptop.ko is not ACPI driver. It is using Dell SMBIOS calls. 
And I do not know about Dell specific ACPI interface for keyboard 
backlight. So Darren, ask this question someone from Dell.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight

[Darren Hart]

On Wed, Nov 19, 2014 at 08:51:28PM +0100, Pali Rohár wrote:
> On Wednesday 19 November 2014 20:23:36 Matthew Garrett wrote:
> > On Wed, Nov 19, 2014 at 10:34:16AM -0800, Darren Hart wrote:
> > > I'm somewhat concerned that this patch doubles the size of
> > > this driver. When we're adding this much code, I have to
> > > ask - does it make sense to grow this driver rather than
> > > create a new one?
> > 
> > There'd be a fair amount of code duplication in splitting it.
> > 
> 
> Yes. dell-laptop.ko implements functions for doing Dell SMBIOS 
> calls which are needed for keyboard backlight.
> 
> > > There is no ACPI backlight driver on these systems? We need
> > > a platform driver?
> > 
> > ACPI doesn't specify keyboard backlight control, so this ends
> > up being very vendor specific.
> 
> dell-laptop.ko is not ACPI driver. It is using Dell SMBIOS calls. 
> And I do not know about Dell specific ACPI interface for keyboard 
> backlight. So Darren, ask this question someone from Dell.

Right, I asked because there is a block in the original driver to defer
to an ACPI backlight driver if it exists. I understand this is a
keyboard backlight driver and is different, but I wanted to check.

So let's go ahead and move forward with dell-laptop.c being the right
place for this addition.

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 8514 bytes --]

Hello,

I removed other lines so mail is not too long.

On Wednesday 19 November 2014 19:34:16 Darren Hart wrote:
> > +static int kbd_get_info(struct kbd_info *info)
> > +{
> > +	u8 units;
> > +	int ret;
> > +
> > +	get_buffer();
> > +
> > +	buffer->input[0] = 0x0;
> > +	dell_send_request(buffer, 4, 11);
> > +	ret = buffer->output[0];
> > +
> > +	if (ret == 0) {
> 
> Generally speaking, check for error to keep the main logic
> from getting indented.
> 
> 	if (buffer->output[0]) {
> 		ret = -EINVAL;
> 		goto out;
> 	}
> 
> > +		info->modes = buffer->output[1] & 0xFFFF;
> > +		info->type = (buffer->output[1] >> 24) & 0xFF;
> > +		info->triggers = buffer->output[2] & 0xFF;
> > +		units = (buffer->output[2] >> 8) & 0xFF;
> > +		info->levels = (buffer->output[2] >> 16) & 0xFF;
> > +		if (units & BIT(0))
> > +			info->seconds = (buffer->output[3] >> 0) & 0xFF;
> > +		if (units & BIT(1))
> > +			info->minutes = (buffer->output[3] >> 8) & 0xFF;
> > +		if (units & BIT(2))
> > +			info->hours = (buffer->output[3] >> 16) & 0xFF;
> > +		if (units & BIT(3))
> > +			info->days = (buffer->output[3] >> 24) & 0xFF;
> > +	}
> > +
> 
>  out:
> > +	release_buffer();
> > +
> > +	if (ret)
> > +		return -EINVAL;
> 
> Drop this.
> 
> > +	return 0;
> 
> return ret;
> 
> In this particular case, it might be shorter by a line or two
> to drop the ret variable and simply release_buffer() and
> return -EINVAL in the error check above and just return 0
> here.
> 

Ok. But somewhere it is not possible.

> > +}
> > +
> > +static unsigned int kbd_get_max_level(void)
> > +{
> > +	if (kbd_info.levels != 0)
> > +		return kbd_info.levels;
> 
> This test.... does nothing? if it is 0, you still return 0
> below :-)
> 
> > +	if (kbd_mode_levels_count > 0)
> > +		return kbd_mode_levels_count - 1;
> > +	return 0;
> 
> So the function is:
> 
> return kbd_mode_levels_count > 0 ? kbd_mode_levels_count - 1 :
> kbd_info.levels;
> 
> The if blocks are more legible, so that's fine, but the first
> one doesn't seem to do anything and you can replace the final
> return with return kbd_info.levels.
> 

There are two main way for setting keyboard backlight level. One 
is setting level register and other one is setting special 
keyboard mode. And some dell laptops support only first and some 
only second. So this code choose first available/valid method and 
then return correct data. I'm not sure if those two methods are 
only one and also do not know if in future there will be 
something other. Because of this I use code pattern:

if (check_method_1) return value_method_1;
if (check_method_2) return value_method_2;
...
return unsupported;

Same pattern logic is used in all functions which doing something 
with keyboard backlight level. (I will not other functions).

> > +static int kbd_set_state(struct kbd_state *state)
> > +{
> > +	int ret;
> > +
> > +	get_buffer();
> > +	buffer->input[0] = 0x2;
> > +	buffer->input[1] = BIT(state->mode_bit) & 0xFFFF;
> > +	buffer->input[1] |= (state->triggers & 0xFF) << 16;
> > +	buffer->input[1] |= (state->timeout_value & 0x3F) << 24;
> > +	buffer->input[1] |= (state->timeout_unit & 0x3) << 30;
> > +	buffer->input[2] = state->als_setting & 0xFF;
> > +	buffer->input[2] |= (state->level & 0xFF) << 16;
> > +	dell_send_request(buffer, 4, 11);
> > +	ret = buffer->output[0];
> > +	release_buffer();
> > +
> > +	if (ret)
> > +		return -EINVAL;
> 
> Also, is EINVAL right here and elsewhere? Or did something
> fail? EXIO?
> 

According to Dell documentation, return value is stored in 
buffer->output[0] and can be:

0   Completed successfully
-1  Completed with error
-2  Function not supported

So we can return something other too (not always -EINVAL). Do you 
have any idea which errno should we return for -1 and -2?

> > +
> > +	return 0;
> > +}
> > +
> > +static int kbd_set_state_safe(struct kbd_state *state,
> > struct kbd_state *old) +{
> > +	int ret;
> > +
> > +	ret = kbd_set_state(state);
> > +	if (ret == 0)
> > +		return 0;
> > +
> > +	if (kbd_set_state(old))
> > +		pr_err("Setting old previous keyboard state 
failed\n");
> 
> And if we got an error and kbd_set_state(old) doesn't return
> !0, what's the state of things? Still a failure a presume?
> 

In some cases some laptops do not have to support everything. And 
when I (and Gabriele too) tried to set something unsupported Dell 
BIOS just resetted all settings to some default values. So this 
function try to set new state and if it fails then it try to 
restore previous settings.

> > +
> > +	return ret;
> > +}

> > +static void kbd_init(void)
> > +{
> > +	struct kbd_state state;
> > +	int ret;
> > +	int i;
> > +
> > +	ret = kbd_get_info(&kbd_info);
> > +
> > +	if (ret == 0) {
> > +
> 
> Checking for success, let's invert and avoid the indentation
> here too.
> 

Again this is hard and not possible. This function first process 
data from kbd_get_info (if does not fail), then process data for 
kbd_tokens (via function find_token_id) and then set 
kbd_led_present to true if at least kbd_get_info or kbd_tokens 
succed.

> > +		....
> > +
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(kbd_tokens); ++i)
> > +		if (find_token_id(kbd_tokens[i]) != -1)
> > +			kbd_token_bits |= BIT(i);
> > +
> > +	if (kbd_token_bits != 0 || ret == 0)
> > +		kbd_led_present = true;
> > +}
> > +
> > +static ssize_t kbd_led_timeout_store(struct device *dev,
> > +				     struct device_attribute *attr,
> > +				     const char *buf, size_t count)
> > +{
> > +	struct kbd_state state;
> > +	struct kbd_state new_state;
> > +	int ret;
> > +	bool convert;
> > +	char ch;
> > +	u8 unit;
> > +	int value;
> > +	int i;
> 
> Decreasing line lenth please.
> 

What do you mean?

> > +	if (convert) {
> > +		/* NOTE: this switch fall down */
> 
> "fall down" ? As in, it intentionally doesn't have breaks?
> 

This code convert "value" in "units" to new value in minutes 
units. So for unit == days it is: 24*60*60... So no breaks.

> > +		switch (unit) {
> > +		case KBD_TIMEOUT_DAYS:
> > +			value *= 24;
> > +		case KBD_TIMEOUT_HOURS:
> > +			value *= 60;
> > +		case KBD_TIMEOUT_MINUTES:
> > +			value *= 60;
> > +			unit = KBD_TIMEOUT_SECONDS;
> > +	 	}
> > +
> > +		if (value <= kbd_info.seconds && kbd_info.seconds) {
> > +			unit = KBD_TIMEOUT_SECONDS;
> > +		} else if (value/60 <= kbd_info.minutes &&
> > kbd_info.minutes) {
> 
> One space around operators like / and * please, applies
> throughout.
> 

Ok.

> > +	if (kbd_als_supported)
> > +		als_enabled = kbd_is_als_mode_bit(state.mode_bit);
> > +	else
> > +		als_enabled = false;
> > +
> > +	if (kbd_triggers_supported)
> > +		triggers_enabled =
> > kbd_is_trigger_mode_bit(state.mode_bit); +	else
> > +		triggers_enabled = false;
> 
> Could skip the else blocks with initial assignments.
> 

Ok.

> > +
> > +	enable_als = false;
> > +	disable_als = false;
> 
> Can skip these too.
> 

Ok.

> > +			if (triggers_enabled) {
> > +				new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
> > +				kbd_set_level(&new_state, kbd_previous_level);
> > +			} else
> > +				new_state.mode_bit = KBD_MODE_BIT_ON;
> 
> if one block needs braces, use them throughout.
> Apply throughout.
> 

Ok.

> > +static enum led_brightness kbd_led_level_get(struct
> > led_classdev *led_cdev) +{
> > +	int ret;
> > +	u16 num;
> > +	struct kbd_state state;
> > +
> > +	if (kbd_get_max_level()) {
> > +		ret = kbd_get_state(&state);
> > +		if (ret)
> > +			return 0;
> > +		ret = kbd_get_level(&state);
> > +		if (ret < 0)
> > +			return 0;
> > +		return ret;
> > +	} else if (kbd_get_valid_token_counts()) {
> > +		ret = kbd_get_first_active_token_bit();
> > +		if (ret < 0)
> > +			return 0;
> > +		for (num = kbd_token_bits; num != 0 && ret > 0; --ret)
> > +			num &= num - 1; /* clear the first bit set */
> > +		if (num == 0)
> > +			return 0;
> > +		return ffs(num) - 1;
> > +	} else {
> > +		pr_warn("Keyboard brightness level control not
> > supported\n"); +		return 0;
> > +	}
> 
> You can drop the else blocks from the above as every case
> returns explicitly.
> 
> if (condA)
> 	return retA;
> if (condB)
> 	return retB
> return ret
> 
> (checkpatch.pl catches this)
> 

Ok, this is possible. I will change it.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight

[Darren Hart]

On Wed, Nov 19, 2014 at 09:41:20PM +0100, Pali Rohár wrote:
> Hello,

Hi Pali,

> 
> I removed other lines so mail is not too long.
> 
> On Wednesday 19 November 2014 19:34:16 Darren Hart wrote:
...
> > > +}
> > > +
> > > +static unsigned int kbd_get_max_level(void)
> > > +{
> > > +	if (kbd_info.levels != 0)
> > > +		return kbd_info.levels;
> > 
> > This test.... does nothing? if it is 0, you still return 0
> > below :-)
> > 
> > > +	if (kbd_mode_levels_count > 0)
> > > +		return kbd_mode_levels_count - 1;
> > > +	return 0;
> > 
> > So the function is:
> > 
> > return kbd_mode_levels_count > 0 ? kbd_mode_levels_count - 1 :
> > kbd_info.levels;
> > 
> > The if blocks are more legible, so that's fine, but the first
> > one doesn't seem to do anything and you can replace the final
> > return with return kbd_info.levels.
> > 
> 
> There are two main way for setting keyboard backlight level. One 
> is setting level register and other one is setting special 
> keyboard mode. And some dell laptops support only first and some 
> only second. So this code choose first available/valid method and 
> then return correct data. I'm not sure if those two methods are 
> only one and also do not know if in future there will be 
> something other. Because of this I use code pattern:
> 
> if (check_method_1) return value_method_1;
> if (check_method_2) return value_method_2;
> ...
> return unsupported;
> 
> Same pattern logic is used in all functions which doing something 
> with keyboard backlight level. (I will not other functions).

Fair enough. Clear, legible, consistent coding is preferable to a few micro
optimizations that the compiler is likely to catch anyway. I withdraw the
comment :-)

> 
> > > +static int kbd_set_state(struct kbd_state *state)
> > > +{
> > > +	int ret;
> > > +
> > > +	get_buffer();
> > > +	buffer->input[0] = 0x2;
> > > +	buffer->input[1] = BIT(state->mode_bit) & 0xFFFF;
> > > +	buffer->input[1] |= (state->triggers & 0xFF) << 16;
> > > +	buffer->input[1] |= (state->timeout_value & 0x3F) << 24;
> > > +	buffer->input[1] |= (state->timeout_unit & 0x3) << 30;
> > > +	buffer->input[2] = state->als_setting & 0xFF;
> > > +	buffer->input[2] |= (state->level & 0xFF) << 16;
> > > +	dell_send_request(buffer, 4, 11);
> > > +	ret = buffer->output[0];
> > > +	release_buffer();
> > > +
> > > +	if (ret)
> > > +		return -EINVAL;
> > 
> > Also, is EINVAL right here and elsewhere? Or did something
> > fail? EXIO?
> > 
> 
> According to Dell documentation, return value is stored in 
> buffer->output[0] and can be:
> 
> 0   Completed successfully
> -1  Completed with error
> -2  Function not supported
> 
> So we can return something other too (not always -EINVAL). Do you 
> have any idea which errno should we return for -1 and -2?

For -1, I should think  -EIO (I/O Error)
For -2, I'd expect -ENXIO (No such device or address)

Cc Rafael, Mika, linux-acpi in case they have a better idea.

> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int kbd_set_state_safe(struct kbd_state *state,
> > > struct kbd_state *old) +{
> > > +	int ret;
> > > +
> > > +	ret = kbd_set_state(state);
> > > +	if (ret == 0)
> > > +		return 0;
> > > +
> > > +	if (kbd_set_state(old))
> > > +		pr_err("Setting old previous keyboard state 
> failed\n");
> > 
> > And if we got an error and kbd_set_state(old) doesn't return
> > !0, what's the state of things? Still a failure a presume?
> > 
> 
> In some cases some laptops do not have to support everything. And 
> when I (and Gabriele too) tried to set something unsupported Dell 
> BIOS just resetted all settings to some default values. So this 
> function try to set new state and if it fails then it try to 
> restore previous settings.

OK, that deserves a comment then as the rationale isn't obvious.

> 
> > > +
> > > +	return ret;
> > > +}
> 
> > > +static void kbd_init(void)
> > > +{
> > > +	struct kbd_state state;
> > > +	int ret;
> > > +	int i;
> > > +
> > > +	ret = kbd_get_info(&kbd_info);
> > > +
> > > +	if (ret == 0) {
> > > +
> > 
> > Checking for success, let's invert and avoid the indentation
> > here too.
> > 
> 
> Again this is hard and not possible. This function first process 
> data from kbd_get_info (if does not fail), then process data for 
> kbd_tokens (via function find_token_id) and then set 
> kbd_led_present to true if at least kbd_get_info or kbd_tokens 
> succed.

The goal here is to avoid more than 3 levels of indentations for improved
legibility. Often there are logical inversions and such we can make to
accomplish this. When that fails, we break things up into functions, static
inlines, etc.

For reference:
https://lkml.org/lkml/2007/6/15/449

Which elaborates on CodingStyle Chapter 1: Indentation a bit.

...
> > > +static ssize_t kbd_led_timeout_store(struct device *dev,
> > > +				     struct device_attribute *attr,
> > > +				     const char *buf, size_t count)
> > > +{
> > > +	struct kbd_state state;
> > > +	struct kbd_state new_state;
> > > +	int ret;
> > > +	bool convert;
> > > +	char ch;
> > > +	u8 unit;
> > > +	int value;
> > > +	int i;
> > 
> > Decreasing line lenth please.
> > 
> 
> What do you mean?

This is a nit, but one other maintainers have imposed on me, as a means to
improve legibility. The preference is to declare variables in decreasing line
length, longest to shortest:

struct kbd_state new_state;
struct kbd_state state;
bool convert;
int value;
u8 unit;
char ch;
int ret;
int i;

This is a generalization and sometimes there are good reasons for doing
something else, such as logical groupings for commenting groups, etc. But since
this list appeared mostly random, defaulting to decreasing line length is
preferred.

> 
> > > +	if (convert) {
> > > +		/* NOTE: this switch fall down */
> > 
> > "fall down" ? As in, it intentionally doesn't have breaks?
> > 
> 
> This code convert "value" in "units" to new value in minutes 
> units. So for unit == days it is: 24*60*60... So no breaks.
> 

Right, so the language of the comment just wasn't clear, try:

/* Convert value from seconds to minutes */

> > > +		switch (unit) {
> > > +		case KBD_TIMEOUT_DAYS:
> > > +			value *= 24;
> > > +		case KBD_TIMEOUT_HOURS:
> > > +			value *= 60;
> > > +		case KBD_TIMEOUT_MINUTES:
> > > +			value *= 60;
> > > +			unit = KBD_TIMEOUT_SECONDS;
> > > +	 	}
> > > +

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 7359 bytes --]

On Friday 21 November 2014 21:39:30 Darren Hart wrote:
> On Wed, Nov 19, 2014 at 09:41:20PM +0100, Pali Rohár wrote:
> > Hello,
> 
> Hi Pali,
> 
> > I removed other lines so mail is not too long.
> 
> > On Wednesday 19 November 2014 19:34:16 Darren Hart wrote:
> ...
> 
> > > > +}
> > > > +
> > > > +static unsigned int kbd_get_max_level(void)
> > > > +{
> > > > +	if (kbd_info.levels != 0)
> > > > +		return kbd_info.levels;
> > > 
> > > This test.... does nothing? if it is 0, you still return 0
> > > below :-)
> > > 
> > > > +	if (kbd_mode_levels_count > 0)
> > > > +		return kbd_mode_levels_count - 1;
> > > > +	return 0;
> > > 
> > > So the function is:
> > > 
> > > return kbd_mode_levels_count > 0 ? kbd_mode_levels_count -
> > > 1 : kbd_info.levels;
> > > 
> > > The if blocks are more legible, so that's fine, but the
> > > first one doesn't seem to do anything and you can replace
> > > the final return with return kbd_info.levels.
> > 
> > There are two main way for setting keyboard backlight level.
> > One is setting level register and other one is setting
> > special keyboard mode. And some dell laptops support only
> > first and some only second. So this code choose first
> > available/valid method and then return correct data. I'm
> > not sure if those two methods are only one and also do not
> > know if in future there will be something other. Because of
> > this I use code pattern:
> > 
> > if (check_method_1) return value_method_1;
> > if (check_method_2) return value_method_2;
> > ...
> > return unsupported;
> > 
> > Same pattern logic is used in all functions which doing
> > something with keyboard backlight level. (I will not other
> > functions).
> 
> Fair enough. Clear, legible, consistent coding is preferable
> to a few micro optimizations that the compiler is likely to
> catch anyway. I withdraw the comment :-)
> 

Ok.

> > > > +static int kbd_set_state(struct kbd_state *state)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	get_buffer();
> > > > +	buffer->input[0] = 0x2;
> > > > +	buffer->input[1] = BIT(state->mode_bit) & 0xFFFF;
> > > > +	buffer->input[1] |= (state->triggers & 0xFF) << 16;
> > > > +	buffer->input[1] |= (state->timeout_value & 0x3F) <<
> > > > 24; +	buffer->input[1] |= (state->timeout_unit & 0x3)
> > > > << 30; +	buffer->input[2] = state->als_setting & 0xFF;
> > > > +	buffer->input[2] |= (state->level & 0xFF) << 16;
> > > > +	dell_send_request(buffer, 4, 11);
> > > > +	ret = buffer->output[0];
> > > > +	release_buffer();
> > > > +
> > > > +	if (ret)
> > > > +		return -EINVAL;
> > > 
> > > Also, is EINVAL right here and elsewhere? Or did something
> > > fail? EXIO?
> > 
> > According to Dell documentation, return value is stored in
> > buffer->output[0] and can be:
> > 
> > 0   Completed successfully
> > -1  Completed with error
> > -2  Function not supported
> > 
> > So we can return something other too (not always -EINVAL).
> > Do you have any idea which errno should we return for -1
> > and -2?
> 
> For -1, I should think  -EIO (I/O Error)
> For -2, I'd expect -ENXIO (No such device or address)
> 

What about -ENOSYS for -2?

> Cc Rafael, Mika, linux-acpi in case they have a better idea.
> 
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int kbd_set_state_safe(struct kbd_state *state,
> > > > struct kbd_state *old) +{
> > > > +	int ret;
> > > > +
> > > > +	ret = kbd_set_state(state);
> > > > +	if (ret == 0)
> > > > +		return 0;
> > > > +
> > > > +	if (kbd_set_state(old))
> > > > +		pr_err("Setting old previous keyboard state
> > 
> > failed\n");
> > 
> > > And if we got an error and kbd_set_state(old) doesn't
> > > return !0, what's the state of things? Still a failure a
> > > presume?
> > 
> > In some cases some laptops do not have to support
> > everything. And when I (and Gabriele too) tried to set
> > something unsupported Dell BIOS just resetted all settings
> > to some default values. So this function try to set new
> > state and if it fails then it try to restore previous
> > settings.
> 
> OK, that deserves a comment then as the rationale isn't
> obvious.
> 

Ok, I will add comment.

> > > > +
> > > > +	return ret;
> > > > +}
> > > > 
> > > > +static void kbd_init(void)
> > > > +{
> > > > +	struct kbd_state state;
> > > > +	int ret;
> > > > +	int i;
> > > > +
> > > > +	ret = kbd_get_info(&kbd_info);
> > > > +
> > > > +	if (ret == 0) {
> > > > +
> > > 
> > > Checking for success, let's invert and avoid the
> > > indentation here too.
> > 
> > Again this is hard and not possible. This function first
> > process data from kbd_get_info (if does not fail), then
> > process data for kbd_tokens (via function find_token_id)
> > and then set kbd_led_present to true if at least
> > kbd_get_info or kbd_tokens succed.
> 
> The goal here is to avoid more than 3 levels of indentations
> for improved legibility. Often there are logical inversions
> and such we can make to accomplish this. When that fails, we
> break things up into functions, static inlines, etc.
> 
> For reference:
> https://lkml.org/lkml/2007/6/15/449
> 
> Which elaborates on CodingStyle Chapter 1: Indentation a bit.
> 
> ...
> 

Ok, I will move it into two static inline functions (one for 
kbd_get_info and second for kbd_tokens).

> > > > +static ssize_t kbd_led_timeout_store(struct device
> > > > *dev, +				     struct device_attribute *attr,
> > > > +				     const char *buf, size_t count)
> > > > +{
> > > > +	struct kbd_state state;
> > > > +	struct kbd_state new_state;
> > > > +	int ret;
> > > > +	bool convert;
> > > > +	char ch;
> > > > +	u8 unit;
> > > > +	int value;
> > > > +	int i;
> > > 
> > > Decreasing line lenth please.
> > 
> > What do you mean?
> 
> This is a nit, but one other maintainers have imposed on me,
> as a means to improve legibility. The preference is to
> declare variables in decreasing line length, longest to
> shortest:
> 
> struct kbd_state new_state;
> struct kbd_state state;
> bool convert;
> int value;
> u8 unit;
> char ch;
> int ret;
> int i;
> 
> This is a generalization and sometimes there are good reasons
> for doing something else, such as logical groupings for
> commenting groups, etc. But since this list appeared mostly
> random, defaulting to decreasing line length is preferred.
> 

Ok. I'm not native English speaker and I did not understand what 
"Decreasing line lenth" means...

> > > > +	if (convert) {
> > > > +		/* NOTE: this switch fall down */
> > > 
> > > "fall down" ? As in, it intentionally doesn't have breaks?
> > 
> > This code convert "value" in "units" to new value in minutes
> > units. So for unit == days it is: 24*60*60... So no breaks.
> 
> Right, so the language of the comment just wasn't clear, try:
> 
> /* Convert value from seconds to minutes */
> 

Err... to seconds :-) But OK, I will change comment.

> > > > +		switch (unit) {
> > > > +		case KBD_TIMEOUT_DAYS:
> > > > +			value *= 24;
> > > > +		case KBD_TIMEOUT_HOURS:
> > > > +			value *= 60;
> > > > +		case KBD_TIMEOUT_MINUTES:
> > > > +			value *= 60;
> > > > +			unit = KBD_TIMEOUT_SECONDS;
> > > > +	 	}
> > > > +

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight

[Darren Hart]

On Sat, Nov 22, 2014 at 07:46:25PM +0100, Pali Rohár wrote:

> > > 0   Completed successfully
> > > -1  Completed with error
> > > -2  Function not supported
> > > 
> > > So we can return something other too (not always -EINVAL).
> > > Do you have any idea which errno should we return for -1
> > > and -2?
> > 
> > For -1, I should think  -EIO (I/O Error)
> > For -2, I'd expect -ENXIO (No such device or address)
> > 
> 
> What about -ENOSYS for -2?

No. This specific topic came up at kernel summit this year. ENOSYS is
specifically for not implemented system calls.

> 
> > > > > +	if (convert) {
> > > > > +		/* NOTE: this switch fall down */
> > > > 
> > > > "fall down" ? As in, it intentionally doesn't have breaks?
> > > 
> > > This code convert "value" in "units" to new value in minutes
> > > units. So for unit == days it is: 24*60*60... So no breaks.
> > 
> > Right, so the language of the comment just wasn't clear, try:
> > 
> > /* Convert value from seconds to minutes */
> > 
> 
> Err... to seconds :-) But OK, I will change comment.

Oops, duh.

/* Convert value from current units to seconds. */

> 
> > > > > +		switch (unit) {
> > > > > +		case KBD_TIMEOUT_DAYS:
> > > > > +			value *= 24;
> > > > > +		case KBD_TIMEOUT_HOURS:
> > > > > +			value *= 60;
> > > > > +		case KBD_TIMEOUT_MINUTES:
> > > > > +			value *= 60;
> > > > > +			unit = KBD_TIMEOUT_SECONDS;
> > > > > +	 	}

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1668 bytes --]

On Friday 21 November 2014 23:09:40 Darren Hart wrote:
> On Sat, Nov 22, 2014 at 07:46:25PM +0100, Pali Rohár wrote:
> > > > 0   Completed successfully
> > > > -1  Completed with error
> > > > -2  Function not supported
> > > > 
> > > > So we can return something other too (not always
> > > > -EINVAL). Do you have any idea which errno should we
> > > > return for -1 and -2?
> > > 
> > > For -1, I should think  -EIO (I/O Error)
> > > For -2, I'd expect -ENXIO (No such device or address)
> > 
> > What about -ENOSYS for -2?
> 
> No. This specific topic came up at kernel summit this year.
> ENOSYS is specifically for not implemented system calls.
> 

Ok, I will use -ENXIO.

> > > > > > +	if (convert) {
> > > > > > +		/* NOTE: this switch fall down */
> > > > > 
> > > > > "fall down" ? As in, it intentionally doesn't have
> > > > > breaks?
> > > > 
> > > > This code convert "value" in "units" to new value in
> > > > minutes units. So for unit == days it is: 24*60*60...
> > > > So no breaks.
> > > 
> > > Right, so the language of the comment just wasn't clear,
> > > try:
> > > 
> > > /* Convert value from seconds to minutes */
> > 
> > Err... to seconds :-) But OK, I will change comment.
> 
> Oops, duh.
> 
> /* Convert value from current units to seconds. */
> 
> > > > > > +		switch (unit) {
> > > > > > +		case KBD_TIMEOUT_DAYS:
> > > > > > +			value *= 24;
> > > > > > +		case KBD_TIMEOUT_HOURS:
> > > > > > +			value *= 60;
> > > > > > +		case KBD_TIMEOUT_MINUTES:
> > > > > > +			value *= 60;
> > > > > > +			unit = KBD_TIMEOUT_SECONDS;
> > > > > > +	 	}

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v2] platform: x86: dell-laptop: Add support for keyboard backlight

[Pali Rohár]

This patch adds support for configuring keyboard backlight settings on supported
Dell laptops. It exports kernel leds interface and uses Dell SMBIOS tokens or
keyboard class interface.

With this patch it is possible to set:
* keyboard backlight level
* timeout after which will be backlight automatically turned off
* input activity triggers (keyboard, touchpad, mouse) which enable backlight
* ambient light settings

Settings are exported via sysfs:
/sys/class/leds/dell::kbd_backlight/

Code is based on newly released documentation by Dell in libsmbios project.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
---
Changes since v1:
* returns also -EIO/-ENXIO (with helper function dell_smi_error)
* simplify code to have less levels of indentation
* style fixes
* add some comments
---
 drivers/platform/x86/dell-laptop.c | 1023 +++++++++++++++++++++++++++++++++++-
 1 file changed, 1019 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 233d2ee..62e1c99 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -2,9 +2,11 @@
  *  Driver for Dell laptop extras
  *
  *  Copyright (c) Red Hat <mjg@redhat.com>
+ *  Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@gmail.com>
+ *  Copyright (c) 2014 Pali Rohár <pali.rohar@gmail.com>
  *
- *  Based on documentation in the libsmbios package, Copyright (C) 2005 Dell
- *  Inc.
+ *  Based on documentation in the libsmbios package:
+ *  Copyright (C) 2005-2014 Dell Inc.
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License version 2 as
@@ -32,6 +34,13 @@
 #include "../../firmware/dcdbas.h"
 
 #define BRIGHTNESS_TOKEN 0x7d
+#define KBD_LED_OFF_TOKEN 0x01E1
+#define KBD_LED_ON_TOKEN 0x01E2
+#define KBD_LED_AUTO_TOKEN 0x01E3
+#define KBD_LED_AUTO_25_TOKEN 0x02EA
+#define KBD_LED_AUTO_50_TOKEN 0x02EB
+#define KBD_LED_AUTO_75_TOKEN 0x02EC
+#define KBD_LED_AUTO_100_TOKEN 0x02F6
 
 /* This structure will be modified by the firmware when we enter
  * system management mode, hence the volatiles */
@@ -62,6 +71,10 @@ struct calling_interface_structure {
 
 struct quirk_entry {
 	u8 touchpad_led;
+
+	/* Ordered list of timeouts expressed in seconds.
+	 * The list must end with -1 */
+	int kbd_timeouts[];
 };
 
 static struct quirk_entry *quirks;
@@ -76,6 +89,10 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
 	return 1;
 }
 
+static struct quirk_entry quirk_dell_xps13_9333 = {
+	.kbd_timeouts = { 0, 5, 15, 60, 5*60, 15*60, -1 },
+};
+
 static int da_command_address;
 static int da_command_code;
 static int da_num_tokens;
@@ -268,6 +285,15 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
 		},
 		.driver_data = &quirk_dell_vostro_v130,
 	},
+	{
+		.callback = dmi_matched,
+		.ident = "Dell XPS13 9333",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "XPS13 9333"),
+		},
+		.driver_data = &quirk_dell_xps13_9333,
+	},
 	{ }
 };
 
@@ -332,17 +358,29 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
 	}
 }
 
-static int find_token_location(int tokenid)
+static int find_token_id(int tokenid)
 {
 	int i;
+
 	for (i = 0; i < da_num_tokens; i++) {
 		if (da_tokens[i].tokenID == tokenid)
-			return da_tokens[i].location;
+			return i;
 	}
 
 	return -1;
 }
 
+static int find_token_location(int tokenid)
+{
+	int id;
+
+	id = find_token_id(tokenid);
+	if (id == -1)
+		return -1;
+
+	return da_tokens[id].location;
+}
+
 static struct calling_interface_buffer *
 dell_send_request(struct calling_interface_buffer *buffer, int class,
 		  int select)
@@ -363,6 +401,20 @@ dell_send_request(struct calling_interface_buffer *buffer, int class,
 	return buffer;
 }
 
+static inline int dell_smi_error(int value)
+{
+	switch (value) {
+	case 0: /* Completed successfully */
+		return 0;
+	case -1: /* Completed with error */
+		return -EIO;
+	case -2: /* Function not supported */
+		return -ENXIO;
+	default: /* Unknown error */
+		return -EINVAL;
+	}
+}
+
 /* Derived from information in DellWirelessCtl.cpp:
    Class 17, select 11 is radio control. It returns an array of 32-bit values.
 
@@ -790,6 +842,964 @@ static void touchpad_led_exit(void)
 	led_classdev_unregister(&touchpad_led);
 }
 
+/* Derived from information in smbios-keyboard-ctl:
+
+ cbClass 4
+ cbSelect 11
+ Keyboar illumination
+ cbArg1 determines the function to be performed
+
+ cbArg1 0x0 = Get Feature Information
+  cbRES1         Standard return codes (0, -1, -2)
+  cbRES2, word0  Bitmap of user-selectable modes
+     bit 0     Always off (All systems)
+     bit 1     Always on (Travis ATG, Siberia)
+     bit 2     Auto: ALS-based On; ALS-based Off (Travis ATG)
+     bit 3     Auto: ALS- and input-activity-based On; input-activity based Off
+     bit 4     Auto: Input-activity-based On; input-activity based Off
+     bit 5     Auto: Input-activity-based On (illumination level 25%); input-activity based Off
+     bit 6     Auto: Input-activity-based On (illumination level 50%); input-activity based Off
+     bit 7     Auto: Input-activity-based On (illumination level 75%); input-activity based Off
+     bit 8     Auto: Input-activity-based On (illumination level 100%); input-activity based Off
+     bits 9-15 Reserved for future use
+  cbRES2, byte2  Reserved for future use
+  cbRES2, byte3  Keyboard illumination type
+     0         Reserved
+     1         Tasklight
+     2         Backlight
+     3-255     Reserved for future use
+  cbRES3, byte0  Supported auto keyboard illumination trigger bitmap.
+     bit 0     Any keystroke
+     bit 1     Touchpad activity
+     bit 2     Pointing stick
+     bit 3     Any mouse
+     bits 4-7  Reserved for future use
+  cbRES3, byte1  Supported timeout unit bitmap
+     bit 0     Seconds
+     bit 1     Minutes
+     bit 2     Hours
+     bit 3     Days
+     bits 4-7  Reserved for future use
+  cbRES3, byte2  Number of keyboard light brightness levels
+  cbRES4, byte0  Maximum acceptable seconds value (0 if seconds not supported).
+  cbRES4, byte1  Maximum acceptable minutes value (0 if minutes not supported).
+  cbRES4, byte2  Maximum acceptable hours value (0 if hours not supported).
+  cbRES4, byte3  Maximum acceptable days value (0 if days not supported)
+
+ cbArg1 0x1 = Get Current State
+  cbRES1         Standard return codes (0, -1, -2)
+  cbRES2, word0  Bitmap of current mode state
+     bit 0     Always off (All systems)
+     bit 1     Always on (Travis ATG, Siberia)
+     bit 2     Auto: ALS-based On; ALS-based Off (Travis ATG)
+     bit 3     Auto: ALS- and input-activity-based On; input-activity based Off
+     bit 4     Auto: Input-activity-based On; input-activity based Off
+     bit 5     Auto: Input-activity-based On (illumination level 25%); input-activity based Off
+     bit 6     Auto: Input-activity-based On (illumination level 50%); input-activity based Off
+     bit 7     Auto: Input-activity-based On (illumination level 75%); input-activity based Off
+     bit 8     Auto: Input-activity-based On (illumination level 100%); input-activity based Off
+     bits 9-15 Reserved for future use
+     Note: Only One bit can be set
+  cbRES2, byte2  Currently active auto keyboard illumination triggers.
+     bit 0     Any keystroke
+     bit 1     Touchpad activity
+     bit 2     Pointing stick
+     bit 3     Any mouse
+     bits 4-7  Reserved for future use
+  cbRES2, byte3  Current Timeout
+     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 byte
+     are set upon return from the [Get feature information] call.
+  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.
+
+ cbArg1 0x2 = Set New State
+  cbRES1         Standard return codes (0, -1, -2)
+  cbArg2, word0  Bitmap of current mode state
+     bit 0     Always off (All systems)
+     bit 1     Always on (Travis ATG, Siberia)
+     bit 2     Auto: ALS-based On; ALS-based Off (Travis ATG)
+     bit 3     Auto: ALS- and input-activity-based On; input-activity based Off
+     bit 4     Auto: Input-activity-based On; input-activity based Off
+     bit 5     Auto: Input-activity-based On (illumination level 25%); input-activity based Off
+     bit 6     Auto: Input-activity-based On (illumination level 50%); input-activity based Off
+     bit 7     Auto: Input-activity-based On (illumination level 75%); input-activity based Off
+     bit 8     Auto: Input-activity-based On (illumination level 100%); input-activity based Off
+     bits 9-15 Reserved for future use
+     Note: Only One bit can be set
+  cbArg2, byte2  Desired auto keyboard illumination triggers. Must remain inactive to allow
+                 keyboard to turn off automatically.
+     bit 0     Any keystroke
+     bit 1     Touchpad activity
+     bit 2     Pointing stick
+     bit 3     Any mouse
+     bits 4-7  Reserved for future use
+  cbArg2, byte3  Desired Timeout
+     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
+  cbArg3, byte0  Desired setting of ALS value that turns the light on or off.
+  cbArg3, byte2  Desired keyboard light level.
+*/
+
+
+enum kbd_timeout_unit {
+	KBD_TIMEOUT_SECONDS = 0,
+	KBD_TIMEOUT_MINUTES,
+	KBD_TIMEOUT_HOURS,
+	KBD_TIMEOUT_DAYS,
+};
+
+enum kbd_mode_bit {
+	KBD_MODE_BIT_OFF = 0,
+	KBD_MODE_BIT_ON,
+	KBD_MODE_BIT_ALS,
+	KBD_MODE_BIT_TRIGGER_ALS,
+	KBD_MODE_BIT_TRIGGER,
+	KBD_MODE_BIT_TRIGGER_25,
+	KBD_MODE_BIT_TRIGGER_50,
+	KBD_MODE_BIT_TRIGGER_75,
+	KBD_MODE_BIT_TRIGGER_100,
+};
+
+#define kbd_is_als_mode_bit(bit) \
+	((bit) == KBD_MODE_BIT_ALS || (bit) == KBD_MODE_BIT_TRIGGER_ALS)
+#define kbd_is_trigger_mode_bit(bit) \
+	((bit) >= KBD_MODE_BIT_TRIGGER_ALS && (bit) <= KBD_MODE_BIT_TRIGGER_100)
+#define kbd_is_level_mode_bit(bit) \
+	((bit) >= KBD_MODE_BIT_TRIGGER_25 && (bit) <= KBD_MODE_BIT_TRIGGER_100)
+
+struct kbd_info {
+	u16 modes;
+	u8 type;
+	u8 triggers;
+	u8 levels;
+	u8 seconds;
+	u8 minutes;
+	u8 hours;
+	u8 days;
+};
+
+struct kbd_state {
+	u8 mode_bit;
+	u8 triggers;
+	u8 timeout_value;
+	u8 timeout_unit;
+	u8 als_setting;
+	u8 als_value;
+	u8 level;
+};
+
+static const int kbd_tokens[] = {
+	KBD_LED_OFF_TOKEN,
+	KBD_LED_AUTO_25_TOKEN,
+	KBD_LED_AUTO_50_TOKEN,
+	KBD_LED_AUTO_75_TOKEN,
+	KBD_LED_AUTO_100_TOKEN,
+	KBD_LED_ON_TOKEN,
+};
+
+static u16 kbd_token_bits;
+
+static struct kbd_info kbd_info;
+static bool kbd_als_supported;
+static bool kbd_triggers_supported;
+
+static u8 kbd_mode_levels[16];
+static int kbd_mode_levels_count;
+
+static u8 kbd_previous_level;
+static u8 kbd_previous_mode_bit;
+
+static bool kbd_led_present;
+
+static int kbd_get_info(struct kbd_info *info)
+{
+	u8 units;
+	int ret;
+
+	get_buffer();
+
+	buffer->input[0] = 0x0;
+	dell_send_request(buffer, 4, 11);
+	ret = buffer->output[0];
+
+	if (ret) {
+		ret = dell_smi_error(ret);
+		goto out;
+	}
+
+	info->modes = buffer->output[1] & 0xFFFF;
+	info->type = (buffer->output[1] >> 24) & 0xFF;
+	info->triggers = buffer->output[2] & 0xFF;
+	units = (buffer->output[2] >> 8) & 0xFF;
+	info->levels = (buffer->output[2] >> 16) & 0xFF;
+
+	if (units & BIT(0))
+		info->seconds = (buffer->output[3] >> 0) & 0xFF;
+	if (units & BIT(1))
+		info->minutes = (buffer->output[3] >> 8) & 0xFF;
+	if (units & BIT(2))
+		info->hours = (buffer->output[3] >> 16) & 0xFF;
+	if (units & BIT(3))
+		info->days = (buffer->output[3] >> 24) & 0xFF;
+
+out:
+	release_buffer();
+	return ret;
+}
+
+static unsigned int kbd_get_max_level(void)
+{
+	if (kbd_info.levels != 0)
+		return kbd_info.levels;
+	if (kbd_mode_levels_count > 0)
+		return kbd_mode_levels_count - 1;
+	return 0;
+}
+
+static int kbd_get_level(struct kbd_state *state)
+{
+	int i;
+
+	if (kbd_info.levels != 0)
+		return state->level;
+
+	if (kbd_mode_levels_count > 0) {
+		for (i = 0; i < kbd_mode_levels_count; ++i)
+			if (kbd_mode_levels[i] == state->mode_bit)
+				return i;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int kbd_set_level(struct kbd_state *state, u8 level)
+{
+	if (kbd_info.levels != 0) {
+		if (level != 0)
+			kbd_previous_level = level;
+		if (state->level == level)
+			return 0;
+		state->level = level;
+		if (level != 0 && state->mode_bit == KBD_MODE_BIT_OFF)
+			state->mode_bit = kbd_previous_mode_bit;
+		else if (level == 0 && state->mode_bit != KBD_MODE_BIT_OFF) {
+			kbd_previous_mode_bit = state->mode_bit;
+			state->mode_bit = KBD_MODE_BIT_OFF;
+		}
+		return 0;
+	}
+
+	if (kbd_mode_levels_count > 0 && level < kbd_mode_levels_count) {
+		if (level != 0)
+			kbd_previous_level = level;
+		state->mode_bit = kbd_mode_levels[level];
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int kbd_get_state(struct kbd_state *state)
+{
+	int ret;
+
+	get_buffer();
+
+	buffer->input[0] = 0x1;
+	dell_send_request(buffer, 4, 11);
+	ret = buffer->output[0];
+
+	if (ret) {
+		ret = dell_smi_error(ret);
+		goto out;
+	}
+
+	state->mode_bit = ffs(buffer->output[1] & 0xFFFF);
+	if (state->mode_bit != 0)
+		state->mode_bit--;
+
+	state->triggers = (buffer->output[1] >> 16) & 0xFF;
+	state->timeout_value = (buffer->output[1] >> 24) & 0x3F;
+	state->timeout_unit = (buffer->output[1] >> 30) & 0x3;
+	state->als_setting = buffer->output[2] & 0xFF;
+	state->als_value = (buffer->output[2] >> 8) & 0xFF;
+	state->level = (buffer->output[2] >> 16) & 0xFF;
+
+out:
+	release_buffer();
+	return ret;
+}
+
+static int kbd_set_state(struct kbd_state *state)
+{
+	int ret;
+
+	get_buffer();
+	buffer->input[0] = 0x2;
+	buffer->input[1] = BIT(state->mode_bit) & 0xFFFF;
+	buffer->input[1] |= (state->triggers & 0xFF) << 16;
+	buffer->input[1] |= (state->timeout_value & 0x3F) << 24;
+	buffer->input[1] |= (state->timeout_unit & 0x3) << 30;
+	buffer->input[2] = state->als_setting & 0xFF;
+	buffer->input[2] |= (state->level & 0xFF) << 16;
+	dell_send_request(buffer, 4, 11);
+	ret = buffer->output[0];
+	release_buffer();
+
+	return dell_smi_error(ret);
+}
+
+static int kbd_set_state_safe(struct kbd_state *state, struct kbd_state *old)
+{
+	int ret;
+
+	ret = kbd_set_state(state);
+	if (ret == 0)
+		return 0;
+
+	/*
+	 * When setting new state failed, restore previous old one. This is
+	 * needed because on some machines BIOS set default state when setting
+	 * new failed. And default state could be also eveything is turned off.
+	 */
+
+	if (kbd_set_state(old))
+		pr_err("Setting old previous keyboard state failed\n");
+
+	return ret;
+}
+
+static int kbd_set_token_bit(u8 bit)
+{
+	int id;
+	int ret;
+
+	if (bit >= ARRAY_SIZE(kbd_tokens))
+		return -EINVAL;
+
+	id = find_token_id(kbd_tokens[bit]);
+	if (id == -1)
+		return -EINVAL;
+
+	get_buffer();
+	buffer->input[0] = da_tokens[id].location;
+	buffer->input[1] = da_tokens[id].value;
+	dell_send_request(buffer, 1, 0);
+	ret = buffer->output[0];
+	release_buffer();
+
+	return dell_smi_error(ret);
+}
+
+static int kbd_get_token_bit(u8 bit)
+{
+	int id;
+	int ret;
+	int val;
+
+	if (bit >= ARRAY_SIZE(kbd_tokens))
+		return -EINVAL;
+
+	id = find_token_id(kbd_tokens[bit]);
+	if (id == -1)
+		return -EINVAL;
+
+	get_buffer();
+	buffer->input[0] = da_tokens[id].location;
+	dell_send_request(buffer, 0, 0);
+	ret = buffer->output[0];
+	val = buffer->output[1];
+	release_buffer();
+
+	if (ret)
+		return dell_smi_error(ret);
+
+	return (val == da_tokens[id].value);
+}
+
+static int kbd_get_first_active_token_bit(void)
+{
+	int i;
+	int ret;
+
+	for (i = 0; i < ARRAY_SIZE(kbd_tokens); ++i) {
+		ret = kbd_get_token_bit(i);
+		if (ret == 1)
+			return i;
+	}
+
+	return ret;
+}
+
+static int kbd_get_valid_token_counts(void)
+{
+	return hweight16(kbd_token_bits);
+}
+
+static inline int kbd_init_info(void)
+{
+	struct kbd_state state;
+	int ret;
+	int i;
+
+	ret = kbd_get_info(&kbd_info);
+	if (ret)
+		return ret;
+
+	kbd_get_state(&state);
+
+	/* NOTE: timeout value is stored in 6 bits so max value is 63 */
+	if (kbd_info.seconds > 63)
+		kbd_info.seconds = 63;
+	if (kbd_info.minutes > 63)
+		kbd_info.minutes = 63;
+	if (kbd_info.hours > 63)
+		kbd_info.hours = 63;
+	if (kbd_info.days > 63)
+		kbd_info.days = 63;
+
+	/* NOTE: On tested machines ON mode did not work and caused
+	 *       problems (turned backlight off) so do not use it
+	 */
+	kbd_info.modes &= ~BIT(KBD_MODE_BIT_ON);
+
+	kbd_previous_level = kbd_get_level(&state);
+	kbd_previous_mode_bit = state.mode_bit;
+
+	if (kbd_previous_level == 0 && kbd_get_max_level() != 0)
+		kbd_previous_level = 1;
+
+	if (kbd_previous_mode_bit == KBD_MODE_BIT_OFF) {
+		kbd_previous_mode_bit =
+			ffs(kbd_info.modes & ~BIT(KBD_MODE_BIT_OFF));
+		if (kbd_previous_mode_bit != 0)
+			kbd_previous_mode_bit--;
+	}
+
+	if (kbd_info.modes & (BIT(KBD_MODE_BIT_ALS) |
+			      BIT(KBD_MODE_BIT_TRIGGER_ALS)))
+		kbd_als_supported = true;
+
+	if (kbd_info.modes & (
+	    BIT(KBD_MODE_BIT_TRIGGER_ALS) | BIT(KBD_MODE_BIT_TRIGGER) |
+	    BIT(KBD_MODE_BIT_TRIGGER_25) | BIT(KBD_MODE_BIT_TRIGGER_50) |
+	    BIT(KBD_MODE_BIT_TRIGGER_75) | BIT(KBD_MODE_BIT_TRIGGER_100)
+	   ))
+		kbd_triggers_supported = true;
+
+	for (i = 0; i < 16; ++i)
+		if (kbd_is_level_mode_bit(i) && (BIT(i) & kbd_info.modes))
+			kbd_mode_levels[1+kbd_mode_levels_count++] = i;
+
+	if (kbd_mode_levels_count > 0) {
+		for (i = 0; i < 16; ++i) {
+			if (BIT(i) & kbd_info.modes) {
+				kbd_mode_levels[0] = i;
+				break;
+			}
+		}
+		kbd_mode_levels_count++;
+	}
+
+	return 0;
+
+}
+
+static inline void kbd_init_tokens(void)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(kbd_tokens); ++i)
+		if (find_token_id(kbd_tokens[i]) != -1)
+			kbd_token_bits |= BIT(i);
+}
+
+static void kbd_init(void)
+{
+	int ret;
+
+	ret = kbd_init_info();
+	kbd_init_tokens();
+
+	if (kbd_token_bits != 0 || ret == 0)
+		kbd_led_present = true;
+}
+
+static ssize_t kbd_led_timeout_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct kbd_state new_state;
+	struct kbd_state state;
+	bool convert;
+	int value;
+	int ret;
+	char ch;
+	u8 unit;
+	int i;
+
+	ret = sscanf(buf, "%d %c", &value, &ch);
+	if (ret < 1)
+		return -EINVAL;
+	else if (ret == 1)
+		ch = 's';
+
+	if (value < 0)
+		return -EINVAL;
+
+	convert = false;
+
+	switch (ch) {
+	case 's':
+		if (value > kbd_info.seconds)
+			convert = true;
+		unit = KBD_TIMEOUT_SECONDS;
+		break;
+	case 'm':
+		if (value > kbd_info.minutes)
+			convert = true;
+		unit = KBD_TIMEOUT_MINUTES;
+		break;
+	case 'h':
+		if (value > kbd_info.hours)
+			convert = true;
+		unit = KBD_TIMEOUT_HOURS;
+		break;
+	case 'd':
+		if (value > kbd_info.days)
+			convert = true;
+		unit = KBD_TIMEOUT_DAYS;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (quirks && quirks->kbd_timeouts)
+		convert = true;
+
+	if (convert) {
+		/* Convert value from current units to seconds */
+		switch (unit) {
+		case KBD_TIMEOUT_DAYS:
+			value *= 24;
+		case KBD_TIMEOUT_HOURS:
+			value *= 60;
+		case KBD_TIMEOUT_MINUTES:
+			value *= 60;
+			unit = KBD_TIMEOUT_SECONDS;
+		}
+
+		if (quirks && quirks->kbd_timeouts) {
+			for (i = 0; quirks->kbd_timeouts[i] != -1; i++) {
+				if (value <= quirks->kbd_timeouts[i]) {
+					value = quirks->kbd_timeouts[i];
+					break;
+				}
+			}
+		}
+
+		if (value <= kbd_info.seconds && kbd_info.seconds) {
+			unit = KBD_TIMEOUT_SECONDS;
+		} else if (value / 60 <= kbd_info.minutes && kbd_info.minutes) {
+			value /= 60;
+			unit = KBD_TIMEOUT_MINUTES;
+		} else if (value / (60 * 60) <= kbd_info.hours && kbd_info.hours) {
+			value /= (60 * 60);
+			unit = KBD_TIMEOUT_HOURS;
+		} else if (value / (60 * 60 * 24) <= kbd_info.days && kbd_info.days) {
+			value /= (60 * 60 * 24);
+			unit = KBD_TIMEOUT_DAYS;
+		} else {
+			return -EINVAL;
+		}
+	}
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	new_state = state;
+	new_state.timeout_value = value;
+	new_state.timeout_unit = unit;
+
+	ret = kbd_set_state_safe(&new_state, &state);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t kbd_led_timeout_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct kbd_state state;
+	int ret;
+	int len;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	len = sprintf(buf, "%d", state.timeout_value);
+
+	switch (state.timeout_unit) {
+	case KBD_TIMEOUT_SECONDS:
+		return len + sprintf(buf+len, "s\n");
+	case KBD_TIMEOUT_MINUTES:
+		return len + sprintf(buf+len, "m\n");
+	case KBD_TIMEOUT_HOURS:
+		return len + sprintf(buf+len, "h\n");
+	case KBD_TIMEOUT_DAYS:
+		return len + sprintf(buf+len, "d\n");
+	default:
+		return -EINVAL;
+	}
+
+	return len;
+}
+
+static DEVICE_ATTR(stop_timeout, S_IRUGO | S_IWUSR,
+		   kbd_led_timeout_show, kbd_led_timeout_store);
+
+static const char * const kbd_led_triggers[] = {
+	"keyboard",
+	"touchpad",
+	/*"trackstick"*/ NULL, /* NOTE: trackstick is just alias for touchpad */
+	"mouse",
+};
+
+static ssize_t kbd_led_triggers_store(struct device *dev,
+				      struct device_attribute *attr,
+				      const char *buf, size_t count)
+{
+	struct kbd_state new_state;
+	struct kbd_state state;
+	bool triggers_enabled = false;
+	bool als_enabled = false;
+	bool disable_als = false;
+	bool enable_als = false;
+	int trigger_bit = -1;
+	char trigger[21];
+	int i, ret;
+
+	ret = sscanf(buf, "%20s", trigger);
+	if (ret != 1)
+		return -EINVAL;
+
+	if (trigger[0] != '+' && trigger[0] != '-')
+		return -EINVAL;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	if (kbd_als_supported)
+		als_enabled = kbd_is_als_mode_bit(state.mode_bit);
+
+	if (kbd_triggers_supported)
+		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
+
+	if (kbd_als_supported) {
+		if (strcmp(trigger, "+als") == 0) {
+			if (als_enabled)
+				return count;
+			enable_als = true;
+		} else if (strcmp(trigger, "-als") == 0) {
+			if (!als_enabled)
+				return count;
+			disable_als = true;
+		}
+	}
+
+	if (enable_als || disable_als) {
+		new_state = state;
+		if (enable_als) {
+			if (triggers_enabled)
+				new_state.mode_bit = KBD_MODE_BIT_TRIGGER_ALS;
+			else
+				new_state.mode_bit = KBD_MODE_BIT_ALS;
+		} else {
+			if (triggers_enabled) {
+				new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
+				kbd_set_level(&new_state, kbd_previous_level);
+			} else {
+				new_state.mode_bit = KBD_MODE_BIT_ON;
+			}
+		}
+		if (!(kbd_info.modes & BIT(new_state.mode_bit)))
+			return -EINVAL;
+		ret = kbd_set_state_safe(&new_state, &state);
+		if (ret)
+			return ret;
+		kbd_previous_mode_bit = new_state.mode_bit;
+		return count;
+	}
+
+	if (kbd_triggers_supported) {
+		for (i = 0; i < ARRAY_SIZE(kbd_led_triggers); ++i) {
+			if (!(kbd_info.triggers & BIT(i)))
+				continue;
+			if (!kbd_led_triggers[i])
+				continue;
+			if (strcmp(trigger+1, kbd_led_triggers[i]) != 0)
+				continue;
+			if (trigger[0] == '+' &&
+			    triggers_enabled && (state.triggers & BIT(i)))
+				return count;
+			if (trigger[0] == '-' &&
+			    (!triggers_enabled || !(state.triggers & BIT(i))))
+				return count;
+			trigger_bit = i;
+			break;
+		}
+	}
+
+	if (trigger_bit != -1) {
+		new_state = state;
+		if (trigger[0] == '+')
+			new_state.triggers |= BIT(trigger_bit);
+		else {
+			new_state.triggers &= ~BIT(trigger_bit);
+			/* NOTE: trackstick bit (2) must be disabled when
+			 *       disabling touchpad bit (1), otherwise touchpad
+			 *       bit (1) will not be disabled */
+			if (trigger_bit == 1)
+				new_state.triggers &= ~BIT(2);
+		}
+		if ((kbd_info.triggers & new_state.triggers) !=
+		    new_state.triggers)
+			return -EINVAL;
+		if (new_state.triggers && !triggers_enabled) {
+			if (als_enabled)
+				new_state.mode_bit = KBD_MODE_BIT_TRIGGER_ALS;
+			else {
+				new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
+				kbd_set_level(&new_state, kbd_previous_level);
+			}
+		} else if (new_state.triggers == 0) {
+			if (als_enabled)
+				new_state.mode_bit = KBD_MODE_BIT_ALS;
+			else
+				kbd_set_level(&new_state, 0);
+		}
+		if (!(kbd_info.modes & BIT(new_state.mode_bit)))
+			return -EINVAL;
+		ret = kbd_set_state_safe(&new_state, &state);
+		if (ret)
+			return ret;
+		if (new_state.mode_bit != KBD_MODE_BIT_OFF)
+			kbd_previous_mode_bit = new_state.mode_bit;
+		return count;
+	}
+
+	return -EINVAL;
+}
+
+static ssize_t kbd_led_triggers_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct kbd_state state;
+	bool triggers_enabled;
+	int level, i, ret;
+	int len = 0;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	len = 0;
+
+	if (kbd_triggers_supported) {
+		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
+		level = kbd_get_level(&state);
+		for (i = 0; i < ARRAY_SIZE(kbd_led_triggers); ++i) {
+			if (!(kbd_info.triggers & BIT(i)))
+				continue;
+			if (!kbd_led_triggers[i])
+				continue;
+			if ((triggers_enabled || level <= 0) &&
+			    (state.triggers & BIT(i)))
+				buf[len++] = '+';
+			else
+				buf[len++] = '-';
+			len += sprintf(buf+len, "%s ", kbd_led_triggers[i]);
+		}
+	}
+
+	if (kbd_als_supported) {
+		if (kbd_is_als_mode_bit(state.mode_bit))
+			len += sprintf(buf+len, "+als ");
+		else
+			len += sprintf(buf+len, "-als ");
+	}
+
+	if (len)
+		buf[len - 1] = '\n';
+
+	return len;
+}
+
+static DEVICE_ATTR(start_triggers, S_IRUGO | S_IWUSR,
+		   kbd_led_triggers_show, kbd_led_triggers_store);
+
+static ssize_t kbd_led_als_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct kbd_state state;
+	struct kbd_state new_state;
+	u8 setting;
+	int ret;
+
+	ret = kstrtou8(buf, 10, &setting);
+	if (ret)
+		return ret;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	new_state = state;
+	new_state.als_setting = setting;
+
+	ret = kbd_set_state_safe(&new_state, &state);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t kbd_led_als_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct kbd_state state;
+	int ret;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "%d\n", state.als_setting);
+}
+
+static DEVICE_ATTR(als_setting, S_IRUGO | S_IWUSR,
+		   kbd_led_als_show, kbd_led_als_store);
+
+static struct attribute *kbd_led_attrs[] = {
+	&dev_attr_stop_timeout.attr,
+	&dev_attr_start_triggers.attr,
+	&dev_attr_als_setting.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(kbd_led);
+
+static enum led_brightness kbd_led_level_get(struct led_classdev *led_cdev)
+{
+	int ret;
+	u16 num;
+	struct kbd_state state;
+
+	if (kbd_get_max_level()) {
+		ret = kbd_get_state(&state);
+		if (ret)
+			return 0;
+		ret = kbd_get_level(&state);
+		if (ret < 0)
+			return 0;
+		return ret;
+	}
+
+	if (kbd_get_valid_token_counts()) {
+		ret = kbd_get_first_active_token_bit();
+		if (ret < 0)
+			return 0;
+		for (num = kbd_token_bits; num != 0 && ret > 0; --ret)
+			num &= num - 1; /* clear the first bit set */
+		if (num == 0)
+			return 0;
+		return ffs(num) - 1;
+	}
+
+	pr_warn("Keyboard brightness level control not supported\n");
+	return 0;
+}
+
+static void kbd_led_level_set(struct led_classdev *led_cdev,
+			      enum led_brightness value)
+{
+	struct kbd_state state;
+	struct kbd_state new_state;
+	u16 num;
+
+	if (kbd_get_max_level()) {
+		if (kbd_get_state(&state))
+			return;
+		new_state = state;
+		if (kbd_set_level(&new_state, value))
+			return;
+		kbd_set_state_safe(&new_state, &state);
+		return;
+	}
+
+	if (kbd_get_valid_token_counts()) {
+		for (num = kbd_token_bits; num != 0 && value > 0; --value)
+			num &= num - 1; /* clear the first bit set */
+		if (num == 0)
+			return;
+		kbd_set_token_bit(ffs(num) - 1);
+		return;
+	}
+
+	pr_warn("Keyboard brightness level control not supported\n");
+}
+
+static struct led_classdev kbd_led = {
+	.name           = "dell::kbd_backlight",
+	.brightness_set = kbd_led_level_set,
+	.brightness_get = kbd_led_level_get,
+	.groups         = kbd_led_groups,
+};
+
+static int __init kbd_led_init(struct device *dev)
+{
+	kbd_init();
+	if (!kbd_led_present)
+		return -ENODEV;
+	kbd_led.max_brightness = kbd_get_max_level();
+	if (!kbd_led.max_brightness) {
+		kbd_led.max_brightness = kbd_get_valid_token_counts();
+		if (kbd_led.max_brightness)
+			kbd_led.max_brightness--;
+	}
+	return led_classdev_register(dev, &kbd_led);
+}
+
+static void brightness_set_exit(struct led_classdev *led_cdev,
+				enum led_brightness value)
+{
+	/* Don't change backlight level on exit */
+};
+
+static void kbd_led_exit(void)
+{
+	if (!kbd_led_present)
+		return;
+	kbd_led.brightness_set = brightness_set_exit;
+	led_classdev_unregister(&kbd_led);
+}
+
 static int __init dell_init(void)
 {
 	int max_intensity = 0;
@@ -842,6 +1852,8 @@ static int __init dell_init(void)
 	if (quirks && quirks->touchpad_led)
 		touchpad_led_init(&platform_device->dev);
 
+	kbd_led_init(&platform_device->dev);
+
 	dell_laptop_dir = debugfs_create_dir("dell_laptop", NULL);
 	if (dell_laptop_dir != NULL)
 		debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
@@ -909,6 +1921,7 @@ static void __exit dell_exit(void)
 	debugfs_remove_recursive(dell_laptop_dir);
 	if (quirks && quirks->touchpad_led)
 		touchpad_led_exit();
+	kbd_led_exit();
 	i8042_remove_filter(dell_laptop_i8042_filter);
 	cancel_delayed_work_sync(&dell_rfkill_work);
 	backlight_device_unregister(dell_backlight_device);
@@ -925,5 +1938,7 @@ module_init(dell_init);
 module_exit(dell_exit);
 
 MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
+MODULE_AUTHOR("Gabriele Mazzotta <gabriele.mzt@gmail.com>");
+MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
 MODULE_DESCRIPTION("Dell laptop driver");
 MODULE_LICENSE("GPL");
-- 
1.7.9.5

Re: [PATCH v2] platform: x86: dell-laptop: Add support for keyboard backlight

[Darren Hart]

On Sun, Nov 23, 2014 at 03:50:45PM +0100, Pali Rohár wrote:
> This patch adds support for configuring keyboard backlight settings on supported
> Dell laptops. It exports kernel leds interface and uses Dell SMBIOS tokens or
> keyboard class interface.
> 
> With this patch it is possible to set:
> * keyboard backlight level
> * timeout after which will be backlight automatically turned off
> * input activity triggers (keyboard, touchpad, mouse) which enable backlight
> * ambient light settings
> 
> Settings are exported via sysfs:
> /sys/class/leds/dell::kbd_backlight/
> 
> Code is based on newly released documentation by Dell in libsmbios project.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>

Queued in Testing branch, but still pending a more thorough final review from me
as it is such a large patch.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v2] platform: x86: dell-laptop: Add support for keyboard backlight

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1290 bytes --]

On Sunday 23 November 2014 15:50:45 Pali Rohár wrote:
> This patch adds support for configuring keyboard backlight
> settings on supported Dell laptops. It exports kernel leds
> interface and uses Dell SMBIOS tokens or keyboard class
> interface.
> 
> With this patch it is possible to set:
> * keyboard backlight level
> * timeout after which will be backlight automatically turned
> off * input activity triggers (keyboard, touchpad, mouse)
> which enable backlight * ambient light settings
> 
> Settings are exported via sysfs:
> /sys/class/leds/dell::kbd_backlight/
> 
> Code is based on newly released documentation by Dell in
> libsmbios project.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> ---
> Changes since v1:
> * returns also -EIO/-ENXIO (with helper function
> dell_smi_error) * simplify code to have less levels of
> indentation
> * style fixes
> * add some comments
> ---
>  drivers/platform/x86/dell-laptop.c | 1023
> +++++++++++++++++++++++++++++++++++- 1 file changed, 1019
> insertions(+), 4 deletions(-)
> 

Darren, Matthew: can you review this v2 code?

I would like to see keyboard backlight support in 3.19 kernel.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2] platform: x86: dell-laptop: Add support for keyboard backlight

[Darren Hart]

On Sun, Nov 23, 2014 at 03:50:45PM +0100, Pali Rohár wrote:
> This patch adds support for configuring keyboard backlight settings on supported
> Dell laptops. It exports kernel leds interface and uses Dell SMBIOS tokens or
> keyboard class interface.
> 
> With this patch it is possible to set:
> * keyboard backlight level
> * timeout after which will be backlight automatically turned off
> * input activity triggers (keyboard, touchpad, mouse) which enable backlight
> * ambient light settings
> 
> Settings are exported via sysfs:
> /sys/class/leds/dell::kbd_backlight/
> 
> Code is based on newly released documentation by Dell in libsmbios project.
> 

Hi Pali,

I would really like to see this broken up. Possibly adding levels and timeouts
as separate patches for example. It is quite difficult to review this all at
once in a reasonable amount of time.

During this review I caught a few more CodingStyle violations, and raised some
questions about the timeout and levels mechanisms.

> @@ -62,6 +71,10 @@ struct calling_interface_structure {
>  
>  struct quirk_entry {
>  	u8 touchpad_led;
> +
> +	/* Ordered list of timeouts expressed in seconds.
> +	 * The list must end with -1 */

Despite other instances in this file, block comments are documented in
CodingStyle as:

/*
 * Comment text.
 */

The old ones should be cleaned up eventually, but new ones need to be done
according to CodingStyle. Please correct throughout.

> +	int kbd_timeouts[];
>  };
>  
>  static struct quirk_entry *quirks;
> @@ -76,6 +89,10 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
>  	return 1;
>  }
>  
> +static struct quirk_entry quirk_dell_xps13_9333 = {
> +	.kbd_timeouts = { 0, 5, 15, 60, 5*60, 15*60, -1 },

Where did these values come from? Were they documented in the libsmbios project?
Can you provide a URL to that? These really should be described by the firmware,
but if they aren't, nothing we can do about it.

> @@ -790,6 +842,964 @@ static void touchpad_led_exit(void)
>  	led_classdev_unregister(&touchpad_led);
>  }
>  
> +/* Derived from information in smbios-keyboard-ctl:

See block comment above.

> +
> + cbClass 4
> + cbSelect 11
> + Keyboar illumination

Keyboard

> + cbArg1 determines the function to be performed
> +
> + cbArg1 0x0 = Get Feature Information
> +  cbRES1         Standard return codes (0, -1, -2)
> +  cbRES2, word0  Bitmap of user-selectable modes
> +     bit 0     Always off (All systems)
> +     bit 1     Always on (Travis ATG, Siberia)
> +     bit 2     Auto: ALS-based On; ALS-based Off (Travis ATG)
> +     bit 3     Auto: ALS- and input-activity-based On; input-activity based Off
> +     bit 4     Auto: Input-activity-based On; input-activity based Off
> +     bit 5     Auto: Input-activity-based On (illumination level 25%); input-activity based Off
> +     bit 6     Auto: Input-activity-based On (illumination level 50%); input-activity based Off
> +     bit 7     Auto: Input-activity-based On (illumination level 75%); input-activity based Off
> +     bit 8     Auto: Input-activity-based On (illumination level 100%); input-activity based Off
> +     bits 9-15 Reserved for future use
> +  cbRES2, byte2  Reserved for future use
> +  cbRES2, byte3  Keyboard illumination type
> +     0         Reserved
> +     1         Tasklight
> +     2         Backlight
> +     3-255     Reserved for future use
> +  cbRES3, byte0  Supported auto keyboard illumination trigger bitmap.
> +     bit 0     Any keystroke
> +     bit 1     Touchpad activity
> +     bit 2     Pointing stick
> +     bit 3     Any mouse
> +     bits 4-7  Reserved for future use
> +  cbRES3, byte1  Supported timeout unit bitmap
> +     bit 0     Seconds
> +     bit 1     Minutes
> +     bit 2     Hours
> +     bit 3     Days
> +     bits 4-7  Reserved for future use
> +  cbRES3, byte2  Number of keyboard light brightness levels
> +  cbRES4, byte0  Maximum acceptable seconds value (0 if seconds not supported).
> +  cbRES4, byte1  Maximum acceptable minutes value (0 if minutes not supported).
> +  cbRES4, byte2  Maximum acceptable hours value (0 if hours not supported).
> +  cbRES4, byte3  Maxiomum acceptable days value (0 if days not supported)

Maximum             ^

This interface appears to define all possible values for the timeout between
cbRES3[1] and cbRES4[*]. Why is the kbd_timeouts[] array a quirk with fixed
values? It seems it could indeed be dynamically determined.

> +
> +struct kbd_info {
> +	u16 modes;
> +	u8 type;
> +	u8 triggers;
> +	u8 levels;
> +	u8 seconds;
> +	u8 minutes;
> +	u8 hours;
> +	u8 days;
> +};
> +
> +
> +static u8 kbd_mode_levels[16];
> +static int kbd_mode_levels_count;

I'm confused by these. How are they different from kbd_info.levels?

We seem to check the latter first and fall back to these if that is 0.... why?

> +static int kbd_get_info(struct kbd_info *info)
> +{
> +	u8 units;
> +	int ret;
> +
> +	get_buffer();
> +
> +	buffer->input[0] = 0x0;
> +	dell_send_request(buffer, 4, 11);
> +	ret = buffer->output[0];
> +
> +	if (ret) {
> +		ret = dell_smi_error(ret);
> +		goto out;
> +	}
> +
> +	info->modes = buffer->output[1] & 0xFFFF;
> +	info->type = (buffer->output[1] >> 24) & 0xFF;
> +	info->triggers = buffer->output[2] & 0xFF;
> +	units = (buffer->output[2] >> 8) & 0xFF;
> +	info->levels = (buffer->output[2] >> 16) & 0xFF;
> +
> +	if (units & BIT(0))
> +		info->seconds = (buffer->output[3] >> 0) & 0xFF;
> +	if (units & BIT(1))
> +		info->minutes = (buffer->output[3] >> 8) & 0xFF;
> +	if (units & BIT(2))
> +		info->hours = (buffer->output[3] >> 16) & 0xFF;
> +	if (units & BIT(3))
> +		info->days = (buffer->output[3] >> 24) & 0xFF;
> +
> +out:

Please indent gotos by one space. This improves diffs context by not treating the goto label
like a function.

> +	release_buffer();
> +	return ret;
> +}
> +
> +static unsigned int kbd_get_max_level(void)
> +{
> +	if (kbd_info.levels != 0)
> +		return kbd_info.levels;
> +	if (kbd_mode_levels_count > 0)
> +		return kbd_mode_levels_count - 1;

Here for example. In what scenario does this happen?

> +	return 0;
> +}
> +



> +static inline int kbd_init_info(void)
> +{
> +	struct kbd_state state;
> +	int ret;
> +	int i;
> +
> +	ret = kbd_get_info(&kbd_info);
> +	if (ret)
> +		return ret;
> +
> +	kbd_get_state(&state);
> +
> +	/* NOTE: timeout value is stored in 6 bits so max value is 63 */
> +	if (kbd_info.seconds > 63)
> +		kbd_info.seconds = 63;
> +	if (kbd_info.minutes > 63)
> +		kbd_info.minutes = 63;
> +	if (kbd_info.hours > 63)
> +		kbd_info.hours = 63;
> +	if (kbd_info.days > 63)
> +		kbd_info.days = 63;
> +
> +	/* NOTE: On tested machines ON mode did not work and caused
> +	 *       problems (turned backlight off) so do not use it
> +	 */
> +	kbd_info.modes &= ~BIT(KBD_MODE_BIT_ON);
> +
> +	kbd_previous_level = kbd_get_level(&state);
> +	kbd_previous_mode_bit = state.mode_bit;
> +
> +	if (kbd_previous_level == 0 && kbd_get_max_level() != 0)
> +		kbd_previous_level = 1;
> +
> +	if (kbd_previous_mode_bit == KBD_MODE_BIT_OFF) {
> +		kbd_previous_mode_bit =
> +			ffs(kbd_info.modes & ~BIT(KBD_MODE_BIT_OFF));
> +		if (kbd_previous_mode_bit != 0)
> +			kbd_previous_mode_bit--;
> +	}
> +
> +	if (kbd_info.modes & (BIT(KBD_MODE_BIT_ALS) |
> +			      BIT(KBD_MODE_BIT_TRIGGER_ALS)))
> +		kbd_als_supported = true;
> +
> +	if (kbd_info.modes & (
> +	    BIT(KBD_MODE_BIT_TRIGGER_ALS) | BIT(KBD_MODE_BIT_TRIGGER) |
> +	    BIT(KBD_MODE_BIT_TRIGGER_25) | BIT(KBD_MODE_BIT_TRIGGER_50) |
> +	    BIT(KBD_MODE_BIT_TRIGGER_75) | BIT(KBD_MODE_BIT_TRIGGER_100)
> +	   ))
> +		kbd_triggers_supported = true;
> +
> +	for (i = 0; i < 16; ++i)

Doesn't this only apply to bits 5-8? Why not just loop through those?

for (i = KBD_MODE_BIT_TRIGGER_25; i <= KBD_MODE_BIT_TRIGGER_100)

The level_mode_bit check becomes unecessary.

> +		if (kbd_is_level_mode_bit(i) && (BIT(i) & kbd_info.modes))
> +			kbd_mode_levels[1+kbd_mode_levels_count++] = i;

One space around operators like + please.

What is kbd_mode_levels[0] reserved for? The current level? Isn't that what
kbd_state.level is for?

> +
> +	if (kbd_mode_levels_count > 0) {
> +		for (i = 0; i < 16; ++i) {

If 0-4 don't apply here, why loop through them? Should we just set levels[0] to
0 if it isn't one of 5-8?

If bits 9-16 are reserved, it seems it would be best to avoid checking for them
as they might return a false positive, and we'd be setting kbd_mode_levels to an
undefined value.

> +			if (BIT(i) & kbd_info.modes) {
> +				kbd_mode_levels[0] = i;
> +				break;
> +			}
> +		}
> +		kbd_mode_levels_count++;
> +	}
> +
> +	return 0;
-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v2] platform: x86: dell-laptop: Add support for keyboard backlight

[Gabriele Mazzotta]

On Wednesday 03 December 2014 00:43:21 Darren Hart wrote:
> > +     int kbd_timeouts[];
> >
> >  };
> >  
> >  static struct quirk_entry *quirks;
> >
> > @@ -76,6 +89,10 @@ static int __init dmi_matched(const struct
> > dmi_system_id *dmi)>
> >       return 1;
> >  }
> >  
> >
> > +static struct quirk_entry quirk_dell_xps13_9333 = {
> > +     .kbd_timeouts = { 0, 5, 15, 60, 5*60, 15*60, -1 },
> 
> Where did these values come from? Were they documented in the
> libsmbios project? Can you provide a URL to that? These really should
> be described by the firmware, but if they aren't, nothing we can do
> about it.

I took those values from a Windows utility provided by Dell. I tried
to find a reason for that specific list to exist, but I couldn't. The
reason why it's there is that the BIOS of my laptop accepts any timeout,
but it silently sets the timeout to 0 (i.e. illumination never off)
if a value not in that list is given. So, given the wide range of
of possible input values, we added that quirk. This is something my
laptop does, Pali's behaves differently and such a list is not needed.

Re: [PATCH v2] platform: x86: dell-laptop: Add support for keyboard backlight

[Darren Hart]

On Thu, Dec 04, 2014 at 09:50:02AM +0100, Gabriele Mazzotta wrote:
> On Wednesday 03 December 2014 00:43:21 Darren Hart wrote:
> > > +     int kbd_timeouts[];
> > >
> > >  };
> > >  
> > >  static struct quirk_entry *quirks;
> > >
> > > @@ -76,6 +89,10 @@ static int __init dmi_matched(const struct
> > > dmi_system_id *dmi)>
> > >       return 1;
> > >  }
> > >  
> > >
> > > +static struct quirk_entry quirk_dell_xps13_9333 = {
> > > +     .kbd_timeouts = { 0, 5, 15, 60, 5*60, 15*60, -1 },
> > 
> > Where did these values come from? Were they documented in the
> > libsmbios project? Can you provide a URL to that? These really should
> > be described by the firmware, but if they aren't, nothing we can do
> > about it.
> 
> I took those values from a Windows utility provided by Dell. I tried
> to find a reason for that specific list to exist, but I couldn't. The
> reason why it's there is that the BIOS of my laptop accepts any timeout,
> but it silently sets the timeout to 0 (i.e. illumination never off)
> if a value not in that list is given. So, given the wide range of
> of possible input values, we added that quirk. This is something my
> laptop does, Pali's behaves differently and such a list is not needed.

Let's get a comment above the quirk describing the scenario.


-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v2] platform: x86: dell-laptop: Add support for keyboard backlight

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1546 bytes --]

On Wednesday 03 December 2014 12:51:37 Darren Hart wrote:
> On Thu, Dec 04, 2014 at 09:50:02AM +0100, Gabriele Mazzotta 
wrote:
> > On Wednesday 03 December 2014 00:43:21 Darren Hart wrote:
> > > > +     int kbd_timeouts[];
> > > > 
> > > >  };
> > > >  
> > > >  static struct quirk_entry *quirks;
> > > > 
> > > > @@ -76,6 +89,10 @@ static int __init dmi_matched(const
> > > > struct dmi_system_id *dmi)>
> > > > 
> > > >       return 1;
> > > >  
> > > >  }
> > > > 
> > > > +static struct quirk_entry quirk_dell_xps13_9333 = {
> > > > +     .kbd_timeouts = { 0, 5, 15, 60, 5*60, 15*60, -1 },
> > > 
> > > Where did these values come from? Were they documented in
> > > the libsmbios project? Can you provide a URL to that?
> > > These really should be described by the firmware, but if
> > > they aren't, nothing we can do about it.
> > 
> > I took those values from a Windows utility provided by Dell.
> > I tried to find a reason for that specific list to exist,
> > but I couldn't. The reason why it's there is that the BIOS
> > of my laptop accepts any timeout, but it silently sets the
> > timeout to 0 (i.e. illumination never off) if a value not
> > in that list is given. So, given the wide range of of
> > possible input values, we added that quirk. This is
> > something my laptop does, Pali's behaves differently and
> > such a list is not needed.
> 
> Let's get a comment above the quirk describing the scenario.

Ok, I will add comment above quirk.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2] platform: x86: dell-laptop: Add support for keyboard backlight

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 11490 bytes --]

On Wednesday 03 December 2014 09:43:21 Darren Hart wrote:
> On Sun, Nov 23, 2014 at 03:50:45PM +0100, Pali Rohár wrote:
> > This patch adds support for configuring keyboard backlight
> > settings on supported Dell laptops. It exports kernel leds
> > interface and uses Dell SMBIOS tokens or keyboard class
> > interface.
> > 
> > With this patch it is possible to set:
> > * keyboard backlight level
> > * timeout after which will be backlight automatically turned
> > off * input activity triggers (keyboard, touchpad, mouse)
> > which enable backlight * ambient light settings
> > 
> > Settings are exported via sysfs:
> > /sys/class/leds/dell::kbd_backlight/
> > 
> > Code is based on newly released documentation by Dell in
> > libsmbios project.
> 
> Hi Pali,
> 
> I would really like to see this broken up. Possibly adding
> levels and timeouts as separate patches for example. It is
> quite difficult to review this all at once in a reasonable
> amount of time.
> 
> During this review I caught a few more CodingStyle violations,
> and raised some questions about the timeout and levels
> mechanisms.
> 
> > @@ -62,6 +71,10 @@ struct calling_interface_structure {
> > 
> >  struct quirk_entry {
> >  
> >  	u8 touchpad_led;
> > 
> > +
> > +	/* Ordered list of timeouts expressed in seconds.
> > +	 * The list must end with -1 */
> 
> Despite other instances in this file, block comments are
> documented in CodingStyle as:
> 
> /*
>  * Comment text.
>  */
> 
> The old ones should be cleaned up eventually, but new ones
> need to be done according to CodingStyle. Please correct
> throughout.
> 
> > +	int kbd_timeouts[];
> > 
> >  };
> >  
> >  static struct quirk_entry *quirks;
> > 
> > @@ -76,6 +89,10 @@ static int __init dmi_matched(const
> > struct dmi_system_id *dmi)
> > 
> >  	return 1;
> >  
> >  }
> > 
> > +static struct quirk_entry quirk_dell_xps13_9333 = {
> > +	.kbd_timeouts = { 0, 5, 15, 60, 5*60, 15*60, -1 },
> 
> Where did these values come from? Were they documented in the
> libsmbios project? Can you provide a URL to that? These
> really should be described by the firmware, but if they
> aren't, nothing we can do about it.
> 

They comes from some Windows Dell GUI utility. It is not 
documented in libsmbios project and people behind libsmbios do 
not know why only those values are accepted by BIOS for XPS 
laptop...

> > @@ -790,6 +842,964 @@ static void touchpad_led_exit(void)
> > 
> >  	led_classdev_unregister(&touchpad_led);
> >  
> >  }
> > 
> > +/* Derived from information in smbios-keyboard-ctl:
> See block comment above.
> 
> > +
> > + cbClass 4
> > + cbSelect 11
> > + Keyboar illumination
> 
> Keyboard
> 
> > + cbArg1 determines the function to be performed
> > +
> > + cbArg1 0x0 = Get Feature Information
> > +  cbRES1         Standard return codes (0, -1, -2)
> > +  cbRES2, word0  Bitmap of user-selectable modes
> > +     bit 0     Always off (All systems)
> > +     bit 1     Always on (Travis ATG, Siberia)
> > +     bit 2     Auto: ALS-based On; ALS-based Off (Travis
> > ATG) +     bit 3     Auto: ALS- and input-activity-based
> > On; input-activity based Off +     bit 4     Auto:
> > Input-activity-based On; input-activity based Off +     bit
> > 5     Auto: Input-activity-based On (illumination level
> > 25%); input-activity based Off +     bit 6     Auto:
> > Input-activity-based On (illumination level 50%);
> > input-activity based Off +     bit 7     Auto:
> > Input-activity-based On (illumination level 75%);
> > input-activity based Off +     bit 8     Auto:
> > Input-activity-based On (illumination level 100%);
> > input-activity based Off +     bits 9-15 Reserved for
> > future use
> > +  cbRES2, byte2  Reserved for future use
> > +  cbRES2, byte3  Keyboard illumination type
> > +     0         Reserved
> > +     1         Tasklight
> > +     2         Backlight
> > +     3-255     Reserved for future use
> > +  cbRES3, byte0  Supported auto keyboard illumination
> > trigger bitmap. +     bit 0     Any keystroke
> > +     bit 1     Touchpad activity
> > +     bit 2     Pointing stick
> > +     bit 3     Any mouse
> > +     bits 4-7  Reserved for future use
> > +  cbRES3, byte1  Supported timeout unit bitmap
> > +     bit 0     Seconds
> > +     bit 1     Minutes
> > +     bit 2     Hours
> > +     bit 3     Days
> > +     bits 4-7  Reserved for future use
> > +  cbRES3, byte2  Number of keyboard light brightness levels
> > +  cbRES4, byte0  Maximum acceptable seconds value (0 if
> > seconds not supported). +  cbRES4, byte1  Maximum
> > acceptable minutes value (0 if minutes not supported). + 
> > cbRES4, byte2  Maximum acceptable hours value (0 if hours
> > not supported). +  cbRES4, byte3  Maxiomum acceptable days
> > value (0 if days not supported)
> 
> Maximum             ^
> 
> This interface appears to define all possible values for the
> timeout between cbRES3[1] and cbRES4[*]. Why is the
> kbd_timeouts[] array a quirk with fixed values? It seems it
> could indeed be dynamically determined.
> 

BIOS bug (or BIOS feature?). No idea. For invalid value on XPS 
BIOS just reset keyboard backlight to someting default...

> > +
> > +struct kbd_info {
> > +	u16 modes;
> > +	u8 type;
> > +	u8 triggers;
> > +	u8 levels;
> > +	u8 seconds;
> > +	u8 minutes;
> > +	u8 hours;
> > +	u8 days;
> > +};
> > +
> > +
> > +static u8 kbd_mode_levels[16];
> > +static int kbd_mode_levels_count;
> 
> I'm confused by these. How are they different from
> kbd_info.levels?
> 

There are two interfaces how to set keyboard backlight. Both are 
documented above. One via kbd mode and one via kbd level. Some 
laptops (e.g my E6440) support only kbd mode and some (e.g XPS) 
support only kbd level. So we need to implement both interfaces 
in kernel if we want to support as more machines as possible.

> We seem to check the latter first and fall back to these if
> that is 0.... why?
> 

Because if maximum possible kbd level is 0 then we cannot use it.

> > +static int kbd_get_info(struct kbd_info *info)
> > +{
> > +	u8 units;
> > +	int ret;
> > +
> > +	get_buffer();
> > +
> > +	buffer->input[0] = 0x0;
> > +	dell_send_request(buffer, 4, 11);
> > +	ret = buffer->output[0];
> > +
> > +	if (ret) {
> > +		ret = dell_smi_error(ret);
> > +		goto out;
> > +	}
> > +
> > +	info->modes = buffer->output[1] & 0xFFFF;
> > +	info->type = (buffer->output[1] >> 24) & 0xFF;
> > +	info->triggers = buffer->output[2] & 0xFF;
> > +	units = (buffer->output[2] >> 8) & 0xFF;
> > +	info->levels = (buffer->output[2] >> 16) & 0xFF;
> > +
> > +	if (units & BIT(0))
> > +		info->seconds = (buffer->output[3] >> 0) & 0xFF;
> > +	if (units & BIT(1))
> > +		info->minutes = (buffer->output[3] >> 8) & 0xFF;
> > +	if (units & BIT(2))
> > +		info->hours = (buffer->output[3] >> 16) & 0xFF;
> > +	if (units & BIT(3))
> > +		info->days = (buffer->output[3] >> 24) & 0xFF;
> > +
> 
> > +out:
> Please indent gotos by one space. This improves diffs context
> by not treating the goto label like a function.
> 
> > +	release_buffer();
> > +	return ret;
> > +}
> > +
> > +static unsigned int kbd_get_max_level(void)
> > +{
> > +	if (kbd_info.levels != 0)
> > +		return kbd_info.levels;
> > +	if (kbd_mode_levels_count > 0)
> > +		return kbd_mode_levels_count - 1;
> 
> Here for example. In what scenario does this happen?
> 

When laptop does not support configuring keyboard backlight 
level. You can see that in documentation are more properties 
which can be configured, so if BIOS tell us that this one (level) 
does not support we cannot change it.

> > +	return 0;
> > +}
> > +
> > 
> > 
> > 
> > +static inline int kbd_init_info(void)
> > +{
> > +	struct kbd_state state;
> > +	int ret;
> > +	int i;
> > +
> > +	ret = kbd_get_info(&kbd_info);
> > +	if (ret)
> > +		return ret;
> > +
> > +	kbd_get_state(&state);
> > +
> > +	/* NOTE: timeout value is stored in 6 bits so max value is
> > 63 */ +	if (kbd_info.seconds > 63)
> > +		kbd_info.seconds = 63;
> > +	if (kbd_info.minutes > 63)
> > +		kbd_info.minutes = 63;
> > +	if (kbd_info.hours > 63)
> > +		kbd_info.hours = 63;
> > +	if (kbd_info.days > 63)
> > +		kbd_info.days = 63;
> > +
> > +	/* NOTE: On tested machines ON mode did not work and
> > caused +	 *       problems (turned backlight off) so do not
> > use it +	 */
> > +	kbd_info.modes &= ~BIT(KBD_MODE_BIT_ON);
> > +
> > +	kbd_previous_level = kbd_get_level(&state);
> > +	kbd_previous_mode_bit = state.mode_bit;
> > +
> > +	if (kbd_previous_level == 0 && kbd_get_max_level() != 0)
> > +		kbd_previous_level = 1;
> > +
> > +	if (kbd_previous_mode_bit == KBD_MODE_BIT_OFF) {
> > +		kbd_previous_mode_bit =
> > +			ffs(kbd_info.modes & ~BIT(KBD_MODE_BIT_OFF));
> > +		if (kbd_previous_mode_bit != 0)
> > +			kbd_previous_mode_bit--;
> > +	}
> > +
> > +	if (kbd_info.modes & (BIT(KBD_MODE_BIT_ALS) |
> > +			      BIT(KBD_MODE_BIT_TRIGGER_ALS)))
> > +		kbd_als_supported = true;
> > +
> > +	if (kbd_info.modes & (
> > +	    BIT(KBD_MODE_BIT_TRIGGER_ALS) |
> > BIT(KBD_MODE_BIT_TRIGGER) | +	   
> > BIT(KBD_MODE_BIT_TRIGGER_25) | BIT(KBD_MODE_BIT_TRIGGER_50)
> > | +	    BIT(KBD_MODE_BIT_TRIGGER_75) |
> > BIT(KBD_MODE_BIT_TRIGGER_100) +	   ))
> > +		kbd_triggers_supported = true;
> > +
> > +	for (i = 0; i < 16; ++i)
> 
> Doesn't this only apply to bits 5-8? Why not just loop through
> those?
> 

Some bits are reserved for future use (now undocumented). So once 
we know what they means we can adjust global enum/macro and 
changing this for loop will not be needed. 

> for (i = KBD_MODE_BIT_TRIGGER_25; i <=
> KBD_MODE_BIT_TRIGGER_100)
> 
> The level_mode_bit check becomes unecessary.
> 

I tried to write general code which check all possible modes if 
they are of type which configure level. And because some of them 
are undocumented I used this approach, so in future global 
enum/macro could be extended.

> > +		if (kbd_is_level_mode_bit(i) && (BIT(i) &
> > kbd_info.modes))
> > +			kbd_mode_levels[1+kbd_mode_levels_count++] = i;
> 
> One space around operators like + please.
> 
> What is kbd_mode_levels[0] reserved for? The current level?
> Isn't that what kbd_state.level is for?
> 

Reserved for off mode -- keyboard backlight turned off.

kbd level is for laptops which support kbd level. kbd mode is for 
laptops which support setting level via kbd mode.

> > +
> > +	if (kbd_mode_levels_count > 0) {
> > +		for (i = 0; i < 16; ++i) {
> 
> If 0-4 don't apply here, why loop through them? Should we just
> set levels[0] to 0 if it isn't one of 5-8?
> 

We know that BIOSes are buggy, so I can imagine that off mode 
(which is enum = 0) does not have to be supported... So for 
kbd_mode_levels[0] we set first supported mode (if off is not 
supported we will choose something other, so kernel code will not 
call unsupported mode).

> If bits 9-16 are reserved, it seems it would be best to avoid
> checking for them as they might return a false positive, and
> we'd be setting kbd_mode_levels to an undefined value.
> 
> > +			if (BIT(i) & kbd_info.modes) {
> > +				kbd_mode_levels[0] = i;
> > +				break;
> > +			}
> > +		}
> > +		kbd_mode_levels_count++;
> > +	}
> > +
> > +	return 0;

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2] platform: x86: dell-laptop: Add support for keyboard backlight

[Darren Hart]

On Thu, Dec 04, 2014 at 11:16:47AM +0100, Pali Rohár wrote:
> > > seconds not supported). +  cbRES4, byte1  Maximum
> > > acceptable minutes value (0 if minutes not supported). + 
> > > cbRES4, byte2  Maximum acceptable hours value (0 if hours
> > > not supported). +  cbRES4, byte3  Maxiomum acceptable days
> > > value (0 if days not supported)
> > 
> > Maximum             ^
> > 
> > This interface appears to define all possible values for the
> > timeout between cbRES3[1] and cbRES4[*]. Why is the
> > kbd_timeouts[] array a quirk with fixed values? It seems it
> > could indeed be dynamically determined.
> > 
> 
> BIOS bug (or BIOS feature?). No idea. For invalid value on XPS 
> BIOS just reset keyboard backlight to someting default...


OK good. Let's just get the quirk a comment so it's clear why it's necessary.

...
> 
> > > +
> > > +struct kbd_info {
> > > +	u16 modes;
> > > +	u8 type;
> > > +	u8 triggers;
> > > +	u8 levels;
> > > +	u8 seconds;
> > > +	u8 minutes;
> > > +	u8 hours;
> > > +	u8 days;
> > > +};
> > > +
> > > +
> > > +static u8 kbd_mode_levels[16];
> > > +static int kbd_mode_levels_count;
> > 
> > I'm confused by these. How are they different from
> > kbd_info.levels?
> > 
> 
> There are two interfaces how to set keyboard backlight. Both are 
> documented above. One via kbd mode and one via kbd level. Some 
> laptops (e.g my E6440) support only kbd mode and some (e.g XPS) 
> support only kbd level. So we need to implement both interfaces 
> in kernel if we want to support as more machines as possible.

At the very least, let's get a comment block describing these and the two
interfaces. It wasn't clear to me from the big block of register description
that there were two separate interfaces.

...
> > > +	for (i = 0; i < 16; ++i)
> > 
> > Doesn't this only apply to bits 5-8? Why not just loop through
> > those?
> > 
> 
> Some bits are reserved for future use (now undocumented). So once 
> we know what they means we can adjust global enum/macro and 
> changing this for loop will not be needed. 
> 
> > for (i = KBD_MODE_BIT_TRIGGER_25; i <=
> > KBD_MODE_BIT_TRIGGER_100)
> > 
> > The level_mode_bit check becomes unecessary.
> > 
> 
> I tried to write general code which check all possible modes if 
> they are of type which configure level. And because some of them 
> are undocumented I used this approach, so in future global 
> enum/macro could be extended.

Fair enough. I won't bike shed this.

> 
> > > +		if (kbd_is_level_mode_bit(i) && (BIT(i) &
> > > kbd_info.modes))
> > > +			kbd_mode_levels[1+kbd_mode_levels_count++] = i;
> > 
> > One space around operators like + please.
> > 
> > What is kbd_mode_levels[0] reserved for? The current level?
> > Isn't that what kbd_state.level is for?
> > 
> 
> Reserved for off mode -- keyboard backlight turned off.
> 
> kbd level is for laptops which support kbd level. kbd mode is for 
> laptops which support setting level via kbd mode.
> 
> > > +
> > > +	if (kbd_mode_levels_count > 0) {
> > > +		for (i = 0; i < 16; ++i) {
> > 
> > If 0-4 don't apply here, why loop through them? Should we just
> > set levels[0] to 0 if it isn't one of 5-8?
> > 
> 
> We know that BIOSes are buggy, so I can imagine that off mode 
> (which is enum = 0) does not have to be supported... So for 
> kbd_mode_levels[0] we set first supported mode (if off is not 
> supported we will choose something other, so kernel code will not 
> call unsupported mode).

This sort of defensive programming is good, but it does need a comment.

/*
 * Find the first supported mode and assign to kbd_mode_levels[0].
 * This should be 0 (off), but we cannot depend on the BIOS to
 * support 0.
 */

> 
> > If bits 9-16 are reserved, it seems it would be best to avoid
> > checking for them as they might return a false positive, and
> > we'd be setting kbd_mode_levels to an undefined value.

And I think you've addressed this comment above.

So in general, whenever you're working around bugs in firmware or hardware or
doing anything that might not be obvious to someone reviewing your code without
all the context you've built up, it's important to add a comment explaining why.


-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v2] platform: x86: dell-laptop: Add support for keyboard backlight

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2159 bytes --]

On Wednesday 03 December 2014 09:43:21 Darren Hart wrote:
> On Sun, Nov 23, 2014 at 03:50:45PM +0100, Pali Rohár wrote:
> > This patch adds support for configuring keyboard backlight
> > settings on supported Dell laptops. It exports kernel leds
> > interface and uses Dell SMBIOS tokens or keyboard class
> > interface.
> > 
> > With this patch it is possible to set:
> > * keyboard backlight level
> > * timeout after which will be backlight automatically turned
> > off * input activity triggers (keyboard, touchpad, mouse)
> > which enable backlight * ambient light settings
> > 
> > Settings are exported via sysfs:
> > /sys/class/leds/dell::kbd_backlight/
> > 
> > Code is based on newly released documentation by Dell in
> > libsmbios project.
> 
> Hi Pali,
> 
> I would really like to see this broken up. Possibly adding
> levels and timeouts as separate patches for example. It is
> quite difficult to review this all at once in a reasonable
> amount of time.
> 

It is really hard to split this patch into more parts which every 
one will compile and ideally also work. In my opinion every 
single git commit should be possible to compile and should also 
work (it helps also other developers to use git bisect).

And because we need to pass all (previous unchanged) values in 
smbios call we need to parse all of them.

I understand that it could be hard to review long patch, but 
there are more parts which interacts and do not work without 
other. Also some of settings (e.g keyboard backlight level) could 
be changed via different ways (and on some machines only one is 
working) and also that smbios keyboard interface has complicated 
logic...

> During this review I caught a few more CodingStyle violations,
> and raised some questions about the timeout and levels
> mechanisms.
> 

I will fix style problems in next v3 patch.
 
What to do with Dan Carpenter patch which fixing kbd_timeout 
handling? Should I integrate it into v3? It does not apply on top 
of next v3 patch, because there will be new comment about quirk 
plus style fixes...

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2] platform: x86: dell-laptop: Add support for keyboard backlight

[Darren Hart]

On Fri, Dec 05, 2014 at 03:03:49AM +0100, Pali Rohár wrote:
> On Wednesday 03 December 2014 09:43:21 Darren Hart wrote:
> > On Sun, Nov 23, 2014 at 03:50:45PM +0100, Pali Rohár wrote:
> > > This patch adds support for configuring keyboard backlight
> > > settings on supported Dell laptops. It exports kernel leds
> > > interface and uses Dell SMBIOS tokens or keyboard class
> > > interface.
> > > 
> > > With this patch it is possible to set:
> > > * keyboard backlight level
> > > * timeout after which will be backlight automatically turned
> > > off * input activity triggers (keyboard, touchpad, mouse)
> > > which enable backlight * ambient light settings
> > > 
> > > Settings are exported via sysfs:
> > > /sys/class/leds/dell::kbd_backlight/
> > > 
> > > Code is based on newly released documentation by Dell in
> > > libsmbios project.
> > 
> > Hi Pali,
> > 
> > I would really like to see this broken up. Possibly adding
> > levels and timeouts as separate patches for example. It is
> > quite difficult to review this all at once in a reasonable
> > amount of time.
> > 
> 
> It is really hard to split this patch into more parts which every 
> one will compile and ideally also work. In my opinion every 
> single git commit should be possible to compile and should also 
> work (it helps also other developers to use git bisect).

Of course, that's a given.

> 
> And because we need to pass all (previous unchanged) values in 
> smbios call we need to parse all of them.
> 
> I understand that it could be hard to review long patch, but 
> there are more parts which interacts and do not work without 
> other. Also some of settings (e.g keyboard backlight level) could 
> be changed via different ways (and on some machines only one is 
> working) and also that smbios keyboard interface has complicated 
> logic...

I was thinking about how to break it up, and I don't have an obvious answer. I
considered adding a levels interface first and then the mode_levels, but that's
a lot of work for not much savings. I don't like adding over 1000 lines to a 900
line driver in a single go to add one feature, but I didn't see obvious ways to
a) make it smaller or b) break it up.

So, please incorporate the remaining requested changes, and I'll pull it in.

Patches should really be in next for two weeks before going to Linus for a
merge. As we're already at rc7, that makes 3.19 a stretch. Get it to me by
tomorrow and I'll queue it up.... but it might not make 3.19.

For future patches, please add some additional comments to code that is
non-obvious and if there is any way to break the patches up. This will expedite
the review process (as this subsystem gets fairly little external review, you
end up blocked on me).

> 
> > During this review I caught a few more CodingStyle violations,
> > and raised some questions about the timeout and levels
> > mechanisms.
> > 
> 
> I will fix style problems in next v3 patch.
>  
> What to do with Dan Carpenter patch which fixing kbd_timeout 
> handling? Should I integrate it into v3? It does not apply on top 
> of next v3 patch, because there will be new comment about quirk 
> plus style fixes...

Please include Dan's fix and credit him with it in the commit message. Add him
to the Cc list.

Thanks Pali,

-- 
Darren Hart
Intel Open Source Technology Center

[PATCH v3] platform: x86: dell-laptop: Add support for keyboard backlight

[Pali Rohár]

This patch adds support for configuring keyboard backlight settings on supported
Dell laptops. It exports kernel leds interface and uses Dell SMBIOS tokens or
keyboard class interface.

With this patch it is possible to set:
* keyboard backlight level
* timeout after which will be backlight automatically turned off
* input activity triggers (keyboard, touchpad, mouse) which enable backlight
* ambient light settings

Settings are exported via sysfs:
/sys/class/leds/dell::kbd_backlight/

Code is based on newly released documentation by Dell in libsmbios project.

Thanks to Dan Carpenter who reported bug about unpredictable results in
quirks->kbd_timeouts for loop. His fix adds needs_kbd_timeouts flag to
quirk structure to indicate if kbd_timeouts array is empty or not.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
---
Changes since v2:
* fix bug reported by Dan Carpenter with his patch
* style fixes
* add some comments
Changes since v1:
* returns also -EIO/-ENXIO (with helper function dell_smi_error)
* simplify code to have less levels of indentation
* style fixes
* add some comments
---
 drivers/platform/x86/dell-laptop.c | 1055 +++++++++++++++++++++++++++++++++++-
 1 file changed, 1049 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 233d2ee..08d34f3 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -2,9 +2,11 @@
  *  Driver for Dell laptop extras
  *
  *  Copyright (c) Red Hat <mjg@redhat.com>
+ *  Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@gmail.com>
+ *  Copyright (c) 2014 Pali Rohár <pali.rohar@gmail.com>
  *
- *  Based on documentation in the libsmbios package, Copyright (C) 2005 Dell
- *  Inc.
+ *  Based on documentation in the libsmbios package:
+ *  Copyright (C) 2005-2014 Dell Inc.
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License version 2 as
@@ -32,6 +34,13 @@
 #include "../../firmware/dcdbas.h"
 
 #define BRIGHTNESS_TOKEN 0x7d
+#define KBD_LED_OFF_TOKEN 0x01E1
+#define KBD_LED_ON_TOKEN 0x01E2
+#define KBD_LED_AUTO_TOKEN 0x01E3
+#define KBD_LED_AUTO_25_TOKEN 0x02EA
+#define KBD_LED_AUTO_50_TOKEN 0x02EB
+#define KBD_LED_AUTO_75_TOKEN 0x02EC
+#define KBD_LED_AUTO_100_TOKEN 0x02F6
 
 /* This structure will be modified by the firmware when we enter
  * system management mode, hence the volatiles */
@@ -62,6 +71,13 @@ struct calling_interface_structure {
 
 struct quirk_entry {
 	u8 touchpad_led;
+
+	int needs_kbd_timeouts;
+	/*
+	 * Ordered list of timeouts expressed in seconds.
+	 * The list must end with -1
+	 */
+	int kbd_timeouts[];
 };
 
 static struct quirk_entry *quirks;
@@ -76,6 +92,15 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
 	return 1;
 }
 
+/*
+ * These values come from Windows utility provided by Dell. If any other value
+ * is used then BIOS silently set timeout to 0 without any error message.
+ */
+static struct quirk_entry quirk_dell_xps13_9333 = {
+	.needs_kbd_timeouts = 1,
+	.kbd_timeouts = { 0, 5, 15, 60, 5 * 60, 15 * 60, -1 },
+};
+
 static int da_command_address;
 static int da_command_code;
 static int da_num_tokens;
@@ -268,6 +293,15 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
 		},
 		.driver_data = &quirk_dell_vostro_v130,
 	},
+	{
+		.callback = dmi_matched,
+		.ident = "Dell XPS13 9333",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "XPS13 9333"),
+		},
+		.driver_data = &quirk_dell_xps13_9333,
+	},
 	{ }
 };
 
@@ -332,17 +366,29 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
 	}
 }
 
-static int find_token_location(int tokenid)
+static int find_token_id(int tokenid)
 {
 	int i;
+
 	for (i = 0; i < da_num_tokens; i++) {
 		if (da_tokens[i].tokenID == tokenid)
-			return da_tokens[i].location;
+			return i;
 	}
 
 	return -1;
 }
 
+static int find_token_location(int tokenid)
+{
+	int id;
+
+	id = find_token_id(tokenid);
+	if (id == -1)
+		return -1;
+
+	return da_tokens[id].location;
+}
+
 static struct calling_interface_buffer *
 dell_send_request(struct calling_interface_buffer *buffer, int class,
 		  int select)
@@ -363,6 +409,20 @@ dell_send_request(struct calling_interface_buffer *buffer, int class,
 	return buffer;
 }
 
+static inline int dell_smi_error(int value)
+{
+	switch (value) {
+	case 0: /* Completed successfully */
+		return 0;
+	case -1: /* Completed with error */
+		return -EIO;
+	case -2: /* Function not supported */
+		return -ENXIO;
+	default: /* Unknown error */
+		return -EINVAL;
+	}
+}
+
 /* Derived from information in DellWirelessCtl.cpp:
    Class 17, select 11 is radio control. It returns an array of 32-bit values.
 
@@ -717,7 +777,7 @@ static int dell_send_intensity(struct backlight_device *bd)
 	else
 		dell_send_request(buffer, 1, 1);
 
-out:
+ out:
 	release_buffer();
 	return ret;
 }
@@ -741,7 +801,7 @@ static int dell_get_intensity(struct backlight_device *bd)
 
 	ret = buffer->output[1];
 
-out:
+ out:
 	release_buffer();
 	return ret;
 }
@@ -790,6 +850,984 @@ static void touchpad_led_exit(void)
 	led_classdev_unregister(&touchpad_led);
 }
 
+/*
+ * Derived from information in smbios-keyboard-ctl:
+ *
+ * cbClass 4
+ * cbSelect 11
+ * Keyboard illumination
+ * cbArg1 determines the function to be performed
+ *
+ * cbArg1 0x0 = Get Feature Information
+ *  cbRES1         Standard return codes (0, -1, -2)
+ *  cbRES2, word0  Bitmap of user-selectable modes
+ *     bit 0     Always off (All systems)
+ *     bit 1     Always on (Travis ATG, Siberia)
+ *     bit 2     Auto: ALS-based On; ALS-based Off (Travis ATG)
+ *     bit 3     Auto: ALS- and input-activity-based On; input-activity based Off
+ *     bit 4     Auto: Input-activity-based On; input-activity based Off
+ *     bit 5     Auto: Input-activity-based On (illumination level 25%); input-activity based Off
+ *     bit 6     Auto: Input-activity-based On (illumination level 50%); input-activity based Off
+ *     bit 7     Auto: Input-activity-based On (illumination level 75%); input-activity based Off
+ *     bit 8     Auto: Input-activity-based On (illumination level 100%); input-activity based Off
+ *     bits 9-15 Reserved for future use
+ *  cbRES2, byte2  Reserved for future use
+ *  cbRES2, byte3  Keyboard illumination type
+ *     0         Reserved
+ *     1         Tasklight
+ *     2         Backlight
+ *     3-255     Reserved for future use
+ *  cbRES3, byte0  Supported auto keyboard illumination trigger bitmap.
+ *     bit 0     Any keystroke
+ *     bit 1     Touchpad activity
+ *     bit 2     Pointing stick
+ *     bit 3     Any mouse
+ *     bits 4-7  Reserved for future use
+ *  cbRES3, byte1  Supported timeout unit bitmap
+ *     bit 0     Seconds
+ *     bit 1     Minutes
+ *     bit 2     Hours
+ *     bit 3     Days
+ *     bits 4-7  Reserved for future use
+ *  cbRES3, byte2  Number of keyboard light brightness levels
+ *  cbRES4, byte0  Maximum acceptable seconds value (0 if seconds not supported).
+ *  cbRES4, byte1  Maximum acceptable minutes value (0 if minutes not supported).
+ *  cbRES4, byte2  Maximum acceptable hours value (0 if hours not supported).
+ *  cbRES4, byte3  Maximum acceptable days value (0 if days not supported)
+ *
+ * cbArg1 0x1 = Get Current State
+ *  cbRES1         Standard return codes (0, -1, -2)
+ *  cbRES2, word0  Bitmap of current mode state
+ *     bit 0     Always off (All systems)
+ *     bit 1     Always on (Travis ATG, Siberia)
+ *     bit 2     Auto: ALS-based On; ALS-based Off (Travis ATG)
+ *     bit 3     Auto: ALS- and input-activity-based On; input-activity based Off
+ *     bit 4     Auto: Input-activity-based On; input-activity based Off
+ *     bit 5     Auto: Input-activity-based On (illumination level 25%); input-activity based Off
+ *     bit 6     Auto: Input-activity-based On (illumination level 50%); input-activity based Off
+ *     bit 7     Auto: Input-activity-based On (illumination level 75%); input-activity based Off
+ *     bit 8     Auto: Input-activity-based On (illumination level 100%); input-activity based Off
+ *     bits 9-15 Reserved for future use
+ *     Note: Only One bit can be set
+ *  cbRES2, byte2  Currently active auto keyboard illumination triggers.
+ *     bit 0     Any keystroke
+ *     bit 1     Touchpad activity
+ *     bit 2     Pointing stick
+ *     bit 3     Any mouse
+ *     bits 4-7  Reserved for future use
+ *  cbRES2, byte3  Current Timeout
+ *     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 byte
+ *     are set upon return from the [Get feature information] call.
+ *  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.
+ *
+ * cbArg1 0x2 = Set New State
+ *  cbRES1         Standard return codes (0, -1, -2)
+ *  cbArg2, word0  Bitmap of current mode state
+ *     bit 0     Always off (All systems)
+ *     bit 1     Always on (Travis ATG, Siberia)
+ *     bit 2     Auto: ALS-based On; ALS-based Off (Travis ATG)
+ *     bit 3     Auto: ALS- and input-activity-based On; input-activity based Off
+ *     bit 4     Auto: Input-activity-based On; input-activity based Off
+ *     bit 5     Auto: Input-activity-based On (illumination level 25%); input-activity based Off
+ *     bit 6     Auto: Input-activity-based On (illumination level 50%); input-activity based Off
+ *     bit 7     Auto: Input-activity-based On (illumination level 75%); input-activity based Off
+ *     bit 8     Auto: Input-activity-based On (illumination level 100%); input-activity based Off
+ *     bits 9-15 Reserved for future use
+ *     Note: Only One bit can be set
+ *  cbArg2, byte2  Desired auto keyboard illumination triggers. Must remain inactive to allow
+ *                 keyboard to turn off automatically.
+ *     bit 0     Any keystroke
+ *     bit 1     Touchpad activity
+ *     bit 2     Pointing stick
+ *     bit 3     Any mouse
+ *     bits 4-7  Reserved for future use
+ *  cbArg2, byte3  Desired Timeout
+ *     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
+ *  cbArg3, byte0  Desired setting of ALS value that turns the light on or off.
+ *  cbArg3, byte2  Desired keyboard light level.
+ */
+
+
+enum kbd_timeout_unit {
+	KBD_TIMEOUT_SECONDS = 0,
+	KBD_TIMEOUT_MINUTES,
+	KBD_TIMEOUT_HOURS,
+	KBD_TIMEOUT_DAYS,
+};
+
+enum kbd_mode_bit {
+	KBD_MODE_BIT_OFF = 0,
+	KBD_MODE_BIT_ON,
+	KBD_MODE_BIT_ALS,
+	KBD_MODE_BIT_TRIGGER_ALS,
+	KBD_MODE_BIT_TRIGGER,
+	KBD_MODE_BIT_TRIGGER_25,
+	KBD_MODE_BIT_TRIGGER_50,
+	KBD_MODE_BIT_TRIGGER_75,
+	KBD_MODE_BIT_TRIGGER_100,
+};
+
+#define kbd_is_als_mode_bit(bit) \
+	((bit) == KBD_MODE_BIT_ALS || (bit) == KBD_MODE_BIT_TRIGGER_ALS)
+#define kbd_is_trigger_mode_bit(bit) \
+	((bit) >= KBD_MODE_BIT_TRIGGER_ALS && (bit) <= KBD_MODE_BIT_TRIGGER_100)
+#define kbd_is_level_mode_bit(bit) \
+	((bit) >= KBD_MODE_BIT_TRIGGER_25 && (bit) <= KBD_MODE_BIT_TRIGGER_100)
+
+struct kbd_info {
+	u16 modes;
+	u8 type;
+	u8 triggers;
+	u8 levels;
+	u8 seconds;
+	u8 minutes;
+	u8 hours;
+	u8 days;
+};
+
+struct kbd_state {
+	u8 mode_bit;
+	u8 triggers;
+	u8 timeout_value;
+	u8 timeout_unit;
+	u8 als_setting;
+	u8 als_value;
+	u8 level;
+};
+
+static const int kbd_tokens[] = {
+	KBD_LED_OFF_TOKEN,
+	KBD_LED_AUTO_25_TOKEN,
+	KBD_LED_AUTO_50_TOKEN,
+	KBD_LED_AUTO_75_TOKEN,
+	KBD_LED_AUTO_100_TOKEN,
+	KBD_LED_ON_TOKEN,
+};
+
+static u16 kbd_token_bits;
+
+static struct kbd_info kbd_info;
+static bool kbd_als_supported;
+static bool kbd_triggers_supported;
+
+static u8 kbd_mode_levels[16];
+static int kbd_mode_levels_count;
+
+static u8 kbd_previous_level;
+static u8 kbd_previous_mode_bit;
+
+static bool kbd_led_present;
+
+/*
+ * NOTE: there are three ways how to set keyboard backlight level.
+ * First is via kbd_state.mode_bit (assigning KBD_MODE_BIT_TRIGGER_* value)
+ * Second is via kbd_state.level (assigning number value <= kbd_info.levels)
+ * Third is via SMBIOS tokens (KBD_LED_* in kbd_tokens)
+ *
+ * There are laptops which support only one of above methods. If we want to
+ * support as much as possible machines we need to implement all of methods.
+ * First two methods are using kbd_state structure third is using SMBIOS tokens.
+ * If kbd_info.levels == 0 then machine does not support setting keyboard
+ * backlight level via second way.
+ */
+
+static int kbd_get_info(struct kbd_info *info)
+{
+	u8 units;
+	int ret;
+
+	get_buffer();
+
+	buffer->input[0] = 0x0;
+	dell_send_request(buffer, 4, 11);
+	ret = buffer->output[0];
+
+	if (ret) {
+		ret = dell_smi_error(ret);
+		goto out;
+	}
+
+	info->modes = buffer->output[1] & 0xFFFF;
+	info->type = (buffer->output[1] >> 24) & 0xFF;
+	info->triggers = buffer->output[2] & 0xFF;
+	units = (buffer->output[2] >> 8) & 0xFF;
+	info->levels = (buffer->output[2] >> 16) & 0xFF;
+
+	if (units & BIT(0))
+		info->seconds = (buffer->output[3] >> 0) & 0xFF;
+	if (units & BIT(1))
+		info->minutes = (buffer->output[3] >> 8) & 0xFF;
+	if (units & BIT(2))
+		info->hours = (buffer->output[3] >> 16) & 0xFF;
+	if (units & BIT(3))
+		info->days = (buffer->output[3] >> 24) & 0xFF;
+
+ out:
+	release_buffer();
+	return ret;
+}
+
+static unsigned int kbd_get_max_level(void)
+{
+	if (kbd_info.levels != 0)
+		return kbd_info.levels;
+	if (kbd_mode_levels_count > 0)
+		return kbd_mode_levels_count - 1;
+	return 0;
+}
+
+static int kbd_get_level(struct kbd_state *state)
+{
+	int i;
+
+	if (kbd_info.levels != 0)
+		return state->level;
+
+	if (kbd_mode_levels_count > 0) {
+		for (i = 0; i < kbd_mode_levels_count; ++i)
+			if (kbd_mode_levels[i] == state->mode_bit)
+				return i;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int kbd_set_level(struct kbd_state *state, u8 level)
+{
+	if (kbd_info.levels != 0) {
+		if (level != 0)
+			kbd_previous_level = level;
+		if (state->level == level)
+			return 0;
+		state->level = level;
+		if (level != 0 && state->mode_bit == KBD_MODE_BIT_OFF)
+			state->mode_bit = kbd_previous_mode_bit;
+		else if (level == 0 && state->mode_bit != KBD_MODE_BIT_OFF) {
+			kbd_previous_mode_bit = state->mode_bit;
+			state->mode_bit = KBD_MODE_BIT_OFF;
+		}
+		return 0;
+	}
+
+	if (kbd_mode_levels_count > 0 && level < kbd_mode_levels_count) {
+		if (level != 0)
+			kbd_previous_level = level;
+		state->mode_bit = kbd_mode_levels[level];
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int kbd_get_state(struct kbd_state *state)
+{
+	int ret;
+
+	get_buffer();
+
+	buffer->input[0] = 0x1;
+	dell_send_request(buffer, 4, 11);
+	ret = buffer->output[0];
+
+	if (ret) {
+		ret = dell_smi_error(ret);
+		goto out;
+	}
+
+	state->mode_bit = ffs(buffer->output[1] & 0xFFFF);
+	if (state->mode_bit != 0)
+		state->mode_bit--;
+
+	state->triggers = (buffer->output[1] >> 16) & 0xFF;
+	state->timeout_value = (buffer->output[1] >> 24) & 0x3F;
+	state->timeout_unit = (buffer->output[1] >> 30) & 0x3;
+	state->als_setting = buffer->output[2] & 0xFF;
+	state->als_value = (buffer->output[2] >> 8) & 0xFF;
+	state->level = (buffer->output[2] >> 16) & 0xFF;
+
+ out:
+	release_buffer();
+	return ret;
+}
+
+static int kbd_set_state(struct kbd_state *state)
+{
+	int ret;
+
+	get_buffer();
+	buffer->input[0] = 0x2;
+	buffer->input[1] = BIT(state->mode_bit) & 0xFFFF;
+	buffer->input[1] |= (state->triggers & 0xFF) << 16;
+	buffer->input[1] |= (state->timeout_value & 0x3F) << 24;
+	buffer->input[1] |= (state->timeout_unit & 0x3) << 30;
+	buffer->input[2] = state->als_setting & 0xFF;
+	buffer->input[2] |= (state->level & 0xFF) << 16;
+	dell_send_request(buffer, 4, 11);
+	ret = buffer->output[0];
+	release_buffer();
+
+	return dell_smi_error(ret);
+}
+
+static int kbd_set_state_safe(struct kbd_state *state, struct kbd_state *old)
+{
+	int ret;
+
+	ret = kbd_set_state(state);
+	if (ret == 0)
+		return 0;
+
+	/*
+	 * When setting new state failed, restore previous old one. This is
+	 * needed because on some machines BIOS set default state when setting
+	 * new failed. And default state could be also eveything is turned off.
+	 */
+
+	if (kbd_set_state(old))
+		pr_err("Setting old previous keyboard state failed\n");
+
+	return ret;
+}
+
+static int kbd_set_token_bit(u8 bit)
+{
+	int id;
+	int ret;
+
+	if (bit >= ARRAY_SIZE(kbd_tokens))
+		return -EINVAL;
+
+	id = find_token_id(kbd_tokens[bit]);
+	if (id == -1)
+		return -EINVAL;
+
+	get_buffer();
+	buffer->input[0] = da_tokens[id].location;
+	buffer->input[1] = da_tokens[id].value;
+	dell_send_request(buffer, 1, 0);
+	ret = buffer->output[0];
+	release_buffer();
+
+	return dell_smi_error(ret);
+}
+
+static int kbd_get_token_bit(u8 bit)
+{
+	int id;
+	int ret;
+	int val;
+
+	if (bit >= ARRAY_SIZE(kbd_tokens))
+		return -EINVAL;
+
+	id = find_token_id(kbd_tokens[bit]);
+	if (id == -1)
+		return -EINVAL;
+
+	get_buffer();
+	buffer->input[0] = da_tokens[id].location;
+	dell_send_request(buffer, 0, 0);
+	ret = buffer->output[0];
+	val = buffer->output[1];
+	release_buffer();
+
+	if (ret)
+		return dell_smi_error(ret);
+
+	return (val == da_tokens[id].value);
+}
+
+static int kbd_get_first_active_token_bit(void)
+{
+	int i;
+	int ret;
+
+	for (i = 0; i < ARRAY_SIZE(kbd_tokens); ++i) {
+		ret = kbd_get_token_bit(i);
+		if (ret == 1)
+			return i;
+	}
+
+	return ret;
+}
+
+static int kbd_get_valid_token_counts(void)
+{
+	return hweight16(kbd_token_bits);
+}
+
+static inline int kbd_init_info(void)
+{
+	struct kbd_state state;
+	int ret;
+	int i;
+
+	ret = kbd_get_info(&kbd_info);
+	if (ret)
+		return ret;
+
+	kbd_get_state(&state);
+
+	/* NOTE: timeout value is stored in 6 bits so max value is 63 */
+	if (kbd_info.seconds > 63)
+		kbd_info.seconds = 63;
+	if (kbd_info.minutes > 63)
+		kbd_info.minutes = 63;
+	if (kbd_info.hours > 63)
+		kbd_info.hours = 63;
+	if (kbd_info.days > 63)
+		kbd_info.days = 63;
+
+	/* NOTE: On tested machines ON mode did not work and caused
+	 *       problems (turned backlight off) so do not use it
+	 */
+	kbd_info.modes &= ~BIT(KBD_MODE_BIT_ON);
+
+	kbd_previous_level = kbd_get_level(&state);
+	kbd_previous_mode_bit = state.mode_bit;
+
+	if (kbd_previous_level == 0 && kbd_get_max_level() != 0)
+		kbd_previous_level = 1;
+
+	if (kbd_previous_mode_bit == KBD_MODE_BIT_OFF) {
+		kbd_previous_mode_bit =
+			ffs(kbd_info.modes & ~BIT(KBD_MODE_BIT_OFF));
+		if (kbd_previous_mode_bit != 0)
+			kbd_previous_mode_bit--;
+	}
+
+	if (kbd_info.modes & (BIT(KBD_MODE_BIT_ALS) |
+			      BIT(KBD_MODE_BIT_TRIGGER_ALS)))
+		kbd_als_supported = true;
+
+	if (kbd_info.modes & (
+	    BIT(KBD_MODE_BIT_TRIGGER_ALS) | BIT(KBD_MODE_BIT_TRIGGER) |
+	    BIT(KBD_MODE_BIT_TRIGGER_25) | BIT(KBD_MODE_BIT_TRIGGER_50) |
+	    BIT(KBD_MODE_BIT_TRIGGER_75) | BIT(KBD_MODE_BIT_TRIGGER_100)
+	   ))
+		kbd_triggers_supported = true;
+
+	/* kbd_mode_levels[0] is reserved, see below */
+	for (i = 0; i < 16; ++i)
+		if (kbd_is_level_mode_bit(i) && (BIT(i) & kbd_info.modes))
+			kbd_mode_levels[1 + kbd_mode_levels_count++] = i;
+
+	/*
+	 * Find the first supported mode and assign to kbd_mode_levels[0].
+	 * This should be 0 (off), but we cannot depend on the BIOS to
+	 * support 0.
+	 */
+	if (kbd_mode_levels_count > 0) {
+		for (i = 0; i < 16; ++i) {
+			if (BIT(i) & kbd_info.modes) {
+				kbd_mode_levels[0] = i;
+				break;
+			}
+		}
+		kbd_mode_levels_count++;
+	}
+
+	return 0;
+
+}
+
+static inline void kbd_init_tokens(void)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(kbd_tokens); ++i)
+		if (find_token_id(kbd_tokens[i]) != -1)
+			kbd_token_bits |= BIT(i);
+}
+
+static void kbd_init(void)
+{
+	int ret;
+
+	ret = kbd_init_info();
+	kbd_init_tokens();
+
+	if (kbd_token_bits != 0 || ret == 0)
+		kbd_led_present = true;
+}
+
+static ssize_t kbd_led_timeout_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct kbd_state new_state;
+	struct kbd_state state;
+	bool convert;
+	int value;
+	int ret;
+	char ch;
+	u8 unit;
+	int i;
+
+	ret = sscanf(buf, "%d %c", &value, &ch);
+	if (ret < 1)
+		return -EINVAL;
+	else if (ret == 1)
+		ch = 's';
+
+	if (value < 0)
+		return -EINVAL;
+
+	convert = false;
+
+	switch (ch) {
+	case 's':
+		if (value > kbd_info.seconds)
+			convert = true;
+		unit = KBD_TIMEOUT_SECONDS;
+		break;
+	case 'm':
+		if (value > kbd_info.minutes)
+			convert = true;
+		unit = KBD_TIMEOUT_MINUTES;
+		break;
+	case 'h':
+		if (value > kbd_info.hours)
+			convert = true;
+		unit = KBD_TIMEOUT_HOURS;
+		break;
+	case 'd':
+		if (value > kbd_info.days)
+			convert = true;
+		unit = KBD_TIMEOUT_DAYS;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (quirks && quirks->needs_kbd_timeouts)
+		convert = true;
+
+	if (convert) {
+		/* Convert value from current units to seconds */
+		switch (unit) {
+		case KBD_TIMEOUT_DAYS:
+			value *= 24;
+		case KBD_TIMEOUT_HOURS:
+			value *= 60;
+		case KBD_TIMEOUT_MINUTES:
+			value *= 60;
+			unit = KBD_TIMEOUT_SECONDS;
+		}
+
+		if (quirks && quirks->needs_kbd_timeouts) {
+			for (i = 0; quirks->kbd_timeouts[i] != -1; i++) {
+				if (value <= quirks->kbd_timeouts[i]) {
+					value = quirks->kbd_timeouts[i];
+					break;
+				}
+			}
+		}
+
+		if (value <= kbd_info.seconds && kbd_info.seconds) {
+			unit = KBD_TIMEOUT_SECONDS;
+		} else if (value / 60 <= kbd_info.minutes && kbd_info.minutes) {
+			value /= 60;
+			unit = KBD_TIMEOUT_MINUTES;
+		} else if (value / (60 * 60) <= kbd_info.hours && kbd_info.hours) {
+			value /= (60 * 60);
+			unit = KBD_TIMEOUT_HOURS;
+		} else if (value / (60 * 60 * 24) <= kbd_info.days && kbd_info.days) {
+			value /= (60 * 60 * 24);
+			unit = KBD_TIMEOUT_DAYS;
+		} else {
+			return -EINVAL;
+		}
+	}
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	new_state = state;
+	new_state.timeout_value = value;
+	new_state.timeout_unit = unit;
+
+	ret = kbd_set_state_safe(&new_state, &state);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t kbd_led_timeout_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct kbd_state state;
+	int ret;
+	int len;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	len = sprintf(buf, "%d", state.timeout_value);
+
+	switch (state.timeout_unit) {
+	case KBD_TIMEOUT_SECONDS:
+		return len + sprintf(buf+len, "s\n");
+	case KBD_TIMEOUT_MINUTES:
+		return len + sprintf(buf+len, "m\n");
+	case KBD_TIMEOUT_HOURS:
+		return len + sprintf(buf+len, "h\n");
+	case KBD_TIMEOUT_DAYS:
+		return len + sprintf(buf+len, "d\n");
+	default:
+		return -EINVAL;
+	}
+
+	return len;
+}
+
+static DEVICE_ATTR(stop_timeout, S_IRUGO | S_IWUSR,
+		   kbd_led_timeout_show, kbd_led_timeout_store);
+
+static const char * const kbd_led_triggers[] = {
+	"keyboard",
+	"touchpad",
+	/*"trackstick"*/ NULL, /* NOTE: trackstick is just alias for touchpad */
+	"mouse",
+};
+
+static ssize_t kbd_led_triggers_store(struct device *dev,
+				      struct device_attribute *attr,
+				      const char *buf, size_t count)
+{
+	struct kbd_state new_state;
+	struct kbd_state state;
+	bool triggers_enabled = false;
+	bool als_enabled = false;
+	bool disable_als = false;
+	bool enable_als = false;
+	int trigger_bit = -1;
+	char trigger[21];
+	int i, ret;
+
+	ret = sscanf(buf, "%20s", trigger);
+	if (ret != 1)
+		return -EINVAL;
+
+	if (trigger[0] != '+' && trigger[0] != '-')
+		return -EINVAL;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	if (kbd_als_supported)
+		als_enabled = kbd_is_als_mode_bit(state.mode_bit);
+
+	if (kbd_triggers_supported)
+		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
+
+	if (kbd_als_supported) {
+		if (strcmp(trigger, "+als") == 0) {
+			if (als_enabled)
+				return count;
+			enable_als = true;
+		} else if (strcmp(trigger, "-als") == 0) {
+			if (!als_enabled)
+				return count;
+			disable_als = true;
+		}
+	}
+
+	if (enable_als || disable_als) {
+		new_state = state;
+		if (enable_als) {
+			if (triggers_enabled)
+				new_state.mode_bit = KBD_MODE_BIT_TRIGGER_ALS;
+			else
+				new_state.mode_bit = KBD_MODE_BIT_ALS;
+		} else {
+			if (triggers_enabled) {
+				new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
+				kbd_set_level(&new_state, kbd_previous_level);
+			} else {
+				new_state.mode_bit = KBD_MODE_BIT_ON;
+			}
+		}
+		if (!(kbd_info.modes & BIT(new_state.mode_bit)))
+			return -EINVAL;
+		ret = kbd_set_state_safe(&new_state, &state);
+		if (ret)
+			return ret;
+		kbd_previous_mode_bit = new_state.mode_bit;
+		return count;
+	}
+
+	if (kbd_triggers_supported) {
+		for (i = 0; i < ARRAY_SIZE(kbd_led_triggers); ++i) {
+			if (!(kbd_info.triggers & BIT(i)))
+				continue;
+			if (!kbd_led_triggers[i])
+				continue;
+			if (strcmp(trigger+1, kbd_led_triggers[i]) != 0)
+				continue;
+			if (trigger[0] == '+' &&
+			    triggers_enabled && (state.triggers & BIT(i)))
+				return count;
+			if (trigger[0] == '-' &&
+			    (!triggers_enabled || !(state.triggers & BIT(i))))
+				return count;
+			trigger_bit = i;
+			break;
+		}
+	}
+
+	if (trigger_bit != -1) {
+		new_state = state;
+		if (trigger[0] == '+')
+			new_state.triggers |= BIT(trigger_bit);
+		else {
+			new_state.triggers &= ~BIT(trigger_bit);
+			/* NOTE: trackstick bit (2) must be disabled when
+			 *       disabling touchpad bit (1), otherwise touchpad
+			 *       bit (1) will not be disabled */
+			if (trigger_bit == 1)
+				new_state.triggers &= ~BIT(2);
+		}
+		if ((kbd_info.triggers & new_state.triggers) !=
+		    new_state.triggers)
+			return -EINVAL;
+		if (new_state.triggers && !triggers_enabled) {
+			if (als_enabled)
+				new_state.mode_bit = KBD_MODE_BIT_TRIGGER_ALS;
+			else {
+				new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
+				kbd_set_level(&new_state, kbd_previous_level);
+			}
+		} else if (new_state.triggers == 0) {
+			if (als_enabled)
+				new_state.mode_bit = KBD_MODE_BIT_ALS;
+			else
+				kbd_set_level(&new_state, 0);
+		}
+		if (!(kbd_info.modes & BIT(new_state.mode_bit)))
+			return -EINVAL;
+		ret = kbd_set_state_safe(&new_state, &state);
+		if (ret)
+			return ret;
+		if (new_state.mode_bit != KBD_MODE_BIT_OFF)
+			kbd_previous_mode_bit = new_state.mode_bit;
+		return count;
+	}
+
+	return -EINVAL;
+}
+
+static ssize_t kbd_led_triggers_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct kbd_state state;
+	bool triggers_enabled;
+	int level, i, ret;
+	int len = 0;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	len = 0;
+
+	if (kbd_triggers_supported) {
+		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
+		level = kbd_get_level(&state);
+		for (i = 0; i < ARRAY_SIZE(kbd_led_triggers); ++i) {
+			if (!(kbd_info.triggers & BIT(i)))
+				continue;
+			if (!kbd_led_triggers[i])
+				continue;
+			if ((triggers_enabled || level <= 0) &&
+			    (state.triggers & BIT(i)))
+				buf[len++] = '+';
+			else
+				buf[len++] = '-';
+			len += sprintf(buf+len, "%s ", kbd_led_triggers[i]);
+		}
+	}
+
+	if (kbd_als_supported) {
+		if (kbd_is_als_mode_bit(state.mode_bit))
+			len += sprintf(buf+len, "+als ");
+		else
+			len += sprintf(buf+len, "-als ");
+	}
+
+	if (len)
+		buf[len - 1] = '\n';
+
+	return len;
+}
+
+static DEVICE_ATTR(start_triggers, S_IRUGO | S_IWUSR,
+		   kbd_led_triggers_show, kbd_led_triggers_store);
+
+static ssize_t kbd_led_als_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct kbd_state state;
+	struct kbd_state new_state;
+	u8 setting;
+	int ret;
+
+	ret = kstrtou8(buf, 10, &setting);
+	if (ret)
+		return ret;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	new_state = state;
+	new_state.als_setting = setting;
+
+	ret = kbd_set_state_safe(&new_state, &state);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t kbd_led_als_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct kbd_state state;
+	int ret;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "%d\n", state.als_setting);
+}
+
+static DEVICE_ATTR(als_setting, S_IRUGO | S_IWUSR,
+		   kbd_led_als_show, kbd_led_als_store);
+
+static struct attribute *kbd_led_attrs[] = {
+	&dev_attr_stop_timeout.attr,
+	&dev_attr_start_triggers.attr,
+	&dev_attr_als_setting.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(kbd_led);
+
+static enum led_brightness kbd_led_level_get(struct led_classdev *led_cdev)
+{
+	int ret;
+	u16 num;
+	struct kbd_state state;
+
+	if (kbd_get_max_level()) {
+		ret = kbd_get_state(&state);
+		if (ret)
+			return 0;
+		ret = kbd_get_level(&state);
+		if (ret < 0)
+			return 0;
+		return ret;
+	}
+
+	if (kbd_get_valid_token_counts()) {
+		ret = kbd_get_first_active_token_bit();
+		if (ret < 0)
+			return 0;
+		for (num = kbd_token_bits; num != 0 && ret > 0; --ret)
+			num &= num - 1; /* clear the first bit set */
+		if (num == 0)
+			return 0;
+		return ffs(num) - 1;
+	}
+
+	pr_warn("Keyboard brightness level control not supported\n");
+	return 0;
+}
+
+static void kbd_led_level_set(struct led_classdev *led_cdev,
+			      enum led_brightness value)
+{
+	struct kbd_state state;
+	struct kbd_state new_state;
+	u16 num;
+
+	if (kbd_get_max_level()) {
+		if (kbd_get_state(&state))
+			return;
+		new_state = state;
+		if (kbd_set_level(&new_state, value))
+			return;
+		kbd_set_state_safe(&new_state, &state);
+		return;
+	}
+
+	if (kbd_get_valid_token_counts()) {
+		for (num = kbd_token_bits; num != 0 && value > 0; --value)
+			num &= num - 1; /* clear the first bit set */
+		if (num == 0)
+			return;
+		kbd_set_token_bit(ffs(num) - 1);
+		return;
+	}
+
+	pr_warn("Keyboard brightness level control not supported\n");
+}
+
+static struct led_classdev kbd_led = {
+	.name           = "dell::kbd_backlight",
+	.brightness_set = kbd_led_level_set,
+	.brightness_get = kbd_led_level_get,
+	.groups         = kbd_led_groups,
+};
+
+static int __init kbd_led_init(struct device *dev)
+{
+	kbd_init();
+	if (!kbd_led_present)
+		return -ENODEV;
+	kbd_led.max_brightness = kbd_get_max_level();
+	if (!kbd_led.max_brightness) {
+		kbd_led.max_brightness = kbd_get_valid_token_counts();
+		if (kbd_led.max_brightness)
+			kbd_led.max_brightness--;
+	}
+	return led_classdev_register(dev, &kbd_led);
+}
+
+static void brightness_set_exit(struct led_classdev *led_cdev,
+				enum led_brightness value)
+{
+	/* Don't change backlight level on exit */
+};
+
+static void kbd_led_exit(void)
+{
+	if (!kbd_led_present)
+		return;
+	kbd_led.brightness_set = brightness_set_exit;
+	led_classdev_unregister(&kbd_led);
+}
+
 static int __init dell_init(void)
 {
 	int max_intensity = 0;
@@ -842,6 +1880,8 @@ static int __init dell_init(void)
 	if (quirks && quirks->touchpad_led)
 		touchpad_led_init(&platform_device->dev);
 
+	kbd_led_init(&platform_device->dev);
+
 	dell_laptop_dir = debugfs_create_dir("dell_laptop", NULL);
 	if (dell_laptop_dir != NULL)
 		debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
@@ -909,6 +1949,7 @@ static void __exit dell_exit(void)
 	debugfs_remove_recursive(dell_laptop_dir);
 	if (quirks && quirks->touchpad_led)
 		touchpad_led_exit();
+	kbd_led_exit();
 	i8042_remove_filter(dell_laptop_i8042_filter);
 	cancel_delayed_work_sync(&dell_rfkill_work);
 	backlight_device_unregister(dell_backlight_device);
@@ -925,5 +1966,7 @@ module_init(dell_init);
 module_exit(dell_exit);
 
 MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
+MODULE_AUTHOR("Gabriele Mazzotta <gabriele.mzt@gmail.com>");
+MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
 MODULE_DESCRIPTION("Dell laptop driver");
 MODULE_LICENSE("GPL");
-- 
1.7.9.5

Re: [PATCH v3] platform: x86: dell-laptop: Add support for keyboard backlight

[Darren Hart]

On Fri, Dec 05, 2014 at 12:53:31PM +0100, Pali Rohár wrote:
> This patch adds support for configuring keyboard backlight settings on supported
> Dell laptops. It exports kernel leds interface and uses Dell SMBIOS tokens or
> keyboard class interface.
> 
> With this patch it is possible to set:
> * keyboard backlight level
> * timeout after which will be backlight automatically turned off
> * input activity triggers (keyboard, touchpad, mouse) which enable backlight
> * ambient light settings
> 
> Settings are exported via sysfs:
> /sys/class/leds/dell::kbd_backlight/
> 
> Code is based on newly released documentation by Dell in libsmbios project.
> 
> Thanks to Dan Carpenter who reported bug about unpredictable results in
> quirks->kbd_timeouts for loop. His fix adds needs_kbd_timeouts flag to
> quirk structure to indicate if kbd_timeouts array is empty or not.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>

Applied to testing with minor English corrections to the new comments.

Thank you Pali.

-- 
Darren Hart
Intel Open Source Technology Center

[PATCH] platform: x86: dell-laptop: Add support for keyboard backlight

[Gabriele Mazzotta]

This patch adds the support for the configuration of the keyboard
backlight on supported Dell laptops.

With this patch it is possible to set:
* keyboard backlight level
* timeout after which the backlight will be automatically turned off
* input activity triggers (keyboard, touchpad, mouse) that enable the backlight
* ambient light settings

The settings are exposed via /sys/class/leds/dell::kbd_backlight/

The code is based on the newly released documentation by Dell in the
libsmbios project.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
---
 .../ABI/testing/sysfs-platform-dell-laptop         |   69 ++
 drivers/platform/x86/dell-laptop.c                 | 1089 +++++++++++++++++++-
 2 files changed, 1152 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-laptop

diff --git a/Documentation/ABI/testing/sysfs-platform-dell-laptop b/Documentation/ABI/testing/sysfs-platform-dell-laptop
new file mode 100644
index 0000000..8c6a0b8
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-dell-laptop
@@ -0,0 +1,69 @@
+What:		/sys/class/leds/dell::kbd_backlight/als_enabled
+Date:		December 2014
+KernelVersion:	3.19
+Contact:	Gabriele Mazzotta <gabriele.mzt@gmail.com>,
+		Pali Rohár <pali.rohar@gmail.com>
+Description:
+		This file allows to control the automatic keyboard
+		illumination mode on some systems that have an ambient
+		light sensor. Write 1 to this file to enable the auto
+		mode, 0 to disable it.
+
+What:		/sys/class/leds/dell::kbd_backlight/als_setting
+Date:		December 2014
+KernelVersion:	3.19
+Contact:	Gabriele Mazzotta <gabriele.mzt@gmail.com>,
+		Pali Rohár <pali.rohar@gmail.com>
+Description:
+		This file allows to specifiy the on/off threshold value,
+		as reported by the ambient light sensor.
+
+What:		/sys/class/leds/dell::kbd_backlight/start_triggers
+Date:		December 2014
+KernelVersion:	3.19
+Contact:	Gabriele Mazzotta <gabriele.mzt@gmail.com>,
+		Pali Rohár <pali.rohar@gmail.com>
+Description:
+		This file allows to control the input triggers that
+		turn on the keyboard backlight illumination that is
+		disabled because of inactivity.
+		Read the file to see the triggers available. The ones
+		enabled are preceded by '+', those disabled by '-'.
+
+		To enable a trigger, write its name preceded by '+' to
+		this file. To disable a trigger, write its name preceded
+		by '-' instead.
+
+		For example, to enable the keyboard as trigger run:
+		    echo +keyboard > /sys/class/leds/dell::kbd_backlight/start_triggers
+		To disable it:
+		    echo -keyboard > /sys/class/leds/dell::kbd_backlight/start_triggers
+
+		Note that not all the available triggers can be configured.
+
+What:		/sys/class/leds/dell::kbd_backlight/stop_timeout
+Date:		December 2014
+KernelVersion:	3.19
+Contact:	Gabriele Mazzotta <gabriele.mzt@gmail.com>,
+		Pali Rohár <pali.rohar@gmail.com>
+Description:
+		This file allows to specify the interval after which the
+		keyboard illumination is disabled because of inactivity.
+		The timeouts are expressed in seconds, minutes, hours and
+		days, for which the symbols are 's', 'm', 'h' and 'd'
+		respectively.
+
+		To configure the timeout, write to this file a value along
+		with any the above units. If no unit is specified, the value
+		is assumed to be expressed in seconds.
+
+		For example, to set the timeout to 10 minutes run:
+		    echo 10m > /sys/class/leds/dell::kbd_backlight/stop_timeout
+
+		Note that when this file is read, the returned value might be
+		expressed in a different unit than the one used when the timeout
+		was set.
+
+		Also note that only some timeouts are supported and that
+		some systems might fall back to a specific timeout in case
+		an invalid timeout is written to this file.
diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 3d21efe..d688d80 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -2,9 +2,11 @@
  *  Driver for Dell laptop extras
  *
  *  Copyright (c) Red Hat <mjg@redhat.com>
+ *  Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@gmail.com>
+ *  Copyright (c) 2014 Pali Rohár <pali.rohar@gmail.com>
  *
- *  Based on documentation in the libsmbios package, Copyright (C) 2005 Dell
- *  Inc.
+ *  Based on documentation in the libsmbios package:
+ *  Copyright (C) 2005-2014 Dell Inc.
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License version 2 as
@@ -32,6 +34,13 @@
 #include "../../firmware/dcdbas.h"
 
 #define BRIGHTNESS_TOKEN 0x7d
+#define KBD_LED_OFF_TOKEN 0x01E1
+#define KBD_LED_ON_TOKEN 0x01E2
+#define KBD_LED_AUTO_TOKEN 0x01E3
+#define KBD_LED_AUTO_25_TOKEN 0x02EA
+#define KBD_LED_AUTO_50_TOKEN 0x02EB
+#define KBD_LED_AUTO_75_TOKEN 0x02EC
+#define KBD_LED_AUTO_100_TOKEN 0x02F6
 
 /* This structure will be modified by the firmware when we enter
  * system management mode, hence the volatiles */
@@ -62,6 +71,13 @@ struct calling_interface_structure {
 
 struct quirk_entry {
 	u8 touchpad_led;
+
+	int needs_kbd_timeouts;
+	/*
+	 * Ordered list of timeouts expressed in seconds.
+	 * The list must end with -1
+	 */
+	int kbd_timeouts[];
 };
 
 static struct quirk_entry *quirks;
@@ -76,6 +92,15 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
 	return 1;
 }
 
+/*
+ * These values come from Windows utility provided by Dell. If any other value
+ * is used then BIOS silently set timeout to 0 without any error message.
+ */
+static struct quirk_entry quirk_dell_xps13_9333 = {
+	.needs_kbd_timeouts = 1,
+	.kbd_timeouts = { 0, 5, 15, 60, 5 * 60, 15 * 60, -1 },
+};
+
 static int da_command_address;
 static int da_command_code;
 static int da_num_tokens;
@@ -267,6 +292,15 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
 		},
 		.driver_data = &quirk_dell_vostro_v130,
 	},
+	{
+		.callback = dmi_matched,
+		.ident = "Dell XPS13 9333",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "XPS13 9333"),
+		},
+		.driver_data = &quirk_dell_xps13_9333,
+	},
 	{ }
 };
 
@@ -331,17 +365,29 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
 	}
 }
 
-static int find_token_location(int tokenid)
+static int find_token_id(int tokenid)
 {
 	int i;
+
 	for (i = 0; i < da_num_tokens; i++) {
 		if (da_tokens[i].tokenID == tokenid)
-			return da_tokens[i].location;
+			return i;
 	}
 
 	return -1;
 }
 
+static int find_token_location(int tokenid)
+{
+	int id;
+
+	id = find_token_id(tokenid);
+	if (id == -1)
+		return -1;
+
+	return da_tokens[id].location;
+}
+
 static struct calling_interface_buffer *
 dell_send_request(struct calling_interface_buffer *buffer, int class,
 		  int select)
@@ -362,6 +408,20 @@ dell_send_request(struct calling_interface_buffer *buffer, int class,
 	return buffer;
 }
 
+static inline int dell_smi_error(int value)
+{
+	switch (value) {
+	case 0: /* Completed successfully */
+		return 0;
+	case -1: /* Completed with error */
+		return -EIO;
+	case -2: /* Function not supported */
+		return -ENXIO;
+	default: /* Unknown error */
+		return -EINVAL;
+	}
+}
+
 /* Derived from information in DellWirelessCtl.cpp:
    Class 17, select 11 is radio control. It returns an array of 32-bit values.
 
@@ -716,7 +776,7 @@ static int dell_send_intensity(struct backlight_device *bd)
 	else
 		dell_send_request(buffer, 1, 1);
 
-out:
+ out:
 	release_buffer();
 	return ret;
 }
@@ -740,7 +800,7 @@ static int dell_get_intensity(struct backlight_device *bd)
 
 	ret = buffer->output[1];
 
-out:
+ out:
 	release_buffer();
 	return ret;
 }
@@ -789,6 +849,1018 @@ static void touchpad_led_exit(void)
 	led_classdev_unregister(&touchpad_led);
 }
 
+/*
+ * Derived from information in smbios-keyboard-ctl:
+ *
+ * cbClass 4
+ * cbSelect 11
+ * Keyboard illumination
+ * cbArg1 determines the function to be performed
+ *
+ * cbArg1 0x0 = Get Feature Information
+ *  cbRES1         Standard return codes (0, -1, -2)
+ *  cbRES2, word0  Bitmap of user-selectable modes
+ *     bit 0     Always off (All systems)
+ *     bit 1     Always on (Travis ATG, Siberia)
+ *     bit 2     Auto: ALS-based On; ALS-based Off (Travis ATG)
+ *     bit 3     Auto: ALS- and input-activity-based On; input-activity based Off
+ *     bit 4     Auto: Input-activity-based On; input-activity based Off
+ *     bit 5     Auto: Input-activity-based On (illumination level 25%); input-activity based Off
+ *     bit 6     Auto: Input-activity-based On (illumination level 50%); input-activity based Off
+ *     bit 7     Auto: Input-activity-based On (illumination level 75%); input-activity based Off
+ *     bit 8     Auto: Input-activity-based On (illumination level 100%); input-activity based Off
+ *     bits 9-15 Reserved for future use
+ *  cbRES2, byte2  Reserved for future use
+ *  cbRES2, byte3  Keyboard illumination type
+ *     0         Reserved
+ *     1         Tasklight
+ *     2         Backlight
+ *     3-255     Reserved for future use
+ *  cbRES3, byte0  Supported auto keyboard illumination trigger bitmap.
+ *     bit 0     Any keystroke
+ *     bit 1     Touchpad activity
+ *     bit 2     Pointing stick
+ *     bit 3     Any mouse
+ *     bits 4-7  Reserved for future use
+ *  cbRES3, byte1  Supported timeout unit bitmap
+ *     bit 0     Seconds
+ *     bit 1     Minutes
+ *     bit 2     Hours
+ *     bit 3     Days
+ *     bits 4-7  Reserved for future use
+ *  cbRES3, byte2  Number of keyboard light brightness levels
+ *  cbRES4, byte0  Maximum acceptable seconds value (0 if seconds not supported).
+ *  cbRES4, byte1  Maximum acceptable minutes value (0 if minutes not supported).
+ *  cbRES4, byte2  Maximum acceptable hours value (0 if hours not supported).
+ *  cbRES4, byte3  Maximum acceptable days value (0 if days not supported)
+ *
+ * cbArg1 0x1 = Get Current State
+ *  cbRES1         Standard return codes (0, -1, -2)
+ *  cbRES2, word0  Bitmap of current mode state
+ *     bit 0     Always off (All systems)
+ *     bit 1     Always on (Travis ATG, Siberia)
+ *     bit 2     Auto: ALS-based On; ALS-based Off (Travis ATG)
+ *     bit 3     Auto: ALS- and input-activity-based On; input-activity based Off
+ *     bit 4     Auto: Input-activity-based On; input-activity based Off
+ *     bit 5     Auto: Input-activity-based On (illumination level 25%); input-activity based Off
+ *     bit 6     Auto: Input-activity-based On (illumination level 50%); input-activity based Off
+ *     bit 7     Auto: Input-activity-based On (illumination level 75%); input-activity based Off
+ *     bit 8     Auto: Input-activity-based On (illumination level 100%); input-activity based Off
+ *     bits 9-15 Reserved for future use
+ *     Note: Only One bit can be set
+ *  cbRES2, byte2  Currently active auto keyboard illumination triggers.
+ *     bit 0     Any keystroke
+ *     bit 1     Touchpad activity
+ *     bit 2     Pointing stick
+ *     bit 3     Any mouse
+ *     bits 4-7  Reserved for future use
+ *  cbRES2, byte3  Current Timeout
+ *     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 byte
+ *     are set upon return from the [Get feature information] call.
+ *  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.
+ *
+ * cbArg1 0x2 = Set New State
+ *  cbRES1         Standard return codes (0, -1, -2)
+ *  cbArg2, word0  Bitmap of current mode state
+ *     bit 0     Always off (All systems)
+ *     bit 1     Always on (Travis ATG, Siberia)
+ *     bit 2     Auto: ALS-based On; ALS-based Off (Travis ATG)
+ *     bit 3     Auto: ALS- and input-activity-based On; input-activity based Off
+ *     bit 4     Auto: Input-activity-based On; input-activity based Off
+ *     bit 5     Auto: Input-activity-based On (illumination level 25%); input-activity based Off
+ *     bit 6     Auto: Input-activity-based On (illumination level 50%); input-activity based Off
+ *     bit 7     Auto: Input-activity-based On (illumination level 75%); input-activity based Off
+ *     bit 8     Auto: Input-activity-based On (illumination level 100%); input-activity based Off
+ *     bits 9-15 Reserved for future use
+ *     Note: Only One bit can be set
+ *  cbArg2, byte2  Desired auto keyboard illumination triggers. Must remain inactive to allow
+ *                 keyboard to turn off automatically.
+ *     bit 0     Any keystroke
+ *     bit 1     Touchpad activity
+ *     bit 2     Pointing stick
+ *     bit 3     Any mouse
+ *     bits 4-7  Reserved for future use
+ *  cbArg2, byte3  Desired Timeout
+ *     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
+ *  cbArg3, byte0  Desired setting of ALS value that turns the light on or off.
+ *  cbArg3, byte2  Desired keyboard light level.
+ */
+
+
+enum kbd_timeout_unit {
+	KBD_TIMEOUT_SECONDS = 0,
+	KBD_TIMEOUT_MINUTES,
+	KBD_TIMEOUT_HOURS,
+	KBD_TIMEOUT_DAYS,
+};
+
+enum kbd_mode_bit {
+	KBD_MODE_BIT_OFF = 0,
+	KBD_MODE_BIT_ON,
+	KBD_MODE_BIT_ALS,
+	KBD_MODE_BIT_TRIGGER_ALS,
+	KBD_MODE_BIT_TRIGGER,
+	KBD_MODE_BIT_TRIGGER_25,
+	KBD_MODE_BIT_TRIGGER_50,
+	KBD_MODE_BIT_TRIGGER_75,
+	KBD_MODE_BIT_TRIGGER_100,
+};
+
+#define kbd_is_als_mode_bit(bit) \
+	((bit) == KBD_MODE_BIT_ALS || (bit) == KBD_MODE_BIT_TRIGGER_ALS)
+#define kbd_is_trigger_mode_bit(bit) \
+	((bit) >= KBD_MODE_BIT_TRIGGER_ALS && (bit) <= KBD_MODE_BIT_TRIGGER_100)
+#define kbd_is_level_mode_bit(bit) \
+	((bit) >= KBD_MODE_BIT_TRIGGER_25 && (bit) <= KBD_MODE_BIT_TRIGGER_100)
+
+struct kbd_info {
+	u16 modes;
+	u8 type;
+	u8 triggers;
+	u8 levels;
+	u8 seconds;
+	u8 minutes;
+	u8 hours;
+	u8 days;
+};
+
+struct kbd_state {
+	u8 mode_bit;
+	u8 triggers;
+	u8 timeout_value;
+	u8 timeout_unit;
+	u8 als_setting;
+	u8 als_value;
+	u8 level;
+};
+
+static const int kbd_tokens[] = {
+	KBD_LED_OFF_TOKEN,
+	KBD_LED_AUTO_25_TOKEN,
+	KBD_LED_AUTO_50_TOKEN,
+	KBD_LED_AUTO_75_TOKEN,
+	KBD_LED_AUTO_100_TOKEN,
+	KBD_LED_ON_TOKEN,
+};
+
+static u16 kbd_token_bits;
+
+static struct kbd_info kbd_info;
+static bool kbd_als_supported;
+static bool kbd_triggers_supported;
+
+static u8 kbd_mode_levels[16];
+static int kbd_mode_levels_count;
+
+static u8 kbd_previous_level;
+static u8 kbd_previous_mode_bit;
+
+static bool kbd_led_present;
+
+/*
+ * NOTE: there are three ways to set the keyboard backlight level.
+ * First, via kbd_state.mode_bit (assigning KBD_MODE_BIT_TRIGGER_* value).
+ * Second, via kbd_state.level (assigning numerical value <= kbd_info.levels).
+ * Third, via SMBIOS tokens (KBD_LED_* in kbd_tokens)
+ *
+ * There are laptops which support only one of these methods. If we want to
+ * support as many machines as possible we need to implement all three methods.
+ * The first two methods use the kbd_state structure. The third uses SMBIOS
+ * tokens. If kbd_info.levels == 0, the machine does not support setting the
+ * keyboard backlight level via kbd_state.level.
+ */
+
+static int kbd_get_info(struct kbd_info *info)
+{
+	u8 units;
+	int ret;
+
+	get_buffer();
+
+	buffer->input[0] = 0x0;
+	dell_send_request(buffer, 4, 11);
+	ret = buffer->output[0];
+
+	if (ret) {
+		ret = dell_smi_error(ret);
+		goto out;
+	}
+
+	info->modes = buffer->output[1] & 0xFFFF;
+	info->type = (buffer->output[1] >> 24) & 0xFF;
+	info->triggers = buffer->output[2] & 0xFF;
+	units = (buffer->output[2] >> 8) & 0xFF;
+	info->levels = (buffer->output[2] >> 16) & 0xFF;
+
+	if (units & BIT(0))
+		info->seconds = (buffer->output[3] >> 0) & 0xFF;
+	if (units & BIT(1))
+		info->minutes = (buffer->output[3] >> 8) & 0xFF;
+	if (units & BIT(2))
+		info->hours = (buffer->output[3] >> 16) & 0xFF;
+	if (units & BIT(3))
+		info->days = (buffer->output[3] >> 24) & 0xFF;
+
+ out:
+	release_buffer();
+	return ret;
+}
+
+static unsigned int kbd_get_max_level(void)
+{
+	if (kbd_info.levels != 0)
+		return kbd_info.levels;
+	if (kbd_mode_levels_count > 0)
+		return kbd_mode_levels_count - 1;
+	return 0;
+}
+
+static int kbd_get_level(struct kbd_state *state)
+{
+	int i;
+
+	if (kbd_info.levels != 0)
+		return state->level;
+
+	if (kbd_mode_levels_count > 0) {
+		for (i = 0; i < kbd_mode_levels_count; ++i)
+			if (kbd_mode_levels[i] == state->mode_bit)
+				return i;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int kbd_set_level(struct kbd_state *state, u8 level)
+{
+	if (kbd_info.levels != 0) {
+		if (level != 0)
+			kbd_previous_level = level;
+		if (state->level == level)
+			return 0;
+		state->level = level;
+		if (level != 0 && state->mode_bit == KBD_MODE_BIT_OFF)
+			state->mode_bit = kbd_previous_mode_bit;
+		else if (level == 0 && state->mode_bit != KBD_MODE_BIT_OFF) {
+			kbd_previous_mode_bit = state->mode_bit;
+			state->mode_bit = KBD_MODE_BIT_OFF;
+		}
+		return 0;
+	}
+
+	if (kbd_mode_levels_count > 0 && level < kbd_mode_levels_count) {
+		if (level != 0)
+			kbd_previous_level = level;
+		state->mode_bit = kbd_mode_levels[level];
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int kbd_get_state(struct kbd_state *state)
+{
+	int ret;
+
+	get_buffer();
+
+	buffer->input[0] = 0x1;
+	dell_send_request(buffer, 4, 11);
+	ret = buffer->output[0];
+
+	if (ret) {
+		ret = dell_smi_error(ret);
+		goto out;
+	}
+
+	state->mode_bit = ffs(buffer->output[1] & 0xFFFF);
+	if (state->mode_bit != 0)
+		state->mode_bit--;
+
+	state->triggers = (buffer->output[1] >> 16) & 0xFF;
+	state->timeout_value = (buffer->output[1] >> 24) & 0x3F;
+	state->timeout_unit = (buffer->output[1] >> 30) & 0x3;
+	state->als_setting = buffer->output[2] & 0xFF;
+	state->als_value = (buffer->output[2] >> 8) & 0xFF;
+	state->level = (buffer->output[2] >> 16) & 0xFF;
+
+ out:
+	release_buffer();
+	return ret;
+}
+
+static int kbd_set_state(struct kbd_state *state)
+{
+	int ret;
+
+	get_buffer();
+	buffer->input[0] = 0x2;
+	buffer->input[1] = BIT(state->mode_bit) & 0xFFFF;
+	buffer->input[1] |= (state->triggers & 0xFF) << 16;
+	buffer->input[1] |= (state->timeout_value & 0x3F) << 24;
+	buffer->input[1] |= (state->timeout_unit & 0x3) << 30;
+	buffer->input[2] = state->als_setting & 0xFF;
+	buffer->input[2] |= (state->level & 0xFF) << 16;
+	dell_send_request(buffer, 4, 11);
+	ret = buffer->output[0];
+	release_buffer();
+
+	return dell_smi_error(ret);
+}
+
+static int kbd_set_state_safe(struct kbd_state *state, struct kbd_state *old)
+{
+	int ret;
+
+	ret = kbd_set_state(state);
+	if (ret == 0)
+		return 0;
+
+	/*
+	 * When setting the new state fails,try to restore the previous one.
+	 * This is needed on some machines where BIOS sets a default state when
+	 * setting a new state fails. This default state could be all off.
+	 */
+
+	if (kbd_set_state(old))
+		pr_err("Setting old previous keyboard state failed\n");
+
+	return ret;
+}
+
+static int kbd_set_token_bit(u8 bit)
+{
+	int id;
+	int ret;
+
+	if (bit >= ARRAY_SIZE(kbd_tokens))
+		return -EINVAL;
+
+	id = find_token_id(kbd_tokens[bit]);
+	if (id == -1)
+		return -EINVAL;
+
+	get_buffer();
+	buffer->input[0] = da_tokens[id].location;
+	buffer->input[1] = da_tokens[id].value;
+	dell_send_request(buffer, 1, 0);
+	ret = buffer->output[0];
+	release_buffer();
+
+	return dell_smi_error(ret);
+}
+
+static int kbd_get_token_bit(u8 bit)
+{
+	int id;
+	int ret;
+	int val;
+
+	if (bit >= ARRAY_SIZE(kbd_tokens))
+		return -EINVAL;
+
+	id = find_token_id(kbd_tokens[bit]);
+	if (id == -1)
+		return -EINVAL;
+
+	get_buffer();
+	buffer->input[0] = da_tokens[id].location;
+	dell_send_request(buffer, 0, 0);
+	ret = buffer->output[0];
+	val = buffer->output[1];
+	release_buffer();
+
+	if (ret)
+		return dell_smi_error(ret);
+
+	return (val == da_tokens[id].value);
+}
+
+static int kbd_get_first_active_token_bit(void)
+{
+	int i;
+	int ret;
+
+	for (i = 0; i < ARRAY_SIZE(kbd_tokens); ++i) {
+		ret = kbd_get_token_bit(i);
+		if (ret == 1)
+			return i;
+	}
+
+	return ret;
+}
+
+static int kbd_get_valid_token_counts(void)
+{
+	return hweight16(kbd_token_bits);
+}
+
+static inline int kbd_init_info(void)
+{
+	struct kbd_state state;
+	int ret;
+	int i;
+
+	ret = kbd_get_info(&kbd_info);
+	if (ret)
+		return ret;
+
+	kbd_get_state(&state);
+
+	/* NOTE: timeout value is stored in 6 bits so max value is 63 */
+	if (kbd_info.seconds > 63)
+		kbd_info.seconds = 63;
+	if (kbd_info.minutes > 63)
+		kbd_info.minutes = 63;
+	if (kbd_info.hours > 63)
+		kbd_info.hours = 63;
+	if (kbd_info.days > 63)
+		kbd_info.days = 63;
+
+	/* NOTE: On tested machines ON mode did not work and caused
+	 *       problems (turned backlight off) so do not use it
+	 */
+	kbd_info.modes &= ~BIT(KBD_MODE_BIT_ON);
+
+	kbd_previous_level = kbd_get_level(&state);
+	kbd_previous_mode_bit = state.mode_bit;
+
+	if (kbd_previous_level == 0 && kbd_get_max_level() != 0)
+		kbd_previous_level = 1;
+
+	if (kbd_previous_mode_bit == KBD_MODE_BIT_OFF) {
+		kbd_previous_mode_bit =
+			ffs(kbd_info.modes & ~BIT(KBD_MODE_BIT_OFF));
+		if (kbd_previous_mode_bit != 0)
+			kbd_previous_mode_bit--;
+	}
+
+	if (kbd_info.modes & (BIT(KBD_MODE_BIT_ALS) |
+			      BIT(KBD_MODE_BIT_TRIGGER_ALS)))
+		kbd_als_supported = true;
+
+	if (kbd_info.modes & (
+	    BIT(KBD_MODE_BIT_TRIGGER_ALS) | BIT(KBD_MODE_BIT_TRIGGER) |
+	    BIT(KBD_MODE_BIT_TRIGGER_25) | BIT(KBD_MODE_BIT_TRIGGER_50) |
+	    BIT(KBD_MODE_BIT_TRIGGER_75) | BIT(KBD_MODE_BIT_TRIGGER_100)
+	   ))
+		kbd_triggers_supported = true;
+
+	/* kbd_mode_levels[0] is reserved, see below */
+	for (i = 0; i < 16; ++i)
+		if (kbd_is_level_mode_bit(i) && (BIT(i) & kbd_info.modes))
+			kbd_mode_levels[1 + kbd_mode_levels_count++] = i;
+
+	/*
+	 * Find the first supported mode and assign to kbd_mode_levels[0].
+	 * This should be 0 (off), but we cannot depend on the BIOS to
+	 * support 0.
+	 */
+	if (kbd_mode_levels_count > 0) {
+		for (i = 0; i < 16; ++i) {
+			if (BIT(i) & kbd_info.modes) {
+				kbd_mode_levels[0] = i;
+				break;
+			}
+		}
+		kbd_mode_levels_count++;
+	}
+
+	return 0;
+
+}
+
+static inline void kbd_init_tokens(void)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(kbd_tokens); ++i)
+		if (find_token_id(kbd_tokens[i]) != -1)
+			kbd_token_bits |= BIT(i);
+}
+
+static void kbd_init(void)
+{
+	int ret;
+
+	ret = kbd_init_info();
+	kbd_init_tokens();
+
+	if (kbd_token_bits != 0 || ret == 0)
+		kbd_led_present = true;
+}
+
+static ssize_t kbd_led_timeout_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct kbd_state new_state;
+	struct kbd_state state;
+	bool convert;
+	int value;
+	int ret;
+	char ch;
+	u8 unit;
+	int i;
+
+	ret = sscanf(buf, "%d %c", &value, &ch);
+	if (ret < 1)
+		return -EINVAL;
+	else if (ret == 1)
+		ch = 's';
+
+	if (value < 0)
+		return -EINVAL;
+
+	convert = false;
+
+	switch (ch) {
+	case 's':
+		if (value > kbd_info.seconds)
+			convert = true;
+		unit = KBD_TIMEOUT_SECONDS;
+		break;
+	case 'm':
+		if (value > kbd_info.minutes)
+			convert = true;
+		unit = KBD_TIMEOUT_MINUTES;
+		break;
+	case 'h':
+		if (value > kbd_info.hours)
+			convert = true;
+		unit = KBD_TIMEOUT_HOURS;
+		break;
+	case 'd':
+		if (value > kbd_info.days)
+			convert = true;
+		unit = KBD_TIMEOUT_DAYS;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (quirks && quirks->needs_kbd_timeouts)
+		convert = true;
+
+	if (convert) {
+		/* Convert value from current units to seconds */
+		switch (unit) {
+		case KBD_TIMEOUT_DAYS:
+			value *= 24;
+		case KBD_TIMEOUT_HOURS:
+			value *= 60;
+		case KBD_TIMEOUT_MINUTES:
+			value *= 60;
+			unit = KBD_TIMEOUT_SECONDS;
+		}
+
+		if (quirks && quirks->needs_kbd_timeouts) {
+			for (i = 0; quirks->kbd_timeouts[i] != -1; i++) {
+				if (value <= quirks->kbd_timeouts[i]) {
+					value = quirks->kbd_timeouts[i];
+					break;
+				}
+			}
+		}
+
+		if (value <= kbd_info.seconds && kbd_info.seconds) {
+			unit = KBD_TIMEOUT_SECONDS;
+		} else if (value / 60 <= kbd_info.minutes && kbd_info.minutes) {
+			value /= 60;
+			unit = KBD_TIMEOUT_MINUTES;
+		} else if (value / (60 * 60) <= kbd_info.hours && kbd_info.hours) {
+			value /= (60 * 60);
+			unit = KBD_TIMEOUT_HOURS;
+		} else if (value / (60 * 60 * 24) <= kbd_info.days && kbd_info.days) {
+			value /= (60 * 60 * 24);
+			unit = KBD_TIMEOUT_DAYS;
+		} else {
+			return -EINVAL;
+		}
+	}
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	new_state = state;
+	new_state.timeout_value = value;
+	new_state.timeout_unit = unit;
+
+	ret = kbd_set_state_safe(&new_state, &state);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t kbd_led_timeout_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct kbd_state state;
+	int ret;
+	int len;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	len = sprintf(buf, "%d", state.timeout_value);
+
+	switch (state.timeout_unit) {
+	case KBD_TIMEOUT_SECONDS:
+		return len + sprintf(buf+len, "s\n");
+	case KBD_TIMEOUT_MINUTES:
+		return len + sprintf(buf+len, "m\n");
+	case KBD_TIMEOUT_HOURS:
+		return len + sprintf(buf+len, "h\n");
+	case KBD_TIMEOUT_DAYS:
+		return len + sprintf(buf+len, "d\n");
+	default:
+		return -EINVAL;
+	}
+
+	return len;
+}
+
+static DEVICE_ATTR(stop_timeout, S_IRUGO | S_IWUSR,
+		   kbd_led_timeout_show, kbd_led_timeout_store);
+
+static const char * const kbd_led_triggers[] = {
+	"keyboard",
+	"touchpad",
+	/*"trackstick"*/ NULL, /* NOTE: trackstick is just alias for touchpad */
+	"mouse",
+};
+
+static ssize_t kbd_led_triggers_store(struct device *dev,
+				      struct device_attribute *attr,
+				      const char *buf, size_t count)
+{
+	struct kbd_state new_state;
+	struct kbd_state state;
+	bool triggers_enabled = false;
+	int trigger_bit = -1;
+	char trigger[21];
+	int i, ret;
+
+	ret = sscanf(buf, "%20s", trigger);
+	if (ret != 1)
+		return -EINVAL;
+
+	if (trigger[0] != '+' && trigger[0] != '-')
+		return -EINVAL;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	if (kbd_triggers_supported)
+		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
+
+	if (kbd_triggers_supported) {
+		for (i = 0; i < ARRAY_SIZE(kbd_led_triggers); ++i) {
+			if (!(kbd_info.triggers & BIT(i)))
+				continue;
+			if (!kbd_led_triggers[i])
+				continue;
+			if (strcmp(trigger+1, kbd_led_triggers[i]) != 0)
+				continue;
+			if (trigger[0] == '+' &&
+			    triggers_enabled && (state.triggers & BIT(i)))
+				return count;
+			if (trigger[0] == '-' &&
+			    (!triggers_enabled || !(state.triggers & BIT(i))))
+				return count;
+			trigger_bit = i;
+			break;
+		}
+	}
+
+	if (trigger_bit != -1) {
+		new_state = state;
+		if (trigger[0] == '+')
+			new_state.triggers |= BIT(trigger_bit);
+		else {
+			new_state.triggers &= ~BIT(trigger_bit);
+			/* NOTE: trackstick bit (2) must be disabled when
+			 *       disabling touchpad bit (1), otherwise touchpad
+			 *       bit (1) will not be disabled */
+			if (trigger_bit == 1)
+				new_state.triggers &= ~BIT(2);
+		}
+		if ((kbd_info.triggers & new_state.triggers) !=
+		    new_state.triggers)
+			return -EINVAL;
+		if (new_state.triggers && !triggers_enabled) {
+			new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
+			kbd_set_level(&new_state, kbd_previous_level);
+		} else if (new_state.triggers == 0) {
+			kbd_set_level(&new_state, 0);
+		}
+		if (!(kbd_info.modes & BIT(new_state.mode_bit)))
+			return -EINVAL;
+		ret = kbd_set_state_safe(&new_state, &state);
+		if (ret)
+			return ret;
+		if (new_state.mode_bit != KBD_MODE_BIT_OFF)
+			kbd_previous_mode_bit = new_state.mode_bit;
+		return count;
+	}
+
+	return -EINVAL;
+}
+
+static ssize_t kbd_led_triggers_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct kbd_state state;
+	bool triggers_enabled;
+	int level, i, ret;
+	int len = 0;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	len = 0;
+
+	if (kbd_triggers_supported) {
+		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
+		level = kbd_get_level(&state);
+		for (i = 0; i < ARRAY_SIZE(kbd_led_triggers); ++i) {
+			if (!(kbd_info.triggers & BIT(i)))
+				continue;
+			if (!kbd_led_triggers[i])
+				continue;
+			if ((triggers_enabled || level <= 0) &&
+			    (state.triggers & BIT(i)))
+				buf[len++] = '+';
+			else
+				buf[len++] = '-';
+			len += sprintf(buf+len, "%s ", kbd_led_triggers[i]);
+		}
+	}
+
+	if (len)
+		buf[len - 1] = '\n';
+
+	return len;
+}
+
+static DEVICE_ATTR(start_triggers, S_IRUGO | S_IWUSR,
+		   kbd_led_triggers_show, kbd_led_triggers_store);
+
+static ssize_t kbd_led_als_enabled_store(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t count)
+{
+	struct kbd_state new_state;
+	struct kbd_state state;
+	bool triggers_enabled = false;
+	int enable;
+	int ret;
+
+	ret = kstrtoint(buf, 0, &enable);
+	if (ret)
+		return ret;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	if (enable == kbd_is_als_mode_bit(state.mode_bit))
+		return count;
+
+	new_state = state;
+
+	if (kbd_triggers_supported)
+		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
+
+	if (enable) {
+		if (triggers_enabled)
+			new_state.mode_bit = KBD_MODE_BIT_TRIGGER_ALS;
+		else
+			new_state.mode_bit = KBD_MODE_BIT_ALS;
+	} else {
+		if (triggers_enabled) {
+			new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
+			kbd_set_level(&new_state, kbd_previous_level);
+		} else {
+			new_state.mode_bit = KBD_MODE_BIT_ON;
+		}
+	}
+	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
+		return -EINVAL;
+
+	ret = kbd_set_state_safe(&new_state, &state);
+	if (ret)
+		return ret;
+	kbd_previous_mode_bit = new_state.mode_bit;
+
+	return count;
+}
+
+static ssize_t kbd_led_als_enabled_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct kbd_state state;
+	bool enabled = false;
+	int ret;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+	enabled = kbd_is_als_mode_bit(state.mode_bit);
+
+	return sprintf(buf, "%d\n", enabled ? 1 : 0);
+}
+
+static DEVICE_ATTR(als_enabled, S_IRUGO | S_IWUSR,
+		   kbd_led_als_enabled_show, kbd_led_als_enabled_store);
+
+static ssize_t kbd_led_als_setting_store(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t count)
+{
+	struct kbd_state state;
+	struct kbd_state new_state;
+	u8 setting;
+	int ret;
+
+	ret = kstrtou8(buf, 10, &setting);
+	if (ret)
+		return ret;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	new_state = state;
+	new_state.als_setting = setting;
+
+	ret = kbd_set_state_safe(&new_state, &state);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t kbd_led_als_setting_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct kbd_state state;
+	int ret;
+
+	ret = kbd_get_state(&state);
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "%d\n", state.als_setting);
+}
+
+static DEVICE_ATTR(als_setting, S_IRUGO | S_IWUSR,
+		   kbd_led_als_setting_show, kbd_led_als_setting_store);
+
+static struct attribute *kbd_led_attrs[] = {
+	&dev_attr_stop_timeout.attr,
+	&dev_attr_start_triggers.attr,
+	NULL,
+};
+
+static const struct attribute_group kbd_led_group = {
+	.attrs = kbd_led_attrs,
+};
+
+static struct attribute *kbd_led_als_attrs[] = {
+	&dev_attr_als_enabled.attr,
+	&dev_attr_als_setting.attr,
+	NULL,
+};
+
+static const struct attribute_group kbd_led_als_group = {
+	.attrs = kbd_led_als_attrs,
+};
+
+static const struct attribute_group *kbd_led_groups[] = {
+	&kbd_led_group,
+	&kbd_led_als_group,
+	NULL,
+};
+
+static enum led_brightness kbd_led_level_get(struct led_classdev *led_cdev)
+{
+	int ret;
+	u16 num;
+	struct kbd_state state;
+
+	if (kbd_get_max_level()) {
+		ret = kbd_get_state(&state);
+		if (ret)
+			return 0;
+		ret = kbd_get_level(&state);
+		if (ret < 0)
+			return 0;
+		return ret;
+	}
+
+	if (kbd_get_valid_token_counts()) {
+		ret = kbd_get_first_active_token_bit();
+		if (ret < 0)
+			return 0;
+		for (num = kbd_token_bits; num != 0 && ret > 0; --ret)
+			num &= num - 1; /* clear the first bit set */
+		if (num == 0)
+			return 0;
+		return ffs(num) - 1;
+	}
+
+	pr_warn("Keyboard brightness level control not supported\n");
+	return 0;
+}
+
+static void kbd_led_level_set(struct led_classdev *led_cdev,
+			      enum led_brightness value)
+{
+	struct kbd_state state;
+	struct kbd_state new_state;
+	u16 num;
+
+	if (kbd_get_max_level()) {
+		if (kbd_get_state(&state))
+			return;
+		new_state = state;
+		if (kbd_set_level(&new_state, value))
+			return;
+		kbd_set_state_safe(&new_state, &state);
+		return;
+	}
+
+	if (kbd_get_valid_token_counts()) {
+		for (num = kbd_token_bits; num != 0 && value > 0; --value)
+			num &= num - 1; /* clear the first bit set */
+		if (num == 0)
+			return;
+		kbd_set_token_bit(ffs(num) - 1);
+		return;
+	}
+
+	pr_warn("Keyboard brightness level control not supported\n");
+}
+
+static struct led_classdev kbd_led = {
+	.name           = "dell::kbd_backlight",
+	.brightness_set = kbd_led_level_set,
+	.brightness_get = kbd_led_level_get,
+	.groups         = kbd_led_groups,
+};
+
+static int __init kbd_led_init(struct device *dev)
+{
+	kbd_init();
+	if (!kbd_led_present)
+		return -ENODEV;
+	if (!kbd_als_supported)
+		kbd_led_groups[1] = NULL;
+	kbd_led.max_brightness = kbd_get_max_level();
+	if (!kbd_led.max_brightness) {
+		kbd_led.max_brightness = kbd_get_valid_token_counts();
+		if (kbd_led.max_brightness)
+			kbd_led.max_brightness--;
+	}
+	return led_classdev_register(dev, &kbd_led);
+}
+
+static void brightness_set_exit(struct led_classdev *led_cdev,
+				enum led_brightness value)
+{
+	/* Don't change backlight level on exit */
+};
+
+static void kbd_led_exit(void)
+{
+	if (!kbd_led_present)
+		return;
+	kbd_led.brightness_set = brightness_set_exit;
+	led_classdev_unregister(&kbd_led);
+}
+
 static int __init dell_init(void)
 {
 	int max_intensity = 0;
@@ -841,6 +1913,8 @@ static int __init dell_init(void)
 	if (quirks && quirks->touchpad_led)
 		touchpad_led_init(&platform_device->dev);
 
+	kbd_led_init(&platform_device->dev);
+
 	dell_laptop_dir = debugfs_create_dir("dell_laptop", NULL);
 	if (dell_laptop_dir != NULL)
 		debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
@@ -908,6 +1982,7 @@ static void __exit dell_exit(void)
 	debugfs_remove_recursive(dell_laptop_dir);
 	if (quirks && quirks->touchpad_led)
 		touchpad_led_exit();
+	kbd_led_exit();
 	i8042_remove_filter(dell_laptop_i8042_filter);
 	cancel_delayed_work_sync(&dell_rfkill_work);
 	backlight_device_unregister(dell_backlight_device);
@@ -924,5 +1999,7 @@ module_init(dell_init);
 module_exit(dell_exit);
 
 MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
+MODULE_AUTHOR("Gabriele Mazzotta <gabriele.mzt@gmail.com>");
+MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
 MODULE_DESCRIPTION("Dell laptop driver");
 MODULE_LICENSE("GPL");
-- 
2.1.4

Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1632 bytes --]

On Thursday 19 February 2015 11:58:29 Gabriele Mazzotta wrote:
> This patch adds the support for the configuration of the
> keyboard backlight on supported Dell laptops.
> 
> With this patch it is possible to set:
> * keyboard backlight level
> * timeout after which the backlight will be automatically
> turned off * input activity triggers (keyboard, touchpad,
> mouse) that enable the backlight * ambient light settings
> 
> The settings are exposed via
> /sys/class/leds/dell::kbd_backlight/
> 
> The code is based on the newly released documentation by Dell
> in the libsmbios project.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  .../ABI/testing/sysfs-platform-dell-laptop         |   69 ++
>  drivers/platform/x86/dell-laptop.c                 | 1089
> +++++++++++++++++++- 2 files changed, 1152 insertions(+), 6
> deletions(-)
>  create mode 100644
> Documentation/ABI/testing/sysfs-platform-dell-laptop
> 

This patch is same as combination of previous, just contains all 
changes in single patch file. Difference between v3.19-rc5 and 
current version is just als code was moved to separate functions 
and als backlight can be enabled via new sysfs attribute (like in 
second patch which was sent).

I tested this patch on top of linus tree (commit 79513d0) and 
keyboard backlight works fine.

Darren, what do you think, can be this patch which brings 
keyboard backlight support finally moved to mainline kernel?

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight

[Darren Hart]

On Sun, Feb 22, 2015 at 12:04:23PM +0100, Pali Rohár wrote:
> On Thursday 19 February 2015 11:58:29 Gabriele Mazzotta wrote:
> > This patch adds the support for the configuration of the
> > keyboard backlight on supported Dell laptops.
> > 
> > With this patch it is possible to set:
> > * keyboard backlight level
> > * timeout after which the backlight will be automatically
> > turned off * input activity triggers (keyboard, touchpad,
> > mouse) that enable the backlight * ambient light settings
> > 
> > The settings are exposed via
> > /sys/class/leds/dell::kbd_backlight/
> > 
> > The code is based on the newly released documentation by Dell
> > in the libsmbios project.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >  .../ABI/testing/sysfs-platform-dell-laptop         |   69 ++
> >  drivers/platform/x86/dell-laptop.c                 | 1089
> > +++++++++++++++++++- 2 files changed, 1152 insertions(+), 6
> > deletions(-)
> >  create mode 100644
> > Documentation/ABI/testing/sysfs-platform-dell-laptop
> > 
> 
> This patch is same as combination of previous, just contains all 
> changes in single patch file. Difference between v3.19-rc5 and 
> current version is just als code was moved to separate functions 
> and als backlight can be enabled via new sysfs attribute (like in 
> second patch which was sent).
> 
> I tested this patch on top of linus tree (commit 79513d0) and 
> keyboard backlight works fine.
> 
> Darren, what do you think, can be this patch which brings 
> keyboard backlight support finally moved to mainline kernel?

So sorry for the delay folks, bad week :( This landed too late for 3.20... her
durr 4.0, but should make 4.1. Expect to see it in next in the next few days.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH] platform: x86: dell-laptop: Add support for keyboard backlight

[Darren Hart]

On Thu, Feb 19, 2015 at 11:58:29AM +0100, Gabriele Mazzotta wrote:
> This patch adds the support for the configuration of the keyboard
> backlight on supported Dell laptops.
> 
> With this patch it is possible to set:
> * keyboard backlight level
> * timeout after which the backlight will be automatically turned off
> * input activity triggers (keyboard, touchpad, mouse) that enable the backlight
> * ambient light settings
> 
> The settings are exposed via /sys/class/leds/dell::kbd_backlight/
> 
> The code is based on the newly released documentation by Dell in the
> libsmbios project.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>

Queued for 4.1, thanks everyone.

-- 
Darren Hart
Intel Open Source Technology Center