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