Hi Maxim,
On 7/12/23 19:50, Maxim Mikityanskiy wrote:
> SW_TABLET_MODE reports are broken on BIOS versions newer than 1.9.1 on
> HP Elite Dragonfly G2. Analysis of SSDT9 shows that the BTNL method has
> to be called to start getting 0xcc and 0xcd events. Apparently, the
> button_array_present method used to return true on BIOS 1.9.1 and older,
> but it returns false on newer BIOSes due to HEBC returning 0x000033f3
> (bits 0x60000 and 0x20000 are not set).
>
> Add this laptop to button_array_table to force the BTNL call, and also
> add it to dmi_vgbs_allow_list to read the initial state and sync VBDS
> with VBPS, because this laptop has a reliable VGBS method.
>
> Tested with BIOS 1.13.1.
>
> Signed-off-by: Maxim Mikityanskiy <maxtram95@xxxxxxxxx>
Thank you for figuring this out and thank you for the patch.
WRT the need to call BTNL, I expect that if one laptop needs this likely more models will need it and I expect / hope that this will not cause any issues when called everywhere.
So I think it would be better to instead of adding this model to the button_array_table[] it would be better to just always call BTNL (I suspect this is also what Windows does).
Can you give the attached path to always call BTNL a try ?
As for adding the model to dmi_vgbs_allow_list[] on many models it seems that the EC will send a 0xcc / 0xcd event at boot (now that I think about it I guess this may be in response to the BTNL call) and then on that event the SW_TABLET_STATE evdev gets registered and the first event also syncs the state.
Can you check if just using the always call BTNL patch, without the quirk perhaps already makes the SW_TABLET_STATE evdev show up (including the correct state) ?
If no event is send during boot, the n I'm fine with adding the model to the dmi_vgbs_allow_list[], but perhaps this is not necessary?
Regards,
Hans
> ---
> 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 5632bd3c534a..5c78b476ed1e 100644
> --- a/drivers/platform/x86/intel/hid.c
> +++ b/drivers/platform/x86/intel/hid.c
> @@ -128,6 +128,13 @@ static const struct dmi_system_id button_array_table[] = {
> DMI_MATCH(DMI_PRODUCT_NAME, "Surface Go 3"),
> },
> },
> + {
> + .ident = "HP Elite Dragonfly G2",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "HP"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "HP Elite Dragonfly G2 Notebook PC"),
> + },
> + },
> { }
> };
>
> @@ -150,6 +157,12 @@ static const struct dmi_system_id dmi_vgbs_allow_list[] = {
> DMI_MATCH(DMI_PRODUCT_NAME, "Surface Go"),
> },
> },
> + {
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "HP"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "HP Elite Dragonfly G2 Notebook PC"),
> + },
> + },
> { }
> };
>
From 1fb443cb1caf73e940a6b0550b7e79c86edf3aef Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Fri, 14 Jul 2023 17:51:38 +0200
Subject: [PATCH] platform/x86: intel: hid: Always call BTNL ACPI method
On a HP Elite Dragonfly G2 the 0xcc and 0xcd events for SW_TABLET_MODE
are only send after the BTNL ACPI method has been called.
Likely more devices need this, so make the BTNL ACPI method unconditional
instead of only doing it on devices with a 5 button array.
Note this also makes the intel_button_array_enable() call in probe()
unconditional, that function does its own priv->array check. This makes
the intel_button_array_enable() call in probe() consistent with the calls
done on suspend/resume which also rely on the priv->array check inside
the function.
Reported-by: Maxim Mikityanskiy <maxtram95@xxxxxxxxx>
Closes: https://lore.kernel.org/platform-driver-x86/20230712175023.31651-1-maxtram95@xxxxxxxxx/
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/platform/x86/intel/hid.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/drivers/platform/x86/intel/hid.c b/drivers/platform/x86/intel/hid.c
index 5632bd3c534a..641f2797406e 100644
--- a/drivers/platform/x86/intel/hid.c
+++ b/drivers/platform/x86/intel/hid.c
@@ -620,7 +620,7 @@ static bool button_array_present(struct platform_device *device)
static int intel_hid_probe(struct platform_device *device)
{
acpi_handle handle = ACPI_HANDLE(&device->dev);
- unsigned long long mode;
+ unsigned long long mode, dummy;
struct intel_hid_priv *priv;
acpi_status status;
int err;
@@ -692,18 +692,15 @@ static int intel_hid_probe(struct platform_device *device)
if (err)
goto err_remove_notify;
- if (priv->array) {
- unsigned long long dummy;
+ intel_button_array_enable(&device->dev, true);
- intel_button_array_enable(&device->dev, true);
-
- /* Call button load method to enable HID power button */
- if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_BTNL_FN,
- &dummy)) {
- dev_warn(&device->dev,
- "failed to enable HID power button\n");
- }
- }
+ /*
+ * Call button load method to enable HID power button
+ * Always do this since it activates events on some devices without
+ * a button array too.
+ */
+ if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_BTNL_FN, &dummy))
+ dev_warn(&device->dev, "failed to enable HID power button\n");
device_init_wakeup(&device->dev, true);
/*
--
2.41.0
