Hi,
On 2/11/21 8:35 AM, Alexander Kobel wrote:
> ThinkPad X1 Tablets emit HKEY 0x4012 and 0x4013 events when a keyboard
> cover is attached/detached or folded to the back of the device. They are
> used to switch from normal to tablet mode in userspace; e.g., to offer
> touch keyboard choices when focus goes to a text box and no keyboard is
> attached, or to enable autorotation of the display according to the
> built-in orientation sensor.
>
> This patch handles the two events by issuing corresponding
> SW_TABLET_MODE notifications to the ACPI userspace.
>
> Tested as working on a ThinkPad X1 Tablet Gen 2, 20JCS00C00, and as
> non-interfering with a ThinkPad X1 Carbon 7th, 20QESABM02 (normal
> clamshell, so it does not have a keyboard cover).
>
> Signed-off-by: Alexander Kobel <a-kobel@xxxxxxxxxx>
Thank you for your patch, overall this looks good, I have one
request for some refactoring inline / below.
> ---
> drivers/platform/x86/thinkpad_acpi.c | 100 ++++++++++++++++++++++++++-
> 1 file changed, 99 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index c404706379d9..09adcaf49fe8 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -145,6 +145,45 @@ enum {
> TP_ACPI_WGSV_STATE_UWBPWR = 0x0020, /* UWB radio enabled */
> };
>
> +/* ThinkPad GTOP ("Get Tablet OPtion") methods */
> +enum {
> + TP_GTOP_CMD_QUERY_VERSION = 0x0000,
> + TP_GTOP_CMD_QUERY_INTERFACE_TYPE = 0x0100,
> + TP_GTOP_CMD_GET_ATTACH_OPTION = 0x0200,
> +};
> +
> +#define TP_GTOP_MINIMUM_VERSION 0x0103
> +
> +enum {
> + TP_GTOP_INTERFACE_TYPE_RESERVED = 0,
> + TP_GTOP_INTERFACE_TYPE_ANY_TYPE = 1,
> + TP_GTOP_INTERFACE_TYPE_TP_HELIX = 2,
> + TP_GTOP_INTERFACE_TYPE_TP10 = 3,
> + TP_GTOP_INTERFACE_TYPE_TP_X1_TAB = 4,
> +};
> +
> +/* Attach option states for GTOP Type 1 interfaces ("Any type") */
> +#define TP_GTOP_ANY_TYPE_STATE_ANY_OPTION_ATTACHED BIT(0)
> +
> +/*
> + * Attach option states for GTOP Type 4 interfaces ("ThinkPad X1 Tablet series")
> + * 0: detached, 1: attached
> + * except for special meanings for Pico cartridge and folio keyboard cover location
> + */
> +#define TP_GTOP_TP_X1_TAB_STATE_THIN_KBD_ATTACHED BIT(0)
> +#define TP_GTOP_TP_X1_TAB_STATE_PRO_CARTRIDGE_ATTACHED BIT(1)
> +#define TP_GTOP_TP_X1_TAB_STATE_PICO_CARTRIDGE_ATTACH_MASK GENMASK(3, 2)
> +#define TP_GTOP_TP_X1_TAB_STATE_3D_CARTRIDGE_ATTACHED BIT(4)
> +#define TP_GTOP_TP_X1_TAB_STATE_RESERVE1_ATTACHED BIT(5)
> +#define TP_GTOP_TP_X1_TAB_STATE_RESERVE2_ATTACHED BIT(6)
> +#define TP_GTOP_TP_X1_TAB_STATE_FOLIO_KBD_FOLDED_ONTO_BACK BIT(16)
> +
> +/* Special values for Pico cartridge state (bits 3-2) */
> +#define TP_GTOP_TP_X1_TAB_STATE_PICO_CARTRIDGE_DETACHED 0
> +#define TP_GTOP_TP_X1_TAB_STATE_PICO_CARTRIDGE_ATTACHED BIT(2)
> +#define TP_GTOP_TP_X1_TAB_STATE_PICO_CARTRIDGE_BATTERY_ERROR BIT(3)
> +#define TP_GTOP_TP_X1_TAB_STATE_PICO_CARTRIDGE_RESERVED (BIT(2) | BIT(3))
> +
> /* HKEY events */
> enum tpacpi_hkey_event_t {
> /* Hotkey-related */
> @@ -174,6 +213,12 @@ enum tpacpi_hkey_event_t {
> or port replicator */
> TP_HKEY_EV_HOTPLUG_UNDOCK = 0x4011, /* undocked from hotplug
> dock or port replicator */
> + /*
> + * Thinkpad X1 Tablet series devices emit 0x4012 and 0x4013
> + * when keyboard cover is attached, detached or folded onto the back
> + */
> + TP_HKEY_EV_KBD_COVER_ATTACH = 0x4012, /* keyboard cover attached */
> + TP_HKEY_EV_KBD_COVER_DETACH = 0x4013, /* keyboard cover detached or folded back */
>
> /* User-interface events */
> TP_HKEY_EV_LID_CLOSE = 0x5001, /* laptop lid closed */
> @@ -308,6 +353,10 @@ static struct {
> TP_HOTKEY_TABLET_NONE = 0,
> TP_HOTKEY_TABLET_USES_MHKG,
> TP_HOTKEY_TABLET_USES_GMMS,
> + TP_HOTKEY_TABLET_USES_GTOP_ANY_TYPE,
> + TP_HOTKEY_TABLET_USES_GTOP_TP_HELIX,
> + TP_HOTKEY_TABLET_USES_GTOP_TP10,
> + TP_HOTKEY_TABLET_USES_GTOP_TP_X1_TAB,
> } hotkey_tablet;
> u32 kbdlight:1;
> u32 light:1;
> @@ -2166,11 +2215,32 @@ static int hotkey_gmms_get_tablet_mode(int s, int *has_tablet_mode)
> return !!(mode & TP_ACPI_MULTI_MODE_TABLET_LIKE);
> }
>
> +static bool hotkey_gtop_any_type_get_tablet_mode(int s)
> +{
> + return !(s & TP_GTOP_ANY_TYPE_STATE_ANY_OPTION_ATTACHED);
> +}
> +
> +static bool hotkey_gtop_tp_x1_tab_get_tablet_mode(int s)
> +{
> + return (!(s & TP_GTOP_TP_X1_TAB_STATE_THIN_KBD_ATTACHED) ||
> + (s & TP_GTOP_TP_X1_TAB_STATE_FOLIO_KBD_FOLDED_ONTO_BACK));
> +}
> +
> static int hotkey_get_tablet_mode(int *status)
> {
> int s;
>
> switch (tp_features.hotkey_tablet) {
> + case TP_HOTKEY_TABLET_USES_GTOP_ANY_TYPE:
> + if (!acpi_evalf(hkey_handle, &s, "GTOP", "dd", TP_GTOP_CMD_GET_ATTACH_OPTION))
> + return -EIO;
> + *status = hotkey_gtop_any_type_get_tablet_mode(s);
> + break;
> + case TP_HOTKEY_TABLET_USES_GTOP_TP_X1_TAB:
> + if (!acpi_evalf(hkey_handle, &s, "GTOP", "dd", TP_GTOP_CMD_GET_ATTACH_OPTION))
> + return -EIO;
> + *status = hotkey_gtop_tp_x1_tab_get_tablet_mode(s);
> + break;
> case TP_HOTKEY_TABLET_USES_MHKG:
> if (!acpi_evalf(hkey_handle, &s, "MHKG", "d"))
> return -EIO;
> @@ -3213,7 +3283,29 @@ static int hotkey_init_tablet_mode(void)
> int in_tablet_mode = 0, res;
> char *type = NULL;
>
> - if (acpi_evalf(hkey_handle, &res, "GMMS", "qdd", 0)) {
> + if (acpi_evalf(hkey_handle, &res, "GTOP", "qdd", TP_GTOP_CMD_QUERY_VERSION) &&
> + res >= TP_GTOP_MINIMUM_VERSION &&
> + acpi_evalf(hkey_handle, &res, "GTOP", "qdd", TP_GTOP_CMD_QUERY_INTERFACE_TYPE)) {
> + switch (res) {
> + case TP_GTOP_INTERFACE_TYPE_ANY_TYPE:
> + tp_features.hotkey_tablet = TP_HOTKEY_TABLET_USES_GTOP_ANY_TYPE;
> + if (acpi_evalf(hkey_handle, &res, "GTOP", "qdd",
> + TP_GTOP_CMD_GET_ATTACH_OPTION))
> + in_tablet_mode = hotkey_gtop_any_type_get_tablet_mode(res);
> + type = "GTOP";
> + break;
> + case TP_GTOP_INTERFACE_TYPE_TP_X1_TAB:
> + tp_features.hotkey_tablet = TP_HOTKEY_TABLET_USES_GTOP_TP_X1_TAB;
> + if (acpi_evalf(hkey_handle, &res, "GTOP", "qdd",
> + TP_GTOP_CMD_GET_ATTACH_OPTION))
> + in_tablet_mode = hotkey_gtop_tp_x1_tab_get_tablet_mode(res);
> + type = "GTOP";
> + break;
> + default:
> + pr_err("unsupported GTOP type, please report this to %s\n", TPACPI_MAIL);
Please use dev_err(&tpacpi_pdev->dev, ...
and please print the res value;
Also please change this to a dev_warn, see below.
> + break;
> + }
This means that if there is a GTOP, but the TP_GTOP_CMD_QUERY_INTERFACE_TYPE is unknown
we won't register a SW_TABLET_MODE even though there might be a working GMMS function.
Also the size of the tests here makes it hard to see the if ... else if ... else if ...
structure which we have here.
Can you factor out the GTOP probing / detecting into a
hotkey_detect_gtop_tablet_mode() helper which returns true on success?
Then you can also avoid changing all the tests together with &&.
Instead you should end up with something like this:
bool hotkey_detect_gtop_tablet_mode(void) {
int res;
if (!acpi_evalf(hkey_handle, &res, "GTOP", "qdd", TP_GTOP_CMD_QUERY_VERSION))
return false;
if (res < TP_GTOP_MINIMUM_VERSION)
return false;
if (!acpi_evalf(hkey_handle, &res, "GTOP", "qdd", TP_GTOP_CMD_QUERY_INTERFACE_TYPE))
return false;
switch (type) {
case TP_GTOP_INTERFACE_TYPE_ANY_TYPE:
tp_features.hotkey_tablet = TP_HOTKEY_TABLET_USES_GTOP_ANY_TYPE;
break;
case TP_GTOP_INTERFACE_TYPE_TP_X1_TAB:
tp_features.hotkey_tablet = TP_HOTKEY_TABLET_USES_GTOP_TP_X1_TAB;
break;
default:
dev_warn(&tpacpi_pdev->dev, "unsupported GTOP type %d, please report this to %s\n", res, TPACPI_MAIL);
return false;
}
return true;
}
And then in hotkey_init_tablet_mode() you can have just this:
+ if (hotkey_detect_gtop_tablet_mode(status)) {
+ hotkey_get_tablet_mode(&in_tablet_mode);
+ type = "GTOP";
+ } else if (acpi_evalf(hkey_handle, &res, "GMMS", "qdd", 0)) {
int has_tablet_mode;
Which is a lot cleaner and will result in fallback to GMMS in the "unsupported GTOP type" case.
Notice I also reused hotkey_get_tablet_mode() to get the mode here, avoiding some duplication.
Regards,
Hans
>
> in_tablet_mode = hotkey_gmms_get_tablet_mode(res,
> @@ -3989,6 +4081,12 @@ static bool hotkey_notify_dockevent(const u32 hkey,
> case TP_HKEY_EV_HOTPLUG_UNDOCK: /* undocked from port replicator */
> pr_info("undocked from hotplug port replicator\n");
> return true;
> + case TP_HKEY_EV_KBD_COVER_ATTACH:
> + case TP_HKEY_EV_KBD_COVER_DETACH:
> + tpacpi_input_send_tabletsw();
> + hotkey_tablet_mode_notify_change();
> + *send_acpi_ev = false;
> + return true;
>
> default:
> return false;
>
