[PATCH] HID: asus: only support backlight when it's not driven by WMI

[PATCH] HID: asus: only support backlight when it's not driven by WMI

[Daniel Drake]

The Asus GL502VSK has the same 0B05:1837 keyboard as we've seen in
several Republic of Gamers laptops.

However, in this model, the keybard backlight control exposed by hid-asus
has no effect on the keyboard backlight. Instead, the keyboard
backlight is correctly driven by asus-wmi.

With two keyboard backlight devices available (and only the acer-wmi
one working), GNOME is picking the wrong one to drive in the UI.

Avoid this problem by not creating the backlight interface when we
detect a WMI-driven keyboard backlight.

We have also tested Asus GL702VMK which does have the hid-asus
backlight present, and it still works fine with this patch (WMI method
call returns UNSUPPORTED_METHOD).

Signed-off-by: Daniel Drake <drake@endlessm.com>
---
 drivers/hid/Kconfig    |  1 +
 drivers/hid/hid-asus.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 779c5ae47f36..6d95abc9d8a1 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -149,6 +149,7 @@ config HID_APPLEIR
 config HID_ASUS
 	tristate "Asus"
 	depends on LEDS_CLASS
+	depends on ACPI_WMI
 	---help---
 	Support for Asus notebook built-in keyboard and touchpad via i2c, and
 	the Asus Republic of Gamers laptop keyboard special keys.
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 1bb7b63b3150..e6830946b4a4 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -26,6 +26,7 @@
  * any later version.
  */
 
+#include <linux/acpi.h>
 #include <linux/hid.h>
 #include <linux/module.h>
 #include <linux/input/mt.h>
@@ -78,6 +79,12 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 
 #define TRKID_SGN       ((TRKID_MAX + 1) >> 1)
 
+#define ASUS_WMI_MGMT_GUID		"97845ED0-4E6D-11DE-8A39-0800200C9A66"
+#define ASUS_WMI_METHODID_DSTS2		0x53545344 /* Device STatuS #2*/
+#define ASUS_WMI_DEVID_KBD_BACKLIGHT	0x00050021
+#define ASUS_WMI_UNSUPPORTED_METHOD	0xFFFFFFFE
+#define ASUS_WMI_DSTS_PRESENCE_BIT	0x00010000
+
 struct asus_kbd_leds {
 	struct led_classdev cdev;
 	struct hid_device *hdev;
@@ -330,6 +337,48 @@ static void asus_kbd_backlight_work(struct work_struct *work)
 		hid_err(led->hdev, "Asus failed to set keyboard backlight: %d\n", ret);
 }
 
+/* WMI-based keyboard backlight LED control (via asus-wmi driver) takes
+ * precedence. We only activate HID-based backlight control when the
+ * WMI control is not available.
+ */
+static bool asus_kbd_wmi_led_control_present(struct hid_device *hdev)
+{
+	u32 args[] = { ASUS_WMI_DEVID_KBD_BACKLIGHT, 0 };
+	struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
+	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
+	acpi_status status;
+	union acpi_object *obj;
+	u32 value;
+
+	status = wmi_evaluate_method(ASUS_WMI_MGMT_GUID, 0,
+				     ASUS_WMI_METHODID_DSTS2,
+				     &input, &output);
+
+	if (ACPI_FAILURE(status)) {
+		hid_dbg(hdev, "WMI backlight method failed: %d", status);
+		return false;
+	}
+
+	obj = (union acpi_object *)output.pointer;
+	if (!obj || obj->type != ACPI_TYPE_INTEGER) {
+		hid_dbg(hdev, "WMI backlight method unexpected return type");
+		kfree(obj);
+		return false;
+	}
+
+	value = (u32) obj->integer.value;
+	kfree(obj);
+
+	hid_dbg(hdev, "WMI backlight check: method returned %x", value);
+
+	if (value == ASUS_WMI_UNSUPPORTED_METHOD) {
+		hid_dbg(hdev, "WMI backlight method unsupported");
+		return false;
+	}
+
+	return !!(value & ASUS_WMI_DSTS_PRESENCE_BIT);
+}
+
 static int asus_kbd_register_leds(struct hid_device *hdev)
 {
 	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
@@ -417,7 +466,9 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
 
 	drvdata->input = input;
 
-	if (drvdata->enable_backlight && asus_kbd_register_leds(hdev))
+	if (drvdata->enable_backlight &&
+	    !asus_kbd_wmi_led_control_present(hdev) &&
+	    asus_kbd_register_leds(hdev))
 		hid_warn(hdev, "Failed to initialize backlight.\n");
 
 	return 0;
-- 
2.14.1

Re: [PATCH] HID: asus: only support backlight when it's not driven by WMI

[Andy Shevchenko]

On Tue, Jan 9, 2018 at 3:20 AM, Daniel Drake <drake@endlessm.com> wrote:
> The Asus GL502VSK has the same 0B05:1837 keyboard as we've seen in
> several Republic of Gamers laptops.
>
> However, in this model, the keybard backlight control exposed by hid-asus
> has no effect on the keyboard backlight. Instead, the keyboard
> backlight is correctly driven by asus-wmi.
>
> With two keyboard backlight devices available (and only the acer-wmi
> one working), GNOME is picking the wrong one to drive in the UI.
>
> Avoid this problem by not creating the backlight interface when we
> detect a WMI-driven keyboard backlight.
>
> We have also tested Asus GL702VMK which does have the hid-asus
> backlight present, and it still works fine with this patch (WMI method
> call returns UNSUPPORTED_METHOD).

>  config HID_ASUS
>         tristate "Asus"
>         depends on LEDS_CLASS
> +       depends on ACPI_WMI

No, for sure.
Imagine someone who on possession of laptop where it's not needed
having old kernel configuration.
Building new kernel with old configuration will bring a regression.

Selection is also not a solution since we don't need all crap in
kernel because of some particular case.

So, NO.

> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c

> +#define ASUS_WMI_MGMT_GUID             "97845ED0-4E6D-11DE-8A39-0800200C9A66"
> +#define ASUS_WMI_METHODID_DSTS2                0x53545344 /* Device STatuS #2*/
> +#define ASUS_WMI_DEVID_KBD_BACKLIGHT   0x00050021
> +#define ASUS_WMI_UNSUPPORTED_METHOD    0xFFFFFFFE
> +#define ASUS_WMI_DSTS_PRESENCE_BIT     0x00010000

> +/* WMI-based keyboard backlight LED control (via asus-wmi driver) takes
> + * precedence. We only activate HID-based backlight control when the
> + * WMI control is not available.
> + */
> +static bool asus_kbd_wmi_led_control_present(struct hid_device *hdev)
> +{

> +}

I have feelings that the code above should be located somewhere under
drivers/platform/x86.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] HID: asus: only support backlight when it's not driven by WMI

[Daniel Drake]

On Tue, Jan 9, 2018 at 1:32 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> Imagine someone who on possession of laptop where it's not needed
> having old kernel configuration.
> Building new kernel with old configuration will bring a regression.
>
> Selection is also not a solution since we don't need all crap in
> kernel because of some particular case.
>
> So, NO.

If I can't use depends nor select, then what options are left? What
alternative solutions do you have in mind?

Thanks
Daniel

Re: [PATCH] HID: asus: only support backlight when it's not driven by WMI

[Andy Shevchenko]

On Tue, Jan 9, 2018 at 9:41 PM, Daniel Drake <drake@endlessm.com> wrote:
> On Tue, Jan 9, 2018 at 1:32 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> Imagine someone who on possession of laptop where it's not needed
>> having old kernel configuration.
>> Building new kernel with old configuration will bring a regression.
>>
>> Selection is also not a solution since we don't need all crap in
>> kernel because of some particular case.
>>
>> So, NO.
>
> If I can't use depends nor select, then what options are left? What
> alternative solutions do you have in mind?

If you would able to move code under corresponding WMI driver, make it
use any means of autodetection (it looks like you found a way via
checking return code of method call) and
then rely on distributions that they enable necessary modules
(HID_ASUS, ASUS_WMI or alike).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] HID: asus: only support backlight when it's not driven by WMI

[Mustafa Kuscu]

On Tue, Jan 9, 2018 at 10:58 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Jan 9, 2018 at 9:41 PM, Daniel Drake <drake@endlessm.com> wrote:
>> On Tue, Jan 9, 2018 at 1:32 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> Imagine someone who on possession of laptop where it's not needed
>>> having old kernel configuration.
>>> Building new kernel with old configuration will bring a regression.
>>>
>>> Selection is also not a solution since we don't need all crap in
>>> kernel because of some particular case.
>>>
>>> So, NO.
>>
>> If I can't use depends nor select, then what options are left? What
>> alternative solutions do you have in mind?
>
> If you would able to move code under corresponding WMI driver, make it
> use any means of autodetection (it looks like you found a way via
> checking return code of method call) and
> then rely on distributions that they enable necessary modules
> (HID_ASUS, ASUS_WMI or alike).
>
> --
> With Best Regards,
> Andy Shevchenko
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Hi,

It is interesting that the backlight buttons did not work for you. I
think your keyboard (0b05:1837)is a
USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD2.
I also have a ROG laptop (GL553VD) whose keyboard (0b05:1854) is being
identified as USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD1. Its keyboard
backlight shortcuts are enabled only after applying the following
patch. I am wondering why this harmless one liner did not find its way
into the modules tree. It's difficult to apply the patch to every keep
up with the latest. Maybe other ASUS people confirm this and help
pushing it further..
Kind Regards...

Signed-off-by: Mustafa C Kuscu <mustafakuscu@gmail.com>
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index a4a3c38bc145..39068eddc03c 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -593,7 +593,7 @@ static const struct hid_device_id asus_devices[] = {
     { HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK,
         USB_DEVICE_ID_ASUSTEK_I2C_TOUCHPAD), I2C_TOUCHPAD_QUIRKS },
     { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
-        USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD1) },
+        USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD1), QUIRK_USE_KBD_BACKLIGHT },
     { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
         USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD2), QUIRK_USE_KBD_BACKLIGHT },
     { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,

Re: [PATCH] HID: asus: only support backlight when it's not driven by WMI

[Daniel Drake]

Hi Mustafa,
On Fri, Jan 12, 2018 at 1:33 PM, Mustafa Kuscu <mustafakuscu@gmail.com> wrote:

>      { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> -        USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD1) },
> +        USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD1), QUIRK_USE_KBD_BACKLIGHT },

This change is already included in the latest version of Linux so I
assume you are just running an old version.

Andy, still waiting for any more feedback on the issue detailed in
this thread once you have a chance.

Thanks
Daniel

Re: [PATCH] HID: asus: only support backlight when it's not driven by WMI

[Andy Shevchenko]

On Mon, Jan 15, 2018 at 10:33 PM, Daniel Drake <drake@endlessm.com> wrote:
> On Fri, Jan 12, 2018 at 1:33 PM, Mustafa Kuscu <mustafakuscu@gmail.com> wrote:

> Andy, still waiting for any more feedback on the issue detailed in
> this thread once you have a chance.

As I said before I don't like code duplication and even more I don't
like functionality duplication.
So, check what can we re-use from asus-wmi.c and adopt it accordingly.

-- 
With Best Regards,
Andy Shevchenko