Re: [PATCH v3] HID: corsair-void: Add Corsair Void headset family driver
From: Christophe JAILLET
Date: Wed Aug 21 2024 - 04:19:01 EST
Le 20/08/2024 à 02:22, Stuart Hayhurst a écrit :
Introduce a driver for the Corsair Void family of headsets, supporting:
- Battery reporting (power_supply)
- Sidetone setting support
- Physical microphone location reporting
- Headset and receiver firmware version reporting
- Built-in alert triggering
- USB wireless_status
Tested with a Void Pro Wireless, Void Elite Wireless and a Void Elite Wired
Signed-off-by: Stuart Hayhurst <stuart.a.hayhurst@xxxxxxxxx>
---
Hi,
...
+struct corsair_void_drvdata {
+ struct hid_device *hid_dev;
+ struct device *dev;
+
+ char *name;
+ bool is_wired;
Maybe is_wired could be close to other bools below, to improve avoid
holes in the structure?
+ unsigned int sidetone_max;
+
+ struct corsair_void_battery_data battery_data;
+ bool mic_up;
+ bool connected;
+ int fw_receiver_major;
+ int fw_receiver_minor;
+ int fw_headset_major;
+ int fw_headset_minor;
+
+ struct power_supply *battery;
+ struct power_supply_desc battery_desc;
+ struct mutex battery_mutex;
+
+ struct delayed_work delayed_status_work;
+ struct delayed_work delayed_firmware_work;
+ struct work_struct battery_remove_work;
+ struct work_struct battery_add_work;
+};
...
+static ssize_t sidetone_max_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct corsair_void_drvdata *drvdata = dev_get_drvdata(dev);
Missing newline.
+ return sysfs_emit(buf, "%d\n", drvdata->sidetone_max);
+}
...
+static ssize_t send_alert_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct corsair_void_drvdata *drvdata = dev_get_drvdata(dev);
+ struct hid_device *hid_dev = drvdata->hid_dev;
+ unsigned char alert_id;
+ unsigned char *send_buf __free(kfree) = NULL;
+ int ret;
+
+ if (!drvdata->connected || drvdata->is_wired)
+ return -ENODEV;
+
+ /* Only accept 0 or 1 for alert ID */
+ if (kstrtou8(buf, 10, &alert_id) || alert_id >= 2)
+ return -EINVAL;
+
+ send_buf = kmalloc(3, GFP_KERNEL);
+ if (!send_buf)
+ return -ENOMEM;
+
+ /* Packet format to send alert with ID alert_id */
+ send_buf[0] = CORSAIR_VOID_NOTIF_REQUEST_ID;
+ send_buf[1] = 0x02;
+ send_buf[2] = alert_id;
+
+ ret = hid_hw_raw_request(hid_dev, CORSAIR_VOID_NOTIF_REQUEST_ID,
+ send_buf, 3, HID_OUTPUT_REPORT,
+ HID_REQ_SET_REPORT);
+ if (ret < 0) {
+ hid_warn(hid_dev, "failed to send alert request (reason: %d)",
+ ret);
+ } else {
+ ret = count;
+ }
+
I'm not familiar with the hid_hw_raw_request() API, but I think that a
kfree(send_buf) is missing here.
+ return ret;
+}
...
+static int corsair_void_set_sidetone_wireless(struct device *dev, const char *buf,
+ unsigned char sidetone)
+{
+ struct corsair_void_drvdata *drvdata = dev_get_drvdata(dev);
+ struct hid_device *hid_dev = drvdata->hid_dev;
+ unsigned char *send_buf __free(kfree) = NULL;
+ int ret = 0;
Nitpick: No need to init.
+
+ send_buf = kmalloc(12, GFP_KERNEL);
+ if (!send_buf)
+ return -ENOMEM;
+
+ /* Packet format to set sidetone for wireless headsets */
+ send_buf[0] = CORSAIR_VOID_SIDETONE_REQUEST_ID;
+ send_buf[1] = 0x0B;
+ send_buf[2] = 0x00;
+ send_buf[3] = 0xFF;
+ send_buf[4] = 0x04;
+ send_buf[5] = 0x0E;
+ send_buf[6] = 0xFF;
+ send_buf[7] = 0x05;
+ send_buf[8] = 0x01;
+ send_buf[9] = 0x04;
+ send_buf[10] = 0x00;
+ send_buf[11] = sidetone + 200;
+
+ ret = hid_hw_raw_request(hid_dev, CORSAIR_VOID_SIDETONE_REQUEST_ID,
+ send_buf, 12, HID_FEATURE_REPORT,
+ HID_REQ_SET_REPORT);
Missing kfree(send_buf)?
+
+ return ret;
+}
...
+static int corsair_void_request_status(struct hid_device *hid_dev, int id)
+{
+ unsigned char *send_buf __free(kfree) = NULL;
+ int ret;
+
+ send_buf = kmalloc(2, GFP_KERNEL);
+ if (!send_buf)
+ return -ENOMEM;
+
+ /* Packet format to request data item (status / firmware) refresh */
+ send_buf[0] = CORSAIR_VOID_STATUS_REQUEST_ID;
+ send_buf[1] = id;
+
+ /* Send request for data refresh */
+ ret = hid_hw_raw_request(hid_dev, CORSAIR_VOID_STATUS_REQUEST_ID,
+ send_buf, 2, HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
+ if (ret < 0) {
+ switch (id) {
+ case CORSAIR_VOID_STATUS_REPORT_ID:
+ hid_warn(hid_dev, "failed to request battery (reason: %d)",
+ ret);
+ break;
+ case CORSAIR_VOID_FIRMWARE_REPORT_ID:
+ hid_warn(hid_dev, "failed to request firmware (reason: %d)",
+ ret);
+ break;
+ default:
+ hid_warn(hid_dev, "failed to send report %d (reason: %d)",
+ id, ret);
+ break;
+ }
+ } else {
+ ret = 0;
You could save 2 lines if ret was initialized when declared.
+ }
+
Missing kfree(send_buf)?
+ return ret;
+}
...
+static int corsair_void_probe(struct hid_device *hid_dev,
+ const struct hid_device_id *hid_id)
+{
+ int ret;
+ struct corsair_void_drvdata *drvdata;
+ char *name;
+ int name_length;
+
+ if (!hid_is_usb(hid_dev))
+ return -EINVAL;
+
+ drvdata = devm_kzalloc(&hid_dev->dev, sizeof(*drvdata),
+ GFP_KERNEL);
+ if (!drvdata)
+ return -ENOMEM;
+
+ hid_set_drvdata(hid_dev, drvdata);
+ dev_set_drvdata(&hid_dev->dev, drvdata);
+
+ drvdata->dev = &hid_dev->dev;
+ drvdata->hid_dev = hid_dev;
+ drvdata->is_wired = hid_id->driver_data == CORSAIR_VOID_WIRED;
+
+ drvdata->sidetone_max = CORSAIR_VOID_SIDETONE_MAX_WIRELESS;
+ if (drvdata->is_wired)
+ drvdata->sidetone_max = CORSAIR_VOID_SIDETONE_MAX_WIRED;
+
+ /* Set initial values for no wireless headset attached */
+ /* If a headset is attached, it'll be prompted later */
+ corsair_void_set_unknown_wireless_data(drvdata);
+ corsair_void_set_unknown_batt(drvdata);
+
+ /* Receiver version won't be reset after init */
+ /* Headset version already set via set_unknown_wireless_data */
+ drvdata->fw_receiver_major = 0;
+ drvdata->fw_receiver_minor = 0;
+
+ ret = hid_parse(hid_dev);
+ if (ret) {
+ hid_err(hid_dev, "parse failed (reason: %d)\n", ret);
+ return ret;
+ }
+
+ name_length = snprintf(NULL, 0, "corsair-void-%d-battery", hid_dev->id);
+ name = devm_kzalloc(drvdata->dev, name_length + 1, GFP_KERNEL);
+ if (!name)
+ return -ENOMEM;
+ snprintf(name, name_length + 1, "corsair-void-%d-battery", hid_dev->id);
devm_kasprintf() would simplify this.
+
+ drvdata->battery_desc.name = name;
+ drvdata->battery_desc.type = POWER_SUPPLY_TYPE_BATTERY;
+ drvdata->battery_desc.properties = corsair_void_battery_props;
+ drvdata->battery_desc.num_properties = ARRAY_SIZE(corsair_void_battery_props);
+ drvdata->battery_desc.get_property = corsair_void_battery_get_property;
+
+ drvdata->battery = NULL;
+ INIT_WORK(&drvdata->battery_remove_work,
+ corsair_void_battery_remove_work_handler);
+ INIT_WORK(&drvdata->battery_add_work,
+ corsair_void_battery_add_work_handler);
+ ret = devm_mutex_init(drvdata->dev, &drvdata->battery_mutex);
+ if (ret)
+ return ret;
+
+ ret = sysfs_create_group(&hid_dev->dev.kobj, &corsair_void_attr_group);
+ if (ret)
+ return ret;
+
+ /* Any failures after here will need to call hid_hw_stop */
+ ret = hid_hw_start(hid_dev, HID_CONNECT_DEFAULT);
+ if (ret) {
+ hid_err(hid_dev, "hid_hw_start failed (reason: %d)\n", ret);
+ goto failed_after_sysfs;
+ }
+
+ /* Refresh battery data, in case wireless headset is already connected */
+ INIT_DELAYED_WORK(&drvdata->delayed_status_work,
+ corsair_void_status_work_handler);
+ schedule_delayed_work(&drvdata->delayed_status_work,
+ msecs_to_jiffies(100));
+
+ /* Refresh firmware versions */
+ INIT_DELAYED_WORK(&drvdata->delayed_firmware_work,
+ corsair_void_firmware_work_handler);
+ schedule_delayed_work(&drvdata->delayed_firmware_work,
+ msecs_to_jiffies(100));
+
+ return 0;
+
+failed_after_sysfs:
+ sysfs_remove_group(&hid_dev->dev.kobj, &corsair_void_attr_group);
+ return ret;
+}
...
CJ