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

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

[Christian Mayer]

Hi,

i added support for the SteelSeries Arctis 9 headset.

HID: steelseries: add SteelSeries Arctis 9 support
HID: steelseries: export charging state for the SteelSeries Arctis 9 headset
HID: steelseries: export model and manufacturer

drivers/hid/hid-steelseries.c | 138 ++++++++++++++++++++++++++++++++++++------
1 file changed, 120 insertions(+), 18 deletions(-)

Christian

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

[Christian Mayer]

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(-)

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>");
-- 
2.47.1

[PATCH 2/3] HID: steelseries: export charging state for the SteelSeries Arctis 9 headset

[Christian Mayer]

The Arctis 9 headset provides the information if
the power cable is plugged in and charging via the battery report.
This information can be exported.

Signed-off-by: Christian Mayer <git@mayer-bgk.de>
---
 drivers/hid/hid-steelseries.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
index f102045a9b6b..d11296bc0e1e 100644
--- a/drivers/hid/hid-steelseries.c
+++ b/drivers/hid/hid-steelseries.c
@@ -33,6 +33,7 @@ struct steelseries_device {
 	struct power_supply *battery;
 	uint8_t battery_capacity;
 	bool headset_connected;
+	bool battery_charging;
 };
 
 #if IS_BUILTIN(CONFIG_LEDS_CLASS) || \
@@ -450,9 +451,12 @@ static int steelseries_headset_battery_get_property(struct power_supply *psy,
 		val->intval = 1;
 		break;
 	case POWER_SUPPLY_PROP_STATUS:
-		val->intval = sd->headset_connected ?
-			POWER_SUPPLY_STATUS_DISCHARGING :
-			POWER_SUPPLY_STATUS_UNKNOWN;
+		if (sd->headset_connected) {
+			val->intval = sd->battery_charging ?
+				POWER_SUPPLY_STATUS_CHARGING :
+				POWER_SUPPLY_STATUS_DISCHARGING;
+		} else
+			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
 		break;
 	case POWER_SUPPLY_PROP_SCOPE:
 		val->intval = POWER_SUPPLY_SCOPE_DEVICE;
@@ -514,6 +518,7 @@ static int steelseries_headset_battery_register(struct steelseries_device *sd)
 	/* avoid the warning of 0% battery while waiting for the first info */
 	steelseries_headset_set_wireless_status(sd->hdev, false);
 	sd->battery_capacity = 100;
+	sd->battery_charging = false;
 
 	sd->battery = devm_power_supply_register(&sd->hdev->dev,
 			&sd->battery_desc, &battery_cfg);
@@ -650,6 +655,7 @@ static int steelseries_headset_raw_event(struct hid_device *hdev,
 	struct steelseries_device *sd = hid_get_drvdata(hdev);
 	int capacity = sd->battery_capacity;
 	bool connected = sd->headset_connected;
+	bool charging = sd->battery_charging;
 	unsigned long flags;
 
 	/* Not a headset */
@@ -685,6 +691,7 @@ static int steelseries_headset_raw_event(struct hid_device *hdev,
 
 		if (read_buf[0] == 0xaa && read_buf[1] == 0x01) {
 			connected = true;
+			charging = read_buf[4] == 0x01;
 
 			/*
 			 * Found no official documentation about min and max.
@@ -697,6 +704,7 @@ static int steelseries_headset_raw_event(struct hid_device *hdev,
 			 * there is no known status of the device read_buf[0] == 0x55
 			 */
 			connected = false;
+			charging = false;
 		}
 	}
 
@@ -717,6 +725,15 @@ static int steelseries_headset_raw_event(struct hid_device *hdev,
 		power_supply_changed(sd->battery);
 	}
 
+	if (charging != sd->battery_charging) {
+		hid_dbg(sd->hdev,
+			"Battery charging status changed from %scharging to %scharging\n",
+			sd->battery_charging ? "" : "not ",
+			charging ? "" : "not ");
+		sd->battery_charging = charging;
+		power_supply_changed(sd->battery);
+	}
+
 request_battery:
 	spin_lock_irqsave(&sd->lock, flags);
 	if (!sd->removed)
-- 
2.47.1

[PATCH 3/3] HID: steelseries: export model and manufacturer

[Christian Mayer]

Export model and manufacturer with the power supply properties.
This helps identifing the device in the battery overview.
In the case of the Arctis 9 headset, the manufacturer is prefixed twice in
the device name.

Signed-off-by: Christian Mayer <git@mayer-bgk.de>
---
 drivers/hid/hid-steelseries.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
index d11296bc0e1e..32e559376be4 100644
--- a/drivers/hid/hid-steelseries.c
+++ b/drivers/hid/hid-steelseries.c
@@ -447,6 +447,14 @@ static int steelseries_headset_battery_get_property(struct power_supply *psy,
 	int ret = 0;
 
 	switch (psp) {
+	case POWER_SUPPLY_PROP_MODEL_NAME:
+		val->strval = sd->hdev->name;
+		while (!strncmp(val->strval, "SteelSeries ", 12))
+			val->strval += 12;
+		break;
+	case POWER_SUPPLY_PROP_MANUFACTURER:
+		val->strval = "SteelSeries";
+		break;
 	case POWER_SUPPLY_PROP_PRESENT:
 		val->intval = 1;
 		break;
@@ -490,6 +498,8 @@ steelseries_headset_set_wireless_status(struct hid_device *hdev,
 }
 
 static enum power_supply_property steelseries_headset_battery_props[] = {
+	POWER_SUPPLY_PROP_MODEL_NAME,
+	POWER_SUPPLY_PROP_MANUFACTURER,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_SCOPE,
-- 
2.47.1

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

[Bastien Nocera]

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>");

Re: [PATCH 3/3] HID: steelseries: export model and manufacturer

[Bastien Nocera]

On Wed, 2025-01-01 at 15:11 +0000, Christian Mayer wrote:
> Export model and manufacturer with the power supply properties.
> This helps identifing the device in the battery overview.
> In the case of the Arctis 9 headset, the manufacturer is prefixed
> twice in
> the device name.
> 
> Signed-off-by: Christian Mayer <git@mayer-bgk.de>
> ---
>  drivers/hid/hid-steelseries.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-
> steelseries.c
> index d11296bc0e1e..32e559376be4 100644
> --- a/drivers/hid/hid-steelseries.c
> +++ b/drivers/hid/hid-steelseries.c
> @@ -447,6 +447,14 @@ static int
> steelseries_headset_battery_get_property(struct power_supply *psy,
>  	int ret = 0;
>  
>  	switch (psp) {
> +	case POWER_SUPPLY_PROP_MODEL_NAME:
> +		val->strval = sd->hdev->name;
> +		while (!strncmp(val->strval, "SteelSeries ", 12))
> +			val->strval += 12;

Please use constants instead of magic numbers:

#define STEELSERIES_PREFIX "SteelSeries "
#define STEELSERIES_PREFIX_LEN strlen(STEELSERIES_PREFIX)


> +		break;
> +	case POWER_SUPPLY_PROP_MANUFACTURER:
> +		val->strval = "SteelSeries";
> +		break;
>  	case POWER_SUPPLY_PROP_PRESENT:
>  		val->intval = 1;
>  		break;
> @@ -490,6 +498,8 @@ steelseries_headset_set_wireless_status(struct
> hid_device *hdev,
>  }
>  
>  static enum power_supply_property
> steelseries_headset_battery_props[] = {
> +	POWER_SUPPLY_PROP_MODEL_NAME,
> +	POWER_SUPPLY_PROP_MANUFACTURER,
>  	POWER_SUPPLY_PROP_PRESENT,
>  	POWER_SUPPLY_PROP_STATUS,
>  	POWER_SUPPLY_PROP_SCOPE,

Re: [PATCH 2/3] HID: steelseries: export charging state for the SteelSeries Arctis 9 headset

[Bastien Nocera]

On Wed, 2025-01-01 at 15:11 +0000, Christian Mayer wrote:
> The Arctis 9 headset provides the information if
> the power cable is plugged in and charging via the battery report.
> This information can be exported.
> 
> Signed-off-by: Christian Mayer <git@mayer-bgk.de>

Reviewed-by: Bastien Nocera <hadess@hadess.net>

> ---
>  drivers/hid/hid-steelseries.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-
> steelseries.c
> index f102045a9b6b..d11296bc0e1e 100644
> --- a/drivers/hid/hid-steelseries.c
> +++ b/drivers/hid/hid-steelseries.c
> @@ -33,6 +33,7 @@ struct steelseries_device {
>  	struct power_supply *battery;
>  	uint8_t battery_capacity;
>  	bool headset_connected;
> +	bool battery_charging;
>  };
>  
>  #if IS_BUILTIN(CONFIG_LEDS_CLASS) || \
> @@ -450,9 +451,12 @@ static int
> steelseries_headset_battery_get_property(struct power_supply *psy,
>  		val->intval = 1;
>  		break;
>  	case POWER_SUPPLY_PROP_STATUS:
> -		val->intval = sd->headset_connected ?
> -			POWER_SUPPLY_STATUS_DISCHARGING :
> -			POWER_SUPPLY_STATUS_UNKNOWN;
> +		if (sd->headset_connected) {
> +			val->intval = sd->battery_charging ?
> +				POWER_SUPPLY_STATUS_CHARGING :
> +				POWER_SUPPLY_STATUS_DISCHARGING;
> +		} else
> +			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
>  		break;
>  	case POWER_SUPPLY_PROP_SCOPE:
>  		val->intval = POWER_SUPPLY_SCOPE_DEVICE;
> @@ -514,6 +518,7 @@ static int
> steelseries_headset_battery_register(struct steelseries_device *sd)
>  	/* avoid the warning of 0% battery while waiting for the
> first info */
>  	steelseries_headset_set_wireless_status(sd->hdev, false);
>  	sd->battery_capacity = 100;
> +	sd->battery_charging = false;
>  
>  	sd->battery = devm_power_supply_register(&sd->hdev->dev,
>  			&sd->battery_desc, &battery_cfg);
> @@ -650,6 +655,7 @@ static int steelseries_headset_raw_event(struct
> hid_device *hdev,
>  	struct steelseries_device *sd = hid_get_drvdata(hdev);
>  	int capacity = sd->battery_capacity;
>  	bool connected = sd->headset_connected;
> +	bool charging = sd->battery_charging;
>  	unsigned long flags;
>  
>  	/* Not a headset */
> @@ -685,6 +691,7 @@ static int steelseries_headset_raw_event(struct
> hid_device *hdev,
>  
>  		if (read_buf[0] == 0xaa && read_buf[1] == 0x01) {
>  			connected = true;
> +			charging = read_buf[4] == 0x01;
>  
>  			/*
>  			 * Found no official documentation about min
> and max.
> @@ -697,6 +704,7 @@ static int steelseries_headset_raw_event(struct
> hid_device *hdev,
>  			 * there is no known status of the device
> read_buf[0] == 0x55
>  			 */
>  			connected = false;
> +			charging = false;
>  		}
>  	}
>  
> @@ -717,6 +725,15 @@ static int steelseries_headset_raw_event(struct
> hid_device *hdev,
>  		power_supply_changed(sd->battery);
>  	}
>  
> +	if (charging != sd->battery_charging) {
> +		hid_dbg(sd->hdev,
> +			"Battery charging status changed from
> %scharging to %scharging\n",
> +			sd->battery_charging ? "" : "not ",
> +			charging ? "" : "not ");
> +		sd->battery_charging = charging;
> +		power_supply_changed(sd->battery);
> +	}
> +
>  request_battery:
>  	spin_lock_irqsave(&sd->lock, flags);
>  	if (!sd->removed)

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

[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>

...

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

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

[Christian Mayer]

> 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>");
> 

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

[Christian Mayer]

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

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

[Bastien Nocera]

On Sun, 2025-01-05 at 11:16 +0100, Christian Mayer wrote:
> 
> > On Wed, 2025-01-01 at 15:11 +0000, Christian Mayer wrote:
<snip>
> >   
> > > +	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?

Please send a patch that does it for both headsets, and I'll test it
locally.

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

[Christian Mayer]

Am 09.01.25 um 14:35 schrieb Bastien Nocera:
> On Sun, 2025-01-05 at 11:16 +0100, Christian Mayer wrote:
>>
>>> On Wed, 2025-01-01 at 15:11 +0000, Christian Mayer wrote:
> <snip>
>>>    
>>>> +	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?
> 
> Please send a patch that does it for both headsets, and I'll test it
> locally.
I resent the patches with the requested changes (v2)
Please use 
v2-0001-HID-steelseries-preparation-for-adding-SteelSerie.patch for testing.