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

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();

+}