| Message ID | 20250324210151.6042-3-lkml@antheas.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | HID: asus: Add RGB Support to Asus Z13, Ally, unify backlight asus-wmi, and Z13 QOL | expand |
On Mon, 24 Mar 2025, Antheas Kapenekakis wrote: > ROG keyboards are HID compliant and only care about the endpoint that > produces vendor events (e.g., fan mode) and has the keyboard backlight. > > Therefore, handle all of the endpoints of ROG keyboards as compliant, > by adding HID_QUIRK_INPUT_PER_APP and, for devices other than the vendor > one, by adding QUIRK_HANDLE_GENERIC to stop mutating them. > > Due to HID_QUIRK_INPUT_PER_APP, rgb register is moved into probe, as > the input_* functions are called multiple times (4 for the Z13). > > Reviewed-by: Luke D. Jones <luke@ljones.dev> > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > --- > drivers/hid/hid-asus.c | 69 ++++++++++++++++++++++++++++++++---------- > 1 file changed, 53 insertions(+), 16 deletions(-) > > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c > index 8d4df1b6f143b..96461321c191c 100644 > --- a/drivers/hid/hid-asus.c > +++ b/drivers/hid/hid-asus.c > @@ -84,6 +84,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad"); > #define QUIRK_MEDION_E1239T BIT(10) > #define QUIRK_ROG_NKEY_KEYBOARD BIT(11) > #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12) > +#define QUIRK_HANDLE_GENERIC BIT(13) > > #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \ > QUIRK_NO_INIT_REPORTS | \ > @@ -120,7 +121,6 @@ struct asus_drvdata { > struct input_dev *tp_kbd_input; > struct asus_kbd_leds *kbd_backlight; > const struct asus_touchpad_info *tp; > - bool enable_backlight; > struct power_supply *battery; > struct power_supply_desc battery_desc; > int battery_capacity; > @@ -326,6 +326,10 @@ static int asus_raw_event(struct hid_device *hdev, > { > struct asus_drvdata *drvdata = hid_get_drvdata(hdev); > > + if (drvdata->quirks & QUIRK_HANDLE_GENERIC) > + /* NOOP on generic HID devices to avoid side effects. */ > + return 0; > + > if (drvdata->battery && data[0] == BATTERY_REPORT_ID) > return asus_report_battery(drvdata, data, size); > > @@ -774,6 +778,10 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi) > struct input_dev *input = hi->input; > struct asus_drvdata *drvdata = hid_get_drvdata(hdev); > > + if (drvdata->quirks & QUIRK_HANDLE_GENERIC) > + /* NOOP on generic HID devices to avoid side effects. */ > + return 0; You'd need braces for multiline indented constructs like this, however, I think that comment can appear before the if line which wouldn't require braces. The same applies to many cases below. > + > /* T100CHI uses MULTI_INPUT, bind the touchpad to the mouse hid_input */ > if (drvdata->quirks & QUIRK_T100CHI && > hi->report->id != T100CHI_MOUSE_REPORT_ID) > @@ -827,11 +835,6 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi) > > drvdata->input = input; > > - if (drvdata->enable_backlight && > - !asus_kbd_wmi_led_control_present(hdev) && > - asus_kbd_register_leds(hdev)) > - hid_warn(hdev, "Failed to initialize backlight.\n"); > - > return 0; > } > > @@ -851,6 +854,10 @@ static int asus_input_mapping(struct hid_device *hdev, > return -1; > } > > + if (drvdata->quirks & QUIRK_HANDLE_GENERIC) > + /* NOOP on generic HID devices to avoid side effects. */ > + return 0; > + > /* > * Ignore a bunch of bogus collections in the T100CHI descriptor. > * This avoids a bunch of non-functional hid_input devices getting > @@ -901,15 +908,6 @@ static int asus_input_mapping(struct hid_device *hdev, > return -1; > } > > - /* > - * Check and enable backlight only on devices with UsagePage == > - * 0xff31 to avoid initializing the keyboard firmware multiple > - * times on devices with multiple HID descriptors but same > - * PID/VID. > - */ > - if (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) > - drvdata->enable_backlight = true; > - > set_bit(EV_REP, hi->input->evbit); > return 1; > } > @@ -1026,8 +1024,10 @@ static int __maybe_unused asus_reset_resume(struct hid_device *hdev) > > static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id) > { > - int ret; > + struct hid_report_enum *rep_enum; > struct asus_drvdata *drvdata; > + struct hid_report *rep; > + int ret, is_vendor = 0; > > drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL); > if (drvdata == NULL) { > @@ -1111,12 +1111,45 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id) > return ret; > } > > + /* > + * Check for the vendor interface (0xff31) to init the RGB. The next line seems to be continuation, is . extra? > + * and handle generic devices properly. > + */ > + rep_enum = &hdev->report_enum[HID_INPUT_REPORT]; > + list_for_each_entry(rep, &rep_enum->report_list, list) { > + if ((rep->application & HID_USAGE_PAGE) == 0xff310000) Use a named define for the literal? > + is_vendor = true; > + } > + > + /* > + * For ROG keyboards, make them hid compliant by > + * creating one input per application. For interfaces other than > + * the vendor one, disable hid-asus handlers. > + */ > + if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) { > + if (!is_vendor) > + drvdata->quirks |= QUIRK_HANDLE_GENERIC; > + hdev->quirks |= HID_QUIRK_INPUT_PER_APP; > + } > + > ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); > if (ret) { > hid_err(hdev, "Asus hw start failed: %d\n", ret); > return ret; > } > > + if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) && > + !asus_kbd_wmi_led_control_present(hdev) && > + asus_kbd_register_leds(hdev)) > + hid_warn(hdev, "Failed to initialize backlight.\n"); > + > + /* > + * For ROG keyboards, skip rename for consistency and > + * ->input check as some devices do not have inputs. Please reflow to 80 chars. > + */ > + if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) > + return 0; > + > if (!drvdata->input) { > hid_err(hdev, "Asus input not registered\n"); > ret = -ENOMEM; > @@ -1167,6 +1200,10 @@ static const __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc, > { > struct asus_drvdata *drvdata = hid_get_drvdata(hdev); > > + if (drvdata->quirks & QUIRK_HANDLE_GENERIC) > + /* NOOP on generic HID devices to avoid side effects. */ > + return rdesc; > + > if (drvdata->quirks & QUIRK_FIX_NOTEBOOK_REPORT && > *rsize >= 56 && rdesc[54] == 0x25 && rdesc[55] == 0x65) { > hid_info(hdev, "Fixing up Asus notebook report descriptor\n"); >
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c index 8d4df1b6f143b..96461321c191c 100644 --- a/drivers/hid/hid-asus.c +++ b/drivers/hid/hid-asus.c @@ -84,6 +84,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad"); #define QUIRK_MEDION_E1239T BIT(10) #define QUIRK_ROG_NKEY_KEYBOARD BIT(11) #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12) +#define QUIRK_HANDLE_GENERIC BIT(13) #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \ QUIRK_NO_INIT_REPORTS | \ @@ -120,7 +121,6 @@ struct asus_drvdata { struct input_dev *tp_kbd_input; struct asus_kbd_leds *kbd_backlight; const struct asus_touchpad_info *tp; - bool enable_backlight; struct power_supply *battery; struct power_supply_desc battery_desc; int battery_capacity; @@ -326,6 +326,10 @@ static int asus_raw_event(struct hid_device *hdev, { struct asus_drvdata *drvdata = hid_get_drvdata(hdev); + if (drvdata->quirks & QUIRK_HANDLE_GENERIC) + /* NOOP on generic HID devices to avoid side effects. */ + return 0; + if (drvdata->battery && data[0] == BATTERY_REPORT_ID) return asus_report_battery(drvdata, data, size); @@ -774,6 +778,10 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi) struct input_dev *input = hi->input; struct asus_drvdata *drvdata = hid_get_drvdata(hdev); + if (drvdata->quirks & QUIRK_HANDLE_GENERIC) + /* NOOP on generic HID devices to avoid side effects. */ + return 0; + /* T100CHI uses MULTI_INPUT, bind the touchpad to the mouse hid_input */ if (drvdata->quirks & QUIRK_T100CHI && hi->report->id != T100CHI_MOUSE_REPORT_ID) @@ -827,11 +835,6 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi) drvdata->input = input; - if (drvdata->enable_backlight && - !asus_kbd_wmi_led_control_present(hdev) && - asus_kbd_register_leds(hdev)) - hid_warn(hdev, "Failed to initialize backlight.\n"); - return 0; } @@ -851,6 +854,10 @@ static int asus_input_mapping(struct hid_device *hdev, return -1; } + if (drvdata->quirks & QUIRK_HANDLE_GENERIC) + /* NOOP on generic HID devices to avoid side effects. */ + return 0; + /* * Ignore a bunch of bogus collections in the T100CHI descriptor. * This avoids a bunch of non-functional hid_input devices getting @@ -901,15 +908,6 @@ static int asus_input_mapping(struct hid_device *hdev, return -1; } - /* - * Check and enable backlight only on devices with UsagePage == - * 0xff31 to avoid initializing the keyboard firmware multiple - * times on devices with multiple HID descriptors but same - * PID/VID. - */ - if (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) - drvdata->enable_backlight = true; - set_bit(EV_REP, hi->input->evbit); return 1; } @@ -1026,8 +1024,10 @@ static int __maybe_unused asus_reset_resume(struct hid_device *hdev) static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id) { - int ret; + struct hid_report_enum *rep_enum; struct asus_drvdata *drvdata; + struct hid_report *rep; + int ret, is_vendor = 0; drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL); if (drvdata == NULL) { @@ -1111,12 +1111,45 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id) return ret; } + /* + * Check for the vendor interface (0xff31) to init the RGB. + * and handle generic devices properly. + */ + rep_enum = &hdev->report_enum[HID_INPUT_REPORT]; + list_for_each_entry(rep, &rep_enum->report_list, list) { + if ((rep->application & HID_USAGE_PAGE) == 0xff310000) + is_vendor = true; + } + + /* + * For ROG keyboards, make them hid compliant by + * creating one input per application. For interfaces other than + * the vendor one, disable hid-asus handlers. + */ + if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) { + if (!is_vendor) + drvdata->quirks |= QUIRK_HANDLE_GENERIC; + hdev->quirks |= HID_QUIRK_INPUT_PER_APP; + } + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); if (ret) { hid_err(hdev, "Asus hw start failed: %d\n", ret); return ret; } + if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) && + !asus_kbd_wmi_led_control_present(hdev) && + asus_kbd_register_leds(hdev)) + hid_warn(hdev, "Failed to initialize backlight.\n"); + + /* + * For ROG keyboards, skip rename for consistency and + * ->input check as some devices do not have inputs. + */ + if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) + return 0; + if (!drvdata->input) { hid_err(hdev, "Asus input not registered\n"); ret = -ENOMEM; @@ -1167,6 +1200,10 @@ static const __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc, { struct asus_drvdata *drvdata = hid_get_drvdata(hdev); + if (drvdata->quirks & QUIRK_HANDLE_GENERIC) + /* NOOP on generic HID devices to avoid side effects. */ + return rdesc; + if (drvdata->quirks & QUIRK_FIX_NOTEBOOK_REPORT && *rsize >= 56 && rdesc[54] == 0x25 && rdesc[55] == 0x65) { hid_info(hdev, "Fixing up Asus notebook report descriptor\n");