Re: [PATCH 13/13] HID: playstation: report DualSense hardware and firmware version.
From: Barnabás Pőcze <hidden>
Date: 2020-12-27 17:07:05
Hi 2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c index a55375ac79a9..2f989da906f3 100644 --- a/drivers/hid/hid-playstation.c +++ b/drivers/hid/hid-playstation.c[...] +static ssize_t ps_show_firmware_version(struct device *dev, + struct device_attribute + *attr, char *buf) +{ + struct hid_device *hdev = to_hid_device(dev); + struct ps_device *ps_dev = hid_get_drvdata(hdev); + + return snprintf(buf, PAGE_SIZE, "0x%08x\n", ps_dev->fw_version);
`sysfs_emit()` is preferred over *printf().
+}
+
+static DEVICE_ATTR(firmware_version, 0444, ps_show_firmware_version, NULL);
+
+static ssize_t ps_show_hardware_version(struct device *dev,
+ struct device_attribute
+ *attr, char *buf)
+{
+ struct hid_device *hdev = to_hid_device(dev);
+ struct ps_device *ps_dev = hid_get_drvdata(hdev);
+
+ return snprintf(buf, PAGE_SIZE, "0x%08x\n", ps_dev->hw_version);Same here.
+}
[...]
+static int dualsense_get_firmware_info(struct dualsense *ds)
+{
+ uint8_t *buf;
+ int ret = 0;Is there any reason it needs to be initialized?
+ + buf = kzalloc(DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_FIRMWARE_INFO, buf, + DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE, HID_FEATURE_REPORT, + HID_REQ_GET_REPORT); + if (ret < 0) + goto err_free; + + ds->base.hw_version = get_unaligned_le32(&buf[24]); + ds->base.fw_version = get_unaligned_le32(&buf[28]);
Shouldn't the size of the reply be checked?
quoted hunk ↗ jump to hunk
+ +err_free: + kfree(buf); + return ret; +} + static int dualsense_get_mac_address(struct dualsense *ds) { uint8_t *buf;@@ -1172,6 +1233,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev) } snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds->base.mac_address); + ret = dualsense_get_firmware_info(ds); + if (ret < 0) { + hid_err(hdev, "Failed to get firmware info from DualSense\n"); + return ERR_PTR(ret); + } + ret = ps_devices_list_add((struct ps_device *)ds); if (ret < 0) return ERR_PTR(ret);@@ -1241,6 +1308,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev) /* Set player LEDs to our player id. */ dualsense_set_player_leds(ds); + /* Reporting hardware and firmware is important as there are frequent updates, which + * can change behavior. + */ + hid_info(hdev, "Registered DualSense controller hw_version=%x fw_version=%x\n", + ds->base.hw_version, ds->base.fw_version); + return (struct ps_device *)ds; err:@@ -1295,6 +1368,12 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id) goto err_close; } + ret = sysfs_create_group(&hdev->dev.kobj, &ps_device_attribute_group);
It's a minor thing, but I think `device_{add,remove}_group()` would be better
here in the sense that it expresses the fact that the group is added to a device,
not just any object better.
quoted hunk ↗ jump to hunk
+ if (ret < 0) { + hid_err(hdev, "Failed to register sysfs nodes.\n"); + goto err_close; + } + return ret; err_close:@@ -1313,6 +1392,8 @@ static void ps_remove(struct hid_device *hdev) hid_hw_close(hdev); hid_hw_stop(hdev); + + sysfs_remove_group(&hdev->dev.kobj, &ps_device_attribute_group); } static const struct hid_device_id ps_devices[] = { --2.26.2
Regards, Barnabás Pőcze