Re: [PATCH V5] HID: asus: add support for ASUS N-Key keyboard

From: Jiri Kosina <jikos@kernel.org>
To: Luke Jones <luke@ljones.dev>
Cc: linux-input@vger.kernel.org, benjamin.tissoires@redhat.com,
	rydberg@bitmath.org, Dmitry Torokhov <dmitry.torokhov@gmail.com>
Subject: Re: [PATCH V5] HID: asus: add support for ASUS N-Key keyboard
Date: Tue, 18 Aug 2020 20:37:38 +0200 (CEST)	[thread overview]
Message-ID: <nycvar.YFH.7.76.2008182034140.27422@cbobk.fhfr.pm> (raw)
In-Reply-To: <20200818075916.52267-1-luke@ljones.dev>

On Tue, 18 Aug 2020, Luke Jones wrote:

> From: Luke D Jones <luke@ljones.dev>
> 
> Enable missing functionality of the keyboard found in many ASUS
> Zephyrus laptops: Fn key combos and hotkeys, keyboard backlight
> brightness control, and notify asus-wmi to toggle "fan-mode".
> Two input event codes are added for keyboard LED mode switching
> prev/next.
> 
> The keyboard has many of the same key outputs as the existing G752
> keyboard including a few extras, and varies a little between laptop
> models.
> 
> Additionally the keyboard requires the LED interface to be
> intitialised before such things as keyboard backlight control work.
> 
> Misc changes in scope: update some hardcoded comparisons to use an
> available define, change "Mic Toggle" to use a keycode that works.
> 
> Signed-off-by: Luke D Jones <luke@ljones.dev>
> ---
>  drivers/hid/hid-asus.c                     | 165 ++++++++++++++++++---
>  drivers/hid/hid-ids.h                      |   1 +
>  include/linux/platform_data/x86/asus-wmi.h |   2 +
>  include/uapi/linux/input-event-codes.h     |   7 +

CCing Dmitry for the KEY_KBDILLUM_MODE_PREV / KEY_KBDILLUM_MODE_NEXT 
additions. Dmitry, OK by you please?

[ ... snip ... ]

> -	ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, dmabuf,
> +	// The report ID should be set from the incoming buffer due to LED and key
> +	// interfaces having different pages

Please use the kernel comment style (/* ... */) here.

[ ... snip ... ]
> @@ -751,14 +841,14 @@ static int asus_input_mapping(struct hid_device *hdev,
>  	     usage->hid == (HID_UP_GENDEVCTRLS | 0x0026)))
>  		return -1;
> 
> -	/* ASUS-specific keyboard hotkeys */
> -	if ((usage->hid & HID_USAGE_PAGE) == 0xff310000) {
> +	/* ASUS-specific keyboard hotkeys and led backlight */
> +	if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ASUSVENDOR) {
>  		switch (usage->hid & HID_USAGE) {
>  		case 0x10: asus_map_key_clear(KEY_BRIGHTNESSDOWN);	break;
>  		case 0x20: asus_map_key_clear(KEY_BRIGHTNESSUP);		break;
>  		case 0x35: asus_map_key_clear(KEY_DISPLAY_OFF);		break;
>  		case 0x6c: asus_map_key_clear(KEY_SLEEP);		break;
> -		case 0x7c: asus_map_key_clear(KEY_MICMUTE);		break;
> +		case 0x7c: asus_map_key_clear(KEY_F20);		break;

This change doesn't seem to be mentioned in the changelog; why is it OK in 
general case for other devices sharing this codepath?

Otherwise the patch looks good to me. Thanks,

-- 
Jiri Kosina
SUSE Labs