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 <[email protected]>
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.
> +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.
thanks,
Takashi
