From mboxrd@z Thu Jan 1 00:00:00 1970 From: Benjamin Tissoires Subject: Re: [PATCH] HID: asus: fix special key support for ASUS I2C keyboards Date: Mon, 6 Mar 2017 09:47:01 +0100 Message-ID: <20170306084701.GV7064@mail.corp.redhat.com> References: <1488675034-7220-1-git-send-email-matjaz.hegedic@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Return-path: Received: from mx1.redhat.com ([209.132.183.28]:43858 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752639AbdCFIrS (ORCPT ); Mon, 6 Mar 2017 03:47:18 -0500 Content-Disposition: inline In-Reply-To: <1488675034-7220-1-git-send-email-matjaz.hegedic@gmail.com> Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: Matjaz Hegedic Cc: jikos@kernel.org, linux-input@vger.kernel.org, Daniel Drake Hi, [Adding Daniel in CC, he is also curerntly working on Asus keyboards] On Mar 05 2017 or thereabouts, Matjaz Hegedic wrote: > On some ASUS notebooks (EeeBook X205TA, VivoBook E200HA) the built-in > keyboard uses a vendor-specific HID Usage Page for its special keys > (airplane mode, brightness, volume, etc.) which require remapping. > This cannot be resolved without a kernel driver (eg. udev/hwdb). > In addition, sloppy vendor implementation produces two tables > almost full of dummy usages, which misrepresent this devices' > capabilities and causes X to see it as a pointer/joystick device. > This patch adds the appropriate re-mappings and ignores the incorrect > dummy values. Shouldn't the quirk QUIRK_NO_CONSUMER_USAGES be set on a per-device basis, not globally for each keyboard? Also, is this quirk really needed since v4.9 with 1989dada7ce07848196991c9ebf25ff9c5f14d4e ("HID: input: ignore System Control application usages if not System Controls")? One nitpick inlined below: > > Signed-off-by: Matjaz Hegedic > --- > drivers/hid/hid-asus.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 48 insertions(+), 2 deletions(-) > > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c > index 70b12f8..a6492ec 100644 > --- a/drivers/hid/hid-asus.c > +++ b/drivers/hid/hid-asus.c > @@ -1,12 +1,14 @@ > /* > * HID driver for Asus notebook built-in keyboard. > - * Fixes small logical maximum to match usage maximum. > + * Fixes small logical maximum to match usage maximum, > + * adds special key support, and ignores dummy usages. > * > * Currently supported devices are: > * EeeBook X205TA > * VivoBook E200HA > * > * Copyright (c) 2016 Yusuke Fujimaki > + * Copyright (c) 2017 Matjaz Hegedic > * > * This module based on hid-ortek by > * Copyright (c) 2010 Johnathon Harris > @@ -36,8 +38,11 @@ MODULE_AUTHOR("Yusuke Fujimaki "); > MODULE_AUTHOR("Brendan McGrath "); > MODULE_AUTHOR("Victor Vlasenko "); > MODULE_AUTHOR("Frederik Wenigwieser "); > +MODULE_AUTHOR("Matjaz Hegedic "); > MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad"); > > +#define HID_UP_ASUSVENDOR 0xff310000 > + > #define FEATURE_REPORT_ID 0x0d > #define INPUT_REPORT_ID 0x5d > > @@ -63,15 +68,20 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad"); > #define QUIRK_NO_INIT_REPORTS BIT(1) > #define QUIRK_SKIP_INPUT_MAPPING BIT(2) > #define QUIRK_IS_MULTITOUCH BIT(3) > +#define QUIRK_NO_CONSUMER_USAGES BIT(4) > > #define KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \ > - QUIRK_NO_INIT_REPORTS) > + QUIRK_NO_INIT_REPORTS | \ > + QUIRK_NO_CONSUMER_USAGES) > #define TOUCHPAD_QUIRKS (QUIRK_NO_INIT_REPORTS | \ > QUIRK_SKIP_INPUT_MAPPING | \ > QUIRK_IS_MULTITOUCH) > > #define TRKID_SGN ((TRKID_MAX + 1) >> 1) > > +#define asus_map_key_clear(c) hid_map_usage_clear(hi, usage, bit, \ > + max, EV_KEY, (c)) > + > struct asus_drvdata { > unsigned long quirks; > struct input_dev *input; > @@ -213,6 +223,42 @@ static int asus_input_mapping(struct hid_device *hdev, > return -1; > } > > + if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ASUSVENDOR) { > + switch (usage->hid & HID_USAGE) { > + case 0x10: > + asus_map_key_clear(KEY_BRIGHTNESSDOWN); break; Please put "break" in a separate line. Otherwise, I just feel like there is a fall-through and I got confused. Same applies to the rest of the cases. Cheers, Benjamin > + case 0x20: > + asus_map_key_clear(KEY_BRIGHTNESSUP); break; > + case 0x35: > + asus_map_key_clear(KEY_DISPLAY_OFF); break; > + case 0x6b: > + asus_map_key_clear(KEY_F21); break; > + case 0x6c: > + asus_map_key_clear(KEY_SLEEP); break; > + case 0x88: > + asus_map_key_clear(KEY_RFKILL); break; > + default: > + /* ASUS lazily declares 256 usages, ignore the rest */ > + return -1; > + } > + return 1; > + } > + > + if (drvdata->quirks & QUIRK_NO_CONSUMER_USAGES && > + (usage->hid & HID_USAGE_PAGE) == HID_UP_CONSUMER) { > + switch (usage->hid & HID_USAGE) { > + case 0xe2: /* Mute */ > + case 0xe9: /* Volume up */ > + case 0xea: /* Volume down */ > + return 0; > + default: > + /* Ignore dummy Consumer usages which make the > + * keyboard incorrectly appear as a pointer device. > + */ > + return -1; > + } > + } > + > return 0; > } > > -- > 2.7.4 >