Re: [PATCH 1/3] HID: steelseries: add SteelSeries Arctis 9 support

Christian Mayer <christian@xxxxxxxxxxxx> · Sun, 5 Jan 2025 11:16:16 +0100

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


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@xxxxxxxxxx>");
  MODULE_AUTHOR("Simon Wood <simon@xxxxxxxxxxxxx>");
+MODULE_AUTHOR("Christian Mayer <git@xxxxxxxxxxxx>");









