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@xxxxxxxxxxxx>
>
> Feel free to add my:
> Reviewed-by: Bastien Nocera <hadess@xxxxxxxxxx>
> Tested-by: Bastien Nocera <hadess@xxxxxxxxxx>
>
> 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);
> > >    }
> > >