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

[Christophe JAILLET <christophe.jaillet@wanadoo.fr>]

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