[PATCH 1/3] intel-hid: add support for SW_TABLET_MODE

[PATCH 1/3] intel-hid: add support for SW_TABLET_MODE

[Elia Devito]

Add support for SW_TABLET_MODE for convertibles notebook.

Exactly as intel-vbtn driver, the event code 0xcc is emitted by
convertibles when entering tablet mode and 0xcd when return to
laptop mode.

Signed-off-by: Elia Devito <eliadevito@gmail.com>
---
more info: https://bugzilla.kernel.org/show_bug.cgi?id=207433
 
 drivers/platform/x86/intel-hid.c | 84 ++++++++++++++++++++++++++++++--
 1 file changed, 80 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
index 86261970bd8f..5093c57102cf 100644
--- a/drivers/platform/x86/intel-hid.c
+++ b/drivers/platform/x86/intel-hid.c
@@ -15,6 +15,9 @@
 #include <linux/platform_device.h>
 #include <linux/suspend.h>
 
+/* When NOT in tablet mode, VGBS returns with the flag 0x40 */
+#define TABLET_MODE_FLAG 0x40
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Alex Hung");
 
@@ -61,7 +64,11 @@ static const struct key_entry intel_array_keymap[] = {
 	{ KE_IGNORE, 0xC9, { KEY_ROTATE_LOCK_TOGGLE } },      /* Release */
 	{ KE_KEY,    0xCE, { KEY_POWER } },                   /* Press */
 	{ KE_IGNORE, 0xCF, { KEY_POWER } },                   /* Release */
-	{ KE_END },
+};
+
+static const struct key_entry intel_array_switches[] = {
+	{ KE_SW, 0xCC, { .sw = { SW_TABLET_MODE, 1 } } },  /* Tablet */
+	{ KE_SW, 0xCD, { .sw = { SW_TABLET_MODE, 0 } } },  /* Laptop */
 };
 
 static const struct dmi_system_id button_array_table[] = {
@@ -89,9 +96,23 @@ static const struct dmi_system_id button_array_table[] = {
 	{ }
 };
 
+static const struct dmi_system_id button_array_switches_table[] = {
+	{
+		.matches = {
+			DMI_EXACT_MATCH(DMI_CHASSIS_TYPE, "31" /* Convertible */),
+		},
+	},
+	{ }
+};
+
+#define KEYMAP_LEN \
+	(ARRAY_SIZE(intel_array_keymap) + ARRAY_SIZE(intel_array_switches) + 1)
+
 struct intel_hid_priv {
+	struct key_entry keymap[KEYMAP_LEN];
 	struct input_dev *input_dev;
 	struct input_dev *array;
+	bool has_switches;
 	bool wakeup_mode;
 };
 
@@ -327,23 +348,54 @@ static int intel_hid_input_setup(struct platform_device *device)
 	return input_register_device(priv->input_dev);
 }
 
+static void detect_tablet_mode(struct platform_device *device)
+{
+	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
+	acpi_handle handle = ACPI_HANDLE(&device->dev);
+	unsigned long long vgbs;
+	int m;
+
+	if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_VGBS_FN, &vgbs))
+		return;
+
+	m = !(vgbs & TABLET_MODE_FLAG);
+	input_report_switch(priv->array, SW_TABLET_MODE, m);
+}
+
 static int intel_button_array_input_setup(struct platform_device *device)
 {
 	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
-	int ret;
+	int ret, keymap_len = 0;
 
 	/* Setup input device for 5 button array */
 	priv->array = devm_input_allocate_device(&device->dev);
 	if (!priv->array)
 		return -ENOMEM;
 
-	ret = sparse_keymap_setup(priv->array, intel_array_keymap, NULL);
+	memcpy(&priv->keymap[keymap_len], intel_array_keymap,
+		       ARRAY_SIZE(intel_array_keymap) *
+		       sizeof(struct key_entry));
+	keymap_len += ARRAY_SIZE(intel_array_keymap);
+
+	if (priv->has_switches) {
+		memcpy(&priv->keymap[keymap_len], intel_array_switches,
+		       ARRAY_SIZE(intel_array_switches) *
+		       sizeof(struct key_entry));
+		keymap_len += ARRAY_SIZE(intel_array_switches);
+	}
+
+	priv->keymap[keymap_len].type = KE_END;
+
+	ret = sparse_keymap_setup(priv->array, priv->keymap, NULL);
 	if (ret)
 		return ret;
 
 	priv->array->name = "Intel HID 5 button array";
 	priv->array->id.bustype = BUS_HOST;
 
+	if (priv->has_switches)
+		detect_tablet_mode(device);
+
 	return input_register_device(priv->array);
 }
 
@@ -352,7 +404,10 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 	struct platform_device *device = context;
 	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
 	unsigned long long ev_index;
+	unsigned int val = !(event & 1); /* Even=press, Odd=release */
+	const struct key_entry *ke;
 
+	dev_info(&device->dev, "event 0x%x\n", event);
 	if (priv->wakeup_mode) {
 		/*
 		 * Needed for wakeup from suspend-to-idle to work on some
@@ -367,13 +422,19 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 		if (event == 0xc0 || !priv->array)
 			return;
 
-		if (!sparse_keymap_entry_from_scancode(priv->array, event)) {
+		ke = sparse_keymap_entry_from_scancode(priv->array, event);
+		if (!ke) {
 			dev_info(&device->dev, "unknown event 0x%x\n", event);
 			return;
 		}
 
 wakeup:
 		pm_wakeup_hard_event(&device->dev);
+
+		/* report the new switch position to the input subsystem. */
+		if (ke && ke->type == KE_SW)
+			sparse_keymap_report_event(priv->array, event, val, 0);
+
 		return;
 	}
 
@@ -441,6 +502,20 @@ static bool button_array_present(struct platform_device *device)
 	return false;
 }
 
+static bool intel_button_array_has_switches(struct platform_device *device)
+{
+	acpi_handle handle = ACPI_HANDLE(&device->dev);
+	unsigned long long vgbs;
+
+	if (!dmi_check_system(button_array_switches_table))
+		return false;
+
+	if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_VGBS_FN, &vgbs))
+		return false;
+
+	return true;
+}
+
 static int intel_hid_probe(struct platform_device *device)
 {
 	acpi_handle handle = ACPI_HANDLE(&device->dev);
@@ -479,6 +554,7 @@ static int intel_hid_probe(struct platform_device *device)
 
 	/* Setup 5 button array */
 	if (button_array_present(device)) {
+		priv->has_switches = intel_button_array_has_switches(device);
 		dev_info(&device->dev, "platform supports 5 button array\n");
 		err = intel_button_array_input_setup(device);
 		if (err)
-- 
2.28.0

Re: [PATCH 1/3] intel-hid: add support for SW_TABLET_MODE

[Hans de Goede]

Hi Elia,

Thank you for your patch series.

I'm fraid that always enabling this on devices with a chassis_type of 31 is
not a good idea though.

Many 360 degree hinges (yoga) style 2-in-1s use 2 accelerometers to tell
the angle between the 2 halves; and they rely on a HingleAngleService
process under Windows to read out the 2 accelerometers and then call
back into ACPI (through an ACPI DSM on the accelerometer API node)
to tell the firmware about the angle between the 2 halves.

On some devices this is also where the 0xCC / 0xCD events come from
on the intel-hid device. Here is some example DSDT code doing this:

                Device (KXJ0)
                {
                    Name (_ADR, Zero)  // _ADR: Address
                    Name (_HID, "KIOX010A")  // _HID: Hardware ID
                    Name (_CID, "KIOX010A")  // _CID: Compatible ID
                    Name (_DDN, "Kionix KXCJ9 Accelerometer Display")  // _DDN: DOS Device Name
<snip>
                    Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
                    {
                        If ((Arg0 == ToUUID ("1f339696-d475-4e26-8cad-2e9f8e6d7a91")))
                        {
                            If ((Arg2 == Zero))
                            {
                                Return (Buffer (One)
                                {
                                     0x05                                             // .
                                })
                            }

                            If ((Arg2 == One))
                            {
                                IDX1 = 0x10
                                DTA1 = One
                                SPC0 (0x00C509C0, 0x44000201)
                                Notify (HIDD, 0xCD) // Hardware-Specific
                                Return (Buffer (One)
                                {
                                     0x01                                             // .
                                })
                            }

                            If ((Arg2 == 0x02))
                            {
                                IDX1 = 0x10
                                DTA1 = Zero
                                SPC0 (0x00C509C0, 0x44000200)
                                Notify (HIDD, 0xCC) // Hardware-Specific
                                Return (Buffer (One)
                                {
                                     0x00                                             // .
                                })
                            }

Notice the "Notify (HIDD, 0xC?)" calls here, resulting from something
(the HingeAngleService under Windows) calling the DSM. As you can see there
is more happening when this DSM gets called. I actually recently added support
for this to the kxcjk-1013.c accelerometer driver, because sometimes some
devices would come up with their kbd and touchpad events silenced by the
embedded controller of the device and this DSM needs to be called telling
the EC the device is in laptop mode to get the kbd/mouse to work (*).

See here for the patch which I wrote for this:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e5b1032a656e9aa4c7a4df77cb9156a2a651a5f9

Since there is nothing on Linux calling this DSM whenever the mode
changes, SW_TABLET_MODE reporting from the intel-hid driver will be
unreliable on these devices.

Also there is going to be a ton of devices which do use intel-hid for
reporting some buttons and which do have a chassis-type of 31, but which
do not use intel-hid for reporting SW_TABLET_MODE, but use something else,
either something manufacturer specific or e.g. intel-vbtn.

Even when we end up reporting SW_TABLET_MODE=0 all the time this
still causes issues. E.g. GNOME-3.38 and newer will disable
accelerometer based display rotation and will not auto-popup the
onscreen keyboard on focussing a text-field by touch, when
SW_TABLET_MODE==0

So we really must only advertise SW_TABLET_MODE support if it actually
works. As such I believe that it would be better to use a vendor + product
DMI string based allow-list for this now and only add SW_TABLET_MODE support
to intel-hid for devices on the allow-list.

Do you know if the HP and Dell from the bug also have an intel-vbtn
device? I think we need to gather some more info on devices which need
this and see if we can come up with a better way to detect support then
we could go with that + checking chassis_type + a
if (!acpi_dev_present("KIOX010A", NULL, -1)) check to catch the
2-accelerometers needing some equivalent of HingeAngleService case.

I guess a third option would be to create a second input_dev on
the fly when the first 0xcc / 0xcd events is generated (and hard
code those events for the 2nd inputdev rather then using the
sparse keymap). This also deals with VGBS not always being reliable,
but it would mean that the advantages of advertising SW_TABLET_MODE=0
when in laptopmode (the new GNOME-3.38 behavior) go away until such
events are first send. Then again I have the feeling that at least
some devices will send these with some interval even when not changing
modes which might make the idea of creating a 2nd input-dev on the
fly when receiving the first event work reasonably well.

If you can see if the HP model from the bug does indeed sends
out events without needing a mode change to happen then this might
actually be a viable idea.

Or maybe combine the 2, an allow-list for devices where VGBS works
and on those create the 2nd input device immediately with the
initial state coming from VGBS + dynamic creation on the rest.

Hmm, I must say that I do actually like this combined idea...

Regards,

Hans










*) Note in this case the events are actually suppressed by the
embedded-controller, unlike similar cases where this happens when
libinput sees SW_TABLET_MODE=1



On 12/1/20 8:55 PM, Elia Devito wrote:
> Add support for SW_TABLET_MODE for convertibles notebook.
> 
> Exactly as intel-vbtn driver, the event code 0xcc is emitted by
> convertibles when entering tablet mode and 0xcd when return to
> laptop mode.
> 
> Signed-off-by: Elia Devito <eliadevito@gmail.com>
> ---
> more info: https://bugzilla.kernel.org/show_bug.cgi?id=207433
>  
>  drivers/platform/x86/intel-hid.c | 84 ++++++++++++++++++++++++++++++--
>  1 file changed, 80 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
> index 86261970bd8f..5093c57102cf 100644
> --- a/drivers/platform/x86/intel-hid.c
> +++ b/drivers/platform/x86/intel-hid.c
> @@ -15,6 +15,9 @@
>  #include <linux/platform_device.h>
>  #include <linux/suspend.h>
>  
> +/* When NOT in tablet mode, VGBS returns with the flag 0x40 */
> +#define TABLET_MODE_FLAG 0x40
> +
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Alex Hung");
>  
> @@ -61,7 +64,11 @@ static const struct key_entry intel_array_keymap[] = {
>  	{ KE_IGNORE, 0xC9, { KEY_ROTATE_LOCK_TOGGLE } },      /* Release */
>  	{ KE_KEY,    0xCE, { KEY_POWER } },                   /* Press */
>  	{ KE_IGNORE, 0xCF, { KEY_POWER } },                   /* Release */
> -	{ KE_END },
> +};
> +
> +static const struct key_entry intel_array_switches[] = {
> +	{ KE_SW, 0xCC, { .sw = { SW_TABLET_MODE, 1 } } },  /* Tablet */
> +	{ KE_SW, 0xCD, { .sw = { SW_TABLET_MODE, 0 } } },  /* Laptop */
>  };
>  
>  static const struct dmi_system_id button_array_table[] = {
> @@ -89,9 +96,23 @@ static const struct dmi_system_id button_array_table[] = {
>  	{ }
>  };
>  
> +static const struct dmi_system_id button_array_switches_table[] = {
> +	{
> +		.matches = {
> +			DMI_EXACT_MATCH(DMI_CHASSIS_TYPE, "31" /* Convertible */),
> +		},
> +	},
> +	{ }
> +};
> +
> +#define KEYMAP_LEN \
> +	(ARRAY_SIZE(intel_array_keymap) + ARRAY_SIZE(intel_array_switches) + 1)
> +
>  struct intel_hid_priv {
> +	struct key_entry keymap[KEYMAP_LEN];
>  	struct input_dev *input_dev;
>  	struct input_dev *array;
> +	bool has_switches;
>  	bool wakeup_mode;
>  };
>  
> @@ -327,23 +348,54 @@ static int intel_hid_input_setup(struct platform_device *device)
>  	return input_register_device(priv->input_dev);
>  }
>  
> +static void detect_tablet_mode(struct platform_device *device)
> +{
> +	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
> +	acpi_handle handle = ACPI_HANDLE(&device->dev);
> +	unsigned long long vgbs;
> +	int m;
> +
> +	if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_VGBS_FN, &vgbs))
> +		return;
> +
> +	m = !(vgbs & TABLET_MODE_FLAG);
> +	input_report_switch(priv->array, SW_TABLET_MODE, m);
> +}
> +
>  static int intel_button_array_input_setup(struct platform_device *device)
>  {
>  	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
> -	int ret;
> +	int ret, keymap_len = 0;
>  
>  	/* Setup input device for 5 button array */
>  	priv->array = devm_input_allocate_device(&device->dev);
>  	if (!priv->array)
>  		return -ENOMEM;
>  
> -	ret = sparse_keymap_setup(priv->array, intel_array_keymap, NULL);
> +	memcpy(&priv->keymap[keymap_len], intel_array_keymap,
> +		       ARRAY_SIZE(intel_array_keymap) *
> +		       sizeof(struct key_entry));
> +	keymap_len += ARRAY_SIZE(intel_array_keymap);
> +
> +	if (priv->has_switches) {
> +		memcpy(&priv->keymap[keymap_len], intel_array_switches,
> +		       ARRAY_SIZE(intel_array_switches) *
> +		       sizeof(struct key_entry));
> +		keymap_len += ARRAY_SIZE(intel_array_switches);
> +	}
> +
> +	priv->keymap[keymap_len].type = KE_END;
> +
> +	ret = sparse_keymap_setup(priv->array, priv->keymap, NULL);
>  	if (ret)
>  		return ret;
>  
>  	priv->array->name = "Intel HID 5 button array";
>  	priv->array->id.bustype = BUS_HOST;
>  
> +	if (priv->has_switches)
> +		detect_tablet_mode(device);
> +
>  	return input_register_device(priv->array);
>  }
>  
> @@ -352,7 +404,10 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>  	struct platform_device *device = context;
>  	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
>  	unsigned long long ev_index;
> +	unsigned int val = !(event & 1); /* Even=press, Odd=release */
> +	const struct key_entry *ke;
>  
> +	dev_info(&device->dev, "event 0x%x\n", event);
>  	if (priv->wakeup_mode) {
>  		/*
>  		 * Needed for wakeup from suspend-to-idle to work on some
> @@ -367,13 +422,19 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>  		if (event == 0xc0 || !priv->array)
>  			return;
>  
> -		if (!sparse_keymap_entry_from_scancode(priv->array, event)) {
> +		ke = sparse_keymap_entry_from_scancode(priv->array, event);
> +		if (!ke) {
>  			dev_info(&device->dev, "unknown event 0x%x\n", event);
>  			return;
>  		}
>  
>  wakeup:
>  		pm_wakeup_hard_event(&device->dev);
> +
> +		/* report the new switch position to the input subsystem. */
> +		if (ke && ke->type == KE_SW)
> +			sparse_keymap_report_event(priv->array, event, val, 0);
> +
>  		return;
>  	}
>  
> @@ -441,6 +502,20 @@ static bool button_array_present(struct platform_device *device)
>  	return false;
>  }
>  
> +static bool intel_button_array_has_switches(struct platform_device *device)
> +{
> +	acpi_handle handle = ACPI_HANDLE(&device->dev);
> +	unsigned long long vgbs;
> +
> +	if (!dmi_check_system(button_array_switches_table))
> +		return false;
> +
> +	if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_VGBS_FN, &vgbs))
> +		return false;
> +
> +	return true;
> +}
> +
>  static int intel_hid_probe(struct platform_device *device)
>  {
>  	acpi_handle handle = ACPI_HANDLE(&device->dev);
> @@ -479,6 +554,7 @@ static int intel_hid_probe(struct platform_device *device)
>  
>  	/* Setup 5 button array */
>  	if (button_array_present(device)) {
> +		priv->has_switches = intel_button_array_has_switches(device);
>  		dev_info(&device->dev, "platform supports 5 button array\n");
>  		err = intel_button_array_input_setup(device);
>  		if (err)
> 

[PATCH v2 1/2] intel-hid: add support for SW_TABLET_MODE

[Elia Devito]

Some convertible use the intel-hid ACPI interface to report SW_TABLET_MODE,
implement this with DMI based allow-list to be sure to activate support
only on models that effectively have it.

Signed-off-by: Elia Devito <eliadevito@gmail.com>
---
v2:
  patch reworked according to received feedbacks

 drivers/platform/x86/intel-hid.c | 98 ++++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
index 86261970bd8f..fed24d4f28b8 100644
--- a/drivers/platform/x86/intel-hid.c
+++ b/drivers/platform/x86/intel-hid.c
@@ -15,6 +15,9 @@
 #include <linux/platform_device.h>
 #include <linux/suspend.h>
 
+/* When NOT in tablet mode, VGBS returns with the flag 0x40 */
+#define TABLET_MODE_FLAG 0x40
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Alex Hung");
 
@@ -89,9 +92,26 @@ static const struct dmi_system_id button_array_table[] = {
 	{ }
 };
 
+/*
+ * Some convertible use the intel-hid ACPI interface to report SW_TABLET_MODE,
+ * these need to be compared via a DMI based authorization list because some
+ * models have unreliable VGBS return which could cause incorrect
+ * SW_TABLET_MODE report.
+ */
+static const struct dmi_system_id dmi_vgbs_allow_list[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "HP"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "HP Spectre x360 Convertible 15-df0xxx"),
+		},
+	},
+	{ }
+};
+
 struct intel_hid_priv {
 	struct input_dev *input_dev;
 	struct input_dev *array;
+	struct input_dev *switches;
 	bool wakeup_mode;
 };
 
@@ -347,6 +367,38 @@ static int intel_button_array_input_setup(struct platform_device *device)
 	return input_register_device(priv->array);
 }
 
+static int intel_hid_switches_setup(struct platform_device *device)
+{
+	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
+
+	/* Setup input device for switches */
+	priv->switches = devm_input_allocate_device(&device->dev);
+	if (!priv->switches)
+		return -ENOMEM;
+
+	__set_bit(EV_SW, priv->switches->evbit);
+	__set_bit(SW_TABLET_MODE, priv->switches->swbit);
+
+	priv->switches->name = "Intel HID switches";
+	priv->switches->id.bustype = BUS_HOST;
+	return input_register_device(priv->switches);
+}
+
+static void detect_tablet_mode(struct platform_device *device)
+{
+	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
+	acpi_handle handle = ACPI_HANDLE(&device->dev);
+	unsigned long long vgbs;
+	int m;
+
+	if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_VGBS_FN, &vgbs))
+		return;
+
+	m = !(vgbs & TABLET_MODE_FLAG);
+	input_report_switch(priv->switches, SW_TABLET_MODE, m);
+	input_sync(priv->switches);
+}
+
 static void notify_handler(acpi_handle handle, u32 event, void *context)
 {
 	struct platform_device *device = context;
@@ -363,6 +415,13 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 		if (event == 0xce)
 			goto wakeup;
 
+		/*
+		 * Switch events will wake the device and report the new switch
+		 * position to the input subsystem.
+		 */
+		if (priv->switches && (event == 0xcc || event == 0xcd))
+			goto wakeup;
+
 		/* Wake up on 5-button array events only. */
 		if (event == 0xc0 || !priv->array)
 			return;
@@ -374,6 +433,21 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 
 wakeup:
 		pm_wakeup_hard_event(&device->dev);
+
+		if (priv->switches) {
+			if (event == 0xcc) {
+				input_report_switch(priv->switches, SW_TABLET_MODE, 1);
+				input_sync(priv->switches);
+				return;
+			}
+
+			if (event == 0xcd) {
+				input_report_switch(priv->switches, SW_TABLET_MODE, 0);
+				input_sync(priv->switches);
+				return;
+			}
+		}
+
 		return;
 	}
 
@@ -398,6 +472,20 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 		}
 	}
 
+	if (priv->switches) {
+		if (event == 0xcc) {
+			input_report_switch(priv->switches, SW_TABLET_MODE, 1);
+			input_sync(priv->switches);
+			return;
+		}
+
+		if (event == 0xcd) {
+			input_report_switch(priv->switches, SW_TABLET_MODE, 0);
+			input_sync(priv->switches);
+			return;
+		}
+	}
+
 	/* 0xC0 is for HID events, other values are for 5 button array */
 	if (event != 0xc0) {
 		if (!priv->array ||
@@ -485,6 +573,16 @@ static int intel_hid_probe(struct platform_device *device)
 			pr_err("Failed to setup Intel 5 button array hotkeys\n");
 	}
 
+	/* Setup switches for devices that we know VGBS return correctly */
+	if (dmi_check_system(dmi_vgbs_allow_list)) {
+		dev_info(&device->dev, "platform supports switches\n");
+		err = intel_hid_switches_setup(device);
+		if (err)
+			pr_err("Failed to setup Intel HID switches\n");
+		else
+			detect_tablet_mode(device);
+	}
+
 	status = acpi_install_notify_handler(handle,
 					     ACPI_DEVICE_NOTIFY,
 					     notify_handler,
-- 
2.28.0

[PATCH v3 1/2] intel-hid: add support for SW_TABLET_MODE

[Elia Devito]

From: Elia Devito <eliadevito@gmail.com>

Some convertible use the intel-hid ACPI interface to report SW_TABLET_MODE,
implement this with DMI based allow-list to be sure to activate support
only on models that effectively have it.

Signed-off-by: Elia Devito <eliadevito@gmail.com>
---
v2:
 patch reworked according to received feedbacks
  
v3:
 improved code according to received feedbacks

 drivers/platform/x86/intel-hid.c | 95 ++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
index 86261970bd8f..d2f892665ec6 100644
--- a/drivers/platform/x86/intel-hid.c
+++ b/drivers/platform/x86/intel-hid.c
@@ -15,6 +15,9 @@
 #include <linux/platform_device.h>
 #include <linux/suspend.h>
 
+/* When NOT in tablet mode, VGBS returns with the flag 0x40 */
+#define TABLET_MODE_FLAG BIT(6)
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Alex Hung");
 
@@ -89,9 +92,26 @@ static const struct dmi_system_id button_array_table[] = {
 	{ }
 };
 
+/*
+ * Some convertible use the intel-hid ACPI interface to report SW_TABLET_MODE,
+ * these need to be compared via a DMI based authorization list because some
+ * models have unreliable VGBS return which could cause incorrect
+ * SW_TABLET_MODE report.
+ */
+static const struct dmi_system_id dmi_vgbs_allow_list[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "HP"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "HP Spectre x360 Convertible 15-df0xxx"),
+		},
+	},
+	{ }
+};
+
 struct intel_hid_priv {
 	struct input_dev *input_dev;
 	struct input_dev *array;
+	struct input_dev *switches;
 	bool wakeup_mode;
 };
 
@@ -347,6 +367,57 @@ static int intel_button_array_input_setup(struct platform_device *device)
 	return input_register_device(priv->array);
 }
 
+static int intel_hid_switches_setup(struct platform_device *device)
+{
+	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
+
+	/* Setup input device for switches */
+	priv->switches = devm_input_allocate_device(&device->dev);
+	if (!priv->switches)
+		return -ENOMEM;
+
+	__set_bit(EV_SW, priv->switches->evbit);
+	__set_bit(SW_TABLET_MODE, priv->switches->swbit);
+
+	priv->switches->name = "Intel HID switches";
+	priv->switches->id.bustype = BUS_HOST;
+	return input_register_device(priv->switches);
+}
+
+static void report_tablet_mode_state(struct platform_device *device)
+{
+	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
+	acpi_handle handle = ACPI_HANDLE(&device->dev);
+	unsigned long long vgbs;
+	int m;
+
+	if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_VGBS_FN, &vgbs))
+		return;
+
+	m = !(vgbs & TABLET_MODE_FLAG);
+	input_report_switch(priv->switches, SW_TABLET_MODE, m);
+	input_sync(priv->switches);
+}
+
+static bool report_tablet_mode_event(struct input_dev *input_dev, u32 event)
+{
+	if (!input_dev)
+		return false;
+
+	switch (event) {
+	case 0xcc:
+		input_report_switch(input_dev, SW_TABLET_MODE, 1);
+		input_sync(input_dev);
+		return true;
+	case 0xcd:
+		input_report_switch(input_dev, SW_TABLET_MODE, 0);
+		input_sync(input_dev);
+		return true;
+	default:
+		return false;
+	}
+}
+
 static void notify_handler(acpi_handle handle, u32 event, void *context)
 {
 	struct platform_device *device = context;
@@ -363,6 +434,13 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 		if (event == 0xce)
 			goto wakeup;
 
+		/*
+		 * Switch events will wake the device and report the new switch
+		 * position to the input subsystem.
+		 */
+		if (priv->switches && (event == 0xcc || event == 0xcd))
+			goto wakeup;
+
 		/* Wake up on 5-button array events only. */
 		if (event == 0xc0 || !priv->array)
 			return;
@@ -374,6 +452,10 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 
 wakeup:
 		pm_wakeup_hard_event(&device->dev);
+
+		if (report_tablet_mode_event(priv->switches, event))
+			return;
+
 		return;
 	}
 
@@ -398,6 +480,9 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 		}
 	}
 
+	if (report_tablet_mode_event(priv->switches, event))
+		return;
+
 	/* 0xC0 is for HID events, other values are for 5 button array */
 	if (event != 0xc0) {
 		if (!priv->array ||
@@ -485,6 +570,16 @@ static int intel_hid_probe(struct platform_device *device)
 			pr_err("Failed to setup Intel 5 button array hotkeys\n");
 	}
 
+	/* Setup switches for devices that we know VGBS return correctly */
+	if (dmi_check_system(dmi_vgbs_allow_list)) {
+		dev_info(&device->dev, "platform supports switches\n");
+		err = intel_hid_switches_setup(device);
+		if (err)
+			pr_err("Failed to setup Intel HID switches\n");
+		else
+			report_tablet_mode_state(device);
+	}
+
 	status = acpi_install_notify_handler(handle,
 					     ACPI_DEVICE_NOTIFY,
 					     notify_handler,
-- 
2.28.0

Re: [PATCH v3 1/2] intel-hid: add support for SW_TABLET_MODE

[Hans de Goede]

Hi,

On 12/4/20 5:01 PM, Elia Devito wrote:
> From: Elia Devito <eliadevito@gmail.com>
> 
> Some convertible use the intel-hid ACPI interface to report SW_TABLET_MODE,
> implement this with DMI based allow-list to be sure to activate support
> only on models that effectively have it.
> 
> Signed-off-by: Elia Devito <eliadevito@gmail.com>

Thank you for your patch-series.

Note there is one special corner-case which you missed wrt the
dynamic creation of the device on receiving 0xcc/0xcd events
(so patch 2/2). I've prepared a follow-up patch fixing this,
which I will post right after sending this email. The commit
msg of that patch gives more details.

I've applied the series to my review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans

> ---
> v2:
>  patch reworked according to received feedbacks
>   
> v3:
>  improved code according to received feedbacks
> 
>  drivers/platform/x86/intel-hid.c | 95 ++++++++++++++++++++++++++++++++
>  1 file changed, 95 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
> index 86261970bd8f..d2f892665ec6 100644
> --- a/drivers/platform/x86/intel-hid.c
> +++ b/drivers/platform/x86/intel-hid.c
> @@ -15,6 +15,9 @@
>  #include <linux/platform_device.h>
>  #include <linux/suspend.h>
>  
> +/* When NOT in tablet mode, VGBS returns with the flag 0x40 */
> +#define TABLET_MODE_FLAG BIT(6)
> +
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Alex Hung");
>  
> @@ -89,9 +92,26 @@ static const struct dmi_system_id button_array_table[] = {
>  	{ }
>  };
>  
> +/*
> + * Some convertible use the intel-hid ACPI interface to report SW_TABLET_MODE,
> + * these need to be compared via a DMI based authorization list because some
> + * models have unreliable VGBS return which could cause incorrect
> + * SW_TABLET_MODE report.
> + */
> +static const struct dmi_system_id dmi_vgbs_allow_list[] = {
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "HP"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "HP Spectre x360 Convertible 15-df0xxx"),
> +		},
> +	},
> +	{ }
> +};
> +
>  struct intel_hid_priv {
>  	struct input_dev *input_dev;
>  	struct input_dev *array;
> +	struct input_dev *switches;
>  	bool wakeup_mode;
>  };
>  
> @@ -347,6 +367,57 @@ static int intel_button_array_input_setup(struct platform_device *device)
>  	return input_register_device(priv->array);
>  }
>  
> +static int intel_hid_switches_setup(struct platform_device *device)
> +{
> +	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
> +
> +	/* Setup input device for switches */
> +	priv->switches = devm_input_allocate_device(&device->dev);
> +	if (!priv->switches)
> +		return -ENOMEM;
> +
> +	__set_bit(EV_SW, priv->switches->evbit);
> +	__set_bit(SW_TABLET_MODE, priv->switches->swbit);
> +
> +	priv->switches->name = "Intel HID switches";
> +	priv->switches->id.bustype = BUS_HOST;
> +	return input_register_device(priv->switches);
> +}
> +
> +static void report_tablet_mode_state(struct platform_device *device)
> +{
> +	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
> +	acpi_handle handle = ACPI_HANDLE(&device->dev);
> +	unsigned long long vgbs;
> +	int m;
> +
> +	if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_VGBS_FN, &vgbs))
> +		return;
> +
> +	m = !(vgbs & TABLET_MODE_FLAG);
> +	input_report_switch(priv->switches, SW_TABLET_MODE, m);
> +	input_sync(priv->switches);
> +}
> +
> +static bool report_tablet_mode_event(struct input_dev *input_dev, u32 event)
> +{
> +	if (!input_dev)
> +		return false;
> +
> +	switch (event) {
> +	case 0xcc:
> +		input_report_switch(input_dev, SW_TABLET_MODE, 1);
> +		input_sync(input_dev);
> +		return true;
> +	case 0xcd:
> +		input_report_switch(input_dev, SW_TABLET_MODE, 0);
> +		input_sync(input_dev);
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>  static void notify_handler(acpi_handle handle, u32 event, void *context)
>  {
>  	struct platform_device *device = context;
> @@ -363,6 +434,13 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>  		if (event == 0xce)
>  			goto wakeup;
>  
> +		/*
> +		 * Switch events will wake the device and report the new switch
> +		 * position to the input subsystem.
> +		 */
> +		if (priv->switches && (event == 0xcc || event == 0xcd))
> +			goto wakeup;
> +
>  		/* Wake up on 5-button array events only. */
>  		if (event == 0xc0 || !priv->array)
>  			return;
> @@ -374,6 +452,10 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>  
>  wakeup:
>  		pm_wakeup_hard_event(&device->dev);
> +
> +		if (report_tablet_mode_event(priv->switches, event))
> +			return;
> +
>  		return;
>  	}
>  
> @@ -398,6 +480,9 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>  		}
>  	}
>  
> +	if (report_tablet_mode_event(priv->switches, event))
> +		return;
> +
>  	/* 0xC0 is for HID events, other values are for 5 button array */
>  	if (event != 0xc0) {
>  		if (!priv->array ||
> @@ -485,6 +570,16 @@ static int intel_hid_probe(struct platform_device *device)
>  			pr_err("Failed to setup Intel 5 button array hotkeys\n");
>  	}
>  
> +	/* Setup switches for devices that we know VGBS return correctly */
> +	if (dmi_check_system(dmi_vgbs_allow_list)) {
> +		dev_info(&device->dev, "platform supports switches\n");
> +		err = intel_hid_switches_setup(device);
> +		if (err)
> +			pr_err("Failed to setup Intel HID switches\n");
> +		else
> +			report_tablet_mode_state(device);
> +	}
> +
>  	status = acpi_install_notify_handler(handle,
>  					     ACPI_DEVICE_NOTIFY,
>  					     notify_handler,
> 

[PATCH v3 2/2] intel-hid: add alternative method to enable switches

[Elia Devito]

From: Elia Devito <eliadevito@gmail.com>

Some convertible have unreliable VGBS return, in these cases we enable
support when receiving the first event.

Signed-off-by: Elia Devito <eliadevito@gmail.com>
---
 drivers/platform/x86/intel-hid.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
index d2f892665ec6..054bc37da2cf 100644
--- a/drivers/platform/x86/intel-hid.c
+++ b/drivers/platform/x86/intel-hid.c
@@ -423,6 +423,19 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 	struct platform_device *device = context;
 	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
 	unsigned long long ev_index;
+	int err;
+
+	/*
+	 * Some convertible have unreliable VGBS return which could cause incorrect
+	 * SW_TABLET_MODE report, in these cases we enable support when receiving
+	 * the first event instead of during driver setup.
+	 */
+	if (!priv->switches && (event == 0xcc || event == 0xcd)) {
+		dev_info(&device->dev, "switch event received, enable switches supports\n");
+		err = intel_hid_switches_setup(device);
+		if (err)
+			pr_err("Failed to setup Intel HID switches\n");
+	}
 
 	if (priv->wakeup_mode) {
 		/*
-- 
2.28.0

[PATCH v2 2/2] intel-hid: add alternative method to enable switches

[Elia Devito]

Some convertible have unreliable VGBS return, in these cases we enable
support when receiving the first event.

Signed-off-by: Elia Devito <eliadevito@gmail.com>
---
 drivers/platform/x86/intel-hid.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
index fed24d4f28b8..3cc8f8ac48b2 100644
--- a/drivers/platform/x86/intel-hid.c
+++ b/drivers/platform/x86/intel-hid.c
@@ -404,6 +404,19 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 	struct platform_device *device = context;
 	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
 	unsigned long long ev_index;
+	int err;
+
+	/*
+	 * Some convertible have unreliable VGBS return which could cause incorrect
+	 * SW_TABLET_MODE report, in these cases we enable support when receiving
+	 * the first event instead of during driver setup.
+	 */
+	if (!priv->switches && (event == 0xcc || event == 0xcd)) {
+		dev_info(&device->dev, "switch event received, enable switches supports\n");
+		err = intel_hid_switches_setup(device);
+		if (err)
+			pr_err("Failed to setup Intel HID switches\n");
+	}
 
 	if (priv->wakeup_mode) {
 		/*
-- 
2.28.0

Re: [PATCH v2 2/2] intel-hid: add alternative method to enable switches

[Barnabás Pőcze]

Hi


2020. december 3., csütörtök 22:21 keltezéssel, Elia Devito írta:

> [...]
> diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
> index 86261970bd8f..fed24d4f28b8 100644
> --- a/drivers/platform/x86/intel-hid.c
> +++ b/drivers/platform/x86/intel-hid.c
> @@ -15,6 +15,9 @@
>  #include <linux/platform_device.h>
>  #include <linux/suspend.h>
>
> +/* When NOT in tablet mode, VGBS returns with the flag 0x40 */
> +#define TABLET_MODE_FLAG 0x40

I think `BIT(6)` would be better (linux/bits.h).


> +
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Alex Hung");
>
> @@ -89,9 +92,26 @@ static const struct dmi_system_id button_array_table[] = {
>  	{ }
>  };
> [...]
> +static void detect_tablet_mode(struct platform_device *device)

I believe `report_tablet_mode_state()` or something similar would be a more apt name.


> +{
> +	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
> +	acpi_handle handle = ACPI_HANDLE(&device->dev);
> +	unsigned long long vgbs;
> +	int m;
> +
> +	if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_VGBS_FN, &vgbs))
> +		return;
> +
> +	m = !(vgbs & TABLET_MODE_FLAG);
> +	input_report_switch(priv->switches, SW_TABLET_MODE, m);
> +	input_sync(priv->switches);
> +}
> +
>  static void notify_handler(acpi_handle handle, u32 event, void *context)
>  {
>  	struct platform_device *device = context;
> @@ -363,6 +415,13 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>  		if (event == 0xce)
>  			goto wakeup;
>
> +		/*
> +		 * Switch events will wake the device and report the new switch
> +		 * position to the input subsystem.
> +		 */
> +		if (priv->switches && (event == 0xcc || event == 0xcd))
> +			goto wakeup;
> +
>  		/* Wake up on 5-button array events only. */
>  		if (event == 0xc0 || !priv->array)
>  			return;
> @@ -374,6 +433,21 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>
>  wakeup:
>  		pm_wakeup_hard_event(&device->dev);
> +
> +		if (priv->switches) {
> +			if (event == 0xcc) {
> +				input_report_switch(priv->switches, SW_TABLET_MODE, 1);
> +				input_sync(priv->switches);
> +				return;
> +			}
> +
> +			if (event == 0xcd) {
> +				input_report_switch(priv->switches, SW_TABLET_MODE, 0);
> +				input_sync(priv->switches);
> +				return;
> +			}
> +		}
> +
>  		return;
>  	}
>
> @@ -398,6 +472,20 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>  		}
>  	}
>
> +	if (priv->switches) {
> +		if (event == 0xcc) {
> +			input_report_switch(priv->switches, SW_TABLET_MODE, 1);
> +			input_sync(priv->switches);
> +			return;
> +		}
> +
> +		if (event == 0xcd) {
> +			input_report_switch(priv->switches, SW_TABLET_MODE, 0);
> +			input_sync(priv->switches);
> +			return;
> +		}
> +	}

Wouldn't be better to create a new function `bool report_tablet_mode_event()`
which would basically contain the above `if` or better, a `switch`, and then
you could use it here and in the wake-up path like the following:

```
if (report_tablet_mode_event(priv->switches, event))
  return;
```
(or similarly)


> +
>  	/* 0xC0 is for HID events, other values are for 5 button array */
>  	if (event != 0xc0) {
>  		if (!priv->array ||
> @@ -485,6 +573,16 @@ static int intel_hid_probe(struct platform_device *device)
>  			pr_err("Failed to setup Intel 5 button array hotkeys\n");
>  	}
>
> +	/* Setup switches for devices that we know VGBS return correctly */
> +	if (dmi_check_system(dmi_vgbs_allow_list)) {
> +		dev_info(&device->dev, "platform supports switches\n");
> +		err = intel_hid_switches_setup(device);
> +		if (err)
> +			pr_err("Failed to setup Intel HID switches\n");
> +		else
> +			detect_tablet_mode(device);
> +	}
> +
>  	status = acpi_install_notify_handler(handle,
>  					     ACPI_DEVICE_NOTIFY,
>  					     notify_handler,
> --
> 2.28.0


Regards,
Barnabás Pőcze

Re: [PATCH v2 2/2] intel-hid: add alternative method to enable switches

[Barnabás Pőcze]

2020. december 4., péntek 0:45 keltezéssel, Barnabás Pőcze írta:

> Hi
>
> [...]


Oh well, I replied to the wrong email, apologies. :-(

Re: [PATCH v2 2/2] intel-hid: add alternative method to enable switches

[Elia Devito]

Hi Barnabás

In data venerdì 4 dicembre 2020 00:45:10 CET, Barnabás Pőcze ha scritto:
> Hi
> 
> 2020. december 3., csütörtök 22:21 keltezéssel, Elia Devito írta:
> > [...]
> > diff --git a/drivers/platform/x86/intel-hid.c
> > b/drivers/platform/x86/intel-hid.c index 86261970bd8f..fed24d4f28b8
> > 100644
> > --- a/drivers/platform/x86/intel-hid.c
> > +++ b/drivers/platform/x86/intel-hid.c
> > @@ -15,6 +15,9 @@
> > 
> >  #include <linux/platform_device.h>
> >  #include <linux/suspend.h>
> > 
> > +/* When NOT in tablet mode, VGBS returns with the flag 0x40 */
> > +#define TABLET_MODE_FLAG 0x40
> 
> I think `BIT(6)` would be better (linux/bits.h).
> 
Okay,  I will change it

> > +
> > 
> >  MODULE_LICENSE("GPL");
> >  MODULE_AUTHOR("Alex Hung");
> > 
> > @@ -89,9 +92,26 @@ static const struct dmi_system_id button_array_table[]
> > = {> 
> >  	{ }
> >  
> >  };
> > 
> > [...]
> > +static void detect_tablet_mode(struct platform_device *device)
> 
> I believe `report_tablet_mode_state()` or something similar would be a more
> apt name.
Sound good,  I will rename it.

> > +{
> > +	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
> > +	acpi_handle handle = ACPI_HANDLE(&device->dev);
> > +	unsigned long long vgbs;
> > +	int m;
> > +
> > +	if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_VGBS_FN, 
&vgbs))
> > +		return;
> > +
> > +	m = !(vgbs & TABLET_MODE_FLAG);
> > +	input_report_switch(priv->switches, SW_TABLET_MODE, m);
> > +	input_sync(priv->switches);
> > +}
> > +
> > 
> >  static void notify_handler(acpi_handle handle, u32 event, void *context)
> >  {
> >  
> >  	struct platform_device *device = context;
> > 
> > @@ -363,6 +415,13 @@ static void notify_handler(acpi_handle handle, u32
> > event, void *context)> 
> >  		if (event == 0xce)
> >  		
> >  			goto wakeup;
> > 
> > +		/*
> > +		 * Switch events will wake the device and report the 
new switch
> > +		 * position to the input subsystem.
> > +		 */
> > +		if (priv->switches && (event == 0xcc || event == 0xcd))
> > +			goto wakeup;
> > +
> > 
> >  		/* Wake up on 5-button array events only. */
> >  		if (event == 0xc0 || !priv->array)
> >  		
> >  			return;
> > 
> > @@ -374,6 +433,21 @@ static void notify_handler(acpi_handle handle, u32
> > event, void *context)> 
> >  wakeup:
> >  		pm_wakeup_hard_event(&device->dev);
> > 
> > +
> > +		if (priv->switches) {
> > +			if (event == 0xcc) {
> > +				input_report_switch(priv-
>switches, SW_TABLET_MODE, 1);
> > +				input_sync(priv->switches);
> > +				return;
> > +			}
> > +
> > +			if (event == 0xcd) {
> > +				input_report_switch(priv-
>switches, SW_TABLET_MODE, 0);
> > +				input_sync(priv->switches);
> > +				return;
> > +			}
> > +		}
> > +
> > 
> >  		return;
> >  	
> >  	}
> > 
> > @@ -398,6 +472,20 @@ static void notify_handler(acpi_handle handle, u32
> > event, void *context)> 
> >  		}
> >  	
> >  	}
> > 
> > +	if (priv->switches) {
> > +		if (event == 0xcc) {
> > +			input_report_switch(priv->switches, 
SW_TABLET_MODE, 1);
> > +			input_sync(priv->switches);
> > +			return;
> > +		}
> > +
> > +		if (event == 0xcd) {
> > +			input_report_switch(priv->switches, 
SW_TABLET_MODE, 0);
> > +			input_sync(priv->switches);
> > +			return;
> > +		}
> > +	}
> 
> Wouldn't be better to create a new function `bool
> report_tablet_mode_event()` which would basically contain the above `if` or
> better, a `switch`, and then you could use it here and in the wake-up path
> like the following:
> 
> ```
> if (report_tablet_mode_event(priv->switches, event))
>   return;
> ```
> (or similarly)
> 
Looks more clean, I will do.

> > +
> > 
> >  	/* 0xC0 is for HID events, other values are for 5 button array */
> >  	if (event != 0xc0) {
> >  	
> >  		if (!priv->array ||
> > 
> > @@ -485,6 +573,16 @@ static int intel_hid_probe(struct platform_device
> > *device)> 
> >  			pr_err("Failed to setup Intel 5 button array 
hotkeys\n");
> >  	
> >  	}
> > 
> > +	/* Setup switches for devices that we know VGBS return correctly 
*/
> > +	if (dmi_check_system(dmi_vgbs_allow_list)) {
> > +		dev_info(&device->dev, "platform supports switches\n");
> > +		err = intel_hid_switches_setup(device);
> > +		if (err)
> > +			pr_err("Failed to setup Intel HID 
switches\n");
> > +		else
> > +			detect_tablet_mode(device);
> > +	}
> > +
> > 
> >  	status = acpi_install_notify_handler(handle,
> >  	
> >  					     
ACPI_DEVICE_NOTIFY,
> >  					     notify_handler,
> > 
> > --
> > 2.28.0
> 
> Regards,
> Barnabás Pőcze

Thank you for the review

Regards
Elia