support Thinkpad X1 Carbon's adaptive keyboard

RFC PATCH Superseded Shuduo Sang 2 participants 3 messages Mar 3, 2014

Subsystems (2)

ACPI x86 platform drivers

lore

Share & tools

lore

Shuduo Sang

author

2014-03-03 07:20 UTC #0 lore raw

RFC PATCH 1/1 support Thinkpad X1 Carbon's adaptive keyboard

Hi all,

We are working together with Lenovo to enable thinkpad X1 Carbon's fancy feature, Adaptive Keyboard[1] for Linux. Adaptive keyboard has five modes on Windows including Home mode, Web browser mode, Web conference mode, Function mode and Lay-flat mode. We enabled Home mode and Function mode currently. Will try to find out how to enable other modes later. Please review and comment attached patch.

Thanks,
Shuduo

[1]:http://shop.lenovo.com/us/en/laptops/thinkpad/x-series/x1-carbon/#features

Quoted 1 line

From da9d43beaa23071558a8031950cc21cc93946ec6 Mon Sep 17 00:00:00 2001

From: Shuduo Sang <[email protected]>
Date: Mon, 3 Mar 2014 14:29:32 +0800
Subject: [PATCH] support thinkpad X1 Carbon's adaptive keyboard

Thinkpad X1 Carbon's adaptive keyboard has five modes including Home mode, Web browser mode, Web conference mode, Function mode and Lay-flat mode. We support Home mode and Function mode currently.

Signed-off-by Bruce Ma <[email protected]>

Signed-off-by Shuduo Sang <[email protected]>

---
 drivers/platform/x86/thinkpad_acpi.c | 97
++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

(quoted diff) +0 -0

b/drivers/platform/x86/thinkpad_acpi.c
index defb6af..fcb738e 100644

drivers/platform/x86/thinkpad_acpi.c +97 -0

109 lines not on this page · raw message

Signature · 1 line

1.9.0

Bjørn Mork

2014-03-03 11:04 UTC #1 lore raw

Quoted 60 lines · Shuduo Sang

Hi all,
We are working together with Lenovo to enable thinkpad X1 Carbon's fancy feature, Adaptive Keyboard[1] for Linux. Adaptive keyboard has five modes on Windows including Home mode, Web browser mode, Web conference mode, Function mode and Lay-flat mode. We enabled Home mode and Function mode currently. Will try to find out how to enable other modes later. Please review and comment attached patch.
Thanks,
Shuduo
[1]:http://shop.lenovo.com/us/en/laptops/thinkpad/x-series/x1-carbon/#features
From da9d43beaa23071558a8031950cc21cc93946ec6 Mon Sep 17 00:00:00 2001
From: Shuduo Sang <[email protected]>
Date: Mon, 3 Mar 2014 14:29:32 +0800
Subject: [PATCH] support thinkpad X1 Carbon's adaptive keyboard
Thinkpad X1 Carbon's adaptive keyboard has five modes including Home mode, Web browser mode, Web conference mode, Function mode and Lay-flat mode. We support Home mode and Function mode currently.
Signed-off-by Bruce Ma <[email protected]>
Signed-off-by Shuduo Sang <[email protected]>
---
 drivers/platform/x86/thinkpad_acpi.c | 97
++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)
drivers/platform/x86/thinkpad_acpi.c +20 -0
index defb6af..fcb738e 100644
@@ -3437,6 +3437,101 @@ err_exit:
 	return (res < 0)? res : 1;
 }
 
+/* Thinkpad X1 Carbon support 5 modes including Home mode, Web browser
+ * mode, Web conference mode, Function mode and Lay-flat mode.
+ * We support Home mode and Function mode currently.
+ *
+ * Will consider support rest of modes in future.
+ *
+ */
+enum ADAPTIVE_KEY_MODE {
+	HOME_MODE,
+	WEB_BROWSER_MODE,
+	WEB_CONFERENCE_MODE,
+	FUNCTION_MODE,
+	LAYFLAT_MODE
+};
+
+int adaptive_keyboard_modes[] = {
+	HOME_MODE,
+/*	WEB_BROWSER_MODE = 2,
+	WEB_CONFERENCE_MODE = 3, */
+	FUNCTION_MODE

drivers/platform/x86/thinkpad_acpi.c err_exit: · line 3469

Quoted 10 lines

drivers/platform/x86/thinkpad_acpi.c +10 -0

+};
+
+#define DFR_CHANGE_ROW			0x101
+#define DFR_SHOW_QUICKVIEW_ROW		0x102
+
+/* press Fn key a while second, it will switch to Function Mode. Then
+ * release Fn key, previous mode be restored.
+ */
+bool adaptive_keyboard_mode_is_saved;
+int adaptive_keybarod_prev_mode;

speling: s/keybarod/keyboard/

You need to change it where this variable is used below as well.

drivers/platform/x86/thinkpad_acpi.c err_exit: · line 3474

Quoted 4 lines

drivers/platform/x86/thinkpad_acpi.c +4 -0

+static int adaptive_keyboard_get_next_mode(int mode)
+{
+	int i;
+	int max_mode = sizeof(adaptive_keyboard_modes)/sizeof(int) - 1;

ARRAY_SIZE

Quoted 19 lines

drivers/platform/x86/thinkpad_acpi.c +19 -0

+	for (i = 0; i <= max_mode; i++) {
+		if (adaptive_keyboard_modes[i] == mode)
+			break;
+	}
+
+	if (i >= max_mode)
+		i = 0;
+	else
+		i++;
+
+	return adaptive_keyboard_modes[i];
+}
+
+static bool adaptive_keyboard_hotkey_notify_hotkey(unsigned int scancode)
+{
+	u32 current_mode = 0;
+	int new_mode = 0;
+
+	switch (scancode) {

drivers/platform/x86/thinkpad_acpi.c err_exit: · line 3504

Quoted 10 lines

drivers/platform/x86/thinkpad_acpi.c +10 -0

+	case DFR_CHANGE_ROW:
+		if (adaptive_keyboard_mode_is_saved) {
+			new_mode = adaptive_keybarod_prev_mode;
+			adaptive_keyboard_mode_is_saved = false;
+		} else {
+			if (!acpi_evalf(
+					hkey_handle, &current_mode,
+					"GTRW", "dd", 0)) {
+				pr_err("Cannot read adaptive keyboard mode\n");
+				return false;

Not sure about this return value (resulting in an "unhandled HKEY event" message). See below.

Quoted 6 lines

drivers/platform/x86/thinkpad_acpi.c +6 -0

+			} else {
+				new_mode = adaptive_keyboard_get_next_mode(
+						current_mode);
+			}
+		}
+

drivers/platform/x86/thinkpad_acpi.c err_exit: · line 3520

Quoted 10 lines

drivers/platform/x86/thinkpad_acpi.c +10 -0

+		if (!acpi_evalf(hkey_handle, NULL, "STRW", "vd", new_mode))
+			pr_err("Cannot set adaptive keyboard mode\n");
+
+		return true;
+
+	case DFR_SHOW_QUICKVIEW_ROW:
+		if (!acpi_evalf(hkey_handle,
+				&adaptive_keybarod_prev_mode,
+				"GTRW", "dd", 0)) {
+			pr_err("Cannot read adaptive keyboard mode\n");

You should at least return a consistent result for this error. Either return true above or false here.

Quoted 18 lines

drivers/platform/x86/thinkpad_acpi.c +14 -0

+		} else {
+			adaptive_keyboard_mode_is_saved = true;
+
+			if (!acpi_evalf(hkey_handle,
+					NULL, "STRW", "vd", FUNCTION_MODE))
+				pr_err("Cannot set adaptive keyboard mode\n");
+		}
+		return true;
+
+	default:
+		return false;
+	}
+}
+
 static bool hotkey_notify_hotkey(const u32 hkey,
 				 bool *send_acpi_ev,
 				 bool *ignore_acpi_ev)

drivers/platform/x86/thinkpad_acpi.c static bool hotkey_notify_hotkey(const u32 hkey, · line 3557

Quoted 7 lines

drivers/platform/x86/thinkpad_acpi.c +2 -0

@@ -3456,6 +3551,8 @@ static bool hotkey_notify_hotkey(const u32 hkey,
 			*ignore_acpi_ev = true;
 		}
 		return true;
+	} else {
+		return adaptive_keyboard_hotkey_notify_hotkey(scancode);
 	}
 	return false;

This isn't reached anymore...

drivers/platform/x86/thinkpad_acpi.c static bool hotkey_notify_hotkey(const u32 hkey, · line 3558

Quoted 1 line

drivers/platform/x86/thinkpad_acpi.c +0 -0
 }

I also think you should consider how this UI will look if you ever decide to enable all the modes. AFAICS, you will then have inconsistent behaviour for the DFR_CHANGE_ROW scancode in FUNCTION_MODE:

a) if FUNCTION_MODE was enabled by DFR_SHOW_QUICKVIEW_ROW then next mode is x, where x can be any mode active before the DFR_SHOW_QUICKVIEW_ROW event _including_ FUNCTION_MODE
b) if FUNCTION_MODE was enabled by DFR_CHANGE_ROW then next mode is LAYFLAT_MODE

So the user won't really know the effect of a DFR_CHANGE_ROW event in FUNCTION_MODE.

Wouldn't it make more sense if DFR_SHOW_QUICKVIEW_ROW always toggled between FUNCTION_MODE and some other mode, and DFR_CHANGE_ROW always cycled through the modes?

Or is the UI locked by whatever the Windows driver does? (I don't have any X1 Carbon, so I don't know how the users expect this to work)

Bjørn

Shuduo Sang

author

2014-03-03 13:06 UTC #2 lore raw

hi Bjørn,

Thanks for your comment. I will send V2 later.

Quoted 174 lines · Bjørn Mork

Quoted 60 lines · Shuduo Sang
Hi all,
We are working together with Lenovo to enable thinkpad X1 Carbon's fancy feature, Adaptive Keyboard[1] for Linux. Adaptive keyboard has five modes on Windows including Home mode, Web browser mode, Web conference mode, Function mode and Lay-flat mode. We enabled Home mode and Function mode currently. Will try to find out how to enable other modes later. Please review and comment attached patch.
Thanks,
Shuduo
[1]:http://shop.lenovo.com/us/en/laptops/thinkpad/x-series/x1-carbon/#features
From da9d43beaa23071558a8031950cc21cc93946ec6 Mon Sep 17 00:00:00 2001
From: Shuduo Sang <[email protected]>
Date: Mon, 3 Mar 2014 14:29:32 +0800
Subject: [PATCH] support thinkpad X1 Carbon's adaptive keyboard
Thinkpad X1 Carbon's adaptive keyboard has five modes including Home mode, Web browser mode, Web conference mode, Function mode and Lay-flat mode. We support Home mode and Function mode currently.
Signed-off-by Bruce Ma <[email protected]>
Signed-off-by Shuduo Sang <[email protected]>
---
 drivers/platform/x86/thinkpad_acpi.c | 97
++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)
drivers/platform/x86/thinkpad_acpi.c +20 -0
index defb6af..fcb738e 100644
@@ -3437,6 +3437,101 @@ err_exit:
 	return (res < 0)? res : 1;
 }
 
+/* Thinkpad X1 Carbon support 5 modes including Home mode, Web browser
+ * mode, Web conference mode, Function mode and Lay-flat mode.
+ * We support Home mode and Function mode currently.
+ *
+ * Will consider support rest of modes in future.
+ *
+ */
+enum ADAPTIVE_KEY_MODE {
+	HOME_MODE,
+	WEB_BROWSER_MODE,
+	WEB_CONFERENCE_MODE,
+	FUNCTION_MODE,
+	LAYFLAT_MODE
+};
+
+int adaptive_keyboard_modes[] = {
+	HOME_MODE,
+/*	WEB_BROWSER_MODE = 2,
+	WEB_CONFERENCE_MODE = 3, */
+	FUNCTION_MODE
drivers/platform/x86/thinkpad_acpi.c err_exit: · line 3469
Quoted 10 lines
drivers/platform/x86/thinkpad_acpi.c +10 -0
+};
+
+#define DFR_CHANGE_ROW			0x101
+#define DFR_SHOW_QUICKVIEW_ROW		0x102
+
+/* press Fn key a while second, it will switch to Function Mode. Then
+ * release Fn key, previous mode be restored.
+ */
+bool adaptive_keyboard_mode_is_saved;
+int adaptive_keybarod_prev_mode;
speling: s/keybarod/keyboard/
You need to change it where this variable is used below as well.
drivers/platform/x86/thinkpad_acpi.c err_exit: · line 3474
Quoted 4 lines
drivers/platform/x86/thinkpad_acpi.c +4 -0
+static int adaptive_keyboard_get_next_mode(int mode)
+{
+	int i;
+	int max_mode = sizeof(adaptive_keyboard_modes)/sizeof(int) - 1;
ARRAY_SIZE
Quoted 19 lines
drivers/platform/x86/thinkpad_acpi.c +19 -0
+	for (i = 0; i <= max_mode; i++) {
+		if (adaptive_keyboard_modes[i] == mode)
+			break;
+	}
+
+	if (i >= max_mode)
+		i = 0;
+	else
+		i++;
+
+	return adaptive_keyboard_modes[i];
+}
+
+static bool adaptive_keyboard_hotkey_notify_hotkey(unsigned int scancode)
+{
+	u32 current_mode = 0;
+	int new_mode = 0;
+
+	switch (scancode) {
drivers/platform/x86/thinkpad_acpi.c err_exit: · line 3504
Quoted 10 lines
drivers/platform/x86/thinkpad_acpi.c +10 -0
+	case DFR_CHANGE_ROW:
+		if (adaptive_keyboard_mode_is_saved) {
+			new_mode = adaptive_keybarod_prev_mode;
+			adaptive_keyboard_mode_is_saved = false;
+		} else {
+			if (!acpi_evalf(
+					hkey_handle, &current_mode,
+					"GTRW", "dd", 0)) {
+				pr_err("Cannot read adaptive keyboard mode\n");
+				return false;
Not sure about this return value (resulting in an "unhandled HKEY event" message). See below.
Quoted 6 lines
drivers/platform/x86/thinkpad_acpi.c +6 -0
+			} else {
+				new_mode = adaptive_keyboard_get_next_mode(
+						current_mode);
+			}
+		}
+
drivers/platform/x86/thinkpad_acpi.c err_exit: · line 3520
Quoted 10 lines
drivers/platform/x86/thinkpad_acpi.c +10 -0
+		if (!acpi_evalf(hkey_handle, NULL, "STRW", "vd", new_mode))
+			pr_err("Cannot set adaptive keyboard mode\n");
+
+		return true;
+
+	case DFR_SHOW_QUICKVIEW_ROW:
+		if (!acpi_evalf(hkey_handle,
+				&adaptive_keybarod_prev_mode,
+				"GTRW", "dd", 0)) {
+			pr_err("Cannot read adaptive keyboard mode\n");
You should at least return a consistent result for this error. Either return true above or false here.
Quoted 18 lines
drivers/platform/x86/thinkpad_acpi.c +14 -0
+		} else {
+			adaptive_keyboard_mode_is_saved = true;
+
+			if (!acpi_evalf(hkey_handle,
+					NULL, "STRW", "vd", FUNCTION_MODE))
+				pr_err("Cannot set adaptive keyboard mode\n");
+		}
+		return true;
+
+	default:
+		return false;
+	}
+}
+
 static bool hotkey_notify_hotkey(const u32 hkey,
 				 bool *send_acpi_ev,
 				 bool *ignore_acpi_ev)
drivers/platform/x86/thinkpad_acpi.c static bool hotkey_notify_hotkey(const u32 hkey, · line 3557
Quoted 7 lines
drivers/platform/x86/thinkpad_acpi.c +2 -0
@@ -3456,6 +3551,8 @@ static bool hotkey_notify_hotkey(const u32 hkey,
 			*ignore_acpi_ev = true;
 		}
 		return true;
+	} else {
+		return adaptive_keyboard_hotkey_notify_hotkey(scancode);
 	}
 	return false;
This isn't reached anymore...
drivers/platform/x86/thinkpad_acpi.c static bool hotkey_notify_hotkey(const u32 hkey, · line 3558
Quoted 1 line
drivers/platform/x86/thinkpad_acpi.c +0 -0
 }
I also think you should consider how this UI will look if you ever decide to enable all the modes. AFAICS, you will then have inconsistent behaviour for the DFR_CHANGE_ROW scancode in FUNCTION_MODE:

Quoted 6 lines

a) if FUNCTION_MODE was enabled by DFR_SHOW_QUICKVIEW_ROW then next mode is x, where x can be any mode active before the DFR_SHOW_QUICKVIEW_ROW event _including_ FUNCTION_MODE
b) if FUNCTION_MODE was enabled by DFR_CHANGE_ROW then next mode is LAYFLAT_MODE

Press Fn key and don't release will switch Adaptive Keyboard to Function mode whatever current mode is. And release Fn key will restore it to old mode including Function mode. So current logic should not confuse end user here. :)

LAYFLAT_MODE can be triged by putting screen to flat only, not by Fn key. Maybe list LAYFLAT_MODE with other modes will confuse others and make program need special logic to switch mode. I'm thinking over this too. Since unsupported modes still not clear, I wish following patch can handle them good.

Quoted 3 lines

So the user won't really know the effect of a DFR_CHANGE_ROW event in FUNCTION_MODE.

DRF_CHANGE_ROW event in FUNCTION_MODE should always change keyboard to Home mode.

Quoted 7 lines

Wouldn't it make more sense if DFR_SHOW_QUICKVIEW_ROW always toggled between FUNCTION_MODE and some other mode, and DFR_CHANGE_ROW always cycled through the modes?
Or is the UI locked by whatever the Windows driver does? (I don't have any X1 Carbon, so I don't know how the users expect this to work)

On Windows, DFR_CHANGE_ROW will switch keyboard among four modes. On Linux, currently switch between Home mode and Function mode. Since not all Windows function can be mapped to Linux like Web conference, so Linux user don't expect those modes appear too. I think it makes sense too. :)

Quoted 3 lines

Bjørn