From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9711EC32789 for ; Fri, 2 Nov 2018 23:10:25 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 2E88920657 for ; Fri, 2 Nov 2018 23:10:25 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="JxV3tWRP" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2E88920657 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728427AbeKCIT3 (ORCPT ); Sat, 3 Nov 2018 04:19:29 -0400 Received: from mail-ua1-f68.google.com ([209.85.222.68]:46598 "EHLO mail-ua1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727007AbeKCIT3 (ORCPT ); Sat, 3 Nov 2018 04:19:29 -0400 Received: by mail-ua1-f68.google.com with SMTP id v24so1209520uap.13; Fri, 02 Nov 2018 16:10:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=message-id:subject:from:to:cc:date:in-reply-to:references :mime-version:content-transfer-encoding; bh=Ivvk7HbnmoOtibtEd/Mtxll5LMwQi2lxoLLQIOkfYhA=; b=JxV3tWRPDAsHOukJeynkCm9s3OSrmmu2m6CH1s0TXXGaBIYziGI+op6VTB9Vy3zWOK ZIvySVyZiMkmTGte0c/9lrw0hKZCoqyHaBvBolefBjfgFwfBGN6iWXH+N81n4pKIUy/w Logce0khUcG5LD+PtLyX7+jlbqeQYvF8kQH1hgQ4jLhr0gwpgFIW48oWIjB0ozDV34ad a34zMCSyYT2YlSzMhDrdp5UB6WfVl2ATk5FD/4a7O5yTeh4VM4ZskjtsZp/L2esG1d57 fBMVJrGaM6eLGFjYiJ+cu1rEeM9k5nnlIegJzXGle/1SK0cg/V2ikfJhkwwVxzKBicTW I2tQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:mime-version:content-transfer-encoding; bh=Ivvk7HbnmoOtibtEd/Mtxll5LMwQi2lxoLLQIOkfYhA=; b=KsdqcMu2XsUKWxgx4cy3ccxoWDIe0zGqdftdLiZDNRtSx8FTZiBFSNbp8nLbI4eDdk rKS3sosw02E4O6u78tHDNnGvIMGaeYUHHfY635SS08TD8t7Z2eBqK23QX0pI4IUhN0Vh /7HH3EpAlDpn0i0GTVwIWDFSI6QUX3TiQ+NuzWZIG/1HN0oX9WGtyc3FZaV907N22klr O3JrNsHEXYC4KESCTGFe45Z+8h263+eLmkqTZ/BmNKor3Vs8Mw862wSEjOvfqbpayJpf dTKsDzlbiXp+hfFiMZErT1p2yM6k4DryxSdhyE2e/mzGYack9nCPXuiLnoqG+S1a0z0+ NBpw== X-Gm-Message-State: AGRZ1gLXXo2HrKY/qMo1jLKfsJUiYJ0bcqpzOWUVmHR77t2JVYAT5PW+ /j/urfBqsEn5NLeL/2TjDw== X-Google-Smtp-Source: AJdET5cB4kZV3ELmUs3eGAWniWoDfa9oi3wTqx1VfCT1wf+r04kRKof547pv3t5lCQaCKlG/5tXKJg== X-Received: by 2002:ab0:5741:: with SMTP id t1mr6418852uac.87.1541200220788; Fri, 02 Nov 2018 16:10:20 -0700 (PDT) Received: from 960.ws ([2601:902:c200:6512:a7fd:514d:7aab:c075]) by smtp.gmail.com with ESMTPSA id 17-v6sm3280104vkx.49.2018.11.02.16.10.18 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 02 Nov 2018 16:10:19 -0700 (PDT) Message-ID: <3074f3045a43984fc9df4d4bb66d15e3f5293e90.camel@gmail.com> Subject: Re: [PATCH v2 1/3] x86: add support for Huawei WMI hotkeys. From: ayman.bagabas@gmail.com To: Andy Shevchenko Cc: Darren Hart , Andy Shevchenko , Jaroslav Kysela , Takashi Iwai , kailang@realtek.com, Hui Wang , Linux Kernel Mailing List , Platform Driver , ALSA Development Mailing List Date: Fri, 02 Nov 2018 19:10:18 -0400 In-Reply-To: References: <20181102041120.7603-1-ayman.bagabas@gmail.com> <20181102041120.7603-2-ayman.bagabas@gmail.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.28.5 (3.28.5-1.fc28) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 2018-11-02 at 20:20 +0200, Andy Shevchenko wrote: > On Fri, Nov 2, 2018 at 6:11 AM Ayman Bagabas > wrote: > > > > This driver adds support for missing hotkeys on some Huawei > > laptops. > > Currently, only Huawei Matebook X Pro is supported. The driver > > recognizes the following keys: brightness keys, micmute, wlan, and > > Huawei special key. The brightness keys are ignored since they work > > out > > of the box. > > +config HUAWEI_LAPTOP > > + tristate "Huawei WMI hotkeys driver" > > + depends on ACPI > > + 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 micmute > > + led found on these laptops.q > > + Supported devices are: > > + - Matebook X Pro > > > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Huawei WMI Hotkeys Driver > > + * > > + * 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/>. > > + * > > + */ > > +MODULE_LICENSE("GPL"); > > > Here you have the following issues: > - inconsistency between IDs for license (fix accordingly) > - SDPX _and_ License header (remove latter) > > > > +#include > > +#include > > +#include > > Choose one of them. > > > +#include > > +#include > > +#include > > +#include > > Please, keep in order. What order do I have to follow? Does this look right? module.h init.h apci.h wmi.h input.h sparse-keymap.h > > > +static const struct key_entry huawei_wmi_keymap[] __initconst = { > > + { KE_IGNORE, 0x281, { KEY_BRIGHTNESSDOWN } }, > > + { KE_IGNORE, 0x282, { KEY_BRIGHTNESSUP } }, > > + { KE_KEY, 0x287, { KEY_MICMUTE } }, > > + { KE_KEY, 0x289, { KEY_WLAN } }, > > + // Huawei |M| button > > + { KE_KEY, 0x28a, { KEY_PROG1 } }, > > + { KE_END, 0 } > > +}; > > + > > +struct huawei_wmi_device { > > + struct input_dev *inputdev; > > +}; > > Apparently no need to have this, you may use struct input_dev > directly, isn't it? > > > +static struct huawei_wmi_device *wmi_device; > > No global variables, please. What about input_dev as a global variable? How would input_unregister_device access input_dev on exit if it wasn't global? > > > +int huawei_wmi_micmute_led_set(bool on) > > +{ > > + u32 args = (on) ? MICMUTE_LED_ON : MICMUTE_LED_OFF; > > Redundant parens. > > > + struct acpi_buffer input = { (acpi_size)sizeof(args), &args > > }; > > + acpi_status status; > > + > > + status = wmi_evaluate_method(AMW0_GUID, 0, 1, &input, > > NULL); > > + if (ACPI_FAILURE(status)) > > + return status; > > + > > + return 0; > > +} > > +static void huawei_wmi_process_key(struct input_dev *input_dev, > > int code) > > +{ > > + const struct key_entry *key; > > + > > + key = sparse_keymap_entry_from_scancode(input_dev, code); > > + > > This blank line is redundant. > > > + if (!key) { > > + pr_info("%s: Unknown key pressed, code: 0x%04x\n", > > + MODULE_NAME, code); > > dev_info(); no MODULE_NAME, please. > > > + return; > > + } > > + > > + sparse_keymap_report_entry(input_dev, key, 1, true); > > +} > > +static void huawei_wmi_notify(u32 value, void *context) > > +{ > > + struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL > > }; > > + union acpi_object *obj; > > + acpi_status status; > > + > > + status = wmi_get_event_data(value, &response); > > + if (ACPI_FAILURE(status)) { > > + pr_err("%s: Bad event status 0x%x\n", > > + MODULE_NAME, status); > > No MODULE_NAME, please. > If needed, use pr_fmt() macro. > > But I'm pretty sure you may pass pointer to input device and use > dev_err(). > > > + return; > > + } > > + > > + obj = (union acpi_object *)response.pointer; > > + > > No redundant blank lines, please. > > > + if (!obj) > > + return; > > + > > + if (obj->type == ACPI_TYPE_INTEGER) > > + huawei_wmi_process_key(wmi_device->inputdev, > > + obj- > > >integer.value); > > + else > > + pr_info("%s: Unknown response received %d\n", > > + MODULE_NAME, obj->type); > > Ditto about module name. > > > + > > + kfree(response.pointer); > > +} > > + > > +static int huawei_input_init(void) > > +{ > > + acpi_status status; > > + int err; > > + status = wmi_install_notify_handler(EVENT_GUID, > > + huawei_wmi_notify, > > + NULL); > > Pointer to the input device instead of NULL. > > > + > > No redundant blank lines, please. > > > + if (ACPI_FAILURE(status)) { > > + err = -EIO; > > + goto err_free_dev; > > + } > > +} > > > > + wmi_device = kmalloc(sizeof(struct huawei_wmi_device), > > GFP_KERNEL); > > sizeof(*wmi_device) > > > + if (!wmi_device) > > + return -ENOMEM; > > +} > > > > + pr_debug("%s: Driver unloaded successfully\n", > > MODULE_NAME); > > Noise. > > -- > With Best Regards, > Andy Shevchenko