All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Luke D Jones <luke@ljones.dev>
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: Thu, 15 Oct 2020 14:11:29 +0300	[thread overview]
Message-ID: <CAHp75VfOV2DvngsO87PLXwNKQtR-RaC4XzbBk_7wDVSEaBMrzw@mail.gmail.com> (raw)
In-Reply-To: <20201013073508.10476-1-luke@ljones.dev>

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

  parent reply	other threads:[~2020-10-15 11:11 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 [this message]
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

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=CAHp75VfOV2DvngsO87PLXwNKQtR-RaC4XzbBk_7wDVSEaBMrzw@mail.gmail.com \
    --to=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 \
    --cc=luke@ljones.dev \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.