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

[ayman . bagabas]

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@xxxxxxxxx>
> 
> 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