From: Luke Jones <luke@ljones.dev>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: linux-input <linux-input@vger.kernel.org>,
Hans de Goede <hdegoede@redhat.com>,
Jiri Kosina <jikos@kernel.org>,
Andy Shevchenko <andy@infradead.org>,
Benjamin Tissoires <benjamin.tissoires@redhat.com>
Subject: Re: [PATCH V8] HID: ASUS: Add support for ASUS N-Key keyboard
Date: Mon, 19 Oct 2020 10:23:50 +1300 [thread overview]
Message-ID: <Q32FIQ.27DE313FPONY1@ljones.dev> (raw)
In-Reply-To: <CAHp75VfOV2DvngsO87PLXwNKQtR-RaC4XzbBk_7wDVSEaBMrzw@mail.gmail.com>
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.
next prev parent reply other threads:[~2020-10-18 21:24 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Q32FIQ.27DE313FPONY1@ljones.dev \
--to=luke@ljones.dev \
--cc=andy.shevchenko@gmail.com \
--cc=andy@infradead.org \
--cc=benjamin.tissoires@redhat.com \
--cc=hdegoede@redhat.com \
--cc=jikos@kernel.org \
--cc=linux-input@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).