Re: [PATCH] HID: asus: fix special key support for ASUS I2C keyboards

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

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 <matjaz.hegedic@gmail.com>
> ---
>  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 <usk.fujimaki@gmail.com>
> + *  Copyright (c) 2017 Matjaz Hegedic <matjaz.hegedic@gmail.com>
>   *
>   *  This module based on hid-ortek by
>   *  Copyright (c) 2010 Johnathon Harris <jmharris@gmail.com>
> @@ -36,8 +38,11 @@ MODULE_AUTHOR("Yusuke Fujimaki <usk.fujimaki@gmail.com>");
>  MODULE_AUTHOR("Brendan McGrath <redmcg@redmandi.dyndns.org>");
>  MODULE_AUTHOR("Victor Vlasenko <victor.vlasenko@sysgears.com>");
>  MODULE_AUTHOR("Frederik Wenigwieser <frederik.wenigwieser@gmail.com>");
> +MODULE_AUTHOR("Matjaz Hegedic <matjaz.hegedic@gmail.com>");
>  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
>