diff mbox series

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

Message ID 20250112114438.2196-2-git@mayer-bgk.de (mailing list archive)
State New
Delegated to: Jiri Kosina
Headers show
Series HID: steelseries: add SteelSeries Arctis 9 support | expand

Commit Message

Christian Mayer Jan. 12, 2025, 11:44 a.m. UTC
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(-)

Comments

Christophe JAILLET Jan. 12, 2025, 12:02 p.m. UTC | #1
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);
>   }
>
Bastien Nocera Jan. 13, 2025, 1:42 p.m. UTC | #2
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);
> >   }
> >   
>
Christian Mayer Jan. 15, 2025, 5:20 p.m. UTC | #3
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);
>>>    }
>>>    
>>
>
Christophe JAILLET Jan. 16, 2025, 5:38 p.m. UTC | #4
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);
>>>>    }
>>>
>>
> 
> 
>
diff mbox series

Patch

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