Linux Input Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH V8] HID: ASUS: Add support for ASUS N-Key keyboard
@ 2020-10-13  7:35 Luke D Jones
  2020-10-13  7:37 ` Luke Jones
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Luke D Jones @ 2020-10-13  7:35 UTC (permalink / raw)
  To: linux-input; +Cc: hdegoede, jikos, andy, benjamin.tissoires, Luke D Jones

The ASUS N-Key keyboard uses the productId of 0x1866 and is used in
almost all modern ASUS gaming laptops with slight changes to the
firmware. This patch enables: Fn+key hotkeys, keyboard backlight
brightness control, and notify asus-wmi to toggle "fan-mode".

The keyboard has many of the same key outputs as the existing ASUS
keyboard including a few extras, and varies a little between laptop
models.

Additionally this 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.

Signed-off-by: Luke D Jones <luke@ljones.dev>
---
 drivers/hid/hid-asus.c                     | 148 ++++++++++++++++++---
 drivers/hid/hid-ids.h                      |   1 +
 include/linux/platform_data/x86/asus-wmi.h |   1 +
 3 files changed, 135 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index c183caf89d49..a2f5469da0e6 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -26,6 +26,8 @@
 #include <linux/dmi.h>
 #include <linux/hid.h>
 #include <linux/module.h>
+
+#include <linux/acpi.h>
 #include <linux/platform_data/x86/asus-wmi.h>
 #include <linux/input/mt.h>
 #include <linux/usb.h> /* For to_usb_interface for T100 touchpad intf check */
@@ -48,6 +50,8 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 #define INPUT_REPORT_ID 0x5d
 #define FEATURE_KBD_REPORT_ID 0x5a
 #define FEATURE_KBD_REPORT_SIZE 16
+#define FEATURE_KBD_LED_REPORT_ID1 0x5d
+#define FEATURE_KBD_LED_REPORT_ID2 0x5e

 #define SUPPORT_KBD_BACKLIGHT BIT(0)

@@ -80,6 +84,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 #define QUIRK_T101HA_DOCK		BIT(9)
 #define QUIRK_T90CHI			BIT(10)
 #define QUIRK_MEDION_E1239T		BIT(11)
+#define QUIRK_ROG_NKEY_KEYBOARD		BIT(12)

 #define I2C_KEYBOARD_QUIRKS			(QUIRK_FIX_NOTEBOOK_REPORT | \
 						 QUIRK_NO_INIT_REPORTS | \
@@ -305,10 +310,33 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
 	return 0;
 }

+/*
+ * This enables triggering events in asus-wmi
+ */
+static int asus_wmi_send_event(struct asus_drvdata *drvdat, u8 code)
+{
+	int err;
+	u32 retval;
+
+	err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
+		ASUS_WMI_DEVID_NOTIF, code, &retval);
+	if (err) {
+		pr_warn("Failed to notify asus-wmi: %d\n", err);
+		return err;
+	}
+
+	if (retval != 0) {
+		pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);
+		return -EIO;
+	}
+
+	return 0;
+}
+
 static int asus_event(struct hid_device *hdev, struct hid_field *field,
 		      struct hid_usage *usage, __s32 value)
 {
-	if ((usage->hid & HID_USAGE_PAGE) == 0xff310000 &&
+	if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ASUSVENDOR &&
 	    (usage->hid & HID_USAGE) != 0x00 &&
 	    (usage->hid & HID_USAGE) != 0xff && !usage->type) {
 		hid_warn(hdev, "Unmapped Asus vendor usagepage code 0x%02x\n",
@@ -321,6 +349,7 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
 static int asus_raw_event(struct hid_device *hdev,
 		struct hid_report *report, u8 *data, int size)
 {
+	int ret;
 	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);

 	if (drvdata->battery && data[0] == BATTERY_REPORT_ID)
@@ -332,6 +361,35 @@ static int asus_raw_event(struct hid_device *hdev,
 	if (drvdata->quirks & QUIRK_MEDION_E1239T)
 		return asus_e1239t_event(drvdata, data, size);

+	if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
+		/*
+		 * Skip these report ID, the device emits a continuous stream associated
+		 * with the AURA mode it is in which looks like an 'echo'
+		*/
+		if (report->id == FEATURE_KBD_LED_REPORT_ID1 ||
+				report->id == FEATURE_KBD_LED_REPORT_ID2) {
+			return -1;
+		/* Additional report filtering */
+		} else if (report->id == FEATURE_KBD_REPORT_ID) {
+			/* Fn+F5 "fan" symbol, trigger WMI event to toggle next mode */
+			if (data[1] == 0xae) {
+				ret = asus_wmi_send_event(drvdata, 0xae);
+				if (ret < 0) {
+					hid_warn(hdev, "Asus failed to trigger fan control event");
+				}
+				return -1;
+			/*
+			 * G14 and G15 send these codes on some keypresses with no
+			 * discernable reason for doing so. We'll filter them out to avoid
+			 * unmapped warning messages later
+			*/
+			} else if (data[1] == 0xea || data[1] == 0xec || data[1] == 0x02 ||
+					data[1] == 0x8a || data[1] == 0x9e) {
+				return -1;
+			}
+		}
+	}
+
 	return 0;
 }

@@ -344,7 +402,11 @@ static int asus_kbd_set_report(struct hid_device *hdev, u8 *buf, size_t buf_size
 	if (!dmabuf)
 		return -ENOMEM;

-	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
+	*/
+	ret = hid_hw_raw_request(hdev, buf[0], dmabuf,
 				 buf_size, HID_FEATURE_REPORT,
 				 HID_REQ_SET_REPORT);
 	kfree(dmabuf);
@@ -397,6 +459,44 @@ static int asus_kbd_get_functions(struct hid_device *hdev,
 	return ret;
 }

+static int rog_nkey_led_init(struct hid_device *hdev)
+{
+	u8 buf_init_start[] = { FEATURE_KBD_LED_REPORT_ID1, 0xB9 };
+	u8 buf_init2[] = { FEATURE_KBD_LED_REPORT_ID1, 0x41, 0x53, 0x55, 0x53, 0x20,
+				0x54, 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
+	u8 buf_init3[] = { FEATURE_KBD_LED_REPORT_ID1,
+						0x05, 0x20, 0x31, 0x00, 0x08 };
+	int ret;
+
+	hid_info(hdev, "Asus initialise N-KEY Device");
+	/* The first message is an init start */
+	ret = asus_kbd_set_report(hdev, buf_init_start, sizeof(buf_init_start));
+	if (ret < 0)
+		hid_err(hdev, "Asus failed to send init start command: %d\n", ret);
+	/* Followed by a string */
+	ret = asus_kbd_set_report(hdev, buf_init2, sizeof(buf_init2));
+	if (ret < 0)
+		hid_err(hdev, "Asus failed to send init command 1.0: %d\n", ret);
+	/* Followed by a string */
+	ret = asus_kbd_set_report(hdev, buf_init3, sizeof(buf_init3));
+	if (ret < 0)
+		hid_err(hdev, "Asus failed to send init command 1.1: %d\n", ret);
+
+	/* begin second report ID with same data */
+	buf_init2[0] = FEATURE_KBD_LED_REPORT_ID2;
+	buf_init3[0] = FEATURE_KBD_LED_REPORT_ID2;
+
+	ret = asus_kbd_set_report(hdev, buf_init2, sizeof(buf_init2));
+	if (ret < 0)
+		hid_err(hdev, "Asus failed to send init command 2.0: %d\n", ret);
+
+	ret = asus_kbd_set_report(hdev, buf_init3, sizeof(buf_init3));
+	if (ret < 0)
+		hid_err(hdev, "Asus failed to send init command 2.1: %d\n", ret);
+
+	return ret;
+}
+
 static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
 				   enum led_brightness brightness)
 {
@@ -460,19 +560,25 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
 	unsigned char kbd_func;
 	int ret;

-	/* Initialize keyboard */
-	ret = asus_kbd_init(hdev);
-	if (ret < 0)
-		return ret;
+	if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
+		ret = rog_nkey_led_init(hdev);
+		if (ret < 0)
+			return ret;
+	} else {
+		/* Initialize keyboard */
+		ret = asus_kbd_init(hdev);
+		if (ret < 0)
+			return ret;

-	/* Get keyboard functions */
-	ret = asus_kbd_get_functions(hdev, &kbd_func);
-	if (ret < 0)
-		return ret;
+		/* Get keyboard functions */
+		ret = asus_kbd_get_functions(hdev, &kbd_func);
+		if (ret < 0)
+			return ret;

-	/* Check for backlight support */
-	if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
-		return -ENODEV;
+		/* Check for backlight support */
+		if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
+			return -ENODEV;
+	}

 	drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
 					      sizeof(struct asus_kbd_leds),
@@ -751,8 +857,8 @@ 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;
@@ -780,6 +886,15 @@ static int asus_input_mapping(struct hid_device *hdev,
 		/* Fn+F5 "fan" symbol on FX503VD */
 		case 0x99: asus_map_key_clear(KEY_PROG4);		break;

+		/* Fn+Ret "Calc" symbol on device 0x1866, N-KEY Device */
+		case 0x92: asus_map_key_clear(KEY_CALC);		break;
+
+		/* Fn+Left Aura mode previous */
+		case 0xb2: asus_map_key_clear(KEY_PROG2);		break;
+
+		/* Fn+Right Aura mode next */
+		case 0xb3: asus_map_key_clear(KEY_PROG3);		break;
+
 		default:
 			/* ASUS lazily declares 256 usages, ignore the rest,
 			 * as some make the keyboard appear as a pointer device. */
@@ -1126,6 +1241,9 @@ static const struct hid_device_id asus_devices[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
 		USB_DEVICE_ID_ASUSTEK_FX503VD_KEYBOARD),
 	  QUIRK_USE_KBD_BACKLIGHT },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
+	    USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
+	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
 		USB_DEVICE_ID_ASUSTEK_T100TA_KEYBOARD),
 	  QUIRK_T100_KEYBOARD | QUIRK_NO_CONSUMER_USAGES },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 6f370e020feb..c9f930ddcfd7 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -190,6 +190,7 @@
 #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD1 0x1854
 #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD2 0x1837
 #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD3 0x1822
+#define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD	0x1866
 #define USB_DEVICE_ID_ASUSTEK_FX503VD_KEYBOARD	0x1869

 #define USB_VENDOR_ID_ATEN		0x0557
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 897b8332a39f..ddf72cfe721a 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -69,6 +69,7 @@
 /* Input */
 #define ASUS_WMI_DEVID_TOUCHPAD		0x00100011
 #define ASUS_WMI_DEVID_TOUCHPAD_LED	0x00100012
+#define ASUS_WMI_DEVID_NOTIF		0x00100021 /* Emit keyboard event */
 #define ASUS_WMI_DEVID_FNLOCK		0x00100023

 /* Fan, Thermal */
--
2.28.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V8] HID: ASUS: Add support for ASUS N-Key keyboard
  2020-10-13  7:35 [PATCH V8] HID: ASUS: Add support for ASUS N-Key keyboard Luke D Jones
@ 2020-10-13  7:37 ` Luke Jones
  2020-10-15 11:11 ` Andy Shevchenko
  2020-10-16 10:51 ` Hans de Goede
  2 siblings, 0 replies; 10+ messages in thread
From: Luke Jones @ 2020-10-13  7:37 UTC (permalink / raw)
  To: linux-input; +Cc: hdegoede, jikos, andy, benjamin.tissoires

Very sorry all, I hadn't edited the commit message in the V7 patch.
This version contains correct commit message and all previous
requested fixes or changes.

Kind regards,
Luke.

On Tue, Oct 13, 2020 at 20:35, Luke D Jones <luke@ljones.dev> wrote:
> The ASUS N-Key keyboard uses the productId of 0x1866 and is used in
> almost all modern ASUS gaming laptops with slight changes to the
> firmware. This patch enables: Fn+key hotkeys, keyboard backlight
> brightness control, and notify asus-wmi to toggle "fan-mode".
> 
> The keyboard has many of the same key outputs as the existing ASUS
> keyboard including a few extras, and varies a little between laptop
> models.
> 
> Additionally this 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.
> 
> Signed-off-by: Luke D Jones <luke@ljones.dev>
> ---
>  drivers/hid/hid-asus.c                     | 148 
> ++++++++++++++++++---
>  drivers/hid/hid-ids.h                      |   1 +
>  include/linux/platform_data/x86/asus-wmi.h |   1 +
>  3 files changed, 135 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index c183caf89d49..a2f5469da0e6 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -26,6 +26,8 @@
>  #include <linux/dmi.h>
>  #include <linux/hid.h>
>  #include <linux/module.h>
> +
> +#include <linux/acpi.h>
>  #include <linux/platform_data/x86/asus-wmi.h>
>  #include <linux/input/mt.h>
>  #include <linux/usb.h> /* For to_usb_interface for T100 touchpad 
> intf check */
> @@ -48,6 +50,8 @@ MODULE_DESCRIPTION("Asus HID Keyboard and 
> TouchPad");
>  #define INPUT_REPORT_ID 0x5d
>  #define FEATURE_KBD_REPORT_ID 0x5a
>  #define FEATURE_KBD_REPORT_SIZE 16
> +#define FEATURE_KBD_LED_REPORT_ID1 0x5d
> +#define FEATURE_KBD_LED_REPORT_ID2 0x5e
> 
>  #define SUPPORT_KBD_BACKLIGHT BIT(0)
> 
> @@ -80,6 +84,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and 
> TouchPad");
>  #define QUIRK_T101HA_DOCK		BIT(9)
>  #define QUIRK_T90CHI			BIT(10)
>  #define QUIRK_MEDION_E1239T		BIT(11)
> +#define QUIRK_ROG_NKEY_KEYBOARD		BIT(12)
> 
>  #define I2C_KEYBOARD_QUIRKS			(QUIRK_FIX_NOTEBOOK_REPORT | \
>  						 QUIRK_NO_INIT_REPORTS | \
> @@ -305,10 +310,33 @@ static int asus_e1239t_event(struct 
> asus_drvdata *drvdat, u8 *data, int size)
>  	return 0;
>  }
> 
> +/*
> + * This enables triggering events in asus-wmi
> + */
> +static int asus_wmi_send_event(struct asus_drvdata *drvdat, u8 code)
> +{
> +	int err;
> +	u32 retval;
> +
> +	err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
> +		ASUS_WMI_DEVID_NOTIF, code, &retval);
> +	if (err) {
> +		pr_warn("Failed to notify asus-wmi: %d\n", err);
> +		return err;
> +	}
> +
> +	if (retval != 0) {
> +		pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
>  static int asus_event(struct hid_device *hdev, struct hid_field 
> *field,
>  		      struct hid_usage *usage, __s32 value)
>  {
> -	if ((usage->hid & HID_USAGE_PAGE) == 0xff310000 &&
> +	if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ASUSVENDOR &&
>  	    (usage->hid & HID_USAGE) != 0x00 &&
>  	    (usage->hid & HID_USAGE) != 0xff && !usage->type) {
>  		hid_warn(hdev, "Unmapped Asus vendor usagepage code 0x%02x\n",
> @@ -321,6 +349,7 @@ static int asus_event(struct hid_device *hdev, 
> struct hid_field *field,
>  static int asus_raw_event(struct hid_device *hdev,
>  		struct hid_report *report, u8 *data, int size)
>  {
> +	int ret;
>  	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> 
>  	if (drvdata->battery && data[0] == BATTERY_REPORT_ID)
> @@ -332,6 +361,35 @@ static int asus_raw_event(struct hid_device 
> *hdev,
>  	if (drvdata->quirks & QUIRK_MEDION_E1239T)
>  		return asus_e1239t_event(drvdata, data, size);
> 
> +	if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> +		/*
> +		 * Skip these report ID, the device emits a continuous stream 
> associated
> +		 * with the AURA mode it is in which looks like an 'echo'
> +		*/
> +		if (report->id == FEATURE_KBD_LED_REPORT_ID1 ||
> +				report->id == FEATURE_KBD_LED_REPORT_ID2) {
> +			return -1;
> +		/* Additional report filtering */
> +		} else if (report->id == FEATURE_KBD_REPORT_ID) {
> +			/* Fn+F5 "fan" symbol, trigger WMI event to toggle next mode */
> +			if (data[1] == 0xae) {
> +				ret = asus_wmi_send_event(drvdata, 0xae);
> +				if (ret < 0) {
> +					hid_warn(hdev, "Asus failed to trigger fan control event");
> +				}
> +				return -1;
> +			/*
> +			 * G14 and G15 send these codes on some keypresses with no
> +			 * discernable reason for doing so. We'll filter them out to avoid
> +			 * unmapped warning messages later
> +			*/
> +			} else if (data[1] == 0xea || data[1] == 0xec || data[1] == 0x02 
> ||
> +					data[1] == 0x8a || data[1] == 0x9e) {
> +				return -1;
> +			}
> +		}
> +	}
> +
>  	return 0;
>  }
> 
> @@ -344,7 +402,11 @@ static int asus_kbd_set_report(struct hid_device 
> *hdev, u8 *buf, size_t buf_size
>  	if (!dmabuf)
>  		return -ENOMEM;
> 
> -	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
> +	*/
> +	ret = hid_hw_raw_request(hdev, buf[0], dmabuf,
>  				 buf_size, HID_FEATURE_REPORT,
>  				 HID_REQ_SET_REPORT);
>  	kfree(dmabuf);
> @@ -397,6 +459,44 @@ static int asus_kbd_get_functions(struct 
> hid_device *hdev,
>  	return ret;
>  }
> 
> +static int rog_nkey_led_init(struct hid_device *hdev)
> +{
> +	u8 buf_init_start[] = { FEATURE_KBD_LED_REPORT_ID1, 0xB9 };
> +	u8 buf_init2[] = { FEATURE_KBD_LED_REPORT_ID1, 0x41, 0x53, 0x55, 
> 0x53, 0x20,
> +				0x54, 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
> +	u8 buf_init3[] = { FEATURE_KBD_LED_REPORT_ID1,
> +						0x05, 0x20, 0x31, 0x00, 0x08 };
> +	int ret;
> +
> +	hid_info(hdev, "Asus initialise N-KEY Device");
> +	/* The first message is an init start */
> +	ret = asus_kbd_set_report(hdev, buf_init_start, 
> sizeof(buf_init_start));
> +	if (ret < 0)
> +		hid_err(hdev, "Asus failed to send init start command: %d\n", ret);
> +	/* Followed by a string */
> +	ret = asus_kbd_set_report(hdev, buf_init2, sizeof(buf_init2));
> +	if (ret < 0)
> +		hid_err(hdev, "Asus failed to send init command 1.0: %d\n", ret);
> +	/* Followed by a string */
> +	ret = asus_kbd_set_report(hdev, buf_init3, sizeof(buf_init3));
> +	if (ret < 0)
> +		hid_err(hdev, "Asus failed to send init command 1.1: %d\n", ret);
> +
> +	/* begin second report ID with same data */
> +	buf_init2[0] = FEATURE_KBD_LED_REPORT_ID2;
> +	buf_init3[0] = FEATURE_KBD_LED_REPORT_ID2;
> +
> +	ret = asus_kbd_set_report(hdev, buf_init2, sizeof(buf_init2));
> +	if (ret < 0)
> +		hid_err(hdev, "Asus failed to send init command 2.0: %d\n", ret);
> +
> +	ret = asus_kbd_set_report(hdev, buf_init3, sizeof(buf_init3));
> +	if (ret < 0)
> +		hid_err(hdev, "Asus failed to send init command 2.1: %d\n", ret);
> +
> +	return ret;
> +}
> +
>  static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
>  				   enum led_brightness brightness)
>  {
> @@ -460,19 +560,25 @@ static int asus_kbd_register_leds(struct 
> hid_device *hdev)
>  	unsigned char kbd_func;
>  	int ret;
> 
> -	/* Initialize keyboard */
> -	ret = asus_kbd_init(hdev);
> -	if (ret < 0)
> -		return ret;
> +	if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> +		ret = rog_nkey_led_init(hdev);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		/* Initialize keyboard */
> +		ret = asus_kbd_init(hdev);
> +		if (ret < 0)
> +			return ret;
> 
> -	/* Get keyboard functions */
> -	ret = asus_kbd_get_functions(hdev, &kbd_func);
> -	if (ret < 0)
> -		return ret;
> +		/* Get keyboard functions */
> +		ret = asus_kbd_get_functions(hdev, &kbd_func);
> +		if (ret < 0)
> +			return ret;
> 
> -	/* Check for backlight support */
> -	if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> -		return -ENODEV;
> +		/* Check for backlight support */
> +		if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> +			return -ENODEV;
> +	}
> 
>  	drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
>  					      sizeof(struct asus_kbd_leds),
> @@ -751,8 +857,8 @@ 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;
> @@ -780,6 +886,15 @@ static int asus_input_mapping(struct hid_device 
> *hdev,
>  		/* Fn+F5 "fan" symbol on FX503VD */
>  		case 0x99: asus_map_key_clear(KEY_PROG4);		break;
> 
> +		/* Fn+Ret "Calc" symbol on device 0x1866, N-KEY Device */
> +		case 0x92: asus_map_key_clear(KEY_CALC);		break;
> +
> +		/* Fn+Left Aura mode previous */
> +		case 0xb2: asus_map_key_clear(KEY_PROG2);		break;
> +
> +		/* Fn+Right Aura mode next */
> +		case 0xb3: asus_map_key_clear(KEY_PROG3);		break;
> +
>  		default:
>  			/* ASUS lazily declares 256 usages, ignore the rest,
>  			 * as some make the keyboard appear as a pointer device. */
> @@ -1126,6 +1241,9 @@ static const struct hid_device_id 
> asus_devices[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>  		USB_DEVICE_ID_ASUSTEK_FX503VD_KEYBOARD),
>  	  QUIRK_USE_KBD_BACKLIGHT },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> +	    USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
> +	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>  		USB_DEVICE_ID_ASUSTEK_T100TA_KEYBOARD),
>  	  QUIRK_T100_KEYBOARD | QUIRK_NO_CONSUMER_USAGES },
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 6f370e020feb..c9f930ddcfd7 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -190,6 +190,7 @@
>  #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD1 0x1854
>  #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD2 0x1837
>  #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD3 0x1822
> +#define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD	0x1866
>  #define USB_DEVICE_ID_ASUSTEK_FX503VD_KEYBOARD	0x1869
> 
>  #define USB_VENDOR_ID_ATEN		0x0557
> diff --git a/include/linux/platform_data/x86/asus-wmi.h 
> b/include/linux/platform_data/x86/asus-wmi.h
> index 897b8332a39f..ddf72cfe721a 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -69,6 +69,7 @@
>  /* Input */
>  #define ASUS_WMI_DEVID_TOUCHPAD		0x00100011
>  #define ASUS_WMI_DEVID_TOUCHPAD_LED	0x00100012
> +#define ASUS_WMI_DEVID_NOTIF		0x00100021 /* Emit keyboard event */
>  #define ASUS_WMI_DEVID_FNLOCK		0x00100023
> 
>  /* Fan, Thermal */
> --
> 2.28.0
> 



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V8] HID: ASUS: Add support for ASUS N-Key keyboard
  2020-10-13  7:35 [PATCH V8] HID: ASUS: Add support for ASUS N-Key keyboard Luke D Jones
  2020-10-13  7:37 ` Luke Jones
@ 2020-10-15 11:11 ` Andy Shevchenko
  2020-10-18 21:23   ` Luke Jones
  2020-10-18 21:36   ` Luke Jones
  2020-10-16 10:51 ` Hans de Goede
  2 siblings, 2 replies; 10+ messages in thread
From: Andy Shevchenko @ 2020-10-15 11:11 UTC (permalink / raw)
  To: Luke D Jones
  Cc: linux-input, Hans de Goede, Jiri Kosina, Andy Shevchenko,
	Benjamin Tissoires

On Tue, Oct 13, 2020 at 10:35 AM Luke D Jones <luke@ljones.dev> wrote:
>
> The ASUS N-Key keyboard uses the productId of 0x1866 and is used in
> almost all modern ASUS gaming laptops with slight changes to the
> firmware. This patch enables: Fn+key hotkeys, keyboard backlight
> brightness control, and notify asus-wmi to toggle "fan-mode".
>
> The keyboard has many of the same key outputs as the existing ASUS
> keyboard including a few extras, and varies a little between laptop
> models.
>
> Additionally this keyboard requires the LED interface to be
> intitialised before such things as keyboard backlight control work.

initialised

> Misc changes in scope: update some hardcoded comparisons to use an
> available define.
...

> @@ -26,6 +26,8 @@
>  #include <linux/dmi.h>
>  #include <linux/hid.h>
>  #include <linux/module.h>
> +
> +#include <linux/acpi.h>

Blank line is not needed and perhaps put new inclusion somehow ordered
(yes, I see the order is broken, by maybe try your best).

>  #include <linux/platform_data/x86/asus-wmi.h>
>  #include <linux/input/mt.h>
>  #include <linux/usb.h> /* For to_usb_interface for T100 touchpad intf check */

...

> +/*
> + * This enables triggering events in asus-wmi
> + */
> +static int asus_wmi_send_event(struct asus_drvdata *drvdat, u8 code)
> +{
> +       int err;
> +       u32 retval;
> +
> +       err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
> +               ASUS_WMI_DEVID_NOTIF, code, &retval);
> +       if (err) {

> +               pr_warn("Failed to notify asus-wmi: %d\n", err);

dev_warn() ?

> +               return err;
> +       }

> +       if (retval != 0) {

if (retval)

> +               pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);

dev_warn()?

Now a question is why warn and not err level for both messages?
And maybe even hid_err() / hid_warn().

> +               return -EIO;
> +       }
> +
> +       return 0;
> +}

...

>  static int asus_event(struct hid_device *hdev, struct hid_field *field,
>                       struct hid_usage *usage, __s32 value)
>  {
> -       if ((usage->hid & HID_USAGE_PAGE) == 0xff310000 &&
> +       if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ASUSVENDOR &&

Seems like a separate change.

...

> +       int ret;

Inconsistent with the first part of the patch there you used err. So,
be consistent.

> +       if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> +               /*
> +                * Skip these report ID, the device emits a continuous stream associated
> +                * with the AURA mode it is in which looks like an 'echo'

+ period at the end.

> +               */
> +               if (report->id == FEATURE_KBD_LED_REPORT_ID1 ||
> +                               report->id == FEATURE_KBD_LED_REPORT_ID2) {

> +                       return -1;

is -1 a good return code? (this Q for all cases)

> +               /* Additional report filtering */
> +               } else if (report->id == FEATURE_KBD_REPORT_ID) {
> +                       /* Fn+F5 "fan" symbol, trigger WMI event to toggle next mode */
> +                       if (data[1] == 0xae) {
> +                               ret = asus_wmi_send_event(drvdata, 0xae);
> +                               if (ret < 0) {
> +                                       hid_warn(hdev, "Asus failed to trigger fan control event");
> +                               }

> +                               return -1;
> +                       /*
> +                        * G14 and G15 send these codes on some keypresses with no
> +                        * discernable reason for doing so. We'll filter them out to avoid
> +                        * unmapped warning messages later

Period at the end.
This is for all multi-line comments.

> +                       */
> +                       } else if (data[1] == 0xea || data[1] == 0xec || data[1] == 0x02 ||
> +                                       data[1] == 0x8a || data[1] == 0x9e) {
> +                               return -1;
> +                       }
> +               }
> +       }

...

> +static int rog_nkey_led_init(struct hid_device *hdev)
> +{
> +       u8 buf_init_start[] = { FEATURE_KBD_LED_REPORT_ID1, 0xB9 };
> +       u8 buf_init2[] = { FEATURE_KBD_LED_REPORT_ID1, 0x41, 0x53, 0x55, 0x53, 0x20,
> +                               0x54, 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
> +       u8 buf_init3[] = { FEATURE_KBD_LED_REPORT_ID1,
> +                                               0x05, 0x20, 0x31, 0x00, 0x08 };
> +       int ret;
> +
> +       hid_info(hdev, "Asus initialise N-KEY Device");
> +       /* The first message is an init start */
> +       ret = asus_kbd_set_report(hdev, buf_init_start, sizeof(buf_init_start));
> +       if (ret < 0)
> +               hid_err(hdev, "Asus failed to send init start command: %d\n", ret);
> +       /* Followed by a string */
> +       ret = asus_kbd_set_report(hdev, buf_init2, sizeof(buf_init2));
> +       if (ret < 0)
> +               hid_err(hdev, "Asus failed to send init command 1.0: %d\n", ret);
> +       /* Followed by a string */
> +       ret = asus_kbd_set_report(hdev, buf_init3, sizeof(buf_init3));
> +       if (ret < 0)
> +               hid_err(hdev, "Asus failed to send init command 1.1: %d\n", ret);

If you do hid_err() why are you not bailing out?
Mis-leveling of messages otherwise.

> +
> +       /* begin second report ID with same data */
> +       buf_init2[0] = FEATURE_KBD_LED_REPORT_ID2;
> +       buf_init3[0] = FEATURE_KBD_LED_REPORT_ID2;
> +
> +       ret = asus_kbd_set_report(hdev, buf_init2, sizeof(buf_init2));
> +       if (ret < 0)
> +               hid_err(hdev, "Asus failed to send init command 2.0: %d\n", ret);
> +
> +       ret = asus_kbd_set_report(hdev, buf_init3, sizeof(buf_init3));
> +       if (ret < 0)
> +               hid_err(hdev, "Asus failed to send init command 2.1: %d\n", ret);
> +
> +       return ret;
> +}

--
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V8] HID: ASUS: Add support for ASUS N-Key keyboard
  2020-10-13  7:35 [PATCH V8] HID: ASUS: Add support for ASUS N-Key keyboard Luke D Jones
  2020-10-13  7:37 ` Luke Jones
  2020-10-15 11:11 ` Andy Shevchenko
@ 2020-10-16 10:51 ` Hans de Goede
  2020-10-16 20:10   ` Luke Jones
  2 siblings, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2020-10-16 10:51 UTC (permalink / raw)
  To: Luke D Jones, linux-input; +Cc: jikos, andy, benjamin.tissoires

Hi,

First 2 general remarks:

1. Please address the review comments from Andy's review
   Note where Andy said to use dev_warn, please use hid_warn
   for consistency with the other hid_foo logging calls
   which you already add.

2. Existing empty lines (context) in the diff should start
   with a single space (per the unified diff format) but
   it seems that your email client has decided to treat this
   as trailing whitespace and remove these spaces, which
   means that the patch will not apply properly.

   See: https://lore.kernel.org/linux-input/20201013073508.10476-1-luke@ljones.dev/raw
   and look for example at line 200 of that file, there
   should be a space there, but instead the line is empty.


On 10/13/20 9:35 AM, Luke D Jones wrote:
> The ASUS N-Key keyboard uses the productId of 0x1866 and is used in
> almost all modern ASUS gaming laptops with slight changes to the
> firmware. This patch enables: Fn+key hotkeys, keyboard backlight
> brightness control, and notify asus-wmi to toggle "fan-mode".
> 
> The keyboard has many of the same key outputs as the existing ASUS
> keyboard including a few extras, and varies a little between laptop
> models.
> 
> Additionally this 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.

Usually if you need to add a comment like this to a commit message
it is better to break the change out into a separate patch as
Andy pointed out.

> Signed-off-by: Luke D Jones <luke@ljones.dev>
>  drivers/hid/hid-asus.c                     | 148 ++++++++++++++++++---
Normally you would insert the following directly
after your S-o-b for non version 1 patches:

---
Changes in v8:
- Resend of v7 with fixed commit-message

Changes in v7:
- Changed foo
- Add bar
- Removed bla

Notice the changelog gets preceded by a line with just '---'
on it this "cut" line will cause git am to drop it when a
kernel maintainer applies the commit to his tree, the changelog
is mainly there to help reviewers.

Otherwise I have no comments on top of Andy's comment, so one
more revision I think and then this patch is ready to go
I think.

Regards,

Hans


>  drivers/hid/hid-ids.h                      |   1 +
>  include/linux/platform_data/x86/asus-wmi.h |   1 +
>  3 files changed, 135 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index c183caf89d49..a2f5469da0e6 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -26,6 +26,8 @@
>  #include <linux/dmi.h>
>  #include <linux/hid.h>
>  #include <linux/module.h>
> +
> +#include <linux/acpi.h>
>  #include <linux/platform_data/x86/asus-wmi.h>
>  #include <linux/input/mt.h>
>  #include <linux/usb.h> /* For to_usb_interface for T100 touchpad intf check */
> @@ -48,6 +50,8 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>  #define INPUT_REPORT_ID 0x5d
>  #define FEATURE_KBD_REPORT_ID 0x5a
>  #define FEATURE_KBD_REPORT_SIZE 16
> +#define FEATURE_KBD_LED_REPORT_ID1 0x5d
> +#define FEATURE_KBD_LED_REPORT_ID2 0x5e
> 
>  #define SUPPORT_KBD_BACKLIGHT BIT(0)
> 
> @@ -80,6 +84,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>  #define QUIRK_T101HA_DOCK		BIT(9)
>  #define QUIRK_T90CHI			BIT(10)
>  #define QUIRK_MEDION_E1239T		BIT(11)
> +#define QUIRK_ROG_NKEY_KEYBOARD		BIT(12)
> 
>  #define I2C_KEYBOARD_QUIRKS			(QUIRK_FIX_NOTEBOOK_REPORT | \
>  						 QUIRK_NO_INIT_REPORTS | \
> @@ -305,10 +310,33 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
>  	return 0;
>  }
> 
> +/*
> + * This enables triggering events in asus-wmi
> + */
> +static int asus_wmi_send_event(struct asus_drvdata *drvdat, u8 code)
> +{
> +	int err;
> +	u32 retval;
> +
> +	err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
> +		ASUS_WMI_DEVID_NOTIF, code, &retval);
> +	if (err) {
> +		pr_warn("Failed to notify asus-wmi: %d\n", err);
> +		return err;
> +	}
> +
> +	if (retval != 0) {
> +		pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
>  static int asus_event(struct hid_device *hdev, struct hid_field *field,
>  		      struct hid_usage *usage, __s32 value)
>  {
> -	if ((usage->hid & HID_USAGE_PAGE) == 0xff310000 &&
> +	if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ASUSVENDOR &&
>  	    (usage->hid & HID_USAGE) != 0x00 &&
>  	    (usage->hid & HID_USAGE) != 0xff && !usage->type) {
>  		hid_warn(hdev, "Unmapped Asus vendor usagepage code 0x%02x\n",
> @@ -321,6 +349,7 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
>  static int asus_raw_event(struct hid_device *hdev,
>  		struct hid_report *report, u8 *data, int size)
>  {
> +	int ret;
>  	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> 
>  	if (drvdata->battery && data[0] == BATTERY_REPORT_ID)
> @@ -332,6 +361,35 @@ static int asus_raw_event(struct hid_device *hdev,
>  	if (drvdata->quirks & QUIRK_MEDION_E1239T)
>  		return asus_e1239t_event(drvdata, data, size);
> 
> +	if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> +		/*
> +		 * Skip these report ID, the device emits a continuous stream associated
> +		 * with the AURA mode it is in which looks like an 'echo'
> +		*/
> +		if (report->id == FEATURE_KBD_LED_REPORT_ID1 ||
> +				report->id == FEATURE_KBD_LED_REPORT_ID2) {
> +			return -1;
> +		/* Additional report filtering */
> +		} else if (report->id == FEATURE_KBD_REPORT_ID) {
> +			/* Fn+F5 "fan" symbol, trigger WMI event to toggle next mode */
> +			if (data[1] == 0xae) {
> +				ret = asus_wmi_send_event(drvdata, 0xae);
> +				if (ret < 0) {
> +					hid_warn(hdev, "Asus failed to trigger fan control event");
> +				}
> +				return -1;
> +			/*
> +			 * G14 and G15 send these codes on some keypresses with no
> +			 * discernable reason for doing so. We'll filter them out to avoid
> +			 * unmapped warning messages later
> +			*/
> +			} else if (data[1] == 0xea || data[1] == 0xec || data[1] == 0x02 ||
> +					data[1] == 0x8a || data[1] == 0x9e) {
> +				return -1;
> +			}
> +		}
> +	}
> +
>  	return 0;
>  }
> 
> @@ -344,7 +402,11 @@ static int asus_kbd_set_report(struct hid_device *hdev, u8 *buf, size_t buf_size
>  	if (!dmabuf)
>  		return -ENOMEM;
> 
> -	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
> +	*/
> +	ret = hid_hw_raw_request(hdev, buf[0], dmabuf,
>  				 buf_size, HID_FEATURE_REPORT,
>  				 HID_REQ_SET_REPORT);
>  	kfree(dmabuf);
> @@ -397,6 +459,44 @@ static int asus_kbd_get_functions(struct hid_device *hdev,
>  	return ret;
>  }
> 
> +static int rog_nkey_led_init(struct hid_device *hdev)
> +{
> +	u8 buf_init_start[] = { FEATURE_KBD_LED_REPORT_ID1, 0xB9 };
> +	u8 buf_init2[] = { FEATURE_KBD_LED_REPORT_ID1, 0x41, 0x53, 0x55, 0x53, 0x20,
> +				0x54, 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
> +	u8 buf_init3[] = { FEATURE_KBD_LED_REPORT_ID1,
> +						0x05, 0x20, 0x31, 0x00, 0x08 };
> +	int ret;
> +
> +	hid_info(hdev, "Asus initialise N-KEY Device");
> +	/* The first message is an init start */
> +	ret = asus_kbd_set_report(hdev, buf_init_start, sizeof(buf_init_start));
> +	if (ret < 0)
> +		hid_err(hdev, "Asus failed to send init start command: %d\n", ret);
> +	/* Followed by a string */
> +	ret = asus_kbd_set_report(hdev, buf_init2, sizeof(buf_init2));
> +	if (ret < 0)
> +		hid_err(hdev, "Asus failed to send init command 1.0: %d\n", ret);
> +	/* Followed by a string */
> +	ret = asus_kbd_set_report(hdev, buf_init3, sizeof(buf_init3));
> +	if (ret < 0)
> +		hid_err(hdev, "Asus failed to send init command 1.1: %d\n", ret);
> +
> +	/* begin second report ID with same data */
> +	buf_init2[0] = FEATURE_KBD_LED_REPORT_ID2;
> +	buf_init3[0] = FEATURE_KBD_LED_REPORT_ID2;
> +
> +	ret = asus_kbd_set_report(hdev, buf_init2, sizeof(buf_init2));
> +	if (ret < 0)
> +		hid_err(hdev, "Asus failed to send init command 2.0: %d\n", ret);
> +
> +	ret = asus_kbd_set_report(hdev, buf_init3, sizeof(buf_init3));
> +	if (ret < 0)
> +		hid_err(hdev, "Asus failed to send init command 2.1: %d\n", ret);
> +
> +	return ret;
> +}
> +
>  static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
>  				   enum led_brightness brightness)
>  {
> @@ -460,19 +560,25 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>  	unsigned char kbd_func;
>  	int ret;
> 
> -	/* Initialize keyboard */
> -	ret = asus_kbd_init(hdev);
> -	if (ret < 0)
> -		return ret;
> +	if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> +		ret = rog_nkey_led_init(hdev);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		/* Initialize keyboard */
> +		ret = asus_kbd_init(hdev);
> +		if (ret < 0)
> +			return ret;
> 
> -	/* Get keyboard functions */
> -	ret = asus_kbd_get_functions(hdev, &kbd_func);
> -	if (ret < 0)
> -		return ret;
> +		/* Get keyboard functions */
> +		ret = asus_kbd_get_functions(hdev, &kbd_func);
> +		if (ret < 0)
> +			return ret;
> 
> -	/* Check for backlight support */
> -	if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> -		return -ENODEV;
> +		/* Check for backlight support */
> +		if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> +			return -ENODEV;
> +	}
> 
>  	drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
>  					      sizeof(struct asus_kbd_leds),
> @@ -751,8 +857,8 @@ 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;
> @@ -780,6 +886,15 @@ static int asus_input_mapping(struct hid_device *hdev,
>  		/* Fn+F5 "fan" symbol on FX503VD */
>  		case 0x99: asus_map_key_clear(KEY_PROG4);		break;
> 
> +		/* Fn+Ret "Calc" symbol on device 0x1866, N-KEY Device */
> +		case 0x92: asus_map_key_clear(KEY_CALC);		break;
> +
> +		/* Fn+Left Aura mode previous */
> +		case 0xb2: asus_map_key_clear(KEY_PROG2);		break;
> +
> +		/* Fn+Right Aura mode next */
> +		case 0xb3: asus_map_key_clear(KEY_PROG3);		break;
> +
>  		default:
>  			/* ASUS lazily declares 256 usages, ignore the rest,
>  			 * as some make the keyboard appear as a pointer device. */
> @@ -1126,6 +1241,9 @@ static const struct hid_device_id asus_devices[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>  		USB_DEVICE_ID_ASUSTEK_FX503VD_KEYBOARD),
>  	  QUIRK_USE_KBD_BACKLIGHT },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> +	    USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
> +	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>  		USB_DEVICE_ID_ASUSTEK_T100TA_KEYBOARD),
>  	  QUIRK_T100_KEYBOARD | QUIRK_NO_CONSUMER_USAGES },
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 6f370e020feb..c9f930ddcfd7 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -190,6 +190,7 @@
>  #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD1 0x1854
>  #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD2 0x1837
>  #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD3 0x1822
> +#define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD	0x1866
>  #define USB_DEVICE_ID_ASUSTEK_FX503VD_KEYBOARD	0x1869
> 
>  #define USB_VENDOR_ID_ATEN		0x0557
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 897b8332a39f..ddf72cfe721a 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -69,6 +69,7 @@
>  /* Input */
>  #define ASUS_WMI_DEVID_TOUCHPAD		0x00100011
>  #define ASUS_WMI_DEVID_TOUCHPAD_LED	0x00100012
> +#define ASUS_WMI_DEVID_NOTIF		0x00100021 /* Emit keyboard event */
>  #define ASUS_WMI_DEVID_FNLOCK		0x00100023
> 
>  /* Fan, Thermal */
> --
> 2.28.0
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V8] HID: ASUS: Add support for ASUS N-Key keyboard
  2020-10-16 10:51 ` Hans de Goede
@ 2020-10-16 20:10   ` Luke Jones
  2020-10-18  8:59     ` Hans de Goede
  0 siblings, 1 reply; 10+ messages in thread
From: Luke Jones @ 2020-10-16 20:10 UTC (permalink / raw)
  To: Hans de Goede; +Cc: linux-input, jikos, andy, benjamin.tissoires

Hi Andy, Hans.

Great, thanks for the feedback, time and patience. Seems
there are a lot of little things for me to learn :)

I'll submit a hopefully final revision in the next day or so
with that one particular change mentioned broken out to
separate patch.

Andy:
 > is -1 a good return code? (this Q for all cases)
Umm, I honestly don't know. Your guidance would be appreciated.
I mostly copied what seemed to be the general case in other
parts.


Cheers,
Luke.

On Fri, Oct 16, 2020 at 12:51, Hans de Goede <hdegoede@redhat.com> 
wrote:
> Hi,
> 
> First 2 general remarks:
> 
> 1. Please address the review comments from Andy's review
>    Note where Andy said to use dev_warn, please use hid_warn
>    for consistency with the other hid_foo logging calls
>    which you already add.
> 
> 2. Existing empty lines (context) in the diff should start
>    with a single space (per the unified diff format) but
>    it seems that your email client has decided to treat this
>    as trailing whitespace and remove these spaces, which
>    means that the patch will not apply properly.
> 
>    See: 
> https://lore.kernel.org/linux-input/20201013073508.10476-1-luke@ljones.dev/raw
>    and look for example at line 200 of that file, there
>    should be a space there, but instead the line is empty.
> 
> 
> On 10/13/20 9:35 AM, Luke D Jones wrote:
>>  The ASUS N-Key keyboard uses the productId of 0x1866 and is used in
>>  almost all modern ASUS gaming laptops with slight changes to the
>>  firmware. This patch enables: Fn+key hotkeys, keyboard backlight
>>  brightness control, and notify asus-wmi to toggle "fan-mode".
>> 
>>  The keyboard has many of the same key outputs as the existing ASUS
>>  keyboard including a few extras, and varies a little between laptop
>>  models.
>> 
>>  Additionally this 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.
> 
> Usually if you need to add a comment like this to a commit message
> it is better to break the change out into a separate patch as
> Andy pointed out.
> 
>>  Signed-off-by: Luke D Jones <luke@ljones.dev>
>>   drivers/hid/hid-asus.c                     | 148 
>> ++++++++++++++++++---
> Normally you would insert the following directly
> after your S-o-b for non version 1 patches:
> 
> ---
> Changes in v8:
> - Resend of v7 with fixed commit-message
> 
> Changes in v7:
> - Changed foo
> - Add bar
> - Removed bla
> 
> Notice the changelog gets preceded by a line with just '---'
> on it this "cut" line will cause git am to drop it when a
> kernel maintainer applies the commit to his tree, the changelog
> is mainly there to help reviewers.
> 
> Otherwise I have no comments on top of Andy's comment, so one
> more revision I think and then this patch is ready to go
> I think.
> 
> Regards,
> 
> Hans
> 
> 
>>   drivers/hid/hid-ids.h                      |   1 +
>>   include/linux/platform_data/x86/asus-wmi.h |   1 +
>>   3 files changed, 135 insertions(+), 15 deletions(-)
>> 
>>  diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>>  index c183caf89d49..a2f5469da0e6 100644
>>  --- a/drivers/hid/hid-asus.c
>>  +++ b/drivers/hid/hid-asus.c
>>  @@ -26,6 +26,8 @@
>>   #include <linux/dmi.h>
>>   #include <linux/hid.h>
>>   #include <linux/module.h>
>>  +
>>  +#include <linux/acpi.h>
>>   #include <linux/platform_data/x86/asus-wmi.h>
>>   #include <linux/input/mt.h>
>>   #include <linux/usb.h> /* For to_usb_interface for T100 touchpad 
>> intf check */
>>  @@ -48,6 +50,8 @@ MODULE_DESCRIPTION("Asus HID Keyboard and 
>> TouchPad");
>>   #define INPUT_REPORT_ID 0x5d
>>   #define FEATURE_KBD_REPORT_ID 0x5a
>>   #define FEATURE_KBD_REPORT_SIZE 16
>>  +#define FEATURE_KBD_LED_REPORT_ID1 0x5d
>>  +#define FEATURE_KBD_LED_REPORT_ID2 0x5e
>> 
>>   #define SUPPORT_KBD_BACKLIGHT BIT(0)
>> 
>>  @@ -80,6 +84,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and 
>> TouchPad");
>>   #define QUIRK_T101HA_DOCK		BIT(9)
>>   #define QUIRK_T90CHI			BIT(10)
>>   #define QUIRK_MEDION_E1239T		BIT(11)
>>  +#define QUIRK_ROG_NKEY_KEYBOARD		BIT(12)
>> 
>>   #define I2C_KEYBOARD_QUIRKS			(QUIRK_FIX_NOTEBOOK_REPORT | \
>>   						 QUIRK_NO_INIT_REPORTS | \
>>  @@ -305,10 +310,33 @@ static int asus_e1239t_event(struct 
>> asus_drvdata *drvdat, u8 *data, int size)
>>   	return 0;
>>   }
>> 
>>  +/*
>>  + * This enables triggering events in asus-wmi
>>  + */
>>  +static int asus_wmi_send_event(struct asus_drvdata *drvdat, u8 
>> code)
>>  +{
>>  +	int err;
>>  +	u32 retval;
>>  +
>>  +	err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
>>  +		ASUS_WMI_DEVID_NOTIF, code, &retval);
>>  +	if (err) {
>>  +		pr_warn("Failed to notify asus-wmi: %d\n", err);
>>  +		return err;
>>  +	}
>>  +
>>  +	if (retval != 0) {
>>  +		pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);
>>  +		return -EIO;
>>  +	}
>>  +
>>  +	return 0;
>>  +}
>>  +
>>   static int asus_event(struct hid_device *hdev, struct hid_field 
>> *field,
>>   		      struct hid_usage *usage, __s32 value)
>>   {
>>  -	if ((usage->hid & HID_USAGE_PAGE) == 0xff310000 &&
>>  +	if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ASUSVENDOR &&
>>   	    (usage->hid & HID_USAGE) != 0x00 &&
>>   	    (usage->hid & HID_USAGE) != 0xff && !usage->type) {
>>   		hid_warn(hdev, "Unmapped Asus vendor usagepage code 0x%02x\n",
>>  @@ -321,6 +349,7 @@ static int asus_event(struct hid_device *hdev, 
>> struct hid_field *field,
>>   static int asus_raw_event(struct hid_device *hdev,
>>   		struct hid_report *report, u8 *data, int size)
>>   {
>>  +	int ret;
>>   	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>> 
>>   	if (drvdata->battery && data[0] == BATTERY_REPORT_ID)
>>  @@ -332,6 +361,35 @@ static int asus_raw_event(struct hid_device 
>> *hdev,
>>   	if (drvdata->quirks & QUIRK_MEDION_E1239T)
>>   		return asus_e1239t_event(drvdata, data, size);
>> 
>>  +	if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
>>  +		/*
>>  +		 * Skip these report ID, the device emits a continuous stream 
>> associated
>>  +		 * with the AURA mode it is in which looks like an 'echo'
>>  +		*/
>>  +		if (report->id == FEATURE_KBD_LED_REPORT_ID1 ||
>>  +				report->id == FEATURE_KBD_LED_REPORT_ID2) {
>>  +			return -1;
>>  +		/* Additional report filtering */
>>  +		} else if (report->id == FEATURE_KBD_REPORT_ID) {
>>  +			/* Fn+F5 "fan" symbol, trigger WMI event to toggle next mode */
>>  +			if (data[1] == 0xae) {
>>  +				ret = asus_wmi_send_event(drvdata, 0xae);
>>  +				if (ret < 0) {
>>  +					hid_warn(hdev, "Asus failed to trigger fan control event");
>>  +				}
>>  +				return -1;
>>  +			/*
>>  +			 * G14 and G15 send these codes on some keypresses with no
>>  +			 * discernable reason for doing so. We'll filter them out to 
>> avoid
>>  +			 * unmapped warning messages later
>>  +			*/
>>  +			} else if (data[1] == 0xea || data[1] == 0xec || data[1] == 
>> 0x02 ||
>>  +					data[1] == 0x8a || data[1] == 0x9e) {
>>  +				return -1;
>>  +			}
>>  +		}
>>  +	}
>>  +
>>   	return 0;
>>   }
>> 
>>  @@ -344,7 +402,11 @@ static int asus_kbd_set_report(struct 
>> hid_device *hdev, u8 *buf, size_t buf_size
>>   	if (!dmabuf)
>>   		return -ENOMEM;
>> 
>>  -	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
>>  +	*/
>>  +	ret = hid_hw_raw_request(hdev, buf[0], dmabuf,
>>   				 buf_size, HID_FEATURE_REPORT,
>>   				 HID_REQ_SET_REPORT);
>>   	kfree(dmabuf);
>>  @@ -397,6 +459,44 @@ static int asus_kbd_get_functions(struct 
>> hid_device *hdev,
>>   	return ret;
>>   }
>> 
>>  +static int rog_nkey_led_init(struct hid_device *hdev)
>>  +{
>>  +	u8 buf_init_start[] = { FEATURE_KBD_LED_REPORT_ID1, 0xB9 };
>>  +	u8 buf_init2[] = { FEATURE_KBD_LED_REPORT_ID1, 0x41, 0x53, 0x55, 
>> 0x53, 0x20,
>>  +				0x54, 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
>>  +	u8 buf_init3[] = { FEATURE_KBD_LED_REPORT_ID1,
>>  +						0x05, 0x20, 0x31, 0x00, 0x08 };
>>  +	int ret;
>>  +
>>  +	hid_info(hdev, "Asus initialise N-KEY Device");
>>  +	/* The first message is an init start */
>>  +	ret = asus_kbd_set_report(hdev, buf_init_start, 
>> sizeof(buf_init_start));
>>  +	if (ret < 0)
>>  +		hid_err(hdev, "Asus failed to send init start command: %d\n", 
>> ret);
>>  +	/* Followed by a string */
>>  +	ret = asus_kbd_set_report(hdev, buf_init2, sizeof(buf_init2));
>>  +	if (ret < 0)
>>  +		hid_err(hdev, "Asus failed to send init command 1.0: %d\n", ret);
>>  +	/* Followed by a string */
>>  +	ret = asus_kbd_set_report(hdev, buf_init3, sizeof(buf_init3));
>>  +	if (ret < 0)
>>  +		hid_err(hdev, "Asus failed to send init command 1.1: %d\n", ret);
>>  +
>>  +	/* begin second report ID with same data */
>>  +	buf_init2[0] = FEATURE_KBD_LED_REPORT_ID2;
>>  +	buf_init3[0] = FEATURE_KBD_LED_REPORT_ID2;
>>  +
>>  +	ret = asus_kbd_set_report(hdev, buf_init2, sizeof(buf_init2));
>>  +	if (ret < 0)
>>  +		hid_err(hdev, "Asus failed to send init command 2.0: %d\n", ret);
>>  +
>>  +	ret = asus_kbd_set_report(hdev, buf_init3, sizeof(buf_init3));
>>  +	if (ret < 0)
>>  +		hid_err(hdev, "Asus failed to send init command 2.1: %d\n", ret);
>>  +
>>  +	return ret;
>>  +}
>>  +
>>   static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
>>   				   enum led_brightness brightness)
>>   {
>>  @@ -460,19 +560,25 @@ static int asus_kbd_register_leds(struct 
>> hid_device *hdev)
>>   	unsigned char kbd_func;
>>   	int ret;
>> 
>>  -	/* Initialize keyboard */
>>  -	ret = asus_kbd_init(hdev);
>>  -	if (ret < 0)
>>  -		return ret;
>>  +	if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
>>  +		ret = rog_nkey_led_init(hdev);
>>  +		if (ret < 0)
>>  +			return ret;
>>  +	} else {
>>  +		/* Initialize keyboard */
>>  +		ret = asus_kbd_init(hdev);
>>  +		if (ret < 0)
>>  +			return ret;
>> 
>>  -	/* Get keyboard functions */
>>  -	ret = asus_kbd_get_functions(hdev, &kbd_func);
>>  -	if (ret < 0)
>>  -		return ret;
>>  +		/* Get keyboard functions */
>>  +		ret = asus_kbd_get_functions(hdev, &kbd_func);
>>  +		if (ret < 0)
>>  +			return ret;
>> 
>>  -	/* Check for backlight support */
>>  -	if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>  -		return -ENODEV;
>>  +		/* Check for backlight support */
>>  +		if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>  +			return -ENODEV;
>>  +	}
>> 
>>   	drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
>>   					      sizeof(struct asus_kbd_leds),
>>  @@ -751,8 +857,8 @@ 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;
>>  @@ -780,6 +886,15 @@ static int asus_input_mapping(struct 
>> hid_device *hdev,
>>   		/* Fn+F5 "fan" symbol on FX503VD */
>>   		case 0x99: asus_map_key_clear(KEY_PROG4);		break;
>> 
>>  +		/* Fn+Ret "Calc" symbol on device 0x1866, N-KEY Device */
>>  +		case 0x92: asus_map_key_clear(KEY_CALC);		break;
>>  +
>>  +		/* Fn+Left Aura mode previous */
>>  +		case 0xb2: asus_map_key_clear(KEY_PROG2);		break;
>>  +
>>  +		/* Fn+Right Aura mode next */
>>  +		case 0xb3: asus_map_key_clear(KEY_PROG3);		break;
>>  +
>>   		default:
>>   			/* ASUS lazily declares 256 usages, ignore the rest,
>>   			 * as some make the keyboard appear as a pointer device. */
>>  @@ -1126,6 +1241,9 @@ static const struct hid_device_id 
>> asus_devices[] = {
>>   	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>   		USB_DEVICE_ID_ASUSTEK_FX503VD_KEYBOARD),
>>   	  QUIRK_USE_KBD_BACKLIGHT },
>>  +	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>  +	    USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
>>  +	  QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>   	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>   		USB_DEVICE_ID_ASUSTEK_T100TA_KEYBOARD),
>>   	  QUIRK_T100_KEYBOARD | QUIRK_NO_CONSUMER_USAGES },
>>  diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>>  index 6f370e020feb..c9f930ddcfd7 100644
>>  --- a/drivers/hid/hid-ids.h
>>  +++ b/drivers/hid/hid-ids.h
>>  @@ -190,6 +190,7 @@
>>   #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD1 0x1854
>>   #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD2 0x1837
>>   #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD3 0x1822
>>  +#define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD	0x1866
>>   #define USB_DEVICE_ID_ASUSTEK_FX503VD_KEYBOARD	0x1869
>> 
>>   #define USB_VENDOR_ID_ATEN		0x0557
>>  diff --git a/include/linux/platform_data/x86/asus-wmi.h 
>> b/include/linux/platform_data/x86/asus-wmi.h
>>  index 897b8332a39f..ddf72cfe721a 100644
>>  --- a/include/linux/platform_data/x86/asus-wmi.h
>>  +++ b/include/linux/platform_data/x86/asus-wmi.h
>>  @@ -69,6 +69,7 @@
>>   /* Input */
>>   #define ASUS_WMI_DEVID_TOUCHPAD		0x00100011
>>   #define ASUS_WMI_DEVID_TOUCHPAD_LED	0x00100012
>>  +#define ASUS_WMI_DEVID_NOTIF		0x00100021 /* Emit keyboard event */
>>   #define ASUS_WMI_DEVID_FNLOCK		0x00100023
>> 
>>   /* Fan, Thermal */
>>  --
>>  2.28.0
>> 
> 



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V8] HID: ASUS: Add support for ASUS N-Key keyboard
  2020-10-16 20:10   ` Luke Jones
@ 2020-10-18  8:59     ` Hans de Goede
  0 siblings, 0 replies; 10+ messages in thread
From: Hans de Goede @ 2020-10-18  8:59 UTC (permalink / raw)
  To: Luke Jones; +Cc: linux-input, jikos, andy, benjamin.tissoires

Hi,

On 10/16/20 10:10 PM, Luke Jones wrote:
> Hi Andy, Hans.
> 
> Great, thanks for the feedback, time and patience. Seems
> there are a lot of little things for me to learn :)
> 
> I'll submit a hopefully final revision in the next day or so
> with that one particular change mentioned broken out to
> separate patch.
> 
> Andy:
>> is -1 a good return code? (this Q for all cases)
> Umm, I honestly don't know. Your guidance would be appreciated.
> I mostly copied what seemed to be the general case in other
> parts.

Return codes should be in the form of -EFOO and the possible EFOO
values are defined in:

/usr/include/asm-generic/errno-base.h

And a set of more exotic ones in:

/usr/include/asm-generic/errno.h

But usually you want to stick to the set found in errno-base.h

So if you get an expected result from the hw, or an error
communicating from the hardware you would use:

	return -EIO;

If you are dealing with user(space) input (which is not applicable
here I think) and the input fails validation, you would return:

	return -EINVAL;

Those 2 are probably the most used ones in kernel drivers.

Regards,

Hans


> On Fri, Oct 16, 2020 at 12:51, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> First 2 general remarks:
>>
>> 1. Please address the review comments from Andy's review
>>    Note where Andy said to use dev_warn, please use hid_warn
>>    for consistency with the other hid_foo logging calls
>>    which you already add.
>>
>> 2. Existing empty lines (context) in the diff should start
>>    with a single space (per the unified diff format) but
>>    it seems that your email client has decided to treat this
>>    as trailing whitespace and remove these spaces, which
>>    means that the patch will not apply properly.
>>
>>    See: https://lore.kernel.org/linux-input/20201013073508.10476-1-luke@ljones.dev/raw
>>    and look for example at line 200 of that file, there
>>    should be a space there, but instead the line is empty.
>>
>>
>> On 10/13/20 9:35 AM, Luke D Jones wrote:
>>>  The ASUS N-Key keyboard uses the productId of 0x1866 and is used in
>>>  almost all modern ASUS gaming laptops with slight changes to the
>>>  firmware. This patch enables: Fn+key hotkeys, keyboard backlight
>>>  brightness control, and notify asus-wmi to toggle "fan-mode".
>>>
>>>  The keyboard has many of the same key outputs as the existing ASUS
>>>  keyboard including a few extras, and varies a little between laptop
>>>  models.
>>>
>>>  Additionally this 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.
>>
>> Usually if you need to add a comment like this to a commit message
>> it is better to break the change out into a separate patch as
>> Andy pointed out.
>>
>>>  Signed-off-by: Luke D Jones <luke@ljones.dev>
>>>   drivers/hid/hid-asus.c                     | 148 ++++++++++++++++++---
>> Normally you would insert the following directly
>> after your S-o-b for non version 1 patches:
>>
>> ---
>> Changes in v8:
>> - Resend of v7 with fixed commit-message
>>
>> Changes in v7:
>> - Changed foo
>> - Add bar
>> - Removed bla
>>
>> Notice the changelog gets preceded by a line with just '---'
>> on it this "cut" line will cause git am to drop it when a
>> kernel maintainer applies the commit to his tree, the changelog
>> is mainly there to help reviewers.
>>
>> Otherwise I have no comments on top of Andy's comment, so one
>> more revision I think and then this patch is ready to go
>> I think.
>>
>> Regards,
>>
>> Hans
>>
>>
>>>   drivers/hid/hid-ids.h                      |   1 +
>>>   include/linux/platform_data/x86/asus-wmi.h |   1 +
>>>   3 files changed, 135 insertions(+), 15 deletions(-)
>>>
>>>  diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>>>  index c183caf89d49..a2f5469da0e6 100644
>>>  --- a/drivers/hid/hid-asus.c
>>>  +++ b/drivers/hid/hid-asus.c
>>>  @@ -26,6 +26,8 @@
>>>   #include <linux/dmi.h>
>>>   #include <linux/hid.h>
>>>   #include <linux/module.h>
>>>  +
>>>  +#include <linux/acpi.h>
>>>   #include <linux/platform_data/x86/asus-wmi.h>
>>>   #include <linux/input/mt.h>
>>>   #include <linux/usb.h> /* For to_usb_interface for T100 touchpad intf check */
>>>  @@ -48,6 +50,8 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>>>   #define INPUT_REPORT_ID 0x5d
>>>   #define FEATURE_KBD_REPORT_ID 0x5a
>>>   #define FEATURE_KBD_REPORT_SIZE 16
>>>  +#define FEATURE_KBD_LED_REPORT_ID1 0x5d
>>>  +#define FEATURE_KBD_LED_REPORT_ID2 0x5e
>>>
>>>   #define SUPPORT_KBD_BACKLIGHT BIT(0)
>>>
>>>  @@ -80,6 +84,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>>>   #define QUIRK_T101HA_DOCK        BIT(9)
>>>   #define QUIRK_T90CHI            BIT(10)
>>>   #define QUIRK_MEDION_E1239T        BIT(11)
>>>  +#define QUIRK_ROG_NKEY_KEYBOARD        BIT(12)
>>>
>>>   #define I2C_KEYBOARD_QUIRKS            (QUIRK_FIX_NOTEBOOK_REPORT | \
>>>                            QUIRK_NO_INIT_REPORTS | \
>>>  @@ -305,10 +310,33 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
>>>       return 0;
>>>   }
>>>
>>>  +/*
>>>  + * This enables triggering events in asus-wmi
>>>  + */
>>>  +static int asus_wmi_send_event(struct asus_drvdata *drvdat, u8 code)
>>>  +{
>>>  +    int err;
>>>  +    u32 retval;
>>>  +
>>>  +    err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
>>>  +        ASUS_WMI_DEVID_NOTIF, code, &retval);
>>>  +    if (err) {
>>>  +        pr_warn("Failed to notify asus-wmi: %d\n", err);
>>>  +        return err;
>>>  +    }
>>>  +
>>>  +    if (retval != 0) {
>>>  +        pr_warn("Failed to notify asus-wmi (retval): 0x%x\n", retval);
>>>  +        return -EIO;
>>>  +    }
>>>  +
>>>  +    return 0;
>>>  +}
>>>  +
>>>   static int asus_event(struct hid_device *hdev, struct hid_field *field,
>>>                 struct hid_usage *usage, __s32 value)
>>>   {
>>>  -    if ((usage->hid & HID_USAGE_PAGE) == 0xff310000 &&
>>>  +    if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ASUSVENDOR &&
>>>           (usage->hid & HID_USAGE) != 0x00 &&
>>>           (usage->hid & HID_USAGE) != 0xff && !usage->type) {
>>>           hid_warn(hdev, "Unmapped Asus vendor usagepage code 0x%02x\n",
>>>  @@ -321,6 +349,7 @@ static int asus_event(struct hid_device *hdev, struct hid_field *field,
>>>   static int asus_raw_event(struct hid_device *hdev,
>>>           struct hid_report *report, u8 *data, int size)
>>>   {
>>>  +    int ret;
>>>       struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>>>
>>>       if (drvdata->battery && data[0] == BATTERY_REPORT_ID)
>>>  @@ -332,6 +361,35 @@ static int asus_raw_event(struct hid_device *hdev,
>>>       if (drvdata->quirks & QUIRK_MEDION_E1239T)
>>>           return asus_e1239t_event(drvdata, data, size);
>>>
>>>  +    if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
>>>  +        /*
>>>  +         * Skip these report ID, the device emits a continuous stream associated
>>>  +         * with the AURA mode it is in which looks like an 'echo'
>>>  +        */
>>>  +        if (report->id == FEATURE_KBD_LED_REPORT_ID1 ||
>>>  +                report->id == FEATURE_KBD_LED_REPORT_ID2) {
>>>  +            return -1;
>>>  +        /* Additional report filtering */
>>>  +        } else if (report->id == FEATURE_KBD_REPORT_ID) {
>>>  +            /* Fn+F5 "fan" symbol, trigger WMI event to toggle next mode */
>>>  +            if (data[1] == 0xae) {
>>>  +                ret = asus_wmi_send_event(drvdata, 0xae);
>>>  +                if (ret < 0) {
>>>  +                    hid_warn(hdev, "Asus failed to trigger fan control event");
>>>  +                }
>>>  +                return -1;
>>>  +            /*
>>>  +             * G14 and G15 send these codes on some keypresses with no
>>>  +             * discernable reason for doing so. We'll filter them out to avoid
>>>  +             * unmapped warning messages later
>>>  +            */
>>>  +            } else if (data[1] == 0xea || data[1] == 0xec || data[1] == 0x02 ||
>>>  +                    data[1] == 0x8a || data[1] == 0x9e) {
>>>  +                return -1;
>>>  +            }
>>>  +        }
>>>  +    }
>>>  +
>>>       return 0;
>>>   }
>>>
>>>  @@ -344,7 +402,11 @@ static int asus_kbd_set_report(struct hid_device *hdev, u8 *buf, size_t buf_size
>>>       if (!dmabuf)
>>>           return -ENOMEM;
>>>
>>>  -    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
>>>  +    */
>>>  +    ret = hid_hw_raw_request(hdev, buf[0], dmabuf,
>>>                    buf_size, HID_FEATURE_REPORT,
>>>                    HID_REQ_SET_REPORT);
>>>       kfree(dmabuf);
>>>  @@ -397,6 +459,44 @@ static int asus_kbd_get_functions(struct hid_device *hdev,
>>>       return ret;
>>>   }
>>>
>>>  +static int rog_nkey_led_init(struct hid_device *hdev)
>>>  +{
>>>  +    u8 buf_init_start[] = { FEATURE_KBD_LED_REPORT_ID1, 0xB9 };
>>>  +    u8 buf_init2[] = { FEATURE_KBD_LED_REPORT_ID1, 0x41, 0x53, 0x55, 0x53, 0x20,
>>>  +                0x54, 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
>>>  +    u8 buf_init3[] = { FEATURE_KBD_LED_REPORT_ID1,
>>>  +                        0x05, 0x20, 0x31, 0x00, 0x08 };
>>>  +    int ret;
>>>  +
>>>  +    hid_info(hdev, "Asus initialise N-KEY Device");
>>>  +    /* The first message is an init start */
>>>  +    ret = asus_kbd_set_report(hdev, buf_init_start, sizeof(buf_init_start));
>>>  +    if (ret < 0)
>>>  +        hid_err(hdev, "Asus failed to send init start command: %d\n", ret);
>>>  +    /* Followed by a string */
>>>  +    ret = asus_kbd_set_report(hdev, buf_init2, sizeof(buf_init2));
>>>  +    if (ret < 0)
>>>  +        hid_err(hdev, "Asus failed to send init command 1.0: %d\n", ret);
>>>  +    /* Followed by a string */
>>>  +    ret = asus_kbd_set_report(hdev, buf_init3, sizeof(buf_init3));
>>>  +    if (ret < 0)
>>>  +        hid_err(hdev, "Asus failed to send init command 1.1: %d\n", ret);
>>>  +
>>>  +    /* begin second report ID with same data */
>>>  +    buf_init2[0] = FEATURE_KBD_LED_REPORT_ID2;
>>>  +    buf_init3[0] = FEATURE_KBD_LED_REPORT_ID2;
>>>  +
>>>  +    ret = asus_kbd_set_report(hdev, buf_init2, sizeof(buf_init2));
>>>  +    if (ret < 0)
>>>  +        hid_err(hdev, "Asus failed to send init command 2.0: %d\n", ret);
>>>  +
>>>  +    ret = asus_kbd_set_report(hdev, buf_init3, sizeof(buf_init3));
>>>  +    if (ret < 0)
>>>  +        hid_err(hdev, "Asus failed to send init command 2.1: %d\n", ret);
>>>  +
>>>  +    return ret;
>>>  +}
>>>  +
>>>   static void asus_kbd_backlight_set(struct led_classdev *led_cdev,
>>>                      enum led_brightness brightness)
>>>   {
>>>  @@ -460,19 +560,25 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
>>>       unsigned char kbd_func;
>>>       int ret;
>>>
>>>  -    /* Initialize keyboard */
>>>  -    ret = asus_kbd_init(hdev);
>>>  -    if (ret < 0)
>>>  -        return ret;
>>>  +    if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
>>>  +        ret = rog_nkey_led_init(hdev);
>>>  +        if (ret < 0)
>>>  +            return ret;
>>>  +    } else {
>>>  +        /* Initialize keyboard */
>>>  +        ret = asus_kbd_init(hdev);
>>>  +        if (ret < 0)
>>>  +            return ret;
>>>
>>>  -    /* Get keyboard functions */
>>>  -    ret = asus_kbd_get_functions(hdev, &kbd_func);
>>>  -    if (ret < 0)
>>>  -        return ret;
>>>  +        /* Get keyboard functions */
>>>  +        ret = asus_kbd_get_functions(hdev, &kbd_func);
>>>  +        if (ret < 0)
>>>  +            return ret;
>>>
>>>  -    /* Check for backlight support */
>>>  -    if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>>  -        return -ENODEV;
>>>  +        /* Check for backlight support */
>>>  +        if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
>>>  +            return -ENODEV;
>>>  +    }
>>>
>>>       drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
>>>                             sizeof(struct asus_kbd_leds),
>>>  @@ -751,8 +857,8 @@ 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;
>>>  @@ -780,6 +886,15 @@ static int asus_input_mapping(struct hid_device *hdev,
>>>           /* Fn+F5 "fan" symbol on FX503VD */
>>>           case 0x99: asus_map_key_clear(KEY_PROG4);        break;
>>>
>>>  +        /* Fn+Ret "Calc" symbol on device 0x1866, N-KEY Device */
>>>  +        case 0x92: asus_map_key_clear(KEY_CALC);        break;
>>>  +
>>>  +        /* Fn+Left Aura mode previous */
>>>  +        case 0xb2: asus_map_key_clear(KEY_PROG2);        break;
>>>  +
>>>  +        /* Fn+Right Aura mode next */
>>>  +        case 0xb3: asus_map_key_clear(KEY_PROG3);        break;
>>>  +
>>>           default:
>>>               /* ASUS lazily declares 256 usages, ignore the rest,
>>>                * as some make the keyboard appear as a pointer device. */
>>>  @@ -1126,6 +1241,9 @@ static const struct hid_device_id asus_devices[] = {
>>>       { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>           USB_DEVICE_ID_ASUSTEK_FX503VD_KEYBOARD),
>>>         QUIRK_USE_KBD_BACKLIGHT },
>>>  +    { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>  +        USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD),
>>>  +      QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
>>>       { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
>>>           USB_DEVICE_ID_ASUSTEK_T100TA_KEYBOARD),
>>>         QUIRK_T100_KEYBOARD | QUIRK_NO_CONSUMER_USAGES },
>>>  diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>>>  index 6f370e020feb..c9f930ddcfd7 100644
>>>  --- a/drivers/hid/hid-ids.h
>>>  +++ b/drivers/hid/hid-ids.h
>>>  @@ -190,6 +190,7 @@
>>>   #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD1 0x1854
>>>   #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD2 0x1837
>>>   #define USB_DEVICE_ID_ASUSTEK_ROG_KEYBOARD3 0x1822
>>>  +#define USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD    0x1866
>>>   #define USB_DEVICE_ID_ASUSTEK_FX503VD_KEYBOARD    0x1869
>>>
>>>   #define USB_VENDOR_ID_ATEN        0x0557
>>>  diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
>>>  index 897b8332a39f..ddf72cfe721a 100644
>>>  --- a/include/linux/platform_data/x86/asus-wmi.h
>>>  +++ b/include/linux/platform_data/x86/asus-wmi.h
>>>  @@ -69,6 +69,7 @@
>>>   /* Input */
>>>   #define ASUS_WMI_DEVID_TOUCHPAD        0x00100011
>>>   #define ASUS_WMI_DEVID_TOUCHPAD_LED    0x00100012
>>>  +#define ASUS_WMI_DEVID_NOTIF        0x00100021 /* Emit keyboard event */
>>>   #define ASUS_WMI_DEVID_FNLOCK        0x00100023
>>>
>>>   /* Fan, Thermal */
>>>  --
>>>  2.28.0
>>>
>>
> 
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V8] HID: ASUS: Add support for ASUS N-Key keyboard
  2020-10-15 11:11 ` Andy Shevchenko
@ 2020-10-18 21:23   ` Luke Jones
  2020-10-18 21:36   ` Luke Jones
  1 sibling, 0 replies; 10+ messages in thread
From: Luke Jones @ 2020-10-18 21:23 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-input, Hans de Goede, Jiri Kosina, Andy Shevchenko,
	Benjamin Tissoires



On Thu, Oct 15, 2020 at 14:11, Andy Shevchenko 
<andy.shevchenko@gmail.com> wrote:
> On Tue, Oct 13, 2020 at 10:35 AM Luke D Jones <luke@ljones.dev> wrote:
>> 
>>  The ASUS N-Key keyboard uses the productId of 0x1866 and is used in
>>  almost all modern ASUS gaming laptops with slight changes to the
>>  firmware. This patch enables: Fn+key hotkeys, keyboard backlight
>>  brightness control, and notify asus-wmi to toggle "fan-mode".
>> 
>>  The keyboard has many of the same key outputs as the existing ASUS
>>  keyboard including a few extras, and varies a little between laptop
>>  models.
>> 
>>  Additionally this keyboard requires the LED interface to be
>>  intitialised before such things as keyboard backlight control work.
> 
> initialised

Done

> 
>>  Misc changes in scope: update some hardcoded comparisons to use an
>>  available define.
> ...
> 
>>  @@ -26,6 +26,8 @@
>>   #include <linux/dmi.h>
>>   #include <linux/hid.h>
>>   #include <linux/module.h>
>>  +
>>  +#include <linux/acpi.h>
> 
> Blank line is not needed and perhaps put new inclusion somehow ordered
> (yes, I see the order is broken, by maybe try your best).

Done

> 
>>   #include <linux/platform_data/x86/asus-wmi.h>
>>   #include <linux/input/mt.h>
>>   #include <linux/usb.h> /* For to_usb_interface for T100 touchpad 
>> intf check */
> 
> ...
> 
>>  +/*
>>  + * This enables triggering events in asus-wmi
>>  + */
>>  +static int asus_wmi_send_event(struct asus_drvdata *drvdat, u8 
>> code)
>>  +{
>>  +       int err;
>>  +       u32 retval;
>>  +
>>  +       err = asus_wmi_evaluate_method(ASUS_WMI_METHODID_DEVS,
>>  +               ASUS_WMI_DEVID_NOTIF, code, &retval);
>>  +       if (err) {
> 
>>  +               pr_warn("Failed to notify asus-wmi: %d\n", err);
> 
> dev_warn() ?

Done (changed to dev_warn. Sorry, I wasn't sure what to use)

> 
>>  +               return err;
>>  +       }
> 
>>  +       if (retval != 0) {
> 
> if (retval)
> 
>>  +               pr_warn("Failed to notify asus-wmi (retval): 
>> 0x%x\n", retval);
> 
> dev_warn()?

Done

> 
> Now a question is why warn and not err level for both messages?
> And maybe even hid_err() / hid_warn().
> 

I guess because it's not a show-stopper? The whole device still works
perfectly fine regardless of the wmi/acpi method call result.

Which I guess raises the question that since this is actually an ACPI 
call
is there another warn function more suitable?

>>  +               return -EIO;
>>  +       }
>>  +
>>  +       return 0;
>>  +}
> 
> ...
> 
>>   static int asus_event(struct hid_device *hdev, struct hid_field 
>> *field,
>>                        struct hid_usage *usage, __s32 value)
>>   {
>>  -       if ((usage->hid & HID_USAGE_PAGE) == 0xff310000 &&
>>  +       if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ASUSVENDOR &&
> 
> Seems like a separate change.
> 

Okay, I've reverted this, will submit a separate patch at a later date.

> ...
> 
>>  +       int ret;
> 
> Inconsistent with the first part of the patch there you used err. So,
> be consistent.

Done. A lot of this patch was made by looking at similar code :)

> 
>>  +       if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
>>  +               /*
>>  +                * Skip these report ID, the device emits a 
>> continuous stream associated
>>  +                * with the AURA mode it is in which looks like an 
>> 'echo'
> 
> + period at the end.
> 
>>  +               */
>>  +               if (report->id == FEATURE_KBD_LED_REPORT_ID1 ||
>>  +                               report->id == 
>> FEATURE_KBD_LED_REPORT_ID2) {
> 
>>  +                       return -1;
> 
> is -1 a good return code? (this Q for all cases)
> 
>>  +               /* Additional report filtering */
>>  +               } else if (report->id == FEATURE_KBD_REPORT_ID) {
>>  +                       /* Fn+F5 "fan" symbol, trigger WMI event to 
>> toggle next mode */
>>  +                       if (data[1] == 0xae) {
>>  +                               ret = asus_wmi_send_event(drvdata, 
>> 0xae);
>>  +                               if (ret < 0) {
>>  +                                       hid_warn(hdev, "Asus failed 
>> to trigger fan control event");
>>  +                               }
> 
>>  +                               return -1;
>>  +                       /*
>>  +                        * G14 and G15 send these codes on some 
>> keypresses with no
>>  +                        * discernable reason for doing so. We'll 
>> filter them out to avoid
>>  +                        * unmapped warning messages later
> 
> Period at the end.
> This is for all multi-line comments.

Done

> 
>>  +                       */
>>  +                       } else if (data[1] == 0xea || data[1] == 
>> 0xec || data[1] == 0x02 ||
>>  +                                       data[1] == 0x8a || data[1] 
>> == 0x9e) {
>>  +                               return -1;
>>  +                       }
>>  +               }
>>  +       }
> 
> ...
> 
>>  +static int rog_nkey_led_init(struct hid_device *hdev)
>>  +{
>>  +       u8 buf_init_start[] = { FEATURE_KBD_LED_REPORT_ID1, 0xB9 };
>>  +       u8 buf_init2[] = { FEATURE_KBD_LED_REPORT_ID1, 0x41, 0x53, 
>> 0x55, 0x53, 0x20,
>>  +                               0x54, 0x65, 0x63, 0x68, 0x2e, 0x49, 
>> 0x6e, 0x63, 0x2e, 0x00 };
>>  +       u8 buf_init3[] = { FEATURE_KBD_LED_REPORT_ID1,
>>  +                                               0x05, 0x20, 0x31, 
>> 0x00, 0x08 };
>>  +       int ret;
>>  +
>>  +       hid_info(hdev, "Asus initialise N-KEY Device");
>>  +       /* The first message is an init start */
>>  +       ret = asus_kbd_set_report(hdev, buf_init_start, 
>> sizeof(buf_init_start));
>>  +       if (ret < 0)
>>  +               hid_err(hdev, "Asus failed to send init start 
>> command: %d\n", ret);
>>  +       /* Followed by a string */
>>  +       ret = asus_kbd_set_report(hdev, buf_init2, 
>> sizeof(buf_init2));
>>  +       if (ret < 0)
>>  +               hid_err(hdev, "Asus failed to send init command 
>> 1.0: %d\n", ret);
>>  +       /* Followed by a string */
>>  +       ret = asus_kbd_set_report(hdev, buf_init3, 
>> sizeof(buf_init3));
>>  +       if (ret < 0)
>>  +               hid_err(hdev, "Asus failed to send init command 
>> 1.1: %d\n", ret);
> 
> If you do hid_err() why are you not bailing out?
> Mis-leveling of messages otherwise.
> 

Corrected to hid_warn. Most of the device is still completely usable 
without the
LED's working so it doesn't seem like good behaviour to kill 
functionality.

Thanks Andy. A new patch will be coming in the next few days.

Kind regards, Luke.



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V8] HID: ASUS: Add support for ASUS N-Key keyboard
  2020-10-15 11:11 ` Andy Shevchenko
  2020-10-18 21:23   ` Luke Jones
@ 2020-10-18 21:36   ` Luke Jones
  2020-10-19  9:54     ` Hans de Goede
  1 sibling, 1 reply; 10+ messages in thread
From: Luke Jones @ 2020-10-18 21:36 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-input, Hans de Goede, Jiri Kosina, Andy Shevchenko,
	Benjamin Tissoires



>>  +               */
>>  +               if (report->id == FEATURE_KBD_LED_REPORT_ID1 ||
>>  +                               report->id == 
>> FEATURE_KBD_LED_REPORT_ID2) {
> 
>>  +                       return -1;
> 
> is -1 a good return code? (this Q for all cases)
> 
>>  +               /* Additional report filtering */
>>  +               } else if (report->id == FEATURE_KBD_REPORT_ID) {
>>  +                       /* Fn+F5 "fan" symbol, trigger WMI event to 
>> toggle next mode */
>>  +                       if (data[1] == 0xae) {
>>  +                               ret = asus_wmi_send_event(drvdata, 
>> 0xae);
>>  +                               if (ret < 0) {
>>  +                                       hid_warn(hdev, "Asus failed 
>> to trigger fan control event");
>>  +                               }
> 
>>  +                               return -1;
>> 

In the case of this block I really don't have any idea how
to handle it. I want to stop these particular keycodes from
being evaluated elsewhere. Returning -1 seemed to be the only
way to do this, unless my understanding is very incorrect.

Any help or guidance on how to handle this is definitely
appreciated.

Cheers,
Luke.




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V8] HID: ASUS: Add support for ASUS N-Key keyboard
  2020-10-18 21:36   ` Luke Jones
@ 2020-10-19  9:54     ` Hans de Goede
  2020-10-19 11:11       ` Andy Shevchenko
  0 siblings, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2020-10-19  9:54 UTC (permalink / raw)
  To: Luke Jones, Andy Shevchenko
  Cc: linux-input, Jiri Kosina, Andy Shevchenko, Benjamin Tissoires

Hi,

On 10/18/20 11:36 PM, Luke Jones wrote:
> 
> 
>>>  +               */
>>>  +               if (report->id == FEATURE_KBD_LED_REPORT_ID1 ||
>>>  +                               report->id == FEATURE_KBD_LED_REPORT_ID2) {
>>
>>>  +                       return -1;
>>
>> is -1 a good return code? (this Q for all cases)
>>
>>>  +               /* Additional report filtering */
>>>  +               } else if (report->id == FEATURE_KBD_REPORT_ID) {
>>>  +                       /* Fn+F5 "fan" symbol, trigger WMI event to toggle next mode */
>>>  +                       if (data[1] == 0xae) {
>>>  +                               ret = asus_wmi_send_event(drvdata, 0xae);
>>>  +                               if (ret < 0) {
>>>  +                                       hid_warn(hdev, "Asus failed to trigger fan control event");
>>>  +                               }
>>
>>>  +                               return -1;
>>>
> 
> In the case of this block I really don't have any idea how
> to handle it. I want to stop these particular keycodes from
> being evaluated elsewhere. Returning -1 seemed to be the only
> way to do this, unless my understanding is very incorrect.
> 
> Any help or guidance on how to handle this is definitely
> appreciated.

Sorry, I missed that Andy's comment on this where for the raw-event handler,
in this case -1 has the special meaning of don't process this event further,
rather then being an error code.

So, since in this case -1 has a special meaning and it is NOT an error
code, using -1 is fine. IOW you can keep this part as is.

Regards,

Hans


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V8] HID: ASUS: Add support for ASUS N-Key keyboard
  2020-10-19  9:54     ` Hans de Goede
@ 2020-10-19 11:11       ` Andy Shevchenko
  0 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2020-10-19 11:11 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Luke Jones, linux-input, Jiri Kosina, Andy Shevchenko,
	Benjamin Tissoires

On Mon, Oct 19, 2020 at 12:54 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 10/18/20 11:36 PM, Luke Jones wrote:

> >>>  +               if (report->id == FEATURE_KBD_LED_REPORT_ID1 ||
> >>>  +                               report->id == FEATURE_KBD_LED_REPORT_ID2) {
> >>
> >>>  +                       return -1;
> >>
> >> is -1 a good return code? (this Q for all cases)

> >>>  +                               ret = asus_wmi_send_event(drvdata, 0xae);
> >>>  +                               if (ret < 0) {
> >>>  +                                       hid_warn(hdev, "Asus failed to trigger fan control event");
> >>>  +                               }
> >>
> >>>  +                               return -1;
> >
> > In the case of this block I really don't have any idea how
> > to handle it. I want to stop these particular keycodes from
> > being evaluated elsewhere. Returning -1 seemed to be the only
> > way to do this, unless my understanding is very incorrect.
> >
> > Any help or guidance on how to handle this is definitely
> > appreciated.
>
> Sorry, I missed that Andy's comment on this where for the raw-event handler,
> in this case -1 has the special meaning of don't process this event further,
> rather then being an error code.

Good to know, thanks!

> So, since in this case -1 has a special meaning and it is NOT an error
> code, using -1 is fine. IOW you can keep this part as is.

I agree with Hans, you may ignore my question in those cases.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-13  7:35 [PATCH V8] HID: ASUS: Add support for ASUS N-Key keyboard Luke D Jones
2020-10-13  7:37 ` Luke Jones
2020-10-15 11:11 ` Andy Shevchenko
2020-10-18 21:23   ` Luke Jones
2020-10-18 21:36   ` Luke Jones
2020-10-19  9:54     ` Hans de Goede
2020-10-19 11:11       ` Andy Shevchenko
2020-10-16 10:51 ` Hans de Goede
2020-10-16 20:10   ` Luke Jones
2020-10-18  8:59     ` Hans de Goede

Linux Input Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-input/0 linux-input/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-input linux-input/ https://lore.kernel.org/linux-input \
		linux-input@vger.kernel.org
	public-inbox-index linux-input

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-input


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git