On Mon, 2018-12-03 at 13:00 +0100, Takashi Iwai wrote:
On Fri, 30 Nov 2018 00:57:37 +0100, Ayman Bagabas wrote:
This driver adds support for missing hotkeys on some Huawei laptops. Laptops such as the Matebook X have non functioning hotkeys. Whereas newer laptops such as the Matebook X Pro come with working hotkeys out of the box.
Old laptops, such as the Matebook X, report hotkey events through ACPI device "\WMI0". However, new laptops, such as the Matebook X Pro, does not have this WMI device.
All the hotkeys on the Matebook X Pro work fine without this patch except (micmute, wlan, and huawei key). These keys and the brightness keys report events to "\AMW0" ACPI device. One problem is that brightness keys on the Matebook X Pro work without this patch. This results in reporting two brightness key press events one is captured by ACPI and another by this driver.
A solution would be to check if such event came from the "\AMW0" WMI driver then skip reporting event. Another solution would be to leave this to user-space to handle. Which can be achieved by using "hwdb" tables and remap those keys to "unknown". This solution seems more natural to me because it leaves the decision to user-space.
Signed-off-by: Ayman Bagabas ayman.bagabas@gmail.com
The new patch looks much better than the previous one, thanks for working on it.
Just a few comments:
+struct huawei_wmi_priv {
- struct input_dev *idev;
- struct led_classdev cdev;
- acpi_handle handle;
Is this handle set in anywhere? I couldn't see it in your patch. If it's supposed to be NULL, passing NULL explicitly makes your intention clearer.
+static int huawei_wmi_leds_setup(struct wmi_device *wdev) +{
- struct huawei_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
- acpi_status status;
- // Skip registering LED subsystem if no ACPI method was found.
- status = acpi_get_handle(priv->handle, "\_SB.PCI0.LPCB.EC0",
&priv->handle);
- if (ACPI_FAILURE(status))
return 0;- if (acpi_has_method(priv->handle, "SPIN"))
priv->acpi_method = "SPIN";- else if (acpi_has_method(priv->handle, "WPIN"))
priv->acpi_method = "WPIN";- else
return 0;- priv->cdev.name = "platform::micmute";
- priv->cdev.max_brightness = 1;
- priv->cdev.brightness_set_blocking =
huawei_wmi_micmute_led_set;
- priv->cdev.default_trigger = "audio-micmute";
- priv->cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
- priv->cdev.dev = &wdev->dev;
What about suspend/resume? When the driver is bound wit HD-audio, the HD-audio will restore the state at resume, so it would work. But, by providing the LED class device, it is supposed to work even without HD-audio, so it might make sense to pass LED_CORE_SUSPENDRESUME, too.
Besides that, is there anything needed for wmi_device suspend/resume?
+static int __init huawei_wmi_init(void) +{
- if (!(wmi_has_guid(WMI0_EVENT_GUID) ||
wmi_has_guid(AMW0_EVENT_GUID))) {
pr_debug("Compatible WMI GUID not found\n");return -ENODEV;- }
This is superfluous when you implement with wmi_driver. In theory, the supported GUID can be added dynamically via sysfs, too.
I left it that way so it doesn't insert the module if these GUIDs were not found. Should I drop that and use module_wmi_driver instead?
thanks,
Takashi