[PATCH v3 1/3] thinkpad_acpi: Move tablet detection into separate function

[PATCH v3 1/3] thinkpad_acpi: Move tablet detection into separate function

[Lyude]

The hotkey events and ACPI handles used for detecting tablet mode on a
few of the newer thinkpad models (Yoga X1 and the Yoga 260 specifically)
have been changed around, so unfortunately this means we're definitely
going to need to probe for multiple types of tablet mode support. Since
the hotkey_init() is already a lot larger than it should be, let's split
up this detection into its own function to make things a little easier
to read.

As well, since we're going to have multiple types of tablet modes, make
hotkey_tablet into an enum so we can also use it to indicate the type of
tablet mode reporting the machine supports.

Suggested by Daniel Martin <consume.noise@gmail.com>
Signed-off-by: Lyude <lyude@redhat.com>
Cc: Daniel Martin <consume.noise@gmail.com>

Signed-off-by: Lyude <lyude@redhat.com>
---
Changes since v1:
- Don't use bool for in_tablet_mode (fixes complaints from kbuild test
  robot)
Changes since v2:
- Move changes out of git message
- Fix commit message
- Fix variable ordering
- Remove explicit value assignment in hotkey_tablet enum

 drivers/platform/x86/thinkpad_acpi.c | 50 +++++++++++++++++++++++++++---------
 1 file changed, 38 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index b65ce75..dfa1af8 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -302,7 +302,10 @@ static struct {
 	u32 hotkey:1;
 	u32 hotkey_mask:1;
 	u32 hotkey_wlsw:1;
-	u32 hotkey_tablet:1;
+	enum {
+		TP_HOTKEY_TABLET_NONE = 0,
+		TP_HOTKEY_TABLET_USES_MHKG,
+	} hotkey_tablet;
 	u32 kbdlight:1;
 	u32 light:1;
 	u32 light_status:1;
@@ -3117,6 +3120,34 @@ static const struct tpacpi_quirk tpacpi_hotkey_qtable[] __initconst = {
 typedef u16 tpacpi_keymap_entry_t;
 typedef tpacpi_keymap_entry_t tpacpi_keymap_t[TPACPI_HOTKEY_MAP_LEN];
 
+static int
+hotkey_init_tablet_mode(void)
+{
+	bool in_tablet_mode;
+	char *type;
+	int res;
+
+	/* For X41t, X60t, X61t Tablets... */
+	if (acpi_evalf(hkey_handle, &res, "MHKG", "qd")) {
+		tp_features.hotkey_tablet = TP_HOTKEY_TABLET_USES_MHKG;
+		in_tablet_mode = !!(res & TP_HOTKEY_TABLET_MASK);
+		type = "MHKG";
+	}
+
+	if (!tp_features.hotkey_tablet)
+		return 0;
+
+	pr_info("Tablet mode switch found (type: %s), currently in %s mode\n",
+		type, in_tablet_mode ? "tablet" : "laptop");
+
+	res = add_to_attr_set(hotkey_dev_attributes,
+			      &dev_attr_hotkey_tablet_mode.attr);
+	if (res)
+		return -1;
+
+	return in_tablet_mode;
+}
+
 static int __init hotkey_init(struct ibm_init_struct *iibm)
 {
 	/* Requirements for changing the default keymaps:
@@ -3464,17 +3495,6 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 		res = add_to_attr_set(hotkey_dev_attributes,
 				&dev_attr_hotkey_radio_sw.attr);
 
-	/* For X41t, X60t, X61t Tablets... */
-	if (!res && acpi_evalf(hkey_handle, &status, "MHKG", "qd")) {
-		tp_features.hotkey_tablet = 1;
-		tabletsw_state = !!(status & TP_HOTKEY_TABLET_MASK);
-		pr_info("possible tablet mode switch found; "
-			"ThinkPad in %s mode\n",
-			(tabletsw_state) ? "tablet" : "laptop");
-		res = add_to_attr_set(hotkey_dev_attributes,
-				&dev_attr_hotkey_tablet_mode.attr);
-	}
-
 	if (!res)
 		res = register_attr_set_with_sysfs(
 				hotkey_dev_attributes,
@@ -3482,6 +3502,12 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 	if (res)
 		goto err_exit;
 
+	res = hotkey_init_tablet_mode();
+	if (res < 0)
+		goto err_exit;
+
+	tabletsw_state = res;
+
 	/* Set up key map */
 	hotkey_keycode_map = kmalloc(TPACPI_HOTKEY_MAP_SIZE,
 					GFP_KERNEL);
-- 
2.7.4

[PATCH 2/3] thinkpad_acpi: Don't repeat ourselves in hotkey_init_tablet_mode()

[Lyude]

There's no need to have multiple copies of the logic we use for checking
whether or not we're in tablet mode, so just use
hotkey_get_tablet_mode() when checking the initial state in
hotkey_init_tablet_mode().

Cc: Daniel Martin <consume.noise@gmail.com>
Signed-off-by: Lyude <lyude@redhat.com>
---
 drivers/platform/x86/thinkpad_acpi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index dfa1af8..ad93c41 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3130,13 +3130,16 @@ hotkey_init_tablet_mode(void)
 	/* For X41t, X60t, X61t Tablets... */
 	if (acpi_evalf(hkey_handle, &res, "MHKG", "qd")) {
 		tp_features.hotkey_tablet = TP_HOTKEY_TABLET_USES_MHKG;
-		in_tablet_mode = !!(res & TP_HOTKEY_TABLET_MASK);
 		type = "MHKG";
 	}
 
 	if (!tp_features.hotkey_tablet)
 		return 0;
 
+	res = hotkey_get_tablet_mode(&in_tablet_mode);
+	if (res)
+		return res;
+
 	pr_info("Tablet mode switch found (type: %s), currently in %s mode\n",
 		type, in_tablet_mode ? "tablet" : "laptop");
 
-- 
2.7.4

[PATCH v4 3/3] thinkpad_acpi: Add support for X1 Yoga (2016) Tablet Mode

[Lyude]

For whatever reason, the X1 Yoga doesn't support the normal method of
querying for tablet mode. Instead of providing the MHKG method under the
hotkey handle, we're instead given the CMMD method under the EC handle.
Values on this handle are either 0x1, laptop mode, or 0x6, tablet mode.

Cc: Daniel Martin <consume.noise@gmail.com>
Signed-off-by: Lyude <lyude@redhat.com>
---
Changes since v1:
- Clarify kernel output when finding the tablet mode switch
Changes since v2:
- Rebase on top of previous patch
- Use an enum for hotkey_tablet. This does make a bit more sense then
  just adding another flag.
- Call hotkey_tablet_mode_notify_change() when getting TABLET_CHANGED
  event.
Changes since v3:
- Move changelog below ---

 drivers/platform/x86/thinkpad_acpi.c | 37 ++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index ad93c41..c60701e 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -190,6 +190,9 @@ enum tpacpi_hkey_event_t {
 	TP_HKEY_EV_LID_OPEN		= 0x5002, /* laptop lid opened */
 	TP_HKEY_EV_TABLET_TABLET	= 0x5009, /* tablet swivel up */
 	TP_HKEY_EV_TABLET_NOTEBOOK	= 0x500a, /* tablet swivel down */
+	TP_HKEY_EV_TABLET_CHANGED	= 0x60c0, /* X1 Yoga (2016):
+						   * enter/leave tablet mode
+						   */
 	TP_HKEY_EV_PEN_INSERTED		= 0x500b, /* tablet pen inserted */
 	TP_HKEY_EV_PEN_REMOVED		= 0x500c, /* tablet pen removed */
 	TP_HKEY_EV_BRGHT_CHANGED	= 0x5010, /* backlight control event */
@@ -305,6 +308,7 @@ static struct {
 	enum {
 		TP_HOTKEY_TABLET_NONE = 0,
 		TP_HOTKEY_TABLET_USES_MHKG,
+		TP_HOTKEY_TABLET_USES_CMMD,
 	} hotkey_tablet;
 	u32 kbdlight:1;
 	u32 light:1;
@@ -2062,6 +2066,8 @@ static void hotkey_poll_setup(const bool may_warn);
 
 /* HKEY.MHKG() return bits */
 #define TP_HOTKEY_TABLET_MASK (1 << 3)
+/* ThinkPad X1 Yoga (2016) */
+#define TP_EC_CMMD_TABLET_MODE 0x6
 
 static int hotkey_get_wlsw(void)
 {
@@ -2086,10 +2092,23 @@ static int hotkey_get_tablet_mode(int *status)
 {
 	int s;
 
-	if (!acpi_evalf(hkey_handle, &s, "MHKG", "d"))
-		return -EIO;
+	switch (tp_features.hotkey_tablet) {
+	case TP_HOTKEY_TABLET_USES_MHKG:
+		if (!acpi_evalf(hkey_handle, &s, "MHKG", "d"))
+			return -EIO;
+
+		*status = ((s & TP_HOTKEY_TABLET_MASK) != 0);
+		break;
+	case TP_HOTKEY_TABLET_USES_CMMD:
+		if (!acpi_evalf(ec_handle, &s, "CMMD", "d"))
+			return -EIO;
+
+		*status = (s == TP_EC_CMMD_TABLET_MODE);
+		break;
+	default:
+		break;
+	}
 
-	*status = ((s & TP_HOTKEY_TABLET_MASK) != 0);
 	return 0;
 }
 
@@ -3127,10 +3146,14 @@ hotkey_init_tablet_mode(void)
 	char *type;
 	int res;
 
-	/* For X41t, X60t, X61t Tablets... */
 	if (acpi_evalf(hkey_handle, &res, "MHKG", "qd")) {
+		/* For X41t, X60t, X61t Tablets... */
 		tp_features.hotkey_tablet = TP_HOTKEY_TABLET_USES_MHKG;
 		type = "MHKG";
+	} else if (acpi_evalf(ec_handle, &res, "CMMD", "qd")) {
+		/* For X1 Yoga (2016) */
+		tp_features.hotkey_tablet = TP_HOTKEY_TABLET_USES_CMMD;
+		type = "CMMD";
 	}
 
 	if (!tp_features.hotkey_tablet)
@@ -3928,6 +3951,12 @@ static bool hotkey_notify_6xxx(const u32 hkey,
 		*ignore_acpi_ev = true;
 		return true;
 
+	case TP_HKEY_EV_TABLET_CHANGED:
+		tpacpi_input_send_tabletsw();
+		hotkey_tablet_mode_notify_change();
+		*send_acpi_ev = false;
+		break;
+
 	default:
 		pr_warn("unknown possible thermal alarm or keyboard event received\n");
 		known = false;
-- 
2.7.4

Re: [PATCH v3 1/3] thinkpad_acpi: Move tablet detection into separate function

[Henrique de Moraes Holschuh]

On Mon, 07 Nov 2016, Lyude wrote:
> @@ -3117,6 +3120,34 @@ static const struct tpacpi_quirk tpacpi_hotkey_qtable[] __initconst = {
>  typedef u16 tpacpi_keymap_entry_t;
>  typedef tpacpi_keymap_entry_t tpacpi_keymap_t[TPACPI_HOTKEY_MAP_LEN];
>  
> +static int
> +hotkey_init_tablet_mode(void)
> +{

Function definiton all in one line, please.

Provided that you fix that minor nit,

Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

-- 
  Henrique Holschuh

Re: [PATCH v4 3/3] thinkpad_acpi: Add support for X1 Yoga (2016) Tablet Mode

[Henrique de Moraes Holschuh]

On Mon, 07 Nov 2016, Lyude wrote:
> For whatever reason, the X1 Yoga doesn't support the normal method of
> querying for tablet mode. Instead of providing the MHKG method under the
> hotkey handle, we're instead given the CMMD method under the EC handle.
> Values on this handle are either 0x1, laptop mode, or 0x6, tablet mode.
> 
> Cc: Daniel Martin <consume.noise@gmail.com>
> Signed-off-by: Lyude <lyude@redhat.com>
> ---
> Changes since v1:
> - Clarify kernel output when finding the tablet mode switch
> Changes since v2:
> - Rebase on top of previous patch
> - Use an enum for hotkey_tablet. This does make a bit more sense then
>   just adding another flag.
> - Call hotkey_tablet_mode_notify_change() when getting TABLET_CHANGED
>   event.
> Changes since v3:
> - Move changelog below ---
> 
>  drivers/platform/x86/thinkpad_acpi.c | 37 ++++++++++++++++++++++++++++++++----
>  1 file changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index ad93c41..c60701e 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -190,6 +190,9 @@ enum tpacpi_hkey_event_t {
>  	TP_HKEY_EV_LID_OPEN		= 0x5002, /* laptop lid opened */
>  	TP_HKEY_EV_TABLET_TABLET	= 0x5009, /* tablet swivel up */
>  	TP_HKEY_EV_TABLET_NOTEBOOK	= 0x500a, /* tablet swivel down */
> +	TP_HKEY_EV_TABLET_CHANGED	= 0x60c0, /* X1 Yoga (2016):
> +						   * enter/leave tablet mode
> +						   */

I was not going to nitpick this, but since a new respin will be needed
for the first patch anyway, can you add the BIOS and EC model numbers of
the X1 Yoga (2016) in a comment somewhere in your changes (e.g. in a
comment next to the new tablet mode code you added) ?

These model numbers are the product componet codes used in the
thinkpad-apci-style hardware black/white lists.  You should easily find
them in the dmidecode output, thinkpad-acpi logging, or in the BIOS and
EC firmware update web pages for that specific thinkpad.

Also, please send me off-list a copy of the binary ACPI tables (DSDT and
all XSDTs), and dmidecode output (with serial numbers and UUIDs
XXXX-out) for the X1 Yoga.

Finally, while this is not a requirement at this time, if you could
update the driver documentation with the newer events and any visible
changes to the tablet mode stuff, I'd be grateful.

It is in Documentation/laptops/thinkpad-acpi.txt

I apologise for not asking for these earlier.

Thanks,

-- 
  Henrique Holschuh

Re: [PATCH 2/3] thinkpad_acpi: Don't repeat ourselves in hotkey_init_tablet_mode()

[Henrique de Moraes Holschuh]

On Mon, 07 Nov 2016, Lyude wrote:
> There's no need to have multiple copies of the logic we use for checking
> whether or not we're in tablet mode, so just use
> hotkey_get_tablet_mode() when checking the initial state in
> hotkey_init_tablet_mode().

...

> @@ -3130,13 +3130,16 @@ hotkey_init_tablet_mode(void)
>  	/* For X41t, X60t, X61t Tablets... */
>  	if (acpi_evalf(hkey_handle, &res, "MHKG", "qd")) {
>  		tp_features.hotkey_tablet = TP_HOTKEY_TABLET_USES_MHKG;
> -		in_tablet_mode = !!(res & TP_HOTKEY_TABLET_MASK);
>  		type = "MHKG";
>  	}
>  
>  	if (!tp_features.hotkey_tablet)
>  		return 0;
>  
> +	res = hotkey_get_tablet_mode(&in_tablet_mode);
> +	if (res)
> +		return res;
> +

Won't this way of doing things cause the ACPI methods to be called
twice in a row?

The hotkey_init_tablet_mode() code does mode detection, and thus
hotkey_get_tablet_mode() must be kept in sync with it *anyway* (and not
the opposite), so it doesn't look like a very relevant maintenance
burden...

-- 
  Henrique Holschuh

Re: [PATCH v3 1/3] thinkpad_acpi: Move tablet detection into separate function

[Daniel Martin]

Sorry, I didn't had the time to look at the patches earlier.

Now, I did and was wondering why hotkey_tablet_mode doesn't show up in
sysfs anymore ...

On 7 November 2016 at 18:10, Lyude <lyude@redhat.com> wrote:
> @@ -3464,17 +3495,6 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
>                 res = add_to_attr_set(hotkey_dev_attributes,
>                                 &dev_attr_hotkey_radio_sw.attr);
>
> -       /* For X41t, X60t, X61t Tablets... */
> -       if (!res && acpi_evalf(hkey_handle, &status, "MHKG", "qd")) {
> -               tp_features.hotkey_tablet = 1;
> -               tabletsw_state = !!(status & TP_HOTKEY_TABLET_MASK);
> -               pr_info("possible tablet mode switch found; "
> -                       "ThinkPad in %s mode\n",
> -                       (tabletsw_state) ? "tablet" : "laptop");
> -               res = add_to_attr_set(hotkey_dev_attributes,
> -                               &dev_attr_hotkey_tablet_mode.attr);
> -       }
> -
>         if (!res)
>                 res = register_attr_set_with_sysfs(
>                                 hotkey_dev_attributes,
> @@ -3482,6 +3502,12 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
>         if (res)
>                 goto err_exit;
>
> +       res = hotkey_init_tablet_mode();
> +       if (res < 0)
> +               goto err_exit;
> +
> +       tabletsw_state = res;
> +
>         /* Set up key map */
>         hotkey_keycode_map = kmalloc(TPACPI_HOTKEY_MAP_SIZE,
>                                         GFP_KERNEL);
> --
> 2.7.4
>

This block has to be before register_attr_set_with_sysfs().

With that fixed and a small patch (will be sent soon) on top it works
on my Yoga 260, so:
Tested-by: Daniel Martin <consume.noise@gmail.com>