Re: [PATCH v3 1/3] x86: add support for Huawei WMI hotkeys.

Andy Shevchenko
2018-11-08T19:58:58Z

On Thu, Nov 8, 2018 at 7:17 PM Ayman Bagabas <ayman.bagabas@gmail.com> wrote:
>
> This driver adds support for missing hotkeys on some Huawei laptops.
> Currently, only Huawei Matebook X and Matebook X Pro is supported.
>

Thanks for an update, my comments below.

> +config HUAWEI_LAPTOP
> + tristate "Huawei WMI hotkeys driver"

> + depends on ACPI

Do you need an ACPI dependency be explicit here?

> + depends on ACPI_WMI
> + depends on INPUT
> + select INPUT_SPARSEKMAP
> + help
> + This driver provides support for Huawei WMI hotkeys.
> + It enables the missing keys and adds support to the micmute
> + LED found on some of these laptops.

> +/*
> + * Huawei WMI hotkeys
> + *
> + * Copyright (C) 2018 Ayman Bagabas <ayman.bagabas@gmail.com>

> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <https://www.gnu.org/licenses/>.
> + *

Please, replace this boilerplate text with appropriate SPDX identifier.

> + */

> +#include <linux/init.h>
> +#include <linux/module.h>

One of them should be chosen.

> +static char *event_guid;
> +static struct input_dev *inputdev;

> +int huawei_wmi_micmute_led_set(bool on)
> +{
> + acpi_handle handle = ACPI_HANDLE(&inputdev->dev);
> + char *method;
> + union acpi_object args[3];

> + args[0].type = args[1].type = args[2].type = ACPI_TYPE_INTEGER;
> + args[1].integer.value = 0x04;

Please, don't mix definitions and code.

> + struct acpi_object_list arg_list = {
> + .pointer = args,
> + .count = ARRAY_SIZE(args),
> + };
> +
> + if (acpi_has_method(handle, method = "\\_SB.PCI0.LPCB.EC0.SPIN")) {
> + args[0].integer.value = 0;
> + args[2].integer.value = on ? 1 : 0;
> + } else if (acpi_has_method(handle, method = "\\_SB.PCI0.LPCB.EC0.WPIN")) {
> + args[0].integer.value = 1;
> + args[2].integer.value = on ? 0 : 1;
> + } else {
> + pr_err("Unable to find ACPI method\n");

dev_err() here.

> + return -1;

Return appropriate error code.

> + }
> +
> + acpi_evaluate_object(handle, method, &arg_list, NULL);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(huawei_wmi_micmute_led_set);
> +
> +static void huawei_wmi_process_key(struct input_dev *inputdev, int code)
> +{

> + acpi_status status;
> + unsigned long long result;
> + const char *method = "\\WMI0.WQ00";
> + union acpi_object args[] = {
> + { .type = ACPI_TYPE_INTEGER },
> + };

> + args[0].integer.value = 0;

Don't mix definitions and code.

> + struct acpi_object_list arg_list = {
> + .pointer = args,
> + .count = ARRAY_SIZE(args),
> + };

> + if ((key->sw.code == KEY_BRIGHTNESSUP
> + || key->sw.code == KEY_BRIGHTNESSDOWN)

I believe this can fit one line.

> + && strcmp(event_guid, MBXP_EVENT_GUID) == 0)
> + return;
> +
> + sparse_keymap_report_entry(inputdev, key, 1, true);
> +}

> +static int __init huawei_wmi_init(void)
> +{
> + int err;
> +
> + if (wmi_has_guid(MBX_EVENT_GUID)) {
> + event_guid = MBX_EVENT_GUID;
> + } else if (wmi_has_guid(MBXP_EVENT_GUID)) {
> + event_guid = MBXP_EVENT_GUID;
> + } else {

> + pr_warn("No known WMI GUID found\n");

Simple "Compatible WMI GUID not found\n".

> + return -ENODEV;
> + }
> +

> + err = huawei_wmi_input_init();
> + if (err) {

> + pr_err("Failed to setup input device\n");

Noise.

> + return err;
> + }
> +
> + return 0;

...just do

return huawei_wmi_input_init();

> +}

--
With Best Regards,
Andy Shevchenko