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

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

[Christian Mayer]

Hi,

i added support for the SteelSeries Arctis 9 headset.

Changes in v2:
* Use constants instead of magic numbers for cleaning up model name.
* Remove unnecessary whitespace changes.
* Split up preparations and actual adding suport for the device 
in separate patches.
* Call hid_hw_open/hid_hw_close for all devices
* Fix code style issues
* Optimize capacity mapping for min and max values

Christian Mayer (5):
  HID: steelseries: preparation for adding SteelSeries Arctis 9 support
  HID: steelseries: add SteelSeries Arctis 9 support
  HID: steelseries: export charging state for the SteelSeries Arctis 9
    headset
  HID: steelseries: export model and manufacturer
  HID: steelseries: remove unnecessary return

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

Christian

[PATCH v2 1/5] HID: steelseries: preparation for adding SteelSeries Arctis 9 support

[Christian Mayer]

Refactor code and add calls to hid_hw_open/hid_hw_closed in preparation
for adding support for the SteelSeries Arctis 9 headset.

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

diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
index f9ff5be94309..dc4ab55d7c22 100644
--- a/drivers/hid/hid-steelseries.c
+++ b/drivers/hid/hid-steelseries.c
@@ -377,20 +377,21 @@ static void steelseries_srws1_remove(struct hid_device *hdev)
 #define ARCTIS_1_BATTERY_RESPONSE_LEN		8
 static const char arctis_1_battery_request[] = { 0x06, 0x12 };
 
-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;
 	}
@@ -404,7 +405,8 @@ 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));
 
 	if (ret < 0)
 		hid_dbg(hdev,
@@ -554,6 +556,10 @@ static int steelseries_probe(struct hid_device *hdev, const struct hid_device_id
 	if (ret)
 		return ret;
 
+	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");
@@ -580,6 +586,7 @@ static void steelseries_remove(struct hid_device *hdev)
 
 	cancel_delayed_work_sync(&sd->battery_work);
 
+	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
 }
 
-- 
2.47.1

[PATCH v2 2/5] 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 | 64 +++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
index dc4ab55d7c22..2b98db7f8911 100644
--- a/drivers/hid/hid-steelseries.c
+++ b/drivers/hid/hid-steelseries.c
@@ -19,6 +19,7 @@
 
 #define STEELSERIES_SRWS1		BIT(0)
 #define STEELSERIES_ARCTIS_1		BIT(1)
+#define STEELSERIES_ARCTIS_9		BIT(2)
 
 struct steelseries_device {
 	struct hid_device *hdev;
@@ -375,7 +376,9 @@ static void steelseries_srws1_remove(struct hid_device *hdev)
 #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_request_battery(struct hid_device *hdev,
 	const char *request, size_t len)
@@ -395,6 +398,7 @@ static int steelseries_headset_request_battery(struct hid_device *hdev,
 		hid_err(hdev, "hid_hw_raw_request() failed with %d\n", ret);
 		ret = -ENODATA;
 	}
+
 	kfree(write_buf);
 	return ret;
 }
@@ -407,6 +411,9 @@ static void steelseries_headset_fetch_battery(struct hid_device *hdev)
 	if (sd->quirks & STEELSERIES_ARCTIS_1)
 		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,
@@ -522,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 usage_page)
+{
+	return hdev->rdesc[0] == 0x06 &&
+		hdev->rdesc[1] == usage_page &&
+		hdev->rdesc[2] == 0xff;
+}
+
 static int steelseries_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	struct steelseries_device *sd;
@@ -550,6 +570,10 @@ 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);
@@ -606,6 +630,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)
@@ -637,6 +670,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",
@@ -672,6 +731,10 @@ static const struct hid_device_id steelseries_devices[] = {
 	  HID_USB_DEVICE(USB_VENDOR_ID_STEELSERIES, 0x12b6),
 	.driver_data = STEELSERIES_ARCTIS_1 },
 
+	{ /* SteelSeries Arctis 9 Wireless for XBox */
+	  HID_USB_DEVICE(USB_VENDOR_ID_STEELSERIES, 0x12c2),
+	  .driver_data = STEELSERIES_ARCTIS_9 },
+
 	{ }
 };
 MODULE_DEVICE_TABLE(hid, steelseries_devices);
@@ -690,3 +753,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 v2 3/5] 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 2b98db7f8911..2ee1a6f01852 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);
@@ -646,6 +651,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 */
@@ -681,6 +687,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.
@@ -693,6 +700,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;
 		}
 	}
 
@@ -713,6 +721,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 v2 4/5] 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 | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
index 2ee1a6f01852..1a935802880a 100644
--- a/drivers/hid/hid-steelseries.c
+++ b/drivers/hid/hid-steelseries.c
@@ -439,6 +439,9 @@ static void steelseries_headset_battery_timer_tick(struct work_struct *work)
 	steelseries_headset_fetch_battery(hdev);
 }
 
+#define STEELSERIES_PREFIX "SteelSeries "
+#define STEELSERIES_PREFIX_LEN strlen(STEELSERIES_PREFIX)
+
 static int steelseries_headset_battery_get_property(struct power_supply *psy,
 				enum power_supply_property psp,
 				union power_supply_propval *val)
@@ -447,6 +450,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_PREFIX, STEELSERIES_PREFIX_LEN))
+			val->strval += STEELSERIES_PREFIX_LEN;
+		break;
+	case POWER_SUPPLY_PROP_MANUFACTURER:
+		val->strval = "SteelSeries";
+		break;
 	case POWER_SUPPLY_PROP_PRESENT:
 		val->intval = 1;
 		break;
@@ -490,6 +501,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

[PATCH v2 5/5] HID: steelseries: remove unnecessary return

[Christian Mayer]

Remove unnecessary return in a void function.

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

diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
index 1a935802880a..d4bd7848b8c6 100644
--- a/drivers/hid/hid-steelseries.c
+++ b/drivers/hid/hid-steelseries.c
@@ -370,7 +370,6 @@ static void steelseries_srws1_remove(struct hid_device *hdev)
 
 	hid_hw_stop(hdev);
 	kfree(drv_data);
-	return;
 }
 #endif
 
-- 
2.47.1

Re: [PATCH v2 1/5] HID: steelseries: preparation for adding SteelSeries Arctis 9 support

[Christophe JAILLET]

Le 12/01/2025 à 12:44, Christian Mayer a écrit :
> Refactor code and add calls to hid_hw_open/hid_hw_closed in preparation
> for adding support for the SteelSeries Arctis 9 headset.
> 
> Signed-off-by: Christian Mayer <git@mayer-bgk.de>
> ---
>   drivers/hid/hid-steelseries.c | 19 +++++++++++++------
>   1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
> index f9ff5be94309..dc4ab55d7c22 100644
> --- a/drivers/hid/hid-steelseries.c
> +++ b/drivers/hid/hid-steelseries.c
> @@ -377,20 +377,21 @@ static void steelseries_srws1_remove(struct hid_device *hdev)
>   #define ARCTIS_1_BATTERY_RESPONSE_LEN		8
>   static const char arctis_1_battery_request[] = { 0x06, 0x12 };
>   
> -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;
>   	}
> @@ -404,7 +405,8 @@ 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));
>   
>   	if (ret < 0)
>   		hid_dbg(hdev,
> @@ -554,6 +556,10 @@ static int steelseries_probe(struct hid_device *hdev, const struct hid_device_id
>   	if (ret)
>   		return ret;
>   
> +	ret = hid_hw_open(hdev);
> +	if (ret)
> +		return ret;

Should we call hid_hw_stop() if steelseries_headset_battery_register() 
below fails, as done in the remove funcion?

> +
>   	if (steelseries_headset_battery_register(sd) < 0)
>   		hid_err(sd->hdev,
>   			"Failed to register battery for headset\n");
> @@ -580,6 +586,7 @@ static void steelseries_remove(struct hid_device *hdev)
>   
>   	cancel_delayed_work_sync(&sd->battery_work);
>   
> +	hid_hw_close(hdev);
>   	hid_hw_stop(hdev);
>   }
>   

Re: [PATCH v2 1/5] HID: steelseries: preparation for adding SteelSeries Arctis 9 support

[Bastien Nocera]

On Sun, 2025-01-12 at 13:02 +0100, Christophe JAILLET wrote:
> Le 12/01/2025 à 12:44, Christian Mayer a écrit :
> > Refactor code and add calls to hid_hw_open/hid_hw_closed in
> > preparation
> > for adding support for the SteelSeries Arctis 9 headset.
> > 
> > Signed-off-by: Christian Mayer <git@mayer-bgk.de>

Feel free to add my:
Reviewed-by: Bastien Nocera <hadess@hadess.net>
Tested-by: Bastien Nocera <hadess@hadess.net>

once you've made the change Christophe mentions.

> > ---
> >   drivers/hid/hid-steelseries.c | 19 +++++++++++++------
> >   1 file changed, 13 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-
> > steelseries.c
> > index f9ff5be94309..dc4ab55d7c22 100644
> > --- a/drivers/hid/hid-steelseries.c
> > +++ b/drivers/hid/hid-steelseries.c
> > @@ -377,20 +377,21 @@ static void steelseries_srws1_remove(struct
> > hid_device *hdev)
> >   #define ARCTIS_1_BATTERY_RESPONSE_LEN		8
> >   static const char arctis_1_battery_request[] = { 0x06, 0x12 };
> >   
> > -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;
> >   	}
> > @@ -404,7 +405,8 @@ 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));
> >   
> >   	if (ret < 0)
> >   		hid_dbg(hdev,
> > @@ -554,6 +556,10 @@ static int steelseries_probe(struct hid_device
> > *hdev, const struct hid_device_id
> >   	if (ret)
> >   		return ret;
> >   
> > +	ret = hid_hw_open(hdev);
> > +	if (ret)
> > +		return ret;
> 
> Should we call hid_hw_stop() if
> steelseries_headset_battery_register() 
> below fails, as done in the remove funcion?
> 
> > +
> >   	if (steelseries_headset_battery_register(sd) < 0)
> >   		hid_err(sd->hdev,
> >   			"Failed to register battery for
> > headset\n");
> > @@ -580,6 +586,7 @@ static void steelseries_remove(struct
> > hid_device *hdev)
> >   
> >   	cancel_delayed_work_sync(&sd->battery_work);
> >   
> > +	hid_hw_close(hdev);
> >   	hid_hw_stop(hdev);
> >   }
> >   
> 

Re: [PATCH v2 2/5] HID: steelseries: add SteelSeries Arctis 9 support

[Bastien Nocera]

On Sun, 2025-01-12 at 11:44 +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>

Feel free to add my signoffs once you've remove the one unnecessary
whitespace change.
Reviewed-by: Bastien Nocera <hadess@hadess.net>
Tested-by: Bastien Nocera <hadess@hadess.net>

> ---
>  drivers/hid/hid-steelseries.c | 64
> +++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-
> steelseries.c
> index dc4ab55d7c22..2b98db7f8911 100644
> --- a/drivers/hid/hid-steelseries.c
> +++ b/drivers/hid/hid-steelseries.c
> @@ -19,6 +19,7 @@
>  
>  #define STEELSERIES_SRWS1		BIT(0)
>  #define STEELSERIES_ARCTIS_1		BIT(1)
> +#define STEELSERIES_ARCTIS_9		BIT(2)
>  
>  struct steelseries_device {
>  	struct hid_device *hdev;
> @@ -375,7 +376,9 @@ static void steelseries_srws1_remove(struct
> hid_device *hdev)
>  #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_request_battery(struct hid_device
> *hdev,
>  	const char *request, size_t len)
> @@ -395,6 +398,7 @@ static int
> steelseries_headset_request_battery(struct hid_device *hdev,
>  		hid_err(hdev, "hid_hw_raw_request() failed with
> %d\n", ret);
>  		ret = -ENODATA;
>  	}
> +
>  	kfree(write_buf);
>  	return ret;
>  }

Whitespace change.

> @@ -407,6 +411,9 @@ static void
> steelseries_headset_fetch_battery(struct hid_device *hdev)
>  	if (sd->quirks & STEELSERIES_ARCTIS_1)
>  		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,
> @@ -522,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 usage_page)
> +{
> +	return hdev->rdesc[0] == 0x06 &&
> +		hdev->rdesc[1] == usage_page &&
> +		hdev->rdesc[2] == 0xff;
> +}
> +
>  static int steelseries_probe(struct hid_device *hdev, const struct
> hid_device_id *id)
>  {
>  	struct steelseries_device *sd;
> @@ -550,6 +570,10 @@ 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);
> @@ -606,6 +630,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)
> @@ -637,6 +670,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",
> @@ -672,6 +731,10 @@ static const struct hid_device_id
> steelseries_devices[] = {
>  	  HID_USB_DEVICE(USB_VENDOR_ID_STEELSERIES, 0x12b6),
>  	.driver_data = STEELSERIES_ARCTIS_1 },
>  
> +	{ /* SteelSeries Arctis 9 Wireless for XBox */
> +	  HID_USB_DEVICE(USB_VENDOR_ID_STEELSERIES, 0x12c2),
> +	  .driver_data = STEELSERIES_ARCTIS_9 },
> +
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(hid, steelseries_devices);
> @@ -690,3 +753,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 v2 3/5] HID: steelseries: export charging state for the SteelSeries Arctis 9 headset

[Bastien Nocera]

On Sun, 2025-01-12 at 11:44 +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>
> ---
>  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 2b98db7f8911..2ee1a6f01852 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);
> @@ -646,6 +651,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 */
> @@ -681,6 +687,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;

I would prefer parenthesis around that, but feel free to ignore if it's
in the kernel coding style.

Feel free to add those signoffs once that's either fixed or verified:
Reviewed-by: Bastien Nocera <hadess@hadess.net>
Tested-by: Bastien Nocera <hadess@hadess.net>

>  
>  			/*
>  			 * Found no official documentation about min
> and max.
> @@ -693,6 +700,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;
>  		}
>  	}
>  
> @@ -713,6 +721,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 v2 4/5] HID: steelseries: export model and manufacturer

[Bastien Nocera]

On Sun, 2025-01-12 at 11:44 +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>

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

> ---
>  drivers/hid/hid-steelseries.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-
> steelseries.c
> index 2ee1a6f01852..1a935802880a 100644
> --- a/drivers/hid/hid-steelseries.c
> +++ b/drivers/hid/hid-steelseries.c
> @@ -439,6 +439,9 @@ static void
> steelseries_headset_battery_timer_tick(struct work_struct *work)
>  	steelseries_headset_fetch_battery(hdev);
>  }
>  
> +#define STEELSERIES_PREFIX "SteelSeries "
> +#define STEELSERIES_PREFIX_LEN strlen(STEELSERIES_PREFIX)
> +
>  static int steelseries_headset_battery_get_property(struct
> power_supply *psy,
>  				enum power_supply_property psp,
>  				union power_supply_propval *val)
> @@ -447,6 +450,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_PREFIX,
> STEELSERIES_PREFIX_LEN))
> +			val->strval += STEELSERIES_PREFIX_LEN;
> +		break;
> +	case POWER_SUPPLY_PROP_MANUFACTURER:
> +		val->strval = "SteelSeries";
> +		break;
>  	case POWER_SUPPLY_PROP_PRESENT:
>  		val->intval = 1;
>  		break;
> @@ -490,6 +501,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 v2 5/5] HID: steelseries: remove unnecessary return

[Bastien Nocera]

On Sun, 2025-01-12 at 11:44 +0000, Christian Mayer wrote:
> Remove unnecessary return in a void function.
> 
> Signed-off-by: Christian Mayer <git@mayer-bgk.de>

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

> ---
>  drivers/hid/hid-steelseries.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-
> steelseries.c
> index 1a935802880a..d4bd7848b8c6 100644
> --- a/drivers/hid/hid-steelseries.c
> +++ b/drivers/hid/hid-steelseries.c
> @@ -370,7 +370,6 @@ static void steelseries_srws1_remove(struct
> hid_device *hdev)
>  
>   hid_hw_stop(hdev);
>   kfree(drv_data);
> - return;
>  }
>  #endif
>  

Re: [PATCH v2 1/5] HID: steelseries: preparation for adding SteelSeries Arctis 9 support

[Christian Mayer]

Am 13.01.25 um 14:42 schrieb Bastien Nocera:
> On Sun, 2025-01-12 at 13:02 +0100, Christophe JAILLET wrote:
>> Le 12/01/2025 à 12:44, Christian Mayer a écrit :
>>> Refactor code and add calls to hid_hw_open/hid_hw_closed in
>>> preparation
>>> for adding support for the SteelSeries Arctis 9 headset.
>>>
>>> Signed-off-by: Christian Mayer <git@mayer-bgk.de>
> 
> Feel free to add my:
> Reviewed-by: Bastien Nocera <hadess@hadess.net>
> Tested-by: Bastien Nocera <hadess@hadess.net>
> 
> once you've made the change Christophe mentions.
> 
>>> ---
>>>    drivers/hid/hid-steelseries.c | 19 +++++++++++++------
>>>    1 file changed, 13 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-
>>> steelseries.c
>>> index f9ff5be94309..dc4ab55d7c22 100644
>>> --- a/drivers/hid/hid-steelseries.c
>>> +++ b/drivers/hid/hid-steelseries.c
>>> @@ -377,20 +377,21 @@ static void steelseries_srws1_remove(struct
>>> hid_device *hdev)
>>>    #define ARCTIS_1_BATTERY_RESPONSE_LEN		8
>>>    static const char arctis_1_battery_request[] = { 0x06, 0x12 };
>>>    
>>> -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;
>>>    	}
>>> @@ -404,7 +405,8 @@ 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));
>>>    
>>>    	if (ret < 0)
>>>    		hid_dbg(hdev,
>>> @@ -554,6 +556,10 @@ static int steelseries_probe(struct hid_device
>>> *hdev, const struct hid_device_id
>>>    	if (ret)
>>>    		return ret;
>>>    
>>> +	ret = hid_hw_open(hdev);
>>> +	if (ret)
>>> +		return ret;
>>
>> Should we call hid_hw_stop() if
>> steelseries_headset_battery_register()
>> below fails, as done in the remove funcion?
Did you mean hid_hw_close instead of hid_hw_stop?
If the battery_register fails, the driver will still get assigned to the 
device, because ret is not getting set and is still 0 from hid_hw_open, 
right?

hid_hw_close doesn't get called before hid_hw_stop and the remove 
function will get called when the device disconnects.
That doesn't seem right to me, or am i wrong?

>>
>>> +
>>>    	if (steelseries_headset_battery_register(sd) < 0)
>>>    		hid_err(sd->hdev,
>>>    			"Failed to register battery for
>>> headset\n");
>>> @@ -580,6 +586,7 @@ static void steelseries_remove(struct
>>> hid_device *hdev)
>>>    
>>>    	cancel_delayed_work_sync(&sd->battery_work);
>>>    
>>> +	hid_hw_close(hdev);
>>>    	hid_hw_stop(hdev);
>>>    }
>>>    
>>
> 

Re: [PATCH v2 0/5] HID: steelseries: add SteelSeries Arctis 9 support

[Jiri Kosina]

On Sun, 12 Jan 2025, Christian Mayer wrote:

> Hi,
> 
> i added support for the SteelSeries Arctis 9 headset.
> 
> Changes in v2:
> * Use constants instead of magic numbers for cleaning up model name.
> * Remove unnecessary whitespace changes.
> * Split up preparations and actual adding suport for the device 
> in separate patches.
> * Call hid_hw_open/hid_hw_close for all devices
> * Fix code style issues
> * Optimize capacity mapping for min and max values
> 
> Christian Mayer (5):
>   HID: steelseries: preparation for adding SteelSeries Arctis 9 support
>   HID: steelseries: add SteelSeries Arctis 9 support
>   HID: steelseries: export charging state for the SteelSeries Arctis 9
>     headset
>   HID: steelseries: export model and manufacturer
>   HID: steelseries: remove unnecessary return
> 
>  drivers/hid/hid-steelseries.c | 120 +++++++++++++++++++++++++++++++---
>  1 file changed, 110 insertions(+), 10 deletions(-)

Now in hid.git#for-6.14/steelseries. Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 1/5] HID: steelseries: preparation for adding SteelSeries Arctis 9 support

[Christophe JAILLET]

Le 15/01/2025 à 18:20, Christian Mayer a écrit :
> 
> 
> Am 13.01.25 um 14:42 schrieb Bastien Nocera:
>> On Sun, 2025-01-12 at 13:02 +0100, Christophe JAILLET wrote:
>>> Le 12/01/2025 à 12:44, Christian Mayer a écrit :
>>>> Refactor code and add calls to hid_hw_open/hid_hw_closed in
>>>> preparation
>>>> for adding support for the SteelSeries Arctis 9 headset.
>>>>
>>>> Signed-off-by: Christian Mayer <git@mayer-bgk.de>
>>
>> Feel free to add my:
>> Reviewed-by: Bastien Nocera <hadess@hadess.net>
>> Tested-by: Bastien Nocera <hadess@hadess.net>
>>
>> once you've made the change Christophe mentions.
>>
>>>> ---
>>>>    drivers/hid/hid-steelseries.c | 19 +++++++++++++------
>>>>    1 file changed, 13 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-
>>>> steelseries.c
>>>> index f9ff5be94309..dc4ab55d7c22 100644
>>>> --- a/drivers/hid/hid-steelseries.c
>>>> +++ b/drivers/hid/hid-steelseries.c
>>>> @@ -377,20 +377,21 @@ static void steelseries_srws1_remove(struct
>>>> hid_device *hdev)
>>>>    #define ARCTIS_1_BATTERY_RESPONSE_LEN        8
>>>>    static const char arctis_1_battery_request[] = { 0x06, 0x12 };
>>>> -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;
>>>>        }
>>>> @@ -404,7 +405,8 @@ 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));
>>>>        if (ret < 0)
>>>>            hid_dbg(hdev,
>>>> @@ -554,6 +556,10 @@ static int steelseries_probe(struct hid_device
>>>> *hdev, const struct hid_device_id
>>>>        if (ret)
>>>>            return ret;
>>>> +    ret = hid_hw_open(hdev);
>>>> +    if (ret)
>>>> +        return ret;
>>>
>>> Should we call hid_hw_stop() if
>>> steelseries_headset_battery_register()
>>> below fails, as done in the remove funcion?
> Did you mean hid_hw_close instead of hid_hw_stop?

Yes, I mean hid_hw_close()

> If the battery_register fails, the driver will still get assigned to the 
> device, because ret is not getting set and is still 0 from hid_hw_open, 
> right?

Correct. I miss-read this part.
If steelseries_headset_battery_register() fails, the probe still succeeds.

> 
> hid_hw_close doesn't get called before hid_hw_stop and the remove 
> function will get called when the device disconnects.
> That doesn't seem right to me, or am i wrong?

I think you are right.

Not sure it is 100% future proof, but it looks correct.

Sorry for the noise.

CJ

> 
>>>
>>>> +
>>>>        if (steelseries_headset_battery_register(sd) < 0)
>>>>            hid_err(sd->hdev,
>>>>                "Failed to register battery for
>>>> headset\n");
>>>> @@ -580,6 +586,7 @@ static void steelseries_remove(struct
>>>> hid_device *hdev)
>>>>        cancel_delayed_work_sync(&sd->battery_work);
>>>> +    hid_hw_close(hdev);
>>>>        hid_hw_stop(hdev);
>>>>    }
>>>
>>
> 
> 
>