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

[Bastien Nocera <hadess@hadess.net>]

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