Hi
2021. február 10., szerda 18:54 keltezéssel, Alexander Kobel írta:
> 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>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 91 +++++++++++++++++++++++++++-
> 1 file changed, 90 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index c404706379d9..8c1ff555f10b 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -174,6 +174,11 @@ 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 (and probably other GTOP type 4) emit 0x4012 and 0x4013
The preferred style of multi-line comments is
/* <note the empty line here>
* Lorem ipsum ...
*/
> + * 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 +313,8 @@ 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_X1_TABLET_TYPE,
> } hotkey_tablet;
> u32 kbdlight:1;
> u32 light:1;
> @@ -2166,11 +2173,32 @@ static int hotkey_gmms_get_tablet_mode(int s, int *has_tablet_mode)
> return !!(mode & TP_ACPI_MULTI_MODE_TABLET_LIKE);
> }
>
> +static int hotkey_gtop_any_type_get_tablet_mode(int s)
> +{
> + return !(s & 0x1);
> +}
I believe it would be preferable to do something like
#define TP_GTOP_TYPE_ANY_ATTACH_STATE BIT(0)
/* or possibly use an enum */
...
return !(s & TP_GTOP_TYPE_ANY_ATTACH_STATE);
or let `s` be `unsigned long`, and then
#define TP_GTOP_TYPE_ANY_ATTACH_STATE_BIT 0
...
return !test_bit(TP_GTOP_TYPE_ANY_ATTACH_STATE_BIT, &s);
or something along these lines, and I also think the return type could be `bool`.
> +
> +static int hotkey_gtop_x1_tablet_type_get_tablet_mode(int s)
> +{
> + return (!(s & 0x1) /* keyboard NOT attached */
> + || ((s >> 16) & 0x1) /* or folded onto the back */);
> +}
Same here, I suggest using the `BIT()` macro or `test_bit()` and preferably
name the constants.
> +
> 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", 0x0200))
> + return -EIO;
> + *status = hotkey_gtop_any_type_get_tablet_mode(s);
> + break;
> + case TP_HOTKEY_TABLET_USES_GTOP_X1_TABLET_TYPE:
> + if (!acpi_evalf(hkey_handle, &s, "GTOP", "dd", 0x0200))
> + return -EIO;
> + *status = hotkey_gtop_x1_tablet_type_get_tablet_mode(s);
> + break;
> case TP_HOTKEY_TABLET_USES_MHKG:
> if (!acpi_evalf(hkey_handle, &s, "MHKG", "d"))
> return -EIO;
> @@ -3213,7 +3241,62 @@ 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", 0x0000)
> + && res >= 0x0103
> + && acpi_evalf(hkey_handle, &res, "GTOP", "qdd", 0x0100)) {
Minor thing, but checkpatch prefers `&&` at the end of the line. And the rest of
the code places them at the end, too.
> + /*
> + * GTOP ("Get Tablet OPtion") state ASL method definition:
> + * - Input: 0x0000: Query version
> + * Output: 0x0103 (version 1.03)
> + * - Input: 0x0100: Query interface type
> + * Output: DWORD But 31-0 Interface type
^
Shouldn't that be "Bit"?
> + * 0: Reserved
> + * 1: Any type
> + * 2: ThinkPad Helix series
> + * 3: ThinkPad 10 series
> + * 4: ThinkPad X1 Tablet series
> + * - Input: 0x0200: Get attach option
> + * Output: Option attach state
> + * (0: detached, 1: attached)
> + * version >= 1.03 and interface type 1:
> + * Bit 0: Any option attach state
> + * Bit 31-1: Reserved(0)
> + * version >= 1.03 and interface type 4:
> + * Bit 0: Thin-KBD attach state
> + * Bit 1: Pro-Cartridge attach state
> + * Bit 3-2: Pico-Cartridge attach state
> + * 00: detached
> + * 01: attached
> + * 10: attached with battery error
> + * 11: Reserved
> + * Bit 4: 3D Cartridge attach state
> + * Bit 5: Reserve 1 attach state
> + * Bit 6: Reserve 2 attach state
> + * Bit 15-7: Reserved(0)
> + * Bit 16: Folio keyboard location
> + * (valid if folio attached)
> + * 0: keyboard is NOT folded onto the back
> + * 1: keyboard is folded onto the back of the system
> + * Bit 31-17: Reserved(0)
> + */
> + switch (res) {
> + case 1:
> + tp_features.hotkey_tablet = TP_HOTKEY_TABLET_USES_GTOP_ANY_TYPE;
> + if (acpi_evalf(hkey_handle, &res, "GTOP", "qdd", 0x200))
Please name that `0x200` something like TP_GTOP_CMD_GET_ATTACH_STATE or something
you like. (Same for the rest of the GTOP calls.) I know it's just above in the comment,
but I still think it'd be better to have concrete, more or less self-explanatory names
in the actual command.
> + in_tablet_mode = hotkey_gtop_any_type_get_tablet_mode(res);
> + type = "GTOP";
> + break;
> + case 4:
> + tp_features.hotkey_tablet = TP_HOTKEY_TABLET_USES_GTOP_X1_TABLET_TYPE;
> + if (acpi_evalf(hkey_handle, &res, "GTOP", "qdd", 0x200))
> + in_tablet_mode = hotkey_gtop_x1_tablet_type_get_tablet_mode(res);
> + type = "GTOP";
> + break;
> + default:
> + pr_err("unsupported GTOP type, please report this to %s\n", TPACPI_MAIL);
> + break;
> + }
> + } else if (acpi_evalf(hkey_handle, &res, "GMMS", "qdd", 0)) {
> int has_tablet_mode;
> [...]
Regards,
Barnabás Pőcze
