| Message ID | 20250101151209.100072-2-git@mayer-bgk.de (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: steelseries: add SteelSeries Arctis 9 support | expand |
On Wed, 2025-01-01 at 15:11 +0000, Christian Mayer wrote: > Add support for the SteelSeries Arctis 9 headset. This driver > will export the battery information like it already does for > the Arcits 1 headset. > > Signed-off-by: Christian Mayer <git@mayer-bgk.de> Please remove all the whitespace changes, and split off the steelseries_headset_request_battery() refactoring into its own commit. Detailed review comments below. > --- > drivers/hid/hid-steelseries.c | 105 +++++++++++++++++++++++++++++----- > 1 file changed, 90 insertions(+), 15 deletions(-) > > diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c > index f9ff5be94309..f102045a9b6b 100644 > --- a/drivers/hid/hid-steelseries.c > +++ b/drivers/hid/hid-steelseries.c > @@ -18,7 +18,8 @@ > #include "hid-ids.h" > > #define STEELSERIES_SRWS1 BIT(0) > -#define STEELSERIES_ARCTIS_1 BIT(1) > +#define STEELSERIES_ARCTIS_1 BIT(1) Whitespace change > +#define STEELSERIES_ARCTIS_9 BIT(2) > > struct steelseries_device { > struct hid_device *hdev; > @@ -35,7 +36,7 @@ struct steelseries_device { > }; > > #if IS_BUILTIN(CONFIG_LEDS_CLASS) || \ > - (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) > + (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) Whitespace change > #define SRWS1_NUMBER_LEDS 15 > struct steelseries_srws1_data { > __u16 led_state; > @@ -95,7 +96,7 @@ static const __u8 steelseries_srws1_rdesc_fixed[] = { > 0x29, 0x11, /* Usage Maximum (11h), */ > 0x95, 0x11, /* Report Count (17), */ > 0x81, 0x02, /* Input (Variable), */ > - /* ---- Dial patch starts here ---- */ > + /* ---- Dial patch starts here ---- */ Whitespace change > 0x05, 0x01, /* Usage Page (Desktop), */ > 0x09, 0x33, /* Usage (RX), */ > 0x75, 0x04, /* Report Size (4), */ > @@ -108,7 +109,7 @@ static const __u8 steelseries_srws1_rdesc_fixed[] = { > 0x95, 0x01, /* Report Count (1), */ > 0x25, 0x03, /* Logical Maximum (3), */ > 0x81, 0x02, /* Input (Variable), */ > - /* ---- Dial patch ends here ---- */ > + /* ---- Dial patch ends here ---- */ Whitespace change > 0x06, 0x00, 0xFF, /* Usage Page (FF00h), */ > 0x09, 0x01, /* Usage (01h), */ > 0x75, 0x04, /* Changed Report Size (4), */ > @@ -125,7 +126,7 @@ static const __u8 steelseries_srws1_rdesc_fixed[] = { > }; > > #if IS_BUILTIN(CONFIG_LEDS_CLASS) || \ > - (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) > + (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) Whitespace change > static void steelseries_srws1_set_leds(struct hid_device *hdev, __u16 leds) > { > struct list_head *report_list = &hdev->report_enum[HID_OUTPUT_REPORT].report_list; > @@ -368,32 +369,36 @@ static void steelseries_srws1_remove(struct hid_device *hdev) > > hid_hw_stop(hdev); > kfree(drv_data); > - return; Should be a separate commit. > } > #endif > > #define STEELSERIES_HEADSET_BATTERY_TIMEOUT_MS 3000 > > #define ARCTIS_1_BATTERY_RESPONSE_LEN 8 > +#define ARCTIS_9_BATTERY_RESPONSE_LEN 64 > static const char arctis_1_battery_request[] = { 0x06, 0x12 }; > +static const char arctis_9_battery_request[] = { 0x00, 0x20 }; > > -static int steelseries_headset_arctis_1_fetch_battery(struct hid_device *hdev) > +static int steelseries_headset_request_battery(struct hid_device *hdev, > + const char *request, size_t len) This change should be a two-step process (so two separate commits), first factor out the battery request function so it take the request and len as you've done, then add support for that new device. > { > u8 *write_buf; > int ret; > > /* Request battery information */ > - write_buf = kmemdup(arctis_1_battery_request, sizeof(arctis_1_battery_request), GFP_KERNEL); > + write_buf = kmemdup(request, len, GFP_KERNEL); > if (!write_buf) > return -ENOMEM; > > - ret = hid_hw_raw_request(hdev, arctis_1_battery_request[0], > - write_buf, sizeof(arctis_1_battery_request), > + hid_dbg(hdev, "Sending battery request report"); > + ret = hid_hw_raw_request(hdev, request[0], > + write_buf, len, > HID_OUTPUT_REPORT, HID_REQ_SET_REPORT); > - if (ret < (int)sizeof(arctis_1_battery_request)) { > + if (ret < (int)len) { > hid_err(hdev, "hid_hw_raw_request() failed with %d\n", ret); > ret = -ENODATA; > } > + > kfree(write_buf); > return ret; > } > @@ -404,7 +409,11 @@ static void steelseries_headset_fetch_battery(struct hid_device *hdev) > int ret = 0; > > if (sd->quirks & STEELSERIES_ARCTIS_1) > - ret = steelseries_headset_arctis_1_fetch_battery(hdev); > + ret = steelseries_headset_request_battery(hdev, > + arctis_1_battery_request, sizeof(arctis_1_battery_request)); > + else if (sd->quirks & STEELSERIES_ARCTIS_9) > + ret = steelseries_headset_request_battery(hdev, > + arctis_9_battery_request, sizeof(arctis_9_battery_request)); > > if (ret < 0) > hid_dbg(hdev, > @@ -520,9 +529,22 @@ static int steelseries_headset_battery_register(struct steelseries_device *sd) > INIT_DELAYED_WORK(&sd->battery_work, steelseries_headset_battery_timer_tick); > steelseries_headset_fetch_battery(sd->hdev); > > + if (sd->quirks & STEELSERIES_ARCTIS_9) { > + // The first fetch_battery request can remain unanswered in some cases No C++-style comments. > + schedule_delayed_work(&sd->battery_work, > + msecs_to_jiffies(STEELSERIES_HEADSET_BATTERY_TIMEOUT_MS)); > + } > + > return 0; > } > > +static bool steelseries_is_vendor_usage_page(struct hid_device *hdev, uint8_t usagePage) > +{ > + return hdev->rdesc[0] == 0x06 && > + hdev->rdesc[1] == usagePage && > + hdev->rdesc[2] == 0xff; > +} Should probably be usage_page. > + > static int steelseries_probe(struct hid_device *hdev, const struct hid_device_id *id) > { > struct steelseries_device *sd; > @@ -537,7 +559,7 @@ static int steelseries_probe(struct hid_device *hdev, const struct hid_device_id > > if (sd->quirks & STEELSERIES_SRWS1) { > #if IS_BUILTIN(CONFIG_LEDS_CLASS) || \ > - (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) > + (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) > return steelseries_srws1_probe(hdev, id); > #else > return -ENODEV; > @@ -548,12 +570,22 @@ static int steelseries_probe(struct hid_device *hdev, const struct hid_device_id > if (ret) > return ret; > > + if (sd->quirks & STEELSERIES_ARCTIS_9 && > + !steelseries_is_vendor_usage_page(hdev, 0xc0)) > + return -ENODEV; > + > spin_lock_init(&sd->lock); > > ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); > if (ret) > return ret; > > + if (sd->quirks & STEELSERIES_ARCTIS_9) { > + ret = hid_hw_open(hdev); Is this needed? If so, this probably needs to be added as a separate commit, for all headsets rather than just this one. > + if (ret) > + return ret; > + } > + > if (steelseries_headset_battery_register(sd) < 0) > hid_err(sd->hdev, > "Failed to register battery for headset\n"); > @@ -568,7 +600,7 @@ static void steelseries_remove(struct hid_device *hdev) > > if (sd->quirks & STEELSERIES_SRWS1) { > #if IS_BUILTIN(CONFIG_LEDS_CLASS) || \ > - (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) > + (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) > steelseries_srws1_remove(hdev); > #endif > return; > @@ -580,6 +612,9 @@ static void steelseries_remove(struct hid_device *hdev) > > cancel_delayed_work_sync(&sd->battery_work); > > + if (sd->quirks & STEELSERIES_ARCTIS_9) > + hid_hw_close(hdev); > + > hid_hw_stop(hdev); > } > > @@ -599,6 +634,15 @@ static const __u8 *steelseries_srws1_report_fixup(struct hid_device *hdev, > return rdesc; > } > > +static uint8_t steelseries_headset_map_capacity(uint8_t capacity, uint8_t min_in, uint8_t max_in) > +{ > + if (capacity > max_in) > + return 100; > + if (capacity < min_in) > + return 0; > + return (capacity - min_in) * 100 / (max_in - min_in); > +} > + > static int steelseries_headset_raw_event(struct hid_device *hdev, > struct hid_report *report, u8 *read_buf, > int size) > @@ -630,6 +674,32 @@ static int steelseries_headset_raw_event(struct hid_device *hdev, > } > } > > + if (sd->quirks & STEELSERIES_ARCTIS_9) { > + hid_dbg(sd->hdev, > + "Parsing raw event for Arctis 9 headset (%*ph)\n", size, read_buf); > + if (size < ARCTIS_9_BATTERY_RESPONSE_LEN) { > + if (!delayed_work_pending(&sd->battery_work)) > + goto request_battery; > + return 0; > + } > + > + if (read_buf[0] == 0xaa && read_buf[1] == 0x01) { > + connected = true; > + > + /* > + * Found no official documentation about min and max. > + * Values defined by testing. > + */ > + capacity = steelseries_headset_map_capacity(read_buf[3], 0x68, 0x9d); > + } else { > + /* > + * Device is off and sends the last known status read_buf[1] == 0x03 or > + * there is no known status of the device read_buf[0] == 0x55 > + */ > + connected = false; > + } > + } > + > if (connected != sd->headset_connected) { > hid_dbg(sd->hdev, > "Connected status changed from %sconnected to %sconnected\n", > @@ -663,7 +733,11 @@ static const struct hid_device_id steelseries_devices[] = { > > { /* SteelSeries Arctis 1 Wireless for XBox */ > HID_USB_DEVICE(USB_VENDOR_ID_STEELSERIES, 0x12b6), > - .driver_data = STEELSERIES_ARCTIS_1 }, > + .driver_data = STEELSERIES_ARCTIS_1 }, Whitespace change. > + > + { /* SteelSeries Arctis 9 Wireless for XBox */ > + HID_USB_DEVICE(USB_VENDOR_ID_STEELSERIES, 0x12c2), > + .driver_data = STEELSERIES_ARCTIS_9 }, > > { } > }; > @@ -683,3 +757,4 @@ MODULE_DESCRIPTION("HID driver for Steelseries devices"); > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("Bastien Nocera <hadess@hadess.net>"); > MODULE_AUTHOR("Simon Wood <simon@mungewell.org>"); > +MODULE_AUTHOR("Christian Mayer <git@mayer-bgk.de>");
Le 01/01/2025 à 16:11, Christian Mayer a écrit : > Add support for the SteelSeries Arctis 9 headset. This driver > will export the battery information like it already does for > the Arcits 1 headset. > > Signed-off-by: Christian Mayer <git@mayer-bgk.de> ... > -static int steelseries_headset_arctis_1_fetch_battery(struct hid_device *hdev) > +static int steelseries_headset_request_battery(struct hid_device *hdev, > + const char *request, size_t len) > { > u8 *write_buf; > int ret; > > /* Request battery information */ > - write_buf = kmemdup(arctis_1_battery_request, sizeof(arctis_1_battery_request), GFP_KERNEL); > + write_buf = kmemdup(request, len, GFP_KERNEL); > if (!write_buf) > return -ENOMEM; > > - ret = hid_hw_raw_request(hdev, arctis_1_battery_request[0], > - write_buf, sizeof(arctis_1_battery_request), > + hid_dbg(hdev, "Sending battery request report"); > + ret = hid_hw_raw_request(hdev, request[0], > + write_buf, len, This could be on the same line. > HID_OUTPUT_REPORT, HID_REQ_SET_REPORT); > - if (ret < (int)sizeof(arctis_1_battery_request)) { > + if (ret < (int)len) { > hid_err(hdev, "hid_hw_raw_request() failed with %d\n", ret); > ret = -ENODATA; > } ... > +static uint8_t steelseries_headset_map_capacity(uint8_t capacity, uint8_t min_in, uint8_t max_in) > +{ > + if (capacity > max_in) >= ? > + return 100; > + if (capacity < min_in) <= ? > + return 0; > + return (capacity - min_in) * 100 / (max_in - min_in); > +} ... CJ
> On Wed, 2025-01-01 at 15:11 +0000, Christian Mayer wrote: >> Add support for the SteelSeries Arctis 9 headset. This driver >> will export the battery information like it already does for >> the Arcits 1 headset. >> >> Signed-off-by: Christian Mayer <git@mayer-bgk.de> > > > Please remove all the whitespace changes, and split off the > steelseries_headset_request_battery() refactoring into its own commit. > > Detailed review comments below. Thank you for your feedback Bastien. I will repost my patches with the fixes within the next days. Please see one follow-up question about hid_hw_open below. > >> --- >> drivers/hid/hid-steelseries.c | 105 +++++++++++++++++++++++++++++----- >> 1 file changed, 90 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c >> index f9ff5be94309..f102045a9b6b 100644 >> --- a/drivers/hid/hid-steelseries.c >> +++ b/drivers/hid/hid-steelseries.c >> @@ -18,7 +18,8 @@ >> #include "hid-ids.h" >> >> #define STEELSERIES_SRWS1 BIT(0) >> -#define STEELSERIES_ARCTIS_1 BIT(1) >> +#define STEELSERIES_ARCTIS_1 BIT(1) > > Whitespace change > >> +#define STEELSERIES_ARCTIS_9 BIT(2) >> >> struct steelseries_device { >> struct hid_device *hdev; >> @@ -35,7 +36,7 @@ struct steelseries_device { >> }; >> >> #if IS_BUILTIN(CONFIG_LEDS_CLASS) || \ >> - (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) >> + (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) > > Whitespace change > >> #define SRWS1_NUMBER_LEDS 15 >> struct steelseries_srws1_data { >> __u16 led_state; >> @@ -95,7 +96,7 @@ static const __u8 steelseries_srws1_rdesc_fixed[] = { >> 0x29, 0x11, /* Usage Maximum (11h), */ >> 0x95, 0x11, /* Report Count (17), */ >> 0x81, 0x02, /* Input (Variable), */ >> - /* ---- Dial patch starts here ---- */ >> + /* ---- Dial patch starts here ---- */ > > Whitespace change > >> 0x05, 0x01, /* Usage Page (Desktop), */ >> 0x09, 0x33, /* Usage (RX), */ >> 0x75, 0x04, /* Report Size (4), */ >> @@ -108,7 +109,7 @@ static const __u8 steelseries_srws1_rdesc_fixed[] = { >> 0x95, 0x01, /* Report Count (1), */ >> 0x25, 0x03, /* Logical Maximum (3), */ >> 0x81, 0x02, /* Input (Variable), */ >> - /* ---- Dial patch ends here ---- */ >> + /* ---- Dial patch ends here ---- */ > > Whitespace change > >> 0x06, 0x00, 0xFF, /* Usage Page (FF00h), */ >> 0x09, 0x01, /* Usage (01h), */ >> 0x75, 0x04, /* Changed Report Size (4), */ >> @@ -125,7 +126,7 @@ static const __u8 steelseries_srws1_rdesc_fixed[] = { >> }; >> >> #if IS_BUILTIN(CONFIG_LEDS_CLASS) || \ >> - (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) >> + (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) > > Whitespace change > >> static void steelseries_srws1_set_leds(struct hid_device *hdev, __u16 leds) >> { >> struct list_head *report_list = &hdev->report_enum[HID_OUTPUT_REPORT].report_list; >> @@ -368,32 +369,36 @@ static void steelseries_srws1_remove(struct hid_device *hdev) >> >> hid_hw_stop(hdev); >> kfree(drv_data); >> - return; > > Should be a separate commit. > >> } >> #endif >> >> #define STEELSERIES_HEADSET_BATTERY_TIMEOUT_MS 3000 >> >> #define ARCTIS_1_BATTERY_RESPONSE_LEN 8 >> +#define ARCTIS_9_BATTERY_RESPONSE_LEN 64 >> static const char arctis_1_battery_request[] = { 0x06, 0x12 }; >> +static const char arctis_9_battery_request[] = { 0x00, 0x20 }; >> >> -static int steelseries_headset_arctis_1_fetch_battery(struct hid_device *hdev) >> +static int steelseries_headset_request_battery(struct hid_device *hdev, >> + const char *request, size_t len) > > This change should be a two-step process (so two separate commits), > first factor out the battery request function so it take the request > and len as you've done, then add support for that new device. > >> { >> u8 *write_buf; >> int ret; >> >> /* Request battery information */ >> - write_buf = kmemdup(arctis_1_battery_request, sizeof(arctis_1_battery_request), GFP_KERNEL); >> + write_buf = kmemdup(request, len, GFP_KERNEL); >> if (!write_buf) >> return -ENOMEM; >> >> - ret = hid_hw_raw_request(hdev, arctis_1_battery_request[0], >> - write_buf, sizeof(arctis_1_battery_request), >> + hid_dbg(hdev, "Sending battery request report"); >> + ret = hid_hw_raw_request(hdev, request[0], >> + write_buf, len, >> HID_OUTPUT_REPORT, HID_REQ_SET_REPORT); >> - if (ret < (int)sizeof(arctis_1_battery_request)) { >> + if (ret < (int)len) { >> hid_err(hdev, "hid_hw_raw_request() failed with %d\n", ret); >> ret = -ENODATA; >> } >> + >> kfree(write_buf); >> return ret; >> } >> @@ -404,7 +409,11 @@ static void steelseries_headset_fetch_battery(struct hid_device *hdev) >> int ret = 0; >> >> if (sd->quirks & STEELSERIES_ARCTIS_1) >> - ret = steelseries_headset_arctis_1_fetch_battery(hdev); >> + ret = steelseries_headset_request_battery(hdev, >> + arctis_1_battery_request, sizeof(arctis_1_battery_request)); >> + else if (sd->quirks & STEELSERIES_ARCTIS_9) >> + ret = steelseries_headset_request_battery(hdev, >> + arctis_9_battery_request, sizeof(arctis_9_battery_request)); >> >> if (ret < 0) >> hid_dbg(hdev, >> @@ -520,9 +529,22 @@ static int steelseries_headset_battery_register(struct steelseries_device *sd) >> INIT_DELAYED_WORK(&sd->battery_work, steelseries_headset_battery_timer_tick); >> steelseries_headset_fetch_battery(sd->hdev); >> >> + if (sd->quirks & STEELSERIES_ARCTIS_9) { >> + // The first fetch_battery request can remain unanswered in some cases > > No C++-style comments. > >> + schedule_delayed_work(&sd->battery_work, >> + msecs_to_jiffies(STEELSERIES_HEADSET_BATTERY_TIMEOUT_MS)); >> + } >> + >> return 0; >> } >> >> +static bool steelseries_is_vendor_usage_page(struct hid_device *hdev, uint8_t usagePage) >> +{ >> + return hdev->rdesc[0] == 0x06 && >> + hdev->rdesc[1] == usagePage && >> + hdev->rdesc[2] == 0xff; >> +} > > Should probably be usage_page. > >> + >> static int steelseries_probe(struct hid_device *hdev, const struct hid_device_id *id) >> { >> struct steelseries_device *sd; >> @@ -537,7 +559,7 @@ static int steelseries_probe(struct hid_device *hdev, const struct hid_device_id >> >> if (sd->quirks & STEELSERIES_SRWS1) { >> #if IS_BUILTIN(CONFIG_LEDS_CLASS) || \ >> - (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) >> + (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) >> return steelseries_srws1_probe(hdev, id); >> #else >> return -ENODEV; >> @@ -548,12 +570,22 @@ static int steelseries_probe(struct hid_device *hdev, const struct hid_device_id >> if (ret) >> return ret; >> >> + if (sd->quirks & STEELSERIES_ARCTIS_9 && >> + !steelseries_is_vendor_usage_page(hdev, 0xc0)) >> + return -ENODEV; >> + >> spin_lock_init(&sd->lock); >> >> ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); >> if (ret) >> return ret; >> >> + if (sd->quirks & STEELSERIES_ARCTIS_9) { >> + ret = hid_hw_open(hdev); > > Is this needed? If so, this probably needs to be added as a separate > commit, for all headsets rather than just this one. Yes that's mandatory to get raw_events from the device. I thought about adding this to all headsets, but i was not sure if this breaks anything for the Arctis 1 headset. But i can add this for all headsets, that's fine with me. Would you like me to create a completely separate patch for this or should i submit this with the preparation patch which refactors steelseries_headset_fetch_battery? > >> + if (ret) >> + return ret; >> + } >> + >> if (steelseries_headset_battery_register(sd) < 0) >> hid_err(sd->hdev, >> "Failed to register battery for headset\n"); >> @@ -568,7 +600,7 @@ static void steelseries_remove(struct hid_device *hdev) >> >> if (sd->quirks & STEELSERIES_SRWS1) { >> #if IS_BUILTIN(CONFIG_LEDS_CLASS) || \ >> - (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) >> + (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) >> steelseries_srws1_remove(hdev); >> #endif >> return; >> @@ -580,6 +612,9 @@ static void steelseries_remove(struct hid_device *hdev) >> >> cancel_delayed_work_sync(&sd->battery_work); >> >> + if (sd->quirks & STEELSERIES_ARCTIS_9) >> + hid_hw_close(hdev); >> + >> hid_hw_stop(hdev); >> } >> >> @@ -599,6 +634,15 @@ static const __u8 *steelseries_srws1_report_fixup(struct hid_device *hdev, >> return rdesc; >> } >> >> +static uint8_t steelseries_headset_map_capacity(uint8_t capacity, uint8_t min_in, uint8_t max_in) >> +{ >> + if (capacity > max_in) >> + return 100; >> + if (capacity < min_in) >> + return 0; >> + return (capacity - min_in) * 100 / (max_in - min_in); >> +} >> + >> static int steelseries_headset_raw_event(struct hid_device *hdev, >> struct hid_report *report, u8 *read_buf, >> int size) >> @@ -630,6 +674,32 @@ static int steelseries_headset_raw_event(struct hid_device *hdev, >> } >> } >> >> + if (sd->quirks & STEELSERIES_ARCTIS_9) { >> + hid_dbg(sd->hdev, >> + "Parsing raw event for Arctis 9 headset (%*ph)\n", size, read_buf); >> + if (size < ARCTIS_9_BATTERY_RESPONSE_LEN) { >> + if (!delayed_work_pending(&sd->battery_work)) >> + goto request_battery; >> + return 0; >> + } >> + >> + if (read_buf[0] == 0xaa && read_buf[1] == 0x01) { >> + connected = true; >> + >> + /* >> + * Found no official documentation about min and max. >> + * Values defined by testing. >> + */ >> + capacity = steelseries_headset_map_capacity(read_buf[3], 0x68, 0x9d); >> + } else { >> + /* >> + * Device is off and sends the last known status read_buf[1] == 0x03 or >> + * there is no known status of the device read_buf[0] == 0x55 >> + */ >> + connected = false; >> + } >> + } >> + >> if (connected != sd->headset_connected) { >> hid_dbg(sd->hdev, >> "Connected status changed from %sconnected to %sconnected\n", >> @@ -663,7 +733,11 @@ static const struct hid_device_id steelseries_devices[] = { >> >> { /* SteelSeries Arctis 1 Wireless for XBox */ >> HID_USB_DEVICE(USB_VENDOR_ID_STEELSERIES, 0x12b6), >> - .driver_data = STEELSERIES_ARCTIS_1 }, >> + .driver_data = STEELSERIES_ARCTIS_1 }, > > Whitespace change. > >> + >> + { /* SteelSeries Arctis 9 Wireless for XBox */ >> + HID_USB_DEVICE(USB_VENDOR_ID_STEELSERIES, 0x12c2), >> + .driver_data = STEELSERIES_ARCTIS_9 }, >> >> { } >> }; >> @@ -683,3 +757,4 @@ MODULE_DESCRIPTION("HID driver for Steelseries devices"); >> MODULE_LICENSE("GPL"); >> MODULE_AUTHOR("Bastien Nocera <hadess@hadess.net>"); >> MODULE_AUTHOR("Simon Wood <simon@mungewell.org>"); >> +MODULE_AUTHOR("Christian Mayer <git@mayer-bgk.de>"); >
Am 04.01.25 um 13:45 schrieb Christophe JAILLET: > Le 01/01/2025 à 16:11, Christian Mayer a écrit : >> Add support for the SteelSeries Arctis 9 headset. This driver >> will export the battery information like it already does for >> the Arcits 1 headset. >> >> Signed-off-by: Christian Mayer <git@mayer-bgk.de> > > ... Thanks for your feedback Christophe. I will repost my patches with the fixes for your comments together with the fixes for Bastiens comment. > >> -static int steelseries_headset_arctis_1_fetch_battery(struct >> hid_device *hdev) >> +static int steelseries_headset_request_battery(struct hid_device *hdev, >> + const char *request, size_t len) >> { >> u8 *write_buf; >> int ret; >> /* Request battery information */ >> - write_buf = kmemdup(arctis_1_battery_request, >> sizeof(arctis_1_battery_request), GFP_KERNEL); >> + write_buf = kmemdup(request, len, GFP_KERNEL); >> if (!write_buf) >> return -ENOMEM; >> - ret = hid_hw_raw_request(hdev, arctis_1_battery_request[0], >> - write_buf, sizeof(arctis_1_battery_request), >> + hid_dbg(hdev, "Sending battery request report"); >> + ret = hid_hw_raw_request(hdev, request[0], >> + write_buf, len, > > This could be on the same line. > >> HID_OUTPUT_REPORT, HID_REQ_SET_REPORT); >> - if (ret < (int)sizeof(arctis_1_battery_request)) { >> + if (ret < (int)len) { >> hid_err(hdev, "hid_hw_raw_request() failed with %d\n", ret); >> ret = -ENODATA; >> } > > ... > >> +static uint8_t steelseries_headset_map_capacity(uint8_t capacity, >> uint8_t min_in, uint8_t max_in) >> +{ >> + if (capacity > max_in) > > >= ? > >> + return 100; >> + if (capacity < min_in) > > <= ? > >> + return 0; >> + return (capacity - min_in) * 100 / (max_in - min_in); >> +} > > ... > > CJ
diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c index f9ff5be94309..f102045a9b6b 100644 --- a/drivers/hid/hid-steelseries.c +++ b/drivers/hid/hid-steelseries.c @@ -18,7 +18,8 @@ #include "hid-ids.h" #define STEELSERIES_SRWS1 BIT(0) -#define STEELSERIES_ARCTIS_1 BIT(1) +#define STEELSERIES_ARCTIS_1 BIT(1) +#define STEELSERIES_ARCTIS_9 BIT(2) struct steelseries_device { struct hid_device *hdev; @@ -35,7 +36,7 @@ struct steelseries_device { }; #if IS_BUILTIN(CONFIG_LEDS_CLASS) || \ - (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) + (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) #define SRWS1_NUMBER_LEDS 15 struct steelseries_srws1_data { __u16 led_state; @@ -95,7 +96,7 @@ static const __u8 steelseries_srws1_rdesc_fixed[] = { 0x29, 0x11, /* Usage Maximum (11h), */ 0x95, 0x11, /* Report Count (17), */ 0x81, 0x02, /* Input (Variable), */ - /* ---- Dial patch starts here ---- */ + /* ---- Dial patch starts here ---- */ 0x05, 0x01, /* Usage Page (Desktop), */ 0x09, 0x33, /* Usage (RX), */ 0x75, 0x04, /* Report Size (4), */ @@ -108,7 +109,7 @@ static const __u8 steelseries_srws1_rdesc_fixed[] = { 0x95, 0x01, /* Report Count (1), */ 0x25, 0x03, /* Logical Maximum (3), */ 0x81, 0x02, /* Input (Variable), */ - /* ---- Dial patch ends here ---- */ + /* ---- Dial patch ends here ---- */ 0x06, 0x00, 0xFF, /* Usage Page (FF00h), */ 0x09, 0x01, /* Usage (01h), */ 0x75, 0x04, /* Changed Report Size (4), */ @@ -125,7 +126,7 @@ static const __u8 steelseries_srws1_rdesc_fixed[] = { }; #if IS_BUILTIN(CONFIG_LEDS_CLASS) || \ - (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) + (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) static void steelseries_srws1_set_leds(struct hid_device *hdev, __u16 leds) { struct list_head *report_list = &hdev->report_enum[HID_OUTPUT_REPORT].report_list; @@ -368,32 +369,36 @@ static void steelseries_srws1_remove(struct hid_device *hdev) hid_hw_stop(hdev); kfree(drv_data); - return; } #endif #define STEELSERIES_HEADSET_BATTERY_TIMEOUT_MS 3000 #define ARCTIS_1_BATTERY_RESPONSE_LEN 8 +#define ARCTIS_9_BATTERY_RESPONSE_LEN 64 static const char arctis_1_battery_request[] = { 0x06, 0x12 }; +static const char arctis_9_battery_request[] = { 0x00, 0x20 }; -static int steelseries_headset_arctis_1_fetch_battery(struct hid_device *hdev) +static int steelseries_headset_request_battery(struct hid_device *hdev, + const char *request, size_t len) { u8 *write_buf; int ret; /* Request battery information */ - write_buf = kmemdup(arctis_1_battery_request, sizeof(arctis_1_battery_request), GFP_KERNEL); + write_buf = kmemdup(request, len, GFP_KERNEL); if (!write_buf) return -ENOMEM; - ret = hid_hw_raw_request(hdev, arctis_1_battery_request[0], - write_buf, sizeof(arctis_1_battery_request), + hid_dbg(hdev, "Sending battery request report"); + ret = hid_hw_raw_request(hdev, request[0], + write_buf, len, HID_OUTPUT_REPORT, HID_REQ_SET_REPORT); - if (ret < (int)sizeof(arctis_1_battery_request)) { + if (ret < (int)len) { hid_err(hdev, "hid_hw_raw_request() failed with %d\n", ret); ret = -ENODATA; } + kfree(write_buf); return ret; } @@ -404,7 +409,11 @@ static void steelseries_headset_fetch_battery(struct hid_device *hdev) int ret = 0; if (sd->quirks & STEELSERIES_ARCTIS_1) - ret = steelseries_headset_arctis_1_fetch_battery(hdev); + ret = steelseries_headset_request_battery(hdev, + arctis_1_battery_request, sizeof(arctis_1_battery_request)); + else if (sd->quirks & STEELSERIES_ARCTIS_9) + ret = steelseries_headset_request_battery(hdev, + arctis_9_battery_request, sizeof(arctis_9_battery_request)); if (ret < 0) hid_dbg(hdev, @@ -520,9 +529,22 @@ static int steelseries_headset_battery_register(struct steelseries_device *sd) INIT_DELAYED_WORK(&sd->battery_work, steelseries_headset_battery_timer_tick); steelseries_headset_fetch_battery(sd->hdev); + if (sd->quirks & STEELSERIES_ARCTIS_9) { + // The first fetch_battery request can remain unanswered in some cases + schedule_delayed_work(&sd->battery_work, + msecs_to_jiffies(STEELSERIES_HEADSET_BATTERY_TIMEOUT_MS)); + } + return 0; } +static bool steelseries_is_vendor_usage_page(struct hid_device *hdev, uint8_t usagePage) +{ + return hdev->rdesc[0] == 0x06 && + hdev->rdesc[1] == usagePage && + hdev->rdesc[2] == 0xff; +} + static int steelseries_probe(struct hid_device *hdev, const struct hid_device_id *id) { struct steelseries_device *sd; @@ -537,7 +559,7 @@ static int steelseries_probe(struct hid_device *hdev, const struct hid_device_id if (sd->quirks & STEELSERIES_SRWS1) { #if IS_BUILTIN(CONFIG_LEDS_CLASS) || \ - (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) + (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) return steelseries_srws1_probe(hdev, id); #else return -ENODEV; @@ -548,12 +570,22 @@ static int steelseries_probe(struct hid_device *hdev, const struct hid_device_id if (ret) return ret; + if (sd->quirks & STEELSERIES_ARCTIS_9 && + !steelseries_is_vendor_usage_page(hdev, 0xc0)) + return -ENODEV; + spin_lock_init(&sd->lock); ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); if (ret) return ret; + if (sd->quirks & STEELSERIES_ARCTIS_9) { + ret = hid_hw_open(hdev); + if (ret) + return ret; + } + if (steelseries_headset_battery_register(sd) < 0) hid_err(sd->hdev, "Failed to register battery for headset\n"); @@ -568,7 +600,7 @@ static void steelseries_remove(struct hid_device *hdev) if (sd->quirks & STEELSERIES_SRWS1) { #if IS_BUILTIN(CONFIG_LEDS_CLASS) || \ - (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) + (IS_MODULE(CONFIG_LEDS_CLASS) && IS_MODULE(CONFIG_HID_STEELSERIES)) steelseries_srws1_remove(hdev); #endif return; @@ -580,6 +612,9 @@ static void steelseries_remove(struct hid_device *hdev) cancel_delayed_work_sync(&sd->battery_work); + if (sd->quirks & STEELSERIES_ARCTIS_9) + hid_hw_close(hdev); + hid_hw_stop(hdev); } @@ -599,6 +634,15 @@ static const __u8 *steelseries_srws1_report_fixup(struct hid_device *hdev, return rdesc; } +static uint8_t steelseries_headset_map_capacity(uint8_t capacity, uint8_t min_in, uint8_t max_in) +{ + if (capacity > max_in) + return 100; + if (capacity < min_in) + return 0; + return (capacity - min_in) * 100 / (max_in - min_in); +} + static int steelseries_headset_raw_event(struct hid_device *hdev, struct hid_report *report, u8 *read_buf, int size) @@ -630,6 +674,32 @@ static int steelseries_headset_raw_event(struct hid_device *hdev, } } + if (sd->quirks & STEELSERIES_ARCTIS_9) { + hid_dbg(sd->hdev, + "Parsing raw event for Arctis 9 headset (%*ph)\n", size, read_buf); + if (size < ARCTIS_9_BATTERY_RESPONSE_LEN) { + if (!delayed_work_pending(&sd->battery_work)) + goto request_battery; + return 0; + } + + if (read_buf[0] == 0xaa && read_buf[1] == 0x01) { + connected = true; + + /* + * Found no official documentation about min and max. + * Values defined by testing. + */ + capacity = steelseries_headset_map_capacity(read_buf[3], 0x68, 0x9d); + } else { + /* + * Device is off and sends the last known status read_buf[1] == 0x03 or + * there is no known status of the device read_buf[0] == 0x55 + */ + connected = false; + } + } + if (connected != sd->headset_connected) { hid_dbg(sd->hdev, "Connected status changed from %sconnected to %sconnected\n", @@ -663,7 +733,11 @@ static const struct hid_device_id steelseries_devices[] = { { /* SteelSeries Arctis 1 Wireless for XBox */ HID_USB_DEVICE(USB_VENDOR_ID_STEELSERIES, 0x12b6), - .driver_data = STEELSERIES_ARCTIS_1 }, + .driver_data = STEELSERIES_ARCTIS_1 }, + + { /* SteelSeries Arctis 9 Wireless for XBox */ + HID_USB_DEVICE(USB_VENDOR_ID_STEELSERIES, 0x12c2), + .driver_data = STEELSERIES_ARCTIS_9 }, { } }; @@ -683,3 +757,4 @@ MODULE_DESCRIPTION("HID driver for Steelseries devices"); MODULE_LICENSE("GPL"); MODULE_AUTHOR("Bastien Nocera <hadess@hadess.net>"); MODULE_AUTHOR("Simon Wood <simon@mungewell.org>"); +MODULE_AUTHOR("Christian Mayer <git@mayer-bgk.de>");
Add support for the SteelSeries Arctis 9 headset. This driver will export the battery information like it already does for the Arcits 1 headset. Signed-off-by: Christian Mayer <git@mayer-bgk.de> --- drivers/hid/hid-steelseries.c | 105 +++++++++++++++++++++++++++++----- 1 file changed, 90 insertions(+), 15 deletions(-)