On Thu, Oct 15, 2020 at 14:11, Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
On Tue, Oct 13, 2020 at 10:35 AM Luke D Jones <luke@xxxxxxxxxx> 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.
