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
> 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
