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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

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