Re: [PATCH v8 2/3] x86: add support for Huawei WMI hotkeys.

[Takashi Iwai <tiwai@suse.de>]

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.


> +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