| Message ID | 20250112114438.2196-1-git@mayer-bgk.de |
|---|---|
| Headers | show |
| Series | HID: steelseries: add SteelSeries Arctis 9 support | expand |
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); > > } > > >
On Sun, 2025-01-12 at 11:44 +0000, Christian Mayer wrote: > The Arctis 9 headset provides the information if > the power cable is plugged in and charging via the battery report. > This information can be exported. > > Signed-off-by: Christian Mayer <git@mayer-bgk.de> > --- > drivers/hid/hid-steelseries.c | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) > > diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid- > steelseries.c > index 2b98db7f8911..2ee1a6f01852 100644 > --- a/drivers/hid/hid-steelseries.c > +++ b/drivers/hid/hid-steelseries.c > @@ -33,6 +33,7 @@ struct steelseries_device { > struct power_supply *battery; > uint8_t battery_capacity; > bool headset_connected; > + bool battery_charging; > }; > > #if IS_BUILTIN(CONFIG_LEDS_CLASS) || \ > @@ -450,9 +451,12 @@ static int > steelseries_headset_battery_get_property(struct power_supply *psy, > val->intval = 1; > break; > case POWER_SUPPLY_PROP_STATUS: > - val->intval = sd->headset_connected ? > - POWER_SUPPLY_STATUS_DISCHARGING : > - POWER_SUPPLY_STATUS_UNKNOWN; > + if (sd->headset_connected) { > + val->intval = sd->battery_charging ? > + POWER_SUPPLY_STATUS_CHARGING : > + POWER_SUPPLY_STATUS_DISCHARGING; > + } else > + val->intval = POWER_SUPPLY_STATUS_UNKNOWN; > break; > case POWER_SUPPLY_PROP_SCOPE: > val->intval = POWER_SUPPLY_SCOPE_DEVICE; > @@ -514,6 +518,7 @@ static int > steelseries_headset_battery_register(struct steelseries_device *sd) > /* avoid the warning of 0% battery while waiting for the > first info */ > steelseries_headset_set_wireless_status(sd->hdev, false); > sd->battery_capacity = 100; > + sd->battery_charging = false; > > sd->battery = devm_power_supply_register(&sd->hdev->dev, > &sd->battery_desc, &battery_cfg); > @@ -646,6 +651,7 @@ static int steelseries_headset_raw_event(struct > hid_device *hdev, > struct steelseries_device *sd = hid_get_drvdata(hdev); > int capacity = sd->battery_capacity; > bool connected = sd->headset_connected; > + bool charging = sd->battery_charging; > unsigned long flags; > > /* Not a headset */ > @@ -681,6 +687,7 @@ static int steelseries_headset_raw_event(struct > hid_device *hdev, > > if (read_buf[0] == 0xaa && read_buf[1] == 0x01) { > connected = true; > + charging = read_buf[4] == 0x01; I would prefer parenthesis around that, but feel free to ignore if it's in the kernel coding style. Feel free to add those signoffs once that's either fixed or verified: Reviewed-by: Bastien Nocera <hadess@hadess.net> Tested-by: Bastien Nocera <hadess@hadess.net> > > /* > * Found no official documentation about min > and max. > @@ -693,6 +700,7 @@ static int steelseries_headset_raw_event(struct > hid_device *hdev, > * there is no known status of the device > read_buf[0] == 0x55 > */ > connected = false; > + charging = false; > } > } > > @@ -713,6 +721,15 @@ static int steelseries_headset_raw_event(struct > hid_device *hdev, > power_supply_changed(sd->battery); > } > > + if (charging != sd->battery_charging) { > + hid_dbg(sd->hdev, > + "Battery charging status changed from > %scharging to %scharging\n", > + sd->battery_charging ? "" : "not ", > + charging ? "" : "not "); > + sd->battery_charging = charging; > + power_supply_changed(sd->battery); > + } > + > request_battery: > spin_lock_irqsave(&sd->lock, flags); > if (!sd->removed)
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); >>> } >>> >> >
Hi, i added support for the SteelSeries Arctis 9 headset. Changes in v2: * Use constants instead of magic numbers for cleaning up model name. * Remove unnecessary whitespace changes. * Split up preparations and actual adding suport for the device in separate patches. * Call hid_hw_open/hid_hw_close for all devices * Fix code style issues * Optimize capacity mapping for min and max values Christian Mayer (5): HID: steelseries: preparation for adding SteelSeries Arctis 9 support HID: steelseries: add SteelSeries Arctis 9 support HID: steelseries: export charging state for the SteelSeries Arctis 9 headset HID: steelseries: export model and manufacturer HID: steelseries: remove unnecessary return drivers/hid/hid-steelseries.c | 120 +++++++++++++++++++++++++++++++--- 1 file changed, 110 insertions(+), 10 deletions(-) Christian