[PATCH v2 00/13] HID: new driver for PS5 'DualSense' controller

[PATCH v2 00/13] HID: new driver for PS5 'DualSense' controller

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Hi,

I would like to share an updated version of the hid-playstation driver.
This new revision is very similar to the original release, but with
various small changes and minor improvements. Thanks to everyone who
provided feedback through the mailing list or privately.

Changes since v1:
- Export ps_devices table using MODULE_DEVICE_TABLE.
- Uses 'width - 1' and 'height - 1' for touchpad ABS_MT_POSITION_X/Y.
- Uses 'sysfs_emit' instead of 'snprintf' in sysfs show functions.
- Switched to devm_device_add_group to manage attribute groups.
- Changed player LED lookup to use pointer calculations instead of a loop.
- Added readonly mute set_brightness call to silence ENOTSUP.
- Lightbar is not dynamically allocated and now allocated as part of 'struct dualsense'
- Size checking on hid_hw_raw_request for calibration, firmware and mac addrees info.
- Replaced magic constants with defines e.g. for HID version or output report values.
- Minor code changes e.g. const changes.
- Uses container_of to retrieve 'struct ps_device' from 'struct dualsense'.
- Added an explicit module_init/module_exit to clean up the player IDA.

Thanks,

Roderick Colenbrander
Sony Interactive Entertainment, LLC

Roderick Colenbrander (13):
  HID: playstation: initial DualSense USB support.
  HID: playstation: use DualSense MAC address as unique identifier.
  HID: playstation: add DualSense battery support.
  HID: playstation: add DualSense touchpad support.
  HID: playstation: add DualSense accelerometer and gyroscope support.
  HID: playstation: track devices in list.
  HID: playstation: add DualSense Bluetooth support.
  HID: playstation: add DualSense classic rumble support.
  HID: playstation: add DualSense lightbar support
  HID: playstation: add microphone mute support for DualSense.
  HID: playstation: add DualSense player LEDs support.
  HID: playstation: DualSense set LEDs to default player id.
  HID: playstation: report DualSense hardware and firmware version.

 MAINTAINERS                   |    6 +
 drivers/hid/Kconfig           |   20 +
 drivers/hid/Makefile          |    1 +
 drivers/hid/hid-ids.h         |    1 +
 drivers/hid/hid-playstation.c | 1448 +++++++++++++++++++++++++++++++++
 drivers/hid/hid-quirks.c      |    4 +
 6 files changed, 1480 insertions(+)
 create mode 100644 drivers/hid/hid-playstation.c

-- 
2.26.2

[PATCH v2 01/13] HID: playstation: initial DualSense USB support.

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Implement support for PlayStation DualSense gamepad in USB mode.
Support features include buttons and sticks, which adhere to the
Linux gamepad spec.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 MAINTAINERS                   |   6 +
 drivers/hid/Kconfig           |   9 +
 drivers/hid/Makefile          |   1 +
 drivers/hid/hid-ids.h         |   1 +
 drivers/hid/hid-playstation.c | 321 ++++++++++++++++++++++++++++++++++
 drivers/hid/hid-quirks.c      |   3 +
 6 files changed, 341 insertions(+)
 create mode 100644 drivers/hid/hid-playstation.c

diff --git a/MAINTAINERS b/MAINTAINERS
index f81d598a8556..0ecae30af074 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7918,6 +7918,12 @@ F:	drivers/hid/
 F:	include/linux/hid*
 F:	include/uapi/linux/hid*
 
+HID PLAYSTATION DRIVER
+M:	Roderick Colenbrander <roderick.colenbrander@sony.com>
+L:	linux-input@vger.kernel.org
+S:	Supported
+F:	drivers/hid/hid-playstation.c
+
 HID SENSOR HUB DRIVERS
 M:	Jiri Kosina <jikos@kernel.org>
 M:	Jonathan Cameron <jic23@kernel.org>
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 7bdda1b5b221..d3258e806998 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -853,6 +853,15 @@ config HID_PLANTRONICS
 
 	  Say M here if you may ever plug in a Plantronics USB audio device.
 
+config HID_PLAYSTATION
+	tristate "PlayStation HID Driver"
+	default !EXPERT
+	depends on HID
+	help
+	  Provides support for Sony PS5 controllers including support for
+	  its special functionalities e.g. touchpad, lights and motion
+	  sensors.
+
 config HID_PRIMAX
 	tristate "Primax non-fully HID-compliant devices"
 	depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 014d21fe7dac..3cdbfb60ca57 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -94,6 +94,7 @@ hid-picolcd-$(CONFIG_HID_PICOLCD_CIR)	+= hid-picolcd_cir.o
 hid-picolcd-$(CONFIG_DEBUG_FS)		+= hid-picolcd_debugfs.o
 
 obj-$(CONFIG_HID_PLANTRONICS)	+= hid-plantronics.o
+obj-$(CONFIG_HID_PLAYSTATION)	+= hid-playstation.o
 obj-$(CONFIG_HID_PRIMAX)	+= hid-primax.o
 obj-$(CONFIG_HID_REDRAGON)	+= hid-redragon.o
 obj-$(CONFIG_HID_RETRODE)	+= hid-retrode.o
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 4c5f23640f9c..70c51ec6395c 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -1072,6 +1072,7 @@
 #define USB_DEVICE_ID_SONY_PS4_CONTROLLER	0x05c4
 #define USB_DEVICE_ID_SONY_PS4_CONTROLLER_2	0x09cc
 #define USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE	0x0ba0
+#define USB_DEVICE_ID_SONY_PS5_CONTROLLER	0x0ce6
 #define USB_DEVICE_ID_SONY_MOTION_CONTROLLER	0x03d5
 #define USB_DEVICE_ID_SONY_NAVIGATION_CONTROLLER	0x042f
 #define USB_DEVICE_ID_SONY_BUZZ_CONTROLLER		0x0002
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
new file mode 100644
index 000000000000..3d5fe9069c26
--- /dev/null
+++ b/drivers/hid/hid-playstation.c
@@ -0,0 +1,321 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *  HID driver for Sony DualSense(TM) controller.
+ *
+ *  Copyright (c) 2020 Sony Interactive Entertainment
+ */
+
+#include <linux/bits.h>
+#include <linux/crc32.h>
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/input/mt.h>
+#include <linux/module.h>
+
+#include <asm/unaligned.h>
+
+#include "hid-ids.h"
+
+#define HID_PLAYSTATION_VERSION_PATCH 0x8000
+
+/* Base class for playstation devices. */
+struct ps_device {
+	struct hid_device *hdev;
+
+	int (*parse_report)(struct ps_device *dev, struct hid_report *report, u8 *data, int size);
+};
+
+#define DS_INPUT_REPORT_USB			0x01
+
+/* Button masks for DualSense input report. */
+#define DS_BUTTONS0_HAT_SWITCH	GENMASK(3, 0)
+#define DS_BUTTONS0_SQUARE	BIT(4)
+#define DS_BUTTONS0_CROSS	BIT(5)
+#define DS_BUTTONS0_CIRCLE	BIT(6)
+#define DS_BUTTONS0_TRIANGLE	BIT(7)
+#define DS_BUTTONS1_L1		BIT(0)
+#define DS_BUTTONS1_R1		BIT(1)
+#define DS_BUTTONS1_L2		BIT(2)
+#define DS_BUTTONS1_R2		BIT(3)
+#define DS_BUTTONS1_CREATE	BIT(4)
+#define DS_BUTTONS1_OPTIONS	BIT(5)
+#define DS_BUTTONS1_L3		BIT(6)
+#define DS_BUTTONS1_R3		BIT(7)
+#define DS_BUTTONS2_PS_HOME	BIT(0)
+#define DS_BUTTONS2_TOUCHPAD	BIT(1)
+
+struct dualsense {
+	struct ps_device base;
+	struct input_dev *gamepad;
+};
+
+struct dualsense_touch_point {
+	uint8_t contact;
+	uint8_t x_lo;
+	uint8_t x_hi:4, y_lo:4;
+	uint8_t y_hi;
+} __packed;
+
+/* Main DualSense input report excluding any BT/USB specific headers. */
+struct dualsense_input_report {
+	uint8_t x, y;
+	uint8_t rx, ry;
+	uint8_t z, rz;
+	uint8_t seq_number;
+	uint8_t buttons[4];
+	uint8_t reserved[4];
+
+	/* Motion sensors */
+	__le16 gyro[3]; /* x, y, z */
+	__le16 accel[3]; /* x, y, z */
+	__le32 sensor_timestamp;
+	uint8_t reserved2;
+
+	/* Touchpad */
+	struct dualsense_touch_point points[2];
+
+	uint8_t reserved3[12];
+	uint8_t status;
+	uint8_t reserved4[11];
+} __packed;
+
+/* Common gamepad buttons across DualShock 3 / 4 and DualSense.
+ * Note: for device with a touchpad, touchpad button is not included
+ *        as it will be part of the touchpad device.
+ */
+static const int ps_gamepad_buttons[] = {
+	BTN_WEST, /* Square */
+	BTN_NORTH, /* Triangle */
+	BTN_EAST, /* Circle */
+	BTN_SOUTH, /* Cross */
+	BTN_TL, /* L1 */
+	BTN_TR, /* R1 */
+	BTN_TL2, /* L2 */
+	BTN_TR2, /* R2 */
+	BTN_SELECT, /* Create (PS5) / Share (PS4) */
+	BTN_START, /* Option */
+	BTN_THUMBL, /* L3 */
+	BTN_THUMBR, /* R3 */
+	BTN_MODE, /* PS Home */
+};
+
+static const struct {int x; int y; } ps_gamepad_hat_mapping[] = {
+	{0, -1}, {1, -1}, {1, 0}, {1, 1}, {0, 1}, {-1, 1}, {-1, 0}, {-1, -1},
+	{0, 0}
+};
+
+static struct input_dev *ps_allocate_input_dev(struct hid_device *hdev, const char *name_suffix)
+{
+	struct input_dev *input_dev;
+
+	input_dev = devm_input_allocate_device(&hdev->dev);
+	if (!input_dev)
+		return ERR_PTR(-ENOMEM);
+
+	input_dev->id.bustype = hdev->bus;
+	input_dev->id.vendor = hdev->vendor;
+	input_dev->id.product = hdev->product;
+	input_dev->id.version = hdev->version;
+	input_dev->uniq = hdev->uniq;
+
+	if (name_suffix) {
+		input_dev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "%s %s", hdev->name,
+				name_suffix);
+		if (!input_dev->name)
+			return ERR_PTR(-ENOMEM);
+	} else {
+		input_dev->name = hdev->name;
+	}
+
+	input_set_drvdata(input_dev, hdev);
+
+	return input_dev;
+}
+
+static struct input_dev *ps_gamepad_create(struct hid_device *hdev)
+{
+	struct input_dev *gamepad;
+	unsigned int i;
+	int ret;
+
+	gamepad = ps_allocate_input_dev(hdev, NULL);
+	if (IS_ERR(gamepad))
+		return ERR_PTR(-ENOMEM);
+
+	input_set_abs_params(gamepad, ABS_X, 0, 255, 0, 0);
+	input_set_abs_params(gamepad, ABS_Y, 0, 255, 0, 0);
+	input_set_abs_params(gamepad, ABS_Z, 0, 255, 0, 0);
+	input_set_abs_params(gamepad, ABS_RX, 0, 255, 0, 0);
+	input_set_abs_params(gamepad, ABS_RY, 0, 255, 0, 0);
+	input_set_abs_params(gamepad, ABS_RZ, 0, 255, 0, 0);
+
+	input_set_abs_params(gamepad, ABS_HAT0X, -1, 1, 0, 0);
+	input_set_abs_params(gamepad, ABS_HAT0Y, -1, 1, 0, 0);
+
+	for (i = 0; i < ARRAY_SIZE(ps_gamepad_buttons); i++)
+		input_set_capability(gamepad, EV_KEY, ps_gamepad_buttons[i]);
+
+	ret = input_register_device(gamepad);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return gamepad;
+}
+
+static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *report,
+		u8 *data, int size)
+{
+	struct hid_device *hdev = ps_dev->hdev;
+	struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
+	struct dualsense_input_report *ds_report;
+	uint8_t value;
+
+	/* DualSense in USB uses the full HID report for reportID 1, but
+	 * Bluetooth uses a minimal HID report for reportID 1 and reports
+	 * the full report using reportID 49.
+	 */
+	if (report->id == DS_INPUT_REPORT_USB && hdev->bus == BUS_USB) {
+		ds_report = (struct dualsense_input_report *)&data[1];
+	} else {
+		hid_err(hdev, "Unhandled reportID=%d\n", report->id);
+		return -1;
+	}
+
+	input_report_abs(ds->gamepad, ABS_X,  ds_report->x);
+	input_report_abs(ds->gamepad, ABS_Y,  ds_report->y);
+	input_report_abs(ds->gamepad, ABS_RX, ds_report->rx);
+	input_report_abs(ds->gamepad, ABS_RY, ds_report->ry);
+	input_report_abs(ds->gamepad, ABS_Z,  ds_report->z);
+	input_report_abs(ds->gamepad, ABS_RZ, ds_report->rz);
+
+	value = ds_report->buttons[0] & DS_BUTTONS0_HAT_SWITCH;
+	if (value > 7)
+		value = 8; /* center */
+	input_report_abs(ds->gamepad, ABS_HAT0X, ps_gamepad_hat_mapping[value].x);
+	input_report_abs(ds->gamepad, ABS_HAT0Y, ps_gamepad_hat_mapping[value].y);
+
+	input_report_key(ds->gamepad, BTN_WEST,   ds_report->buttons[0] & DS_BUTTONS0_SQUARE);
+	input_report_key(ds->gamepad, BTN_SOUTH,  ds_report->buttons[0] & DS_BUTTONS0_CROSS);
+	input_report_key(ds->gamepad, BTN_EAST,   ds_report->buttons[0] & DS_BUTTONS0_CIRCLE);
+	input_report_key(ds->gamepad, BTN_NORTH,  ds_report->buttons[0] & DS_BUTTONS0_TRIANGLE);
+	input_report_key(ds->gamepad, BTN_TL,     ds_report->buttons[1] & DS_BUTTONS1_L1);
+	input_report_key(ds->gamepad, BTN_TR,     ds_report->buttons[1] & DS_BUTTONS1_R1);
+	input_report_key(ds->gamepad, BTN_TL2,    ds_report->buttons[1] & DS_BUTTONS1_L2);
+	input_report_key(ds->gamepad, BTN_TR2,    ds_report->buttons[1] & DS_BUTTONS1_R2);
+	input_report_key(ds->gamepad, BTN_SELECT, ds_report->buttons[1] & DS_BUTTONS1_CREATE);
+	input_report_key(ds->gamepad, BTN_START,  ds_report->buttons[1] & DS_BUTTONS1_OPTIONS);
+	input_report_key(ds->gamepad, BTN_THUMBL, ds_report->buttons[1] & DS_BUTTONS1_L3);
+	input_report_key(ds->gamepad, BTN_THUMBR, ds_report->buttons[1] & DS_BUTTONS1_R3);
+	input_report_key(ds->gamepad, BTN_MODE,   ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
+	input_sync(ds->gamepad);
+
+	return 0;
+}
+
+static struct ps_device *dualsense_create(struct hid_device *hdev)
+{
+	struct dualsense *ds;
+	int ret;
+
+	ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL);
+	if (!ds)
+		return ERR_PTR(-ENOMEM);
+
+	/* Patch version to allow userspace to distinguish between
+	 * hid-generic vs hid-playstation axis and button mapping.
+	 */
+	hdev->version |= HID_PLAYSTATION_VERSION_PATCH;
+
+	ds->base.hdev = hdev;
+	ds->base.parse_report = dualsense_parse_report;
+	hid_set_drvdata(hdev, ds);
+
+	ds->gamepad = ps_gamepad_create(hdev);
+	if (IS_ERR(ds->gamepad)) {
+		ret = PTR_ERR(ds->gamepad);
+		goto err;
+	}
+
+	return &ds->base;
+
+err:
+	return ERR_PTR(ret);
+}
+
+static int ps_raw_event(struct hid_device *hdev, struct hid_report *report,
+		u8 *data, int size)
+{
+	struct ps_device *dev = hid_get_drvdata(hdev);
+
+	if (dev && dev->parse_report)
+		return dev->parse_report(dev, report, data, size);
+
+	return 0;
+}
+
+static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+	struct ps_device *dev;
+	int ret;
+
+	ret = hid_parse(hdev);
+	if (ret) {
+		hid_err(hdev, "parse failed\n");
+		return ret;
+	}
+
+	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+	if (ret) {
+		hid_err(hdev, "hw start failed\n");
+		return ret;
+	}
+
+	ret = hid_hw_open(hdev);
+	if (ret) {
+		hid_err(hdev, "hw open failed\n");
+		goto err_stop;
+	}
+
+	if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
+		dev = dualsense_create(hdev);
+		if (IS_ERR(dev)) {
+			hid_err(hdev, "Failed to create dualsense.\n");
+			ret = PTR_ERR(dev);
+			goto err_close;
+		}
+	}
+
+	return ret;
+
+err_close:
+	hid_hw_close(hdev);
+err_stop:
+	hid_hw_stop(hdev);
+	return ret;
+}
+
+static void ps_remove(struct hid_device *hdev)
+{
+	hid_hw_close(hdev);
+	hid_hw_stop(hdev);
+}
+
+static const struct hid_device_id ps_devices[] = {
+	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER) },
+	{ }
+};
+MODULE_DEVICE_TABLE(hid, ps_devices);
+
+static struct hid_driver ps_driver = {
+	.name             = "playstation",
+	.id_table         = ps_devices,
+	.probe            = ps_probe,
+	.remove           = ps_remove,
+	.raw_event        = ps_raw_event,
+};
+
+module_hid_driver(ps_driver);
+
+MODULE_AUTHOR("Sony Interactive Entertainment");
+MODULE_DESCRIPTION("HID Driver for PlayStation peripherals.");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index d9ca874dffac..1ca46cb445be 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -565,6 +565,9 @@ static const struct hid_device_id hid_have_special_driver[] = {
 #if IS_ENABLED(CONFIG_HID_PLANTRONICS)
 	{ HID_USB_DEVICE(USB_VENDOR_ID_PLANTRONICS, HID_ANY_ID) },
 #endif
+#if IS_ENABLED(CONFIG_HID_PLAYSTATION)
+	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER) },
+#endif
 #if IS_ENABLED(CONFIG_HID_PRIMAX)
 	{ HID_USB_DEVICE(USB_VENDOR_ID_PRIMAX, USB_DEVICE_ID_PRIMAX_KEYBOARD) },
 #endif
-- 
2.26.2

Re: [PATCH v2 01/13] HID: playstation: initial DualSense USB support.

[Jiri Kosina]

On Sat, 2 Jan 2021, Roderick Colenbrander wrote:

> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> 
> Implement support for PlayStation DualSense gamepad in USB mode.
> Support features include buttons and sticks, which adhere to the
> Linux gamepad spec.
> 
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> ---
>  MAINTAINERS                   |   6 +
>  drivers/hid/Kconfig           |   9 +
>  drivers/hid/Makefile          |   1 +
>  drivers/hid/hid-ids.h         |   1 +
>  drivers/hid/hid-playstation.c | 321 ++++++++++++++++++++++++++++++++++
>  drivers/hid/hid-quirks.c      |   3 +
>  6 files changed, 341 insertions(+)
>  create mode 100644 drivers/hid/hid-playstation.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f81d598a8556..0ecae30af074 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7918,6 +7918,12 @@ F:	drivers/hid/
>  F:	include/linux/hid*
>  F:	include/uapi/linux/hid*
>  
> +HID PLAYSTATION DRIVER
> +M:	Roderick Colenbrander <roderick.colenbrander@sony.com>
> +L:	linux-input@vger.kernel.org
> +S:	Supported
> +F:	drivers/hid/hid-playstation.c
> +
>  HID SENSOR HUB DRIVERS
>  M:	Jiri Kosina <jikos@kernel.org>
>  M:	Jonathan Cameron <jic23@kernel.org>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 7bdda1b5b221..d3258e806998 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -853,6 +853,15 @@ config HID_PLANTRONICS
>  
>  	  Say M here if you may ever plug in a Plantronics USB audio device.
>  
> +config HID_PLAYSTATION
> +	tristate "PlayStation HID Driver"
> +	default !EXPERT

Minor nit: these '!EXPERT' defaults are there only for drivers that were 
created during the big "let's separate all the quirks from hid-core into 
individual driver" bang that happened ages ago. For new drivers, we follow 
what's common in other driver subsystems, and don't force the default.

No need to resend if it'd be just for this change, I can adjust it when 
applying.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 01/13] HID: playstation: initial DualSense USB support.

[Benjamin Tissoires]

Hi Roderick,

Thanks for the v2. I'll also let the reviewers of v1 do a second pass
on this version, but I have one comment here:

On Sat, Jan 2, 2021 at 11:31 PM Roderick Colenbrander
<roderick@gaikai.com> wrote:
>
> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> Implement support for PlayStation DualSense gamepad in USB mode.
> Support features include buttons and sticks, which adhere to the
> Linux gamepad spec.
>
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> ---
>  MAINTAINERS                   |   6 +
>  drivers/hid/Kconfig           |   9 +
>  drivers/hid/Makefile          |   1 +
>  drivers/hid/hid-ids.h         |   1 +
>  drivers/hid/hid-playstation.c | 321 ++++++++++++++++++++++++++++++++++
>  drivers/hid/hid-quirks.c      |   3 +
>  6 files changed, 341 insertions(+)
>  create mode 100644 drivers/hid/hid-playstation.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f81d598a8556..0ecae30af074 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7918,6 +7918,12 @@ F:       drivers/hid/
>  F:     include/linux/hid*
>  F:     include/uapi/linux/hid*
>
> +HID PLAYSTATION DRIVER
> +M:     Roderick Colenbrander <roderick.colenbrander@sony.com>
> +L:     linux-input@vger.kernel.org
> +S:     Supported
> +F:     drivers/hid/hid-playstation.c
> +
>  HID SENSOR HUB DRIVERS
>  M:     Jiri Kosina <jikos@kernel.org>
>  M:     Jonathan Cameron <jic23@kernel.org>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 7bdda1b5b221..d3258e806998 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -853,6 +853,15 @@ config HID_PLANTRONICS
>
>           Say M here if you may ever plug in a Plantronics USB audio device.
>
> +config HID_PLAYSTATION
> +       tristate "PlayStation HID Driver"
> +       default !EXPERT
> +       depends on HID
> +       help
> +         Provides support for Sony PS5 controllers including support for
> +         its special functionalities e.g. touchpad, lights and motion
> +         sensors.
> +
>  config HID_PRIMAX
>         tristate "Primax non-fully HID-compliant devices"
>         depends on HID
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 014d21fe7dac..3cdbfb60ca57 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -94,6 +94,7 @@ hid-picolcd-$(CONFIG_HID_PICOLCD_CIR) += hid-picolcd_cir.o
>  hid-picolcd-$(CONFIG_DEBUG_FS)         += hid-picolcd_debugfs.o
>
>  obj-$(CONFIG_HID_PLANTRONICS)  += hid-plantronics.o
> +obj-$(CONFIG_HID_PLAYSTATION)  += hid-playstation.o
>  obj-$(CONFIG_HID_PRIMAX)       += hid-primax.o
>  obj-$(CONFIG_HID_REDRAGON)     += hid-redragon.o
>  obj-$(CONFIG_HID_RETRODE)      += hid-retrode.o
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 4c5f23640f9c..70c51ec6395c 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -1072,6 +1072,7 @@
>  #define USB_DEVICE_ID_SONY_PS4_CONTROLLER      0x05c4
>  #define USB_DEVICE_ID_SONY_PS4_CONTROLLER_2    0x09cc
>  #define USB_DEVICE_ID_SONY_PS4_CONTROLLER_DONGLE       0x0ba0
> +#define USB_DEVICE_ID_SONY_PS5_CONTROLLER      0x0ce6
>  #define USB_DEVICE_ID_SONY_MOTION_CONTROLLER   0x03d5
>  #define USB_DEVICE_ID_SONY_NAVIGATION_CONTROLLER       0x042f
>  #define USB_DEVICE_ID_SONY_BUZZ_CONTROLLER             0x0002
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> new file mode 100644
> index 000000000000..3d5fe9069c26
> --- /dev/null
> +++ b/drivers/hid/hid-playstation.c
> @@ -0,0 +1,321 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *  HID driver for Sony DualSense(TM) controller.
> + *
> + *  Copyright (c) 2020 Sony Interactive Entertainment
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/crc32.h>
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/input/mt.h>
> +#include <linux/module.h>
> +
> +#include <asm/unaligned.h>
> +
> +#include "hid-ids.h"
> +
> +#define HID_PLAYSTATION_VERSION_PATCH 0x8000
> +
> +/* Base class for playstation devices. */
> +struct ps_device {
> +       struct hid_device *hdev;
> +
> +       int (*parse_report)(struct ps_device *dev, struct hid_report *report, u8 *data, int size);
> +};
> +
> +#define DS_INPUT_REPORT_USB                    0x01
> +
> +/* Button masks for DualSense input report. */
> +#define DS_BUTTONS0_HAT_SWITCH GENMASK(3, 0)
> +#define DS_BUTTONS0_SQUARE     BIT(4)
> +#define DS_BUTTONS0_CROSS      BIT(5)
> +#define DS_BUTTONS0_CIRCLE     BIT(6)
> +#define DS_BUTTONS0_TRIANGLE   BIT(7)
> +#define DS_BUTTONS1_L1         BIT(0)
> +#define DS_BUTTONS1_R1         BIT(1)
> +#define DS_BUTTONS1_L2         BIT(2)
> +#define DS_BUTTONS1_R2         BIT(3)
> +#define DS_BUTTONS1_CREATE     BIT(4)
> +#define DS_BUTTONS1_OPTIONS    BIT(5)
> +#define DS_BUTTONS1_L3         BIT(6)
> +#define DS_BUTTONS1_R3         BIT(7)
> +#define DS_BUTTONS2_PS_HOME    BIT(0)
> +#define DS_BUTTONS2_TOUCHPAD   BIT(1)
> +
> +struct dualsense {
> +       struct ps_device base;
> +       struct input_dev *gamepad;
> +};
> +
> +struct dualsense_touch_point {
> +       uint8_t contact;
> +       uint8_t x_lo;
> +       uint8_t x_hi:4, y_lo:4;
> +       uint8_t y_hi;
> +} __packed;
> +
> +/* Main DualSense input report excluding any BT/USB specific headers. */
> +struct dualsense_input_report {
> +       uint8_t x, y;
> +       uint8_t rx, ry;
> +       uint8_t z, rz;
> +       uint8_t seq_number;
> +       uint8_t buttons[4];
> +       uint8_t reserved[4];
> +
> +       /* Motion sensors */
> +       __le16 gyro[3]; /* x, y, z */
> +       __le16 accel[3]; /* x, y, z */
> +       __le32 sensor_timestamp;
> +       uint8_t reserved2;
> +
> +       /* Touchpad */
> +       struct dualsense_touch_point points[2];
> +
> +       uint8_t reserved3[12];
> +       uint8_t status;
> +       uint8_t reserved4[11];
> +} __packed;
> +
> +/* Common gamepad buttons across DualShock 3 / 4 and DualSense.
> + * Note: for device with a touchpad, touchpad button is not included
> + *        as it will be part of the touchpad device.
> + */
> +static const int ps_gamepad_buttons[] = {
> +       BTN_WEST, /* Square */
> +       BTN_NORTH, /* Triangle */
> +       BTN_EAST, /* Circle */
> +       BTN_SOUTH, /* Cross */
> +       BTN_TL, /* L1 */
> +       BTN_TR, /* R1 */
> +       BTN_TL2, /* L2 */
> +       BTN_TR2, /* R2 */
> +       BTN_SELECT, /* Create (PS5) / Share (PS4) */
> +       BTN_START, /* Option */
> +       BTN_THUMBL, /* L3 */
> +       BTN_THUMBR, /* R3 */
> +       BTN_MODE, /* PS Home */
> +};
> +
> +static const struct {int x; int y; } ps_gamepad_hat_mapping[] = {
> +       {0, -1}, {1, -1}, {1, 0}, {1, 1}, {0, 1}, {-1, 1}, {-1, 0}, {-1, -1},
> +       {0, 0}
> +};
> +
> +static struct input_dev *ps_allocate_input_dev(struct hid_device *hdev, const char *name_suffix)
> +{
> +       struct input_dev *input_dev;
> +
> +       input_dev = devm_input_allocate_device(&hdev->dev);
> +       if (!input_dev)
> +               return ERR_PTR(-ENOMEM);
> +
> +       input_dev->id.bustype = hdev->bus;
> +       input_dev->id.vendor = hdev->vendor;
> +       input_dev->id.product = hdev->product;
> +       input_dev->id.version = hdev->version;
> +       input_dev->uniq = hdev->uniq;
> +
> +       if (name_suffix) {
> +               input_dev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "%s %s", hdev->name,
> +                               name_suffix);
> +               if (!input_dev->name)
> +                       return ERR_PTR(-ENOMEM);
> +       } else {
> +               input_dev->name = hdev->name;
> +       }
> +
> +       input_set_drvdata(input_dev, hdev);
> +
> +       return input_dev;
> +}
> +
> +static struct input_dev *ps_gamepad_create(struct hid_device *hdev)
> +{
> +       struct input_dev *gamepad;
> +       unsigned int i;
> +       int ret;
> +
> +       gamepad = ps_allocate_input_dev(hdev, NULL);
> +       if (IS_ERR(gamepad))
> +               return ERR_PTR(-ENOMEM);
> +
> +       input_set_abs_params(gamepad, ABS_X, 0, 255, 0, 0);
> +       input_set_abs_params(gamepad, ABS_Y, 0, 255, 0, 0);
> +       input_set_abs_params(gamepad, ABS_Z, 0, 255, 0, 0);
> +       input_set_abs_params(gamepad, ABS_RX, 0, 255, 0, 0);
> +       input_set_abs_params(gamepad, ABS_RY, 0, 255, 0, 0);
> +       input_set_abs_params(gamepad, ABS_RZ, 0, 255, 0, 0);
> +
> +       input_set_abs_params(gamepad, ABS_HAT0X, -1, 1, 0, 0);
> +       input_set_abs_params(gamepad, ABS_HAT0Y, -1, 1, 0, 0);
> +
> +       for (i = 0; i < ARRAY_SIZE(ps_gamepad_buttons); i++)
> +               input_set_capability(gamepad, EV_KEY, ps_gamepad_buttons[i]);
> +
> +       ret = input_register_device(gamepad);
> +       if (ret)
> +               return ERR_PTR(ret);
> +
> +       return gamepad;
> +}
> +
> +static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *report,
> +               u8 *data, int size)
> +{
> +       struct hid_device *hdev = ps_dev->hdev;
> +       struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
> +       struct dualsense_input_report *ds_report;
> +       uint8_t value;
> +
> +       /* DualSense in USB uses the full HID report for reportID 1, but
> +        * Bluetooth uses a minimal HID report for reportID 1 and reports
> +        * the full report using reportID 49.
> +        */
> +       if (report->id == DS_INPUT_REPORT_USB && hdev->bus == BUS_USB) {
> +               ds_report = (struct dualsense_input_report *)&data[1];
> +       } else {
> +               hid_err(hdev, "Unhandled reportID=%d\n", report->id);
> +               return -1;
> +       }
> +
> +       input_report_abs(ds->gamepad, ABS_X,  ds_report->x);
> +       input_report_abs(ds->gamepad, ABS_Y,  ds_report->y);
> +       input_report_abs(ds->gamepad, ABS_RX, ds_report->rx);
> +       input_report_abs(ds->gamepad, ABS_RY, ds_report->ry);
> +       input_report_abs(ds->gamepad, ABS_Z,  ds_report->z);
> +       input_report_abs(ds->gamepad, ABS_RZ, ds_report->rz);
> +
> +       value = ds_report->buttons[0] & DS_BUTTONS0_HAT_SWITCH;
> +       if (value > 7)
> +               value = 8; /* center */
> +       input_report_abs(ds->gamepad, ABS_HAT0X, ps_gamepad_hat_mapping[value].x);
> +       input_report_abs(ds->gamepad, ABS_HAT0Y, ps_gamepad_hat_mapping[value].y);
> +
> +       input_report_key(ds->gamepad, BTN_WEST,   ds_report->buttons[0] & DS_BUTTONS0_SQUARE);
> +       input_report_key(ds->gamepad, BTN_SOUTH,  ds_report->buttons[0] & DS_BUTTONS0_CROSS);
> +       input_report_key(ds->gamepad, BTN_EAST,   ds_report->buttons[0] & DS_BUTTONS0_CIRCLE);
> +       input_report_key(ds->gamepad, BTN_NORTH,  ds_report->buttons[0] & DS_BUTTONS0_TRIANGLE);
> +       input_report_key(ds->gamepad, BTN_TL,     ds_report->buttons[1] & DS_BUTTONS1_L1);
> +       input_report_key(ds->gamepad, BTN_TR,     ds_report->buttons[1] & DS_BUTTONS1_R1);
> +       input_report_key(ds->gamepad, BTN_TL2,    ds_report->buttons[1] & DS_BUTTONS1_L2);
> +       input_report_key(ds->gamepad, BTN_TR2,    ds_report->buttons[1] & DS_BUTTONS1_R2);
> +       input_report_key(ds->gamepad, BTN_SELECT, ds_report->buttons[1] & DS_BUTTONS1_CREATE);
> +       input_report_key(ds->gamepad, BTN_START,  ds_report->buttons[1] & DS_BUTTONS1_OPTIONS);
> +       input_report_key(ds->gamepad, BTN_THUMBL, ds_report->buttons[1] & DS_BUTTONS1_L3);
> +       input_report_key(ds->gamepad, BTN_THUMBR, ds_report->buttons[1] & DS_BUTTONS1_R3);
> +       input_report_key(ds->gamepad, BTN_MODE,   ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
> +       input_sync(ds->gamepad);
> +
> +       return 0;
> +}
> +
> +static struct ps_device *dualsense_create(struct hid_device *hdev)
> +{
> +       struct dualsense *ds;
> +       int ret;
> +
> +       ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL);
> +       if (!ds)
> +               return ERR_PTR(-ENOMEM);
> +
> +       /* Patch version to allow userspace to distinguish between
> +        * hid-generic vs hid-playstation axis and button mapping.
> +        */
> +       hdev->version |= HID_PLAYSTATION_VERSION_PATCH;
> +
> +       ds->base.hdev = hdev;
> +       ds->base.parse_report = dualsense_parse_report;
> +       hid_set_drvdata(hdev, ds);
> +
> +       ds->gamepad = ps_gamepad_create(hdev);
> +       if (IS_ERR(ds->gamepad)) {
> +               ret = PTR_ERR(ds->gamepad);
> +               goto err;
> +       }
> +
> +       return &ds->base;
> +
> +err:
> +       return ERR_PTR(ret);
> +}
> +
> +static int ps_raw_event(struct hid_device *hdev, struct hid_report *report,
> +               u8 *data, int size)
> +{
> +       struct ps_device *dev = hid_get_drvdata(hdev);
> +
> +       if (dev && dev->parse_report)
> +               return dev->parse_report(dev, report, data, size);
> +
> +       return 0;
> +}
> +
> +static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +       struct ps_device *dev;
> +       int ret;
> +
> +       ret = hid_parse(hdev);
> +       if (ret) {
> +               hid_err(hdev, "parse failed\n");
> +               return ret;
> +       }
> +
> +       ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> +       if (ret) {
> +               hid_err(hdev, "hw start failed\n");
> +               return ret;
> +       }
> +
> +       ret = hid_hw_open(hdev);
> +       if (ret) {
> +               hid_err(hdev, "hw open failed\n");
> +               goto err_stop;
> +       }
> +
> +       if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
> +               dev = dualsense_create(hdev);
> +               if (IS_ERR(dev)) {
> +                       hid_err(hdev, "Failed to create dualsense.\n");
> +                       ret = PTR_ERR(dev);
> +                       goto err_close;
> +               }
> +       }
> +
> +       return ret;
> +
> +err_close:
> +       hid_hw_close(hdev);
> +err_stop:
> +       hid_hw_stop(hdev);
> +       return ret;
> +}
> +
> +static void ps_remove(struct hid_device *hdev)
> +{
> +       hid_hw_close(hdev);
> +       hid_hw_stop(hdev);
> +}
> +
> +static const struct hid_device_id ps_devices[] = {
> +       { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER) },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(hid, ps_devices);
> +
> +static struct hid_driver ps_driver = {
> +       .name             = "playstation",
> +       .id_table         = ps_devices,
> +       .probe            = ps_probe,
> +       .remove           = ps_remove,
> +       .raw_event        = ps_raw_event,
> +};
> +
> +module_hid_driver(ps_driver);
> +
> +MODULE_AUTHOR("Sony Interactive Entertainment");
> +MODULE_DESCRIPTION("HID Driver for PlayStation peripherals.");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> index d9ca874dffac..1ca46cb445be 100644
> --- a/drivers/hid/hid-quirks.c
> +++ b/drivers/hid/hid-quirks.c
> @@ -565,6 +565,9 @@ static const struct hid_device_id hid_have_special_driver[] = {
>  #if IS_ENABLED(CONFIG_HID_PLANTRONICS)
>         { HID_USB_DEVICE(USB_VENDOR_ID_PLANTRONICS, HID_ANY_ID) },
>  #endif
> +#if IS_ENABLED(CONFIG_HID_PLAYSTATION)
> +       { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER) },
> +#endif

Can you please drop this hunk?

The DS controller "works" fine with hid-generic, and I'd rather not
have this list grows just for the sake of it.

Cheers,
Benjamin

>  #if IS_ENABLED(CONFIG_HID_PRIMAX)
>         { HID_USB_DEVICE(USB_VENDOR_ID_PRIMAX, USB_DEVICE_ID_PRIMAX_KEYBOARD) },
>  #endif
> --
> 2.26.2
>

Re: [PATCH v2 01/13] HID: playstation: initial DualSense USB support.

[Barnabás Pőcze]

Hi


I have just a couple minor comments.


2021. január 2., szombat 23:30 keltezéssel, Roderick Colenbrander írta:

> From: Roderick Colenbrander roderick.colenbrander@sony.com
>
> Implement support for PlayStation DualSense gamepad in USB mode.
> Support features include buttons and sticks, which adhere to the
> Linux gamepad spec.
>
> Signed-off-by: Roderick Colenbrander roderick.colenbrander@sony.com
> [...]
> +/* Common gamepad buttons across DualShock 3 / 4 and DualSense.
> + * Note: for device with a touchpad, touchpad button is not included
> + *        as it will be part of the touchpad device.
> + */
> +static const int ps_gamepad_buttons[] = {
> +	BTN_WEST, /* Square */
> +	BTN_NORTH, /* Triangle */
> +	BTN_EAST, /* Circle */
> +	BTN_SOUTH, /* Cross */
> +	BTN_TL, /* L1 */
> +	BTN_TR, /* R1 */
> +	BTN_TL2, /* L2 */
> +	BTN_TR2, /* R2 */
> +	BTN_SELECT, /* Create (PS5) / Share (PS4) */
> +	BTN_START, /* Option */
> +	BTN_THUMBL, /* L3 */
> +	BTN_THUMBR, /* R3 */
> +	BTN_MODE, /* PS Home */
> +};
> +
> +static const struct {int x; int y; } ps_gamepad_hat_mapping[] = {
> +	{0, -1}, {1, -1}, {1, 0}, {1, 1}, {0, 1}, {-1, 1}, {-1, 0}, {-1, -1},
> +	{0, 0}
> +};

I believe the preferred way is to have a comma after each array/enum/etc. element
unless it is a terminating entry.


> +
> +static struct input_dev *ps_allocate_input_dev(struct hid_device *hdev, const char *name_suffix)
> +{
> +	struct input_dev *input_dev;
> +
> +	input_dev = devm_input_allocate_device(&hdev->dev);
> +	if (!input_dev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	input_dev->id.bustype = hdev->bus;
> +	input_dev->id.vendor = hdev->vendor;
> +	input_dev->id.product = hdev->product;
> +	input_dev->id.version = hdev->version;
> +	input_dev->uniq = hdev->uniq;
> +
> +	if (name_suffix) {
> +		input_dev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "%s %s", hdev->name,
> +				name_suffix);
> +		if (!input_dev->name)
> +			return ERR_PTR(-ENOMEM);
> +	} else {
> +		input_dev->name = hdev->name;
> +	}
> +
> +	input_set_drvdata(input_dev, hdev);
> +
> +	return input_dev;
> +}
> +
> +static struct input_dev *ps_gamepad_create(struct hid_device *hdev)
> +{
> +	struct input_dev *gamepad;
> +	unsigned int i;
> +	int ret;
> +
> +	gamepad = ps_allocate_input_dev(hdev, NULL);
> +	if (IS_ERR(gamepad))
> +		return ERR_PTR(-ENOMEM);

I know that at the moment ENOMEM is the only possible error, but I believe
`return ERR_CAST(gamepad);` would be better. (Or even just `return gamepad;`.)


> +
> +	input_set_abs_params(gamepad, ABS_X, 0, 255, 0, 0);
> +	input_set_abs_params(gamepad, ABS_Y, 0, 255, 0, 0);
> +	input_set_abs_params(gamepad, ABS_Z, 0, 255, 0, 0);
> +	input_set_abs_params(gamepad, ABS_RX, 0, 255, 0, 0);
> +	input_set_abs_params(gamepad, ABS_RY, 0, 255, 0, 0);
> +	input_set_abs_params(gamepad, ABS_RZ, 0, 255, 0, 0);
> +
> +	input_set_abs_params(gamepad, ABS_HAT0X, -1, 1, 0, 0);
> +	input_set_abs_params(gamepad, ABS_HAT0Y, -1, 1, 0, 0);
> +
> +	for (i = 0; i < ARRAY_SIZE(ps_gamepad_buttons); i++)
> +		input_set_capability(gamepad, EV_KEY, ps_gamepad_buttons[i]);
> +
> +	ret = input_register_device(gamepad);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return gamepad;
> +}
> +
> +static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *report,
> +		u8 *data, int size)
> +{
> +	struct hid_device *hdev = ps_dev->hdev;
> +	struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
> +	struct dualsense_input_report *ds_report;
> +	uint8_t value;
> +

I think `size` should be checked somewhere around here.


> +	/* DualSense in USB uses the full HID report for reportID 1, but
> +	 * Bluetooth uses a minimal HID report for reportID 1 and reports
> +	 * the full report using reportID 49.
> +	 */
> +	if (report->id == DS_INPUT_REPORT_USB && hdev->bus == BUS_USB) {
> +		ds_report = (struct dualsense_input_report *)&data[1];
> +	} else {
> +		hid_err(hdev, "Unhandled reportID=%d\n", report->id);
> +		return -1;
> +	}
> +
> +	input_report_abs(ds->gamepad, ABS_X,  ds_report->x);
> +	input_report_abs(ds->gamepad, ABS_Y,  ds_report->y);
> +	input_report_abs(ds->gamepad, ABS_RX, ds_report->rx);
> +	input_report_abs(ds->gamepad, ABS_RY, ds_report->ry);
> +	input_report_abs(ds->gamepad, ABS_Z,  ds_report->z);
> +	input_report_abs(ds->gamepad, ABS_RZ, ds_report->rz);
> +
> +	value = ds_report->buttons[0] & DS_BUTTONS0_HAT_SWITCH;
> +	if (value > 7)
> +		value = 8; /* center */

This seems a bit flimsy to me, it relies on a different part of the code
being in a certain way that is not enforced by anything. I'd probably do something
like this:

enum {
  HAT_DIR_W = 0,
  HAT_DIR_NW,
  ...
  HAT_DIR_SW,
  HAT_DIR_NONE,
};

static const struct {int x; int y; } ps_gamepad_hat_mapping[] = {
  [HAT_DIR_W] = {0, -1},
  ...
  [HAT_DIR_NONE] = {0, 0},
};

and then

if (value >= ARRAY_SIZE(ps_gamepad_hat_mapping))
  value = HAT_DIR_NONE;

Please consider it. By the way, are values 9..15 actually sent by the controller?


> +	input_report_abs(ds->gamepad, ABS_HAT0X, ps_gamepad_hat_mapping[value].x);
> +	input_report_abs(ds->gamepad, ABS_HAT0Y, ps_gamepad_hat_mapping[value].y);
> +
> +	input_report_key(ds->gamepad, BTN_WEST,   ds_report->buttons[0] & DS_BUTTONS0_SQUARE);
> +	input_report_key(ds->gamepad, BTN_SOUTH,  ds_report->buttons[0] & DS_BUTTONS0_CROSS);
> +	input_report_key(ds->gamepad, BTN_EAST,   ds_report->buttons[0] & DS_BUTTONS0_CIRCLE);
> +	input_report_key(ds->gamepad, BTN_NORTH,  ds_report->buttons[0] & DS_BUTTONS0_TRIANGLE);
> +	input_report_key(ds->gamepad, BTN_TL,     ds_report->buttons[1] & DS_BUTTONS1_L1);
> +	input_report_key(ds->gamepad, BTN_TR,     ds_report->buttons[1] & DS_BUTTONS1_R1);
> +	input_report_key(ds->gamepad, BTN_TL2,    ds_report->buttons[1] & DS_BUTTONS1_L2);
> +	input_report_key(ds->gamepad, BTN_TR2,    ds_report->buttons[1] & DS_BUTTONS1_R2);
> +	input_report_key(ds->gamepad, BTN_SELECT, ds_report->buttons[1] & DS_BUTTONS1_CREATE);
> +	input_report_key(ds->gamepad, BTN_START,  ds_report->buttons[1] & DS_BUTTONS1_OPTIONS);
> +	input_report_key(ds->gamepad, BTN_THUMBL, ds_report->buttons[1] & DS_BUTTONS1_L3);
> +	input_report_key(ds->gamepad, BTN_THUMBR, ds_report->buttons[1] & DS_BUTTONS1_R3);
> +	input_report_key(ds->gamepad, BTN_MODE,   ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
> +	input_sync(ds->gamepad);
> +
> +	return 0;
> +}
> +
> +static struct ps_device *dualsense_create(struct hid_device *hdev)
> +{
> +	struct dualsense *ds;
> +	int ret;
> +
> +	ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL);
> +	if (!ds)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* Patch version to allow userspace to distinguish between
> +	 * hid-generic vs hid-playstation axis and button mapping.
> +	 */
> +	hdev->version |= HID_PLAYSTATION_VERSION_PATCH;
> +
> +	ds->base.hdev = hdev;
> +	ds->base.parse_report = dualsense_parse_report;
> +	hid_set_drvdata(hdev, ds);
> +
> +	ds->gamepad = ps_gamepad_create(hdev);
> +	if (IS_ERR(ds->gamepad)) {
> +		ret = PTR_ERR(ds->gamepad);
> +		goto err;
> +	}
> +
> +	return &ds->base;
> +
> +err:
> +	return ERR_PTR(ret);
> +}
> [...]
> +static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +	struct ps_device *dev;
> +	int ret;
> +
> +	ret = hid_parse(hdev);
> +	if (ret) {
> +		hid_err(hdev, "parse failed\n");
> +		return ret;
> +	}
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> +	if (ret) {
> +		hid_err(hdev, "hw start failed\n");
> +		return ret;
> +	}
> +
> +	ret = hid_hw_open(hdev);
> +	if (ret) {
> +		hid_err(hdev, "hw open failed\n");
> +		goto err_stop;
> +	}
> +
> +	if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {

I'm still not fully seeing the purpose of this `if`. The probe should not be
called for devices not in the id_table, so this seems to me to be a long way
of writing `if (true)`. Or am I missing something?


> +		dev = dualsense_create(hdev);
> +		if (IS_ERR(dev)) {
> +			hid_err(hdev, "Failed to create dualsense.\n");

I think it'd be preferable if all log messages would either be lowercase or
uppercase, not a mix of both. Same for punctuation. This applies to all patches.


> +			ret = PTR_ERR(dev);
> +			goto err_close;
> +		}
> +	}
> +
> +	return ret;
> +
> +err_close:
> +	hid_hw_close(hdev);
> +err_stop:
> +	hid_hw_stop(hdev);
> +	return ret;
> +}
> [...]


Regards,
Barnabás Pőcze

Re: [PATCH v2 01/13] HID: playstation: initial DualSense USB support.

[Roderick Colenbrander]

Hi Barnabas,

Thanks for your suggestions.

On Thu, Jan 7, 2021 at 9:14 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Hi
>
>
> I have just a couple minor comments.
>
>
> 2021. január 2., szombat 23:30 keltezéssel, Roderick Colenbrander írta:
>
> > From: Roderick Colenbrander roderick.colenbrander@sony.com
> >
> > Implement support for PlayStation DualSense gamepad in USB mode.
> > Support features include buttons and sticks, which adhere to the
> > Linux gamepad spec.
> >
> > Signed-off-by: Roderick Colenbrander roderick.colenbrander@sony.com
> > [...]
> > +/* Common gamepad buttons across DualShock 3 / 4 and DualSense.
> > + * Note: for device with a touchpad, touchpad button is not included
> > + *        as it will be part of the touchpad device.
> > + */
> > +static const int ps_gamepad_buttons[] = {
> > +     BTN_WEST, /* Square */
> > +     BTN_NORTH, /* Triangle */
> > +     BTN_EAST, /* Circle */
> > +     BTN_SOUTH, /* Cross */
> > +     BTN_TL, /* L1 */
> > +     BTN_TR, /* R1 */
> > +     BTN_TL2, /* L2 */
> > +     BTN_TR2, /* R2 */
> > +     BTN_SELECT, /* Create (PS5) / Share (PS4) */
> > +     BTN_START, /* Option */
> > +     BTN_THUMBL, /* L3 */
> > +     BTN_THUMBR, /* R3 */
> > +     BTN_MODE, /* PS Home */
> > +};
> > +
> > +static const struct {int x; int y; } ps_gamepad_hat_mapping[] = {
> > +     {0, -1}, {1, -1}, {1, 0}, {1, 1}, {0, 1}, {-1, 1}, {-1, 0}, {-1, -1},
> > +     {0, 0}
> > +};
>
> I believe the preferred way is to have a comma after each array/enum/etc. element
> unless it is a terminating entry.
>
>
> > +
> > +static struct input_dev *ps_allocate_input_dev(struct hid_device *hdev, const char *name_suffix)
> > +{
> > +     struct input_dev *input_dev;
> > +
> > +     input_dev = devm_input_allocate_device(&hdev->dev);
> > +     if (!input_dev)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     input_dev->id.bustype = hdev->bus;
> > +     input_dev->id.vendor = hdev->vendor;
> > +     input_dev->id.product = hdev->product;
> > +     input_dev->id.version = hdev->version;
> > +     input_dev->uniq = hdev->uniq;
> > +
> > +     if (name_suffix) {
> > +             input_dev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "%s %s", hdev->name,
> > +                             name_suffix);
> > +             if (!input_dev->name)
> > +                     return ERR_PTR(-ENOMEM);
> > +     } else {
> > +             input_dev->name = hdev->name;
> > +     }
> > +
> > +     input_set_drvdata(input_dev, hdev);
> > +
> > +     return input_dev;
> > +}
> > +
> > +static struct input_dev *ps_gamepad_create(struct hid_device *hdev)
> > +{
> > +     struct input_dev *gamepad;
> > +     unsigned int i;
> > +     int ret;
> > +
> > +     gamepad = ps_allocate_input_dev(hdev, NULL);
> > +     if (IS_ERR(gamepad))
> > +             return ERR_PTR(-ENOMEM);
>
> I know that at the moment ENOMEM is the only possible error, but I believe
> `return ERR_CAST(gamepad);` would be better. (Or even just `return gamepad;`.)
>
>
> > +
> > +     input_set_abs_params(gamepad, ABS_X, 0, 255, 0, 0);
> > +     input_set_abs_params(gamepad, ABS_Y, 0, 255, 0, 0);
> > +     input_set_abs_params(gamepad, ABS_Z, 0, 255, 0, 0);
> > +     input_set_abs_params(gamepad, ABS_RX, 0, 255, 0, 0);
> > +     input_set_abs_params(gamepad, ABS_RY, 0, 255, 0, 0);
> > +     input_set_abs_params(gamepad, ABS_RZ, 0, 255, 0, 0);
> > +
> > +     input_set_abs_params(gamepad, ABS_HAT0X, -1, 1, 0, 0);
> > +     input_set_abs_params(gamepad, ABS_HAT0Y, -1, 1, 0, 0);
> > +
> > +     for (i = 0; i < ARRAY_SIZE(ps_gamepad_buttons); i++)
> > +             input_set_capability(gamepad, EV_KEY, ps_gamepad_buttons[i]);
> > +
> > +     ret = input_register_device(gamepad);
> > +     if (ret)
> > +             return ERR_PTR(ret);
> > +
> > +     return gamepad;
> > +}
> > +
> > +static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *report,
> > +             u8 *data, int size)
> > +{
> > +     struct hid_device *hdev = ps_dev->hdev;
> > +     struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
> > +     struct dualsense_input_report *ds_report;
> > +     uint8_t value;
> > +
>
> I think `size` should be checked somewhere around here.
>
>
> > +     /* DualSense in USB uses the full HID report for reportID 1, but
> > +      * Bluetooth uses a minimal HID report for reportID 1 and reports
> > +      * the full report using reportID 49.
> > +      */
> > +     if (report->id == DS_INPUT_REPORT_USB && hdev->bus == BUS_USB) {
> > +             ds_report = (struct dualsense_input_report *)&data[1];
> > +     } else {
> > +             hid_err(hdev, "Unhandled reportID=%d\n", report->id);
> > +             return -1;
> > +     }
> > +
> > +     input_report_abs(ds->gamepad, ABS_X,  ds_report->x);
> > +     input_report_abs(ds->gamepad, ABS_Y,  ds_report->y);
> > +     input_report_abs(ds->gamepad, ABS_RX, ds_report->rx);
> > +     input_report_abs(ds->gamepad, ABS_RY, ds_report->ry);
> > +     input_report_abs(ds->gamepad, ABS_Z,  ds_report->z);
> > +     input_report_abs(ds->gamepad, ABS_RZ, ds_report->rz);
> > +
> > +     value = ds_report->buttons[0] & DS_BUTTONS0_HAT_SWITCH;
> > +     if (value > 7)
> > +             value = 8; /* center */
>
> This seems a bit flimsy to me, it relies on a different part of the code
> being in a certain way that is not enforced by anything

What do you mean with not enforced? I'm not saying I'm a big fan of
the code, but HATs seem to work like this. The DualShock4/DualSense
describe it in their HID descriptors with a logical minimum value of 0
and a max value of 7.

The code is very similar to hid-input.c:

static const struct {
__s32 x;
__s32 y;
}  hid_hat_to_axis[] = {{ 0, 0}, { 0,-1}, { 1,-1}, { 1, 0}, { 1, 1}, {
0, 1}, {-1, 1}, {-1, 0}, {-1,-1}};

int hat_dir = usage->hat_dir;
if (!hat_dir)
    hat_dir = (value - usage->hat_min) * 8 / (usage->hat_max -
usage->hat_min + 1) + 1;
if (hat_dir < 0 || hat_dir > 8) hat_dir = 0;
input_event(input, usage->type, usage->code    , hid_hat_to_axis[hat_dir].x);
input_event(input, usage->type, usage->code + 1, hid_hat_to_axis[hat_dir].y);

Main difference seems to be that this code places {0, 0} at the start
and adds a "+1" to avoid having to set the value to "8" when out of
range.

 I'd probably do something
> like this:
>
> enum {
>   HAT_DIR_W = 0,
>   HAT_DIR_NW,
>   ...
>   HAT_DIR_SW,
>   HAT_DIR_NONE,
> };
>
> static const struct {int x; int y; } ps_gamepad_hat_mapping[] = {
>   [HAT_DIR_W] = {0, -1},
>   ...
>   [HAT_DIR_NONE] = {0, 0},
> };
>
> and then
>
> if (value >= ARRAY_SIZE(ps_gamepad_hat_mapping))
>   value = HAT_DIR_NONE;
>
> Please consider it. By the way, are values 9..15 actually sent by the controller?

See above. They are not sent. The Hat Switch in the report descriptor
is reported with a logical minimum of 0 and a max of 8.

>
> > +     input_report_abs(ds->gamepad, ABS_HAT0X, ps_gamepad_hat_mapping[value].x);
> > +     input_report_abs(ds->gamepad, ABS_HAT0Y, ps_gamepad_hat_mapping[value].y);
> > +
> > +     input_report_key(ds->gamepad, BTN_WEST,   ds_report->buttons[0] & DS_BUTTONS0_SQUARE);
> > +     input_report_key(ds->gamepad, BTN_SOUTH,  ds_report->buttons[0] & DS_BUTTONS0_CROSS);
> > +     input_report_key(ds->gamepad, BTN_EAST,   ds_report->buttons[0] & DS_BUTTONS0_CIRCLE);
> > +     input_report_key(ds->gamepad, BTN_NORTH,  ds_report->buttons[0] & DS_BUTTONS0_TRIANGLE);
> > +     input_report_key(ds->gamepad, BTN_TL,     ds_report->buttons[1] & DS_BUTTONS1_L1);
> > +     input_report_key(ds->gamepad, BTN_TR,     ds_report->buttons[1] & DS_BUTTONS1_R1);
> > +     input_report_key(ds->gamepad, BTN_TL2,    ds_report->buttons[1] & DS_BUTTONS1_L2);
> > +     input_report_key(ds->gamepad, BTN_TR2,    ds_report->buttons[1] & DS_BUTTONS1_R2);
> > +     input_report_key(ds->gamepad, BTN_SELECT, ds_report->buttons[1] & DS_BUTTONS1_CREATE);
> > +     input_report_key(ds->gamepad, BTN_START,  ds_report->buttons[1] & DS_BUTTONS1_OPTIONS);
> > +     input_report_key(ds->gamepad, BTN_THUMBL, ds_report->buttons[1] & DS_BUTTONS1_L3);
> > +     input_report_key(ds->gamepad, BTN_THUMBR, ds_report->buttons[1] & DS_BUTTONS1_R3);
> > +     input_report_key(ds->gamepad, BTN_MODE,   ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
> > +     input_sync(ds->gamepad);
> > +
> > +     return 0;
> > +}
> > +
> > +static struct ps_device *dualsense_create(struct hid_device *hdev)
> > +{
> > +     struct dualsense *ds;
> > +     int ret;
> > +
> > +     ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL);
> > +     if (!ds)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     /* Patch version to allow userspace to distinguish between
> > +      * hid-generic vs hid-playstation axis and button mapping.
> > +      */
> > +     hdev->version |= HID_PLAYSTATION_VERSION_PATCH;
> > +
> > +     ds->base.hdev = hdev;
> > +     ds->base.parse_report = dualsense_parse_report;
> > +     hid_set_drvdata(hdev, ds);
> > +
> > +     ds->gamepad = ps_gamepad_create(hdev);
> > +     if (IS_ERR(ds->gamepad)) {
> > +             ret = PTR_ERR(ds->gamepad);
> > +             goto err;
> > +     }
> > +
> > +     return &ds->base;
> > +
> > +err:
> > +     return ERR_PTR(ret);
> > +}
> > [...]
> > +static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > +{
> > +     struct ps_device *dev;
> > +     int ret;
> > +
> > +     ret = hid_parse(hdev);
> > +     if (ret) {
> > +             hid_err(hdev, "parse failed\n");
> > +             return ret;
> > +     }
> > +
> > +     ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> > +     if (ret) {
> > +             hid_err(hdev, "hw start failed\n");
> > +             return ret;
> > +     }
> > +
> > +     ret = hid_hw_open(hdev);
> > +     if (ret) {
> > +             hid_err(hdev, "hw open failed\n");
> > +             goto err_stop;
> > +     }
> > +
> > +     if (hdev->product == USB_DEVICE_ID_SONY_PS5_CONTROLLER) {
>
> I'm still not fully seeing the purpose of this `if`. The probe should not be
> called for devices not in the id_table, so this seems to me to be a long way
> of writing `if (true)`. Or am I missing something?

It is not used. It more there for the future, when we will add
DualShock 4 and perhaps some other devices here.

>
> > +             dev = dualsense_create(hdev);
> > +             if (IS_ERR(dev)) {
> > +                     hid_err(hdev, "Failed to create dualsense.\n");
>
> I think it'd be preferable if all log messages would either be lowercase or
> uppercase, not a mix of both. Same for punctuation. This applies to all patches.
>
>
> > +                     ret = PTR_ERR(dev);
> > +                     goto err_close;
> > +             }
> > +     }
> > +
> > +     return ret;
> > +
> > +err_close:
> > +     hid_hw_close(hdev);
> > +err_stop:
> > +     hid_hw_stop(hdev);
> > +     return ret;
> > +}
> > [...]
>
>
> Regards,
> Barnabás Pőcze



Regards,
Roderick Colenbrander

Re: [PATCH v2 01/13] HID: playstation: initial DualSense USB support.

[Barnabás Pőcze]

Hi


2021. január 8., péntek 8:12 keltezéssel, Roderick Colenbrander írta:

> [...]
> > > +static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *report,
> > > +             u8 *data, int size)
> > > +{
> > > +     struct hid_device *hdev = ps_dev->hdev;
> > > +     struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
> > > +     struct dualsense_input_report *ds_report;
> > > +     uint8_t value;
> > > +
> >
> > I think `size` should be checked somewhere around here.
> >
> >
> > > +     /* DualSense in USB uses the full HID report for reportID 1, but
> > > +      * Bluetooth uses a minimal HID report for reportID 1 and reports
> > > +      * the full report using reportID 49.
> > > +      */
> > > +     if (report->id == DS_INPUT_REPORT_USB && hdev->bus == BUS_USB) {
> > > +             ds_report = (struct dualsense_input_report *)&data[1];
> > > +     } else {
> > > +             hid_err(hdev, "Unhandled reportID=%d\n", report->id);
> > > +             return -1;
> > > +     }
> > > +
> > > +     input_report_abs(ds->gamepad, ABS_X,  ds_report->x);
> > > +     input_report_abs(ds->gamepad, ABS_Y,  ds_report->y);
> > > +     input_report_abs(ds->gamepad, ABS_RX, ds_report->rx);
> > > +     input_report_abs(ds->gamepad, ABS_RY, ds_report->ry);
> > > +     input_report_abs(ds->gamepad, ABS_Z,  ds_report->z);
> > > +     input_report_abs(ds->gamepad, ABS_RZ, ds_report->rz);
> > > +
> > > +     value = ds_report->buttons[0] & DS_BUTTONS0_HAT_SWITCH;
> > > +     if (value > 7)
> > > +             value = 8; /* center */
> >
> > This seems a bit flimsy to me, it relies on a different part of the code
> > being in a certain way that is not enforced by anything
>
> What do you mean with not enforced? I'm not saying I'm a big fan of
> the code, but HATs seem to work like this. The DualShock4/DualSense
> describe it in their HID descriptors with a logical minimum value of 0
> and a max value of 7.

What I really meant is that I think it would be better to somehow decrease
the chances of unintentionally breaking the code by providing stronger
guarantees that `ps_gamepad_hat_mapping[value]` will not be an out of bounds
access. E.g. checking with static_assert that it has 9 elements, using
ARRAY_SIZE() in some way in the condition, etc. I'm not trying to say it's
very probable, but I think it's easily preventable and thus it should be done.

By "not enforced", I meant that there are no compile time or even runtime guarantees
that the `ps_gamepad_hat_mapping` contains as many elements as expected by this
part of the code.


>
> The code is very similar to hid-input.c:
>
> static const struct {
> __s32 x;
> __s32 y;
> }  hid_hat_to_axis[] = {{ 0, 0}, { 0,-1}, { 1,-1}, { 1, 0}, { 1, 1}, {
> 0, 1}, {-1, 1}, {-1, 0}, {-1,-1}};
>
> int hat_dir = usage->hat_dir;
> if (!hat_dir)
>     hat_dir = (value - usage->hat_min) * 8 / (usage->hat_max -
> usage->hat_min + 1) + 1;
> if (hat_dir < 0 || hat_dir > 8) hat_dir = 0;
> input_event(input, usage->type, usage->code    , hid_hat_to_axis[hat_dir].x);
> input_event(input, usage->type, usage->code + 1, hid_hat_to_axis[hat_dir].y);
>
> Main difference seems to be that this code places {0, 0} at the start
> and adds a "+1" to avoid having to set the value to "8" when out of
> range.
>
>  I'd probably do something
> > like this:
> >
> > enum {
> >   HAT_DIR_W = 0,
> >   HAT_DIR_NW,
> >   ...
> >   HAT_DIR_SW,
> >   HAT_DIR_NONE,
> > };
> >
> > static const struct {int x; int y; } ps_gamepad_hat_mapping[] = {
> >   [HAT_DIR_W] = {0, -1},
> >   ...
> >   [HAT_DIR_NONE] = {0, 0},
> > };
> >
> > and then
> >
> > if (value >= ARRAY_SIZE(ps_gamepad_hat_mapping))
> >   value = HAT_DIR_NONE;
> >
> > Please consider it. By the way, are values 9..15 actually sent by the controller?
>
> See above. They are not sent. The Hat Switch in the report descriptor
> is reported with a logical minimum of 0 and a max of 8.
>
> >
> > > +     input_report_abs(ds->gamepad, ABS_HAT0X, ps_gamepad_hat_mapping[value].x);
> > > +     input_report_abs(ds->gamepad, ABS_HAT0Y, ps_gamepad_hat_mapping[value].y);
> > > +
> > > +     input_report_key(ds->gamepad, BTN_WEST,   ds_report->buttons[0] & DS_BUTTONS0_SQUARE);
> > > +     input_report_key(ds->gamepad, BTN_SOUTH,  ds_report->buttons[0] & DS_BUTTONS0_CROSS);
> > > +     input_report_key(ds->gamepad, BTN_EAST,   ds_report->buttons[0] & DS_BUTTONS0_CIRCLE);
> > > +     input_report_key(ds->gamepad, BTN_NORTH,  ds_report->buttons[0] & DS_BUTTONS0_TRIANGLE);
> > > +     input_report_key(ds->gamepad, BTN_TL,     ds_report->buttons[1] & DS_BUTTONS1_L1);
> > > +     input_report_key(ds->gamepad, BTN_TR,     ds_report->buttons[1] & DS_BUTTONS1_R1);
> > > +     input_report_key(ds->gamepad, BTN_TL2,    ds_report->buttons[1] & DS_BUTTONS1_L2);
> > > +     input_report_key(ds->gamepad, BTN_TR2,    ds_report->buttons[1] & DS_BUTTONS1_R2);
> > > +     input_report_key(ds->gamepad, BTN_SELECT, ds_report->buttons[1] & DS_BUTTONS1_CREATE);
> > > +     input_report_key(ds->gamepad, BTN_START,  ds_report->buttons[1] & DS_BUTTONS1_OPTIONS);
> > > +     input_report_key(ds->gamepad, BTN_THUMBL, ds_report->buttons[1] & DS_BUTTONS1_L3);
> > > +     input_report_key(ds->gamepad, BTN_THUMBR, ds_report->buttons[1] & DS_BUTTONS1_R3);
> > > +     input_report_key(ds->gamepad, BTN_MODE,   ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
> > > +     input_sync(ds->gamepad);
> > > +
> > > +     return 0;
> > > +}
> [...]


Regards,
Barnabás Pőcze

[PATCH v2 02/13] HID: playstation: use DualSense MAC address as unique identifier.

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Use the DualSense MAC address as a unique identifier for the HID device.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/hid-playstation.c | 40 +++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 3d5fe9069c26..af4a635997bb 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -21,12 +21,16 @@
 /* Base class for playstation devices. */
 struct ps_device {
 	struct hid_device *hdev;
+	uint8_t mac_address[6];
 
 	int (*parse_report)(struct ps_device *dev, struct hid_report *report, u8 *data, int size);
 };
 
 #define DS_INPUT_REPORT_USB			0x01
 
+#define DS_FEATURE_REPORT_PAIRING_INFO		9
+#define DS_FEATURE_REPORT_PAIRING_INFO_SIZE	19
+
 /* Button masks for DualSense input report. */
 #define DS_BUTTONS0_HAT_SWITCH	GENMASK(3, 0)
 #define DS_BUTTONS0_SQUARE	BIT(4)
@@ -132,6 +136,7 @@ static struct input_dev *ps_allocate_input_dev(struct hid_device *hdev, const ch
 	return input_dev;
 }
 
+
 static struct input_dev *ps_gamepad_create(struct hid_device *hdev)
 {
 	struct input_dev *gamepad;
@@ -162,6 +167,34 @@ static struct input_dev *ps_gamepad_create(struct hid_device *hdev)
 	return gamepad;
 }
 
+static int dualsense_get_mac_address(struct dualsense *ds)
+{
+	uint8_t *buf;
+	int ret = 0;
+
+	buf = kzalloc(DS_FEATURE_REPORT_PAIRING_INFO_SIZE, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_PAIRING_INFO, buf,
+			DS_FEATURE_REPORT_PAIRING_INFO_SIZE, HID_FEATURE_REPORT,
+			HID_REQ_GET_REPORT);
+	if (ret < 0)
+		goto err_free;
+	else if (ret != DS_FEATURE_REPORT_PAIRING_INFO_SIZE) {
+		hid_err(ds->base.hdev, "failed to retrieve DualSense pairing info\n");
+		ret = -EINVAL;
+		goto err_free;
+	}
+
+	/* Note MAC address is stored in little endian order. */
+	memcpy(ds->base.mac_address, &buf[1], sizeof(ds->base.mac_address));
+
+err_free:
+	kfree(buf);
+	return ret;
+}
+
 static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *report,
 		u8 *data, int size)
 {
@@ -230,6 +263,13 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	ds->base.parse_report = dualsense_parse_report;
 	hid_set_drvdata(hdev, ds);
 
+	ret = dualsense_get_mac_address(ds);
+	if (ret < 0) {
+		hid_err(hdev, "Failed to get MAC address from DualSense\n");
+		return ERR_PTR(ret);
+	}
+	snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds->base.mac_address);
+
 	ds->gamepad = ps_gamepad_create(hdev);
 	if (IS_ERR(ds->gamepad)) {
 		ret = PTR_ERR(ds->gamepad);
-- 
2.26.2

Re: [PATCH v2 02/13] HID: playstation: use DualSense MAC address as unique identifier.

[Barnabás Pőcze]

Hi


2021. január 2., szombat 23:30 keltezéssel, Roderick Colenbrander írta:

> From: Roderick Colenbrander roderick.colenbrander@sony.com
>
> Use the DualSense MAC address as a unique identifier for the HID device.
>
> Signed-off-by: Roderick Colenbrander roderick.colenbrander@sony.com
>
> [...]
>  /* Button masks for DualSense input report. */
>  #define DS_BUTTONS0_HAT_SWITCH	GENMASK(3, 0)
>  #define DS_BUTTONS0_SQUARE	BIT(4)
> @@ -132,6 +136,7 @@  static struct input_dev *ps_allocate_input_dev(struct hid_device *hdev, const ch
>  	return input_dev;
>  }
>
> +

I think this extra empty line is not necessary here.


>  static struct input_dev *ps_gamepad_create(struct hid_device *hdev)
>  {
>  	struct input_dev *gamepad;
> @@ -162,6 +167,34 @@  static struct input_dev *ps_gamepad_create(struct hid_device *hdev)
>  	return gamepad;
>  }
>
> +static int dualsense_get_mac_address(struct dualsense *ds)
> +{
> +	uint8_t *buf;
> +	int ret = 0;
> +
> +	buf = kzalloc(DS_FEATURE_REPORT_PAIRING_INFO_SIZE, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_PAIRING_INFO, buf,
> +			DS_FEATURE_REPORT_PAIRING_INFO_SIZE, HID_FEATURE_REPORT,
> +			HID_REQ_GET_REPORT);
> +	if (ret < 0)
> +		goto err_free;
> +	else if (ret != DS_FEATURE_REPORT_PAIRING_INFO_SIZE) {

As per coding style[1], please either use {} for all branches, or just drop the
`else` and maybe add a new line:

```
if (ret < 0)
  goto ...

if (ret != ...) {
  ...
}
```

> +		hid_err(ds->base.hdev, "failed to retrieve DualSense pairing info\n");

I think this message could be improved to better pinpoint the actual problem
that triggers it.


> +		ret = -EINVAL;
> +		goto err_free;
> +	}
> +
> +	/* Note MAC address is stored in little endian order. */

Maybe this comment would be better placed in `struct ps_device`?


> +	memcpy(ds->base.mac_address, &buf[1], sizeof(ds->base.mac_address));
> +
> +err_free:
> +	kfree(buf);
> +	return ret;
> +}
> [...]


[1]: https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces


Regards,
Barnabás Pőcze

[PATCH v2 03/13] HID: playstation: add DualSense battery support.

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Report DualSense battery status information through power_supply class.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/Kconfig           |   1 +
 drivers/hid/hid-playstation.c | 128 +++++++++++++++++++++++++++++++++-
 2 files changed, 126 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index d3258e806998..ef175c1cb15c 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -857,6 +857,7 @@ config HID_PLAYSTATION
 	tristate "PlayStation HID Driver"
 	default !EXPERT
 	depends on HID
+	select POWER_SUPPLY
 	help
 	  Provides support for Sony PS5 controllers including support for
 	  its special functionalities e.g. touchpad, lights and motion
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index af4a635997bb..2fc798d2a8af 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -21,6 +21,13 @@
 /* Base class for playstation devices. */
 struct ps_device {
 	struct hid_device *hdev;
+	spinlock_t lock;
+
+	struct power_supply_desc battery_desc;
+	struct power_supply *battery;
+	uint8_t battery_capacity;
+	int battery_status;
+
 	uint8_t mac_address[6];
 
 	int (*parse_report)(struct ps_device *dev, struct hid_report *report, u8 *data, int size);
@@ -48,6 +55,11 @@ struct ps_device {
 #define DS_BUTTONS2_PS_HOME	BIT(0)
 #define DS_BUTTONS2_TOUCHPAD	BIT(1)
 
+/* Status field of DualSense input report. */
+#define DS_STATUS_BATTERY_CAPACITY	GENMASK(3, 0)
+#define DS_STATUS_CHARGING		GENMASK(7, 4)
+#define DS_STATUS_CHARGING_SHIFT	4
+
 struct dualsense {
 	struct ps_device base;
 	struct input_dev *gamepad;
@@ -136,6 +148,71 @@ static struct input_dev *ps_allocate_input_dev(struct hid_device *hdev, const ch
 	return input_dev;
 }
 
+static enum power_supply_property ps_power_supply_props[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_SCOPE
+};
+
+static int ps_battery_get_property(struct power_supply *psy,
+		enum power_supply_property psp,
+		union power_supply_propval *val)
+{
+	struct ps_device *dev = power_supply_get_drvdata(psy);
+	uint8_t battery_capacity;
+	int battery_status;
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&dev->lock, flags);
+	battery_capacity = dev->battery_capacity;
+	battery_status = dev->battery_status;
+	spin_unlock_irqrestore(&dev->lock, flags);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		val->intval = battery_status;
+		break;
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = 1;
+		break;
+	case POWER_SUPPLY_PROP_CAPACITY:
+		val->intval = battery_capacity;
+		break;
+	case POWER_SUPPLY_PROP_SCOPE:
+		val->intval = POWER_SUPPLY_SCOPE_DEVICE;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return 0;
+}
+
+static int ps_device_register_battery(struct ps_device *dev)
+{
+	struct power_supply *battery;
+	struct power_supply_config battery_cfg = { .drv_data = dev };
+
+	dev->battery_desc.type = POWER_SUPPLY_TYPE_BATTERY;
+	dev->battery_desc.properties = ps_power_supply_props;
+	dev->battery_desc.num_properties = ARRAY_SIZE(ps_power_supply_props);
+	dev->battery_desc.get_property = ps_battery_get_property;
+	dev->battery_desc.name = devm_kasprintf(&dev->hdev->dev, GFP_KERNEL,
+			"ps-controller-battery-%pMR", dev->mac_address);
+
+	battery = devm_power_supply_register(&dev->hdev->dev, &dev->battery_desc, &battery_cfg);
+	if (IS_ERR(battery)) {
+		hid_err(dev->hdev, "Unable to register battery device.\n");
+		return PTR_ERR(battery);
+	}
+	dev->battery = battery;
+
+	power_supply_powers(dev->battery, &dev->hdev->dev);
+	return 0;
+}
 
 static struct input_dev *ps_gamepad_create(struct hid_device *hdev)
 {
@@ -201,7 +278,9 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
 	struct hid_device *hdev = ps_dev->hdev;
 	struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
 	struct dualsense_input_report *ds_report;
-	uint8_t value;
+	uint8_t battery_data, battery_capacity, charging_status, value;
+	int battery_status;
+	unsigned long flags;
 
 	/* DualSense in USB uses the full HID report for reportID 1, but
 	 * Bluetooth uses a minimal HID report for reportID 1 and reports
@@ -242,12 +321,48 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
 	input_report_key(ds->gamepad, BTN_MODE,   ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
 	input_sync(ds->gamepad);
 
+	battery_data = ds_report->status & DS_STATUS_BATTERY_CAPACITY;
+	charging_status = (ds_report->status & DS_STATUS_CHARGING) >> DS_STATUS_CHARGING_SHIFT;
+
+	switch (charging_status) {
+	case 0x0:
+		/* Each unit of battery data corresponds to 10%
+		 * 0 = 0-9%, 1 = 10-19%, .. and 10 = 100%
+		 */
+		battery_capacity = battery_data == 10 ? 100 : battery_data * 10 + 5;
+		battery_status = POWER_SUPPLY_STATUS_DISCHARGING;
+		break;
+	case 0x1:
+		battery_capacity = battery_data == 10 ? 100 : battery_data * 10 + 5;
+		battery_status = POWER_SUPPLY_STATUS_CHARGING;
+		break;
+	case 0x2:
+		battery_capacity = 100;
+		battery_status = POWER_SUPPLY_STATUS_FULL;
+		break;
+	case 0xa: /* voltage or temperature out of range */
+	case 0xb: /* temperature error */
+		battery_capacity = 0;
+		battery_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		break;
+	case 0xf: /* charging error */
+	default:
+		battery_capacity = 0;
+		battery_status = POWER_SUPPLY_STATUS_UNKNOWN;
+	}
+
+	spin_lock_irqsave(&ps_dev->lock, flags);
+	ps_dev->battery_capacity = battery_capacity;
+	ps_dev->battery_status = battery_status;
+	spin_unlock_irqrestore(&ps_dev->lock, flags);
+
 	return 0;
 }
 
 static struct ps_device *dualsense_create(struct hid_device *hdev)
 {
 	struct dualsense *ds;
+	struct ps_device *ps_dev;
 	int ret;
 
 	ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL);
@@ -259,8 +374,11 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	 */
 	hdev->version |= HID_PLAYSTATION_VERSION_PATCH;
 
-	ds->base.hdev = hdev;
-	ds->base.parse_report = dualsense_parse_report;
+	ps_dev = &ds->base;
+	ps_dev->hdev = hdev;
+	ps_dev->battery_capacity = 100; /* initial value until parse_report. */
+	ps_dev->battery_status = POWER_SUPPLY_STATUS_UNKNOWN;
+	ps_dev->parse_report = dualsense_parse_report;
 	hid_set_drvdata(hdev, ds);
 
 	ret = dualsense_get_mac_address(ds);
@@ -276,6 +394,10 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 		goto err;
 	}
 
+	ret = ps_device_register_battery(ps_dev);
+	if (ret < 0)
+		goto err;
+
 	return &ds->base;
 
 err:
-- 
2.26.2

Re: [PATCH v2 03/13] HID: playstation: add DualSense battery support.

[Barnabás Pőcze]

Hi


2021. január 2., szombat 23:30 keltezéssel, Roderick Colenbrander írta:

> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> Report DualSense battery status information through power_supply class.
>
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> [...]
> @@ -21,6 +21,13 @@
>  /* Base class for playstation devices. */
>  struct ps_device {
>  	struct hid_device *hdev;
> +	spinlock_t lock;

I'm not sure, but isn't a spin_lock_init() missing from the code?


> +
> +	struct power_supply_desc battery_desc;
> +	struct power_supply *battery;
> +	uint8_t battery_capacity;
> +	int battery_status;
> +
>  	uint8_t mac_address[6];
>
>  	int (*parse_report)(struct ps_device *dev, struct hid_report *report, u8 *data, int size);
> [...]
> @@ -136,6 +148,71 @@  static struct input_dev *ps_allocate_input_dev(struct hid_device *hdev, const ch
>  	return input_dev;
>  }
>
> +static enum power_supply_property ps_power_supply_props[] = {
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_CAPACITY,
> +	POWER_SUPPLY_PROP_SCOPE

I believe the preferred way is to have a comma after each array/enum/etc. element
unless it is a terminating entry.


> +};
> [...]
> @@ -201,7 +278,9 @@  static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
>  	struct hid_device *hdev = ps_dev->hdev;
>  	struct dualsense *ds = container_of(ps_dev, struct dualsense, base);
>  	struct dualsense_input_report *ds_report;
> -	uint8_t value;
> +	uint8_t battery_data, battery_capacity, charging_status, value;
> +	int battery_status;
> +	unsigned long flags;
>
>  	/* DualSense in USB uses the full HID report for reportID 1, but
>  	 * Bluetooth uses a minimal HID report for reportID 1 and reports
> @@ -242,12 +321,48 @@  static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
>  	input_report_key(ds->gamepad, BTN_MODE,   ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
>  	input_sync(ds->gamepad);
>
> +	battery_data = ds_report->status & DS_STATUS_BATTERY_CAPACITY;
> +	charging_status = (ds_report->status & DS_STATUS_CHARGING) >> DS_STATUS_CHARGING_SHIFT;
> +
> +	switch (charging_status) {
> +	case 0x0:
> +		/* Each unit of battery data corresponds to 10%
> +		 * 0 = 0-9%, 1 = 10-19%, .. and 10 = 100%
> +		 */
> +		battery_capacity = battery_data == 10 ? 100 : battery_data * 10 + 5;

In my opinion `min(battery_data * 10 + 5, 100)` seems cleaner, what do you think?


> +		battery_status = POWER_SUPPLY_STATUS_DISCHARGING;
> +		break;
> +	case 0x1:
> +		battery_capacity = battery_data == 10 ? 100 : battery_data * 10 + 5;
> +		battery_status = POWER_SUPPLY_STATUS_CHARGING;
> +		break;
> +	case 0x2:
> +		battery_capacity = 100;
> +		battery_status = POWER_SUPPLY_STATUS_FULL;
> +		break;
> +	case 0xa: /* voltage or temperature out of range */
> +	case 0xb: /* temperature error */
> +		battery_capacity = 0;
> +		battery_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +		break;
> +	case 0xf: /* charging error */
> +	default:
> +		battery_capacity = 0;
> +		battery_status = POWER_SUPPLY_STATUS_UNKNOWN;
> +	}
> +
> +	spin_lock_irqsave(&ps_dev->lock, flags);
> +	ps_dev->battery_capacity = battery_capacity;
> +	ps_dev->battery_status = battery_status;
> +	spin_unlock_irqrestore(&ps_dev->lock, flags);
> +
>  	return 0;
>  }
>
>  static struct ps_device *dualsense_create(struct hid_device *hdev)
>  {
>  	struct dualsense *ds;
> +	struct ps_device *ps_dev;
>  	int ret;
>
>  	ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL);
> @@ -259,8 +374,11 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
>  	 */
>  	hdev->version |= HID_PLAYSTATION_VERSION_PATCH;
>
> -	ds->base.hdev = hdev;
> -	ds->base.parse_report = dualsense_parse_report;
> +	ps_dev = &ds->base;
> +	ps_dev->hdev = hdev;
> +	ps_dev->battery_capacity = 100; /* initial value until parse_report. */
> +	ps_dev->battery_status = POWER_SUPPLY_STATUS_UNKNOWN;
> +	ps_dev->parse_report = dualsense_parse_report;
>  	hid_set_drvdata(hdev, ds);
>
>  	ret = dualsense_get_mac_address(ds);
> @@ -276,6 +394,10 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
>  		goto err;
>  	}
>
> +	ret = ps_device_register_battery(ps_dev);
> +	if (ret < 0)

I believe if `ps_device_register_battery()` can only return 0 on success or
an errno, then `if (ret)` would be better.


> +		goto err;
> +
>  	return &ds->base;
>
>  err:


Regards,
Barnabás Pőcze

[PATCH v2 04/13] HID: playstation: add DualSense touchpad support.

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Implement support for DualSense touchpad as a separate input device.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/hid-playstation.c | 64 +++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 2fc798d2a8af..06e946c45c64 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -60,9 +60,20 @@ struct ps_device {
 #define DS_STATUS_CHARGING		GENMASK(7, 4)
 #define DS_STATUS_CHARGING_SHIFT	4
 
+/* Status of a DualSense touch point contact.
+ * Contact IDs, with highest bit set are 'inactive'
+ * and any associated data is then invalid.
+ */
+#define DS_TOUCH_POINT_INACTIVE BIT(7)
+
+/* DualSense hardware limits */
+#define DS_TOUCHPAD_WIDTH	1920
+#define DS_TOUCHPAD_HEIGHT	1080
+
 struct dualsense {
 	struct ps_device base;
 	struct input_dev *gamepad;
+	struct input_dev *touchpad;
 };
 
 struct dualsense_touch_point {
@@ -244,6 +255,34 @@ static struct input_dev *ps_gamepad_create(struct hid_device *hdev)
 	return gamepad;
 }
 
+static struct input_dev *ps_touchpad_create(struct hid_device *hdev, int width, int height,
+		unsigned int num_contacts)
+{
+	struct input_dev *touchpad;
+	int ret;
+
+	touchpad = ps_allocate_input_dev(hdev, "Touchpad");
+	if (IS_ERR(touchpad))
+		return ERR_PTR(-ENOMEM);
+
+	/* Map button underneath touchpad to BTN_LEFT. */
+	input_set_capability(touchpad, EV_KEY, BTN_LEFT);
+	__set_bit(INPUT_PROP_BUTTONPAD, touchpad->propbit);
+
+	input_set_abs_params(touchpad, ABS_MT_POSITION_X, 0, width - 1, 0, 0);
+	input_set_abs_params(touchpad, ABS_MT_POSITION_Y, 0, height - 1, 0, 0);
+
+	ret = input_mt_init_slots(touchpad, num_contacts, INPUT_MT_POINTER);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = input_register_device(touchpad);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return touchpad;
+}
+
 static int dualsense_get_mac_address(struct dualsense *ds)
 {
 	uint8_t *buf;
@@ -281,6 +320,7 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
 	uint8_t battery_data, battery_capacity, charging_status, value;
 	int battery_status;
 	unsigned long flags;
+	int i;
 
 	/* DualSense in USB uses the full HID report for reportID 1, but
 	 * Bluetooth uses a minimal HID report for reportID 1 and reports
@@ -321,6 +361,24 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
 	input_report_key(ds->gamepad, BTN_MODE,   ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
 	input_sync(ds->gamepad);
 
+	for (i = 0; i < 2; i++) {
+		bool active = (ds_report->points[i].contact & DS_TOUCH_POINT_INACTIVE) ? false : true;
+
+		input_mt_slot(ds->touchpad, i);
+		input_mt_report_slot_state(ds->touchpad, MT_TOOL_FINGER, active);
+
+		if (active) {
+			int x = (ds_report->points[i].x_hi << 8) | ds_report->points[i].x_lo;
+			int y = (ds_report->points[i].y_hi << 4) | ds_report->points[i].y_lo;
+
+			input_report_abs(ds->touchpad, ABS_MT_POSITION_X, x);
+			input_report_abs(ds->touchpad, ABS_MT_POSITION_Y, y);
+		}
+	}
+	input_mt_sync_frame(ds->touchpad);
+	input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
+	input_sync(ds->touchpad);
+
 	battery_data = ds_report->status & DS_STATUS_BATTERY_CAPACITY;
 	charging_status = (ds_report->status & DS_STATUS_CHARGING) >> DS_STATUS_CHARGING_SHIFT;
 
@@ -394,6 +452,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 		goto err;
 	}
 
+	ds->touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
+	if (IS_ERR(ds->touchpad)) {
+		ret = PTR_ERR(ds->touchpad);
+		goto err;
+	}
+
 	ret = ps_device_register_battery(ps_dev);
 	if (ret < 0)
 		goto err;
-- 
2.26.2

Re: [PATCH v2 04/13] HID: playstation: add DualSense touchpad support.

[Barnabás Pőcze]

Hi


2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:

> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> Implement support for DualSense touchpad as a separate input device.
>
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> [...]
> +static struct input_dev *ps_touchpad_create(struct hid_device *hdev, int width, int height,
> +		unsigned int num_contacts)
> +{
> +	struct input_dev *touchpad;
> +	int ret;
> +
> +	touchpad = ps_allocate_input_dev(hdev, "Touchpad");
> +	if (IS_ERR(touchpad))
> +		return ERR_PTR(-ENOMEM);

I know that at the moment ENOMEM is the only possible error, but I believe
`return ERR_CAST(touchpad);` would be better. (Or even just `return touchpad;`.)


> +
> +	/* Map button underneath touchpad to BTN_LEFT. */
> +	input_set_capability(touchpad, EV_KEY, BTN_LEFT);
> +	__set_bit(INPUT_PROP_BUTTONPAD, touchpad->propbit);
> +
> +	input_set_abs_params(touchpad, ABS_MT_POSITION_X, 0, width - 1, 0, 0);
> +	input_set_abs_params(touchpad, ABS_MT_POSITION_Y, 0, height - 1, 0, 0);
> +
> +	ret = input_mt_init_slots(touchpad, num_contacts, INPUT_MT_POINTER);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ret = input_register_device(touchpad);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return touchpad;
> +}
> [...]

[PATCH v2 05/13] HID: playstation: add DualSense accelerometer and gyroscope support.

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

The DualSense features an accelerometer and gyroscope. The data is
embedded into the main HID input reports. Expose both sensors through
through a separate evdev node.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/hid-playstation.c | 202 ++++++++++++++++++++++++++++++++++
 1 file changed, 202 insertions(+)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 06e946c45c64..80b0dcdbfe41 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -33,8 +33,18 @@ struct ps_device {
 	int (*parse_report)(struct ps_device *dev, struct hid_report *report, u8 *data, int size);
 };
 
+/* Calibration data for playstation motion sensors. */
+struct ps_calibration_data {
+	int abs_code;
+	short bias;
+	int sens_numer;
+	int sens_denom;
+};
+
 #define DS_INPUT_REPORT_USB			0x01
 
+#define DS_FEATURE_REPORT_CALIBRATION		5
+#define DS_FEATURE_REPORT_CALIBRATION_SIZE	41
 #define DS_FEATURE_REPORT_PAIRING_INFO		9
 #define DS_FEATURE_REPORT_PAIRING_INFO_SIZE	19
 
@@ -67,13 +77,27 @@ struct ps_device {
 #define DS_TOUCH_POINT_INACTIVE BIT(7)
 
 /* DualSense hardware limits */
+#define DS_ACC_RES_PER_G	8192
+#define DS_ACC_RANGE		(4*DS_ACC_RES_PER_G)
+#define DS_GYRO_RES_PER_DEG_S	1024
+#define DS_GYRO_RANGE		(2048*DS_GYRO_RES_PER_DEG_S)
 #define DS_TOUCHPAD_WIDTH	1920
 #define DS_TOUCHPAD_HEIGHT	1080
 
 struct dualsense {
 	struct ps_device base;
 	struct input_dev *gamepad;
+	struct input_dev *sensors;
 	struct input_dev *touchpad;
+
+	/* Calibration data for accelerometer and gyroscope. */
+	struct ps_calibration_data accel_calib_data[3];
+	struct ps_calibration_data gyro_calib_data[3];
+
+	/* Timestamp for sensor data */
+	bool sensor_timestamp_initialized;
+	uint32_t prev_sensor_timestamp;
+	uint32_t sensor_timestamp_us;
 };
 
 struct dualsense_touch_point {
@@ -255,6 +279,41 @@ static struct input_dev *ps_gamepad_create(struct hid_device *hdev)
 	return gamepad;
 }
 
+static struct input_dev *ps_sensors_create(struct hid_device *hdev, int accel_range, int accel_res,
+		int gyro_range, int gyro_res)
+{
+	struct input_dev *sensors;
+	int ret;
+
+	sensors = ps_allocate_input_dev(hdev, "Motion Sensors");
+	if (IS_ERR(sensors))
+		return ERR_PTR(-ENOMEM);
+
+	__set_bit(INPUT_PROP_ACCELEROMETER, sensors->propbit);
+
+	/* Accelerometer */
+	input_set_abs_params(sensors, ABS_X, -accel_range, accel_range, 16, 0);
+	input_set_abs_params(sensors, ABS_Y, -accel_range, accel_range, 16, 0);
+	input_set_abs_params(sensors, ABS_Z, -accel_range, accel_range, 16, 0);
+	input_abs_set_res(sensors, ABS_X, accel_res);
+	input_abs_set_res(sensors, ABS_Y, accel_res);
+	input_abs_set_res(sensors, ABS_Z, accel_res);
+
+	/* Gyroscope */
+	input_set_abs_params(sensors, ABS_RX, -gyro_range, gyro_range, 16, 0);
+	input_set_abs_params(sensors, ABS_RY, -gyro_range, gyro_range, 16, 0);
+	input_set_abs_params(sensors, ABS_RZ, -gyro_range, gyro_range, 16, 0);
+	input_abs_set_res(sensors, ABS_RX, gyro_res);
+	input_abs_set_res(sensors, ABS_RY, gyro_res);
+	input_abs_set_res(sensors, ABS_RZ, gyro_res);
+
+	ret = input_register_device(sensors);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return sensors;
+}
+
 static struct input_dev *ps_touchpad_create(struct hid_device *hdev, int width, int height,
 		unsigned int num_contacts)
 {
@@ -283,6 +342,97 @@ static struct input_dev *ps_touchpad_create(struct hid_device *hdev, int width,
 	return touchpad;
 }
 
+static int dualsense_get_calibration_data(struct dualsense *ds)
+{
+	short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
+	short gyro_yaw_bias, gyro_yaw_plus, gyro_yaw_minus;
+	short gyro_roll_bias, gyro_roll_plus, gyro_roll_minus;
+	short gyro_speed_plus, gyro_speed_minus;
+	short acc_x_plus, acc_x_minus;
+	short acc_y_plus, acc_y_minus;
+	short acc_z_plus, acc_z_minus;
+	int speed_2x;
+	int range_2g;
+	int ret = 0;
+	uint8_t *buf;
+
+	buf = kzalloc(DS_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_CALIBRATION, buf,
+			DS_FEATURE_REPORT_CALIBRATION_SIZE, HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+	if (ret < 0)
+		goto err_free;
+	else if (ret != DS_FEATURE_REPORT_CALIBRATION_SIZE) {
+		hid_err(ds->base.hdev, "failed to retrieve DualSense calibration info\n");
+		ret = -EINVAL;
+		goto err_free;
+	}
+
+	gyro_pitch_bias  = get_unaligned_le16(&buf[1]);
+	gyro_yaw_bias    = get_unaligned_le16(&buf[3]);
+	gyro_roll_bias   = get_unaligned_le16(&buf[5]);
+	gyro_pitch_plus  = get_unaligned_le16(&buf[7]);
+	gyro_pitch_minus = get_unaligned_le16(&buf[9]);
+	gyro_yaw_plus    = get_unaligned_le16(&buf[11]);
+	gyro_yaw_minus   = get_unaligned_le16(&buf[13]);
+	gyro_roll_plus   = get_unaligned_le16(&buf[15]);
+	gyro_roll_minus  = get_unaligned_le16(&buf[17]);
+	gyro_speed_plus  = get_unaligned_le16(&buf[19]);
+	gyro_speed_minus = get_unaligned_le16(&buf[21]);
+	acc_x_plus       = get_unaligned_le16(&buf[23]);
+	acc_x_minus      = get_unaligned_le16(&buf[25]);
+	acc_y_plus       = get_unaligned_le16(&buf[27]);
+	acc_y_minus      = get_unaligned_le16(&buf[29]);
+	acc_z_plus       = get_unaligned_le16(&buf[31]);
+	acc_z_minus      = get_unaligned_le16(&buf[33]);
+
+	/* Set gyroscope calibration and normalization parameters.
+	 * Data values will be normalized to 1/DS_GYRO_RES_PER_DEG_S degree/s.
+	 */
+	speed_2x = (gyro_speed_plus + gyro_speed_minus);
+	ds->gyro_calib_data[0].abs_code = ABS_RX;
+	ds->gyro_calib_data[0].bias = gyro_pitch_bias;
+	ds->gyro_calib_data[0].sens_numer = speed_2x*DS_GYRO_RES_PER_DEG_S;
+	ds->gyro_calib_data[0].sens_denom = gyro_pitch_plus - gyro_pitch_minus;
+
+	ds->gyro_calib_data[1].abs_code = ABS_RY;
+	ds->gyro_calib_data[1].bias = gyro_yaw_bias;
+	ds->gyro_calib_data[1].sens_numer = speed_2x*DS_GYRO_RES_PER_DEG_S;
+	ds->gyro_calib_data[1].sens_denom = gyro_yaw_plus - gyro_yaw_minus;
+
+	ds->gyro_calib_data[2].abs_code = ABS_RZ;
+	ds->gyro_calib_data[2].bias = gyro_roll_bias;
+	ds->gyro_calib_data[2].sens_numer = speed_2x*DS_GYRO_RES_PER_DEG_S;
+	ds->gyro_calib_data[2].sens_denom = gyro_roll_plus - gyro_roll_minus;
+
+	/* Set accelerometer calibration and normalization parameters.
+	 * Data values will be normalized to 1/DS_ACC_RES_PER_G G.
+	 */
+	range_2g = acc_x_plus - acc_x_minus;
+	ds->accel_calib_data[0].abs_code = ABS_X;
+	ds->accel_calib_data[0].bias = acc_x_plus - range_2g / 2;
+	ds->accel_calib_data[0].sens_numer = 2*DS_ACC_RES_PER_G;
+	ds->accel_calib_data[0].sens_denom = range_2g;
+
+	range_2g = acc_y_plus - acc_y_minus;
+	ds->accel_calib_data[1].abs_code = ABS_Y;
+	ds->accel_calib_data[1].bias = acc_y_plus - range_2g / 2;
+	ds->accel_calib_data[1].sens_numer = 2*DS_ACC_RES_PER_G;
+	ds->accel_calib_data[1].sens_denom = range_2g;
+
+	range_2g = acc_z_plus - acc_z_minus;
+	ds->accel_calib_data[2].abs_code = ABS_Z;
+	ds->accel_calib_data[2].bias = acc_z_plus - range_2g / 2;
+	ds->accel_calib_data[2].sens_numer = 2*DS_ACC_RES_PER_G;
+	ds->accel_calib_data[2].sens_denom = range_2g;
+
+err_free:
+	kfree(buf);
+	return ret;
+}
+
 static int dualsense_get_mac_address(struct dualsense *ds)
 {
 	uint8_t *buf;
@@ -319,6 +469,7 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
 	struct dualsense_input_report *ds_report;
 	uint8_t battery_data, battery_capacity, charging_status, value;
 	int battery_status;
+	uint16_t sensor_timestamp;
 	unsigned long flags;
 	int i;
 
@@ -361,6 +512,44 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
 	input_report_key(ds->gamepad, BTN_MODE,   ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
 	input_sync(ds->gamepad);
 
+	/* Parse and calibrate gyroscope data. */
+	for (i = 0; i < 3; i++) {
+		int raw_data = (short)le16_to_cpu(ds_report->gyro[i]);
+		int calib_data = mult_frac(ds->gyro_calib_data[i].sens_numer,
+				raw_data - ds->gyro_calib_data[i].bias,
+				ds->gyro_calib_data[i].sens_denom);
+
+		input_report_abs(ds->sensors, ds->gyro_calib_data[i].abs_code, calib_data);
+	}
+
+	/* Parse and calibrate accelerometer data. */
+	for (i = 0; i < 3; i++) {
+		int raw_data = (short)le16_to_cpu(ds_report->accel[i]);
+		int calib_data = mult_frac(ds->accel_calib_data[i].sens_numer,
+				raw_data - ds->accel_calib_data[i].bias,
+				ds->accel_calib_data[i].sens_denom);
+
+		input_report_abs(ds->sensors, ds->accel_calib_data[i].abs_code, calib_data);
+	}
+
+	/* Convert timestamp (in 0.33us unit) to timestamp_us */
+	sensor_timestamp = le32_to_cpu(ds_report->sensor_timestamp);
+	if (!ds->sensor_timestamp_initialized) {
+		ds->sensor_timestamp_us = sensor_timestamp / 3;
+		ds->sensor_timestamp_initialized = true;
+	} else {
+		uint32_t delta;
+
+		if (ds->prev_sensor_timestamp > sensor_timestamp)
+			delta = (U32_MAX - ds->prev_sensor_timestamp + sensor_timestamp + 1);
+		else
+			delta = sensor_timestamp - ds->prev_sensor_timestamp;
+		ds->sensor_timestamp_us += delta / 3;
+	}
+	ds->prev_sensor_timestamp = sensor_timestamp;
+	input_event(ds->sensors, EV_MSC, MSC_TIMESTAMP, ds->sensor_timestamp_us);
+	input_sync(ds->sensors);
+
 	for (i = 0; i < 2; i++) {
 		bool active = (ds_report->points[i].contact & DS_TOUCH_POINT_INACTIVE) ? false : true;
 
@@ -446,12 +635,25 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	}
 	snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds->base.mac_address);
 
+	ret = dualsense_get_calibration_data(ds);
+	if (ret < 0) {
+		hid_err(hdev, "Failed to get calibration data from DualSense\n");
+		goto err;
+	}
+
 	ds->gamepad = ps_gamepad_create(hdev);
 	if (IS_ERR(ds->gamepad)) {
 		ret = PTR_ERR(ds->gamepad);
 		goto err;
 	}
 
+	ds->sensors = ps_sensors_create(hdev, DS_ACC_RANGE, DS_ACC_RES_PER_G,
+			DS_GYRO_RANGE, DS_GYRO_RES_PER_DEG_S);
+	if (IS_ERR(ds->sensors)) {
+		ret = PTR_ERR(ds->sensors);
+		goto err;
+	}
+
 	ds->touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
 	if (IS_ERR(ds->touchpad)) {
 		ret = PTR_ERR(ds->touchpad);
-- 
2.26.2

Re: [PATCH v2 05/13] HID: playstation: add DualSense accelerometer and gyroscope support.

[Florian Märkl]

> +static struct input_dev *ps_sensors_create(struct hid_device *hdev, int accel_range, int accel_res,
> +		int gyro_range, int gyro_res)
> +{
> +	struct input_dev *sensors;
> +	int ret;
> +
> +	sensors = ps_allocate_input_dev(hdev, "Motion Sensors");
> +	if (IS_ERR(sensors))
> +		return ERR_PTR(-ENOMEM);
> +
> +	__set_bit(INPUT_PROP_ACCELEROMETER, sensors->propbit);
> +
> +	/* Accelerometer */
> +	input_set_abs_params(sensors, ABS_X, -accel_range, accel_range, 16, 0);
> +	input_set_abs_params(sensors, ABS_Y, -accel_range, accel_range, 16, 0);
> +	input_set_abs_params(sensors, ABS_Z, -accel_range, accel_range, 16, 0);
> +	input_abs_set_res(sensors, ABS_X, accel_res);
> +	input_abs_set_res(sensors, ABS_Y, accel_res);
> +	input_abs_set_res(sensors, ABS_Z, accel_res);
> +
> +	/* Gyroscope */
> +	input_set_abs_params(sensors, ABS_RX, -gyro_range, gyro_range, 16, 0);
> +	input_set_abs_params(sensors, ABS_RY, -gyro_range, gyro_range, 16, 0);
> +	input_set_abs_params(sensors, ABS_RZ, -gyro_range, gyro_range, 16, 0);
> +	input_abs_set_res(sensors, ABS_RX, gyro_res);
> +	input_abs_set_res(sensors, ABS_RY, gyro_res);
> +	input_abs_set_res(sensors, ABS_RZ, gyro_res);
> +
> +	ret = input_register_device(sensors);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return sensors;
> +}

The bits for EV_MSC/MSC_TIMESTAMP events are not set here, hence
timestamp events would not delivered:

	__set_bit(EV_MSC, sensors->evbit);
	__set_bit(MSC_TIMESTAMP, sensors->mscbit);


>  static int dualsense_get_mac_address(struct dualsense *ds)
>  {
>  	uint8_t *buf;
> @@ -319,6 +469,7 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
>  	struct dualsense_input_report *ds_report;
>  	uint8_t battery_data, battery_capacity, charging_status, value;
>  	int battery_status;
> +	uint16_t sensor_timestamp;

This uint16_t variable overflows just after a few events. Since the
timestamp from the controller is 32bit and the event value too, I assume
this should be too.

-- 
Florian Märkl
https://metallic.software

Re: [PATCH v2 05/13] HID: playstation: add DualSense accelerometer and gyroscope support.

[Roderick Colenbrander]

Hi Florian,

Thanks for your comments.

On Thu, Jan 7, 2021 at 5:34 AM Florian Märkl <linux@florianmaerkl.de> wrote:
>
> > +static struct input_dev *ps_sensors_create(struct hid_device *hdev, int accel_range, int accel_res,
> > +             int gyro_range, int gyro_res)
> > +{
> > +     struct input_dev *sensors;
> > +     int ret;
> > +
> > +     sensors = ps_allocate_input_dev(hdev, "Motion Sensors");
> > +     if (IS_ERR(sensors))
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     __set_bit(INPUT_PROP_ACCELEROMETER, sensors->propbit);
> > +
> > +     /* Accelerometer */
> > +     input_set_abs_params(sensors, ABS_X, -accel_range, accel_range, 16, 0);
> > +     input_set_abs_params(sensors, ABS_Y, -accel_range, accel_range, 16, 0);
> > +     input_set_abs_params(sensors, ABS_Z, -accel_range, accel_range, 16, 0);
> > +     input_abs_set_res(sensors, ABS_X, accel_res);
> > +     input_abs_set_res(sensors, ABS_Y, accel_res);
> > +     input_abs_set_res(sensors, ABS_Z, accel_res);
> > +
> > +     /* Gyroscope */
> > +     input_set_abs_params(sensors, ABS_RX, -gyro_range, gyro_range, 16, 0);
> > +     input_set_abs_params(sensors, ABS_RY, -gyro_range, gyro_range, 16, 0);
> > +     input_set_abs_params(sensors, ABS_RZ, -gyro_range, gyro_range, 16, 0);
> > +     input_abs_set_res(sensors, ABS_RX, gyro_res);
> > +     input_abs_set_res(sensors, ABS_RY, gyro_res);
> > +     input_abs_set_res(sensors, ABS_RZ, gyro_res);
> > +
> > +     ret = input_register_device(sensors);
> > +     if (ret)
> > +             return ERR_PTR(ret);
> > +
> > +     return sensors;
> > +}
>
> The bits for EV_MSC/MSC_TIMESTAMP events are not set here, hence
> timestamp events would not delivered:
>
>         __set_bit(EV_MSC, sensors->evbit);
>         __set_bit(MSC_TIMESTAMP, sensors->mscbit);

I don't know how I missed this... I remember adding it, but nope
apparently not :(

> >  static int dualsense_get_mac_address(struct dualsense *ds)
> >  {
> >       uint8_t *buf;
> > @@ -319,6 +469,7 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
> >       struct dualsense_input_report *ds_report;
> >       uint8_t battery_data, battery_capacity, charging_status, value;
> >       int battery_status;
> > +     uint16_t sensor_timestamp;
>
> This uint16_t variable overflows just after a few events. Since the
> timestamp from the controller is 32bit and the event value too, I assume
> this should be too.

Agreed.

>
> --
> Florian Märkl
> https://metallic.software

Regards,
Roderick

Re: [PATCH v2 05/13] HID: playstation: add DualSense accelerometer and gyroscope support.

[Barnabás Pőcze]

Hi


2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:

> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> The DualSense features an accelerometer and gyroscope. The data is
> embedded into the main HID input reports. Expose both sensors through
> through a separate evdev node.
>
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> [...]
> +static struct input_dev *ps_sensors_create(struct hid_device *hdev, int accel_range, int accel_res,
> +		int gyro_range, int gyro_res)
> +{
> +	struct input_dev *sensors;
> +	int ret;
> +
> +	sensors = ps_allocate_input_dev(hdev, "Motion Sensors");
> +	if (IS_ERR(sensors))
> +		return ERR_PTR(-ENOMEM);

I know that at the moment ENOMEM is the only possible error, but I believe
`return ERR_CAST(sensors);` would be better. (Or even just `return sensors;`.)


> +
> +	__set_bit(INPUT_PROP_ACCELEROMETER, sensors->propbit);
> +
> +	/* Accelerometer */
> +	input_set_abs_params(sensors, ABS_X, -accel_range, accel_range, 16, 0);
> +	input_set_abs_params(sensors, ABS_Y, -accel_range, accel_range, 16, 0);
> +	input_set_abs_params(sensors, ABS_Z, -accel_range, accel_range, 16, 0);
> +	input_abs_set_res(sensors, ABS_X, accel_res);
> +	input_abs_set_res(sensors, ABS_Y, accel_res);
> +	input_abs_set_res(sensors, ABS_Z, accel_res);
> +
> +	/* Gyroscope */
> +	input_set_abs_params(sensors, ABS_RX, -gyro_range, gyro_range, 16, 0);
> +	input_set_abs_params(sensors, ABS_RY, -gyro_range, gyro_range, 16, 0);
> +	input_set_abs_params(sensors, ABS_RZ, -gyro_range, gyro_range, 16, 0);
> +	input_abs_set_res(sensors, ABS_RX, gyro_res);
> +	input_abs_set_res(sensors, ABS_RY, gyro_res);
> +	input_abs_set_res(sensors, ABS_RZ, gyro_res);
> +
> +	ret = input_register_device(sensors);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return sensors;
> +}
> [...]
> +static int dualsense_get_calibration_data(struct dualsense *ds)
> +{
> +	short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
> +	short gyro_yaw_bias, gyro_yaw_plus, gyro_yaw_minus;
> +	short gyro_roll_bias, gyro_roll_plus, gyro_roll_minus;
> +	short gyro_speed_plus, gyro_speed_minus;
> +	short acc_x_plus, acc_x_minus;
> +	short acc_y_plus, acc_y_minus;
> +	short acc_z_plus, acc_z_minus;
> +	int speed_2x;
> +	int range_2g;
> +	int ret = 0;
> +	uint8_t *buf;
> +
> +	buf = kzalloc(DS_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_CALIBRATION, buf,
> +			DS_FEATURE_REPORT_CALIBRATION_SIZE, HID_FEATURE_REPORT, HID_REQ_GET_REPORT);

I think it would be better if lines were aligned. I have missed this in other patches,
so if you decide to make this change, please do it everywhere.


> +	if (ret < 0)
> +		goto err_free;
> +	else if (ret != DS_FEATURE_REPORT_CALIBRATION_SIZE) {

As per coding style[1], please either use {} for all branches, or just drop the
`else` and maybe add a new line:

```
if (ret < 0)
  goto ...

if (ret != ...) {
  ...
}
```


> +		hid_err(ds->base.hdev, "failed to retrieve DualSense calibration info\n");

I think this message could be improved to better pinpoint the exact problem
that triggered it.


> +		ret = -EINVAL;
> +		goto err_free;
> +	}
> +
> +	gyro_pitch_bias  = get_unaligned_le16(&buf[1]);
> +	gyro_yaw_bias    = get_unaligned_le16(&buf[3]);
> +	gyro_roll_bias   = get_unaligned_le16(&buf[5]);
> +	gyro_pitch_plus  = get_unaligned_le16(&buf[7]);
> +	gyro_pitch_minus = get_unaligned_le16(&buf[9]);
> +	gyro_yaw_plus    = get_unaligned_le16(&buf[11]);
> +	gyro_yaw_minus   = get_unaligned_le16(&buf[13]);
> +	gyro_roll_plus   = get_unaligned_le16(&buf[15]);
> +	gyro_roll_minus  = get_unaligned_le16(&buf[17]);
> +	gyro_speed_plus  = get_unaligned_le16(&buf[19]);
> +	gyro_speed_minus = get_unaligned_le16(&buf[21]);
> +	acc_x_plus       = get_unaligned_le16(&buf[23]);
> +	acc_x_minus      = get_unaligned_le16(&buf[25]);
> +	acc_y_plus       = get_unaligned_le16(&buf[27]);
> +	acc_y_minus      = get_unaligned_le16(&buf[29]);
> +	acc_z_plus       = get_unaligned_le16(&buf[31]);
> +	acc_z_minus      = get_unaligned_le16(&buf[33]);
> +
> +	/* Set gyroscope calibration and normalization parameters.
> +	 * Data values will be normalized to 1/DS_GYRO_RES_PER_DEG_S degree/s.
> +	 */

A small note, as written in [2], the preferred style of multi-line comments is different,
so you might want to change the comments. If you decide to make this change, please
do it everywhere.


> +	speed_2x = (gyro_speed_plus + gyro_speed_minus);
> +	ds->gyro_calib_data[0].abs_code = ABS_RX;
> +	ds->gyro_calib_data[0].bias = gyro_pitch_bias;
> +	ds->gyro_calib_data[0].sens_numer = speed_2x*DS_GYRO_RES_PER_DEG_S;
> +	ds->gyro_calib_data[0].sens_denom = gyro_pitch_plus - gyro_pitch_minus;
> +
> +	ds->gyro_calib_data[1].abs_code = ABS_RY;
> +	ds->gyro_calib_data[1].bias = gyro_yaw_bias;
> +	ds->gyro_calib_data[1].sens_numer = speed_2x*DS_GYRO_RES_PER_DEG_S;
> +	ds->gyro_calib_data[1].sens_denom = gyro_yaw_plus - gyro_yaw_minus;
> +
> +	ds->gyro_calib_data[2].abs_code = ABS_RZ;
> +	ds->gyro_calib_data[2].bias = gyro_roll_bias;
> +	ds->gyro_calib_data[2].sens_numer = speed_2x*DS_GYRO_RES_PER_DEG_S;
> +	ds->gyro_calib_data[2].sens_denom = gyro_roll_plus - gyro_roll_minus;
> +
> +	/* Set accelerometer calibration and normalization parameters.
> +	 * Data values will be normalized to 1/DS_ACC_RES_PER_G G.
                                                                ^
Minor thing, but I believe it should be 'g', not 'G'?


> +	 */
> +	range_2g = acc_x_plus - acc_x_minus;
> +	ds->accel_calib_data[0].abs_code = ABS_X;
> +	ds->accel_calib_data[0].bias = acc_x_plus - range_2g / 2;
> +	ds->accel_calib_data[0].sens_numer = 2*DS_ACC_RES_PER_G;
> +	ds->accel_calib_data[0].sens_denom = range_2g;
> +
> +	range_2g = acc_y_plus - acc_y_minus;
> +	ds->accel_calib_data[1].abs_code = ABS_Y;
> +	ds->accel_calib_data[1].bias = acc_y_plus - range_2g / 2;
> +	ds->accel_calib_data[1].sens_numer = 2*DS_ACC_RES_PER_G;
> +	ds->accel_calib_data[1].sens_denom = range_2g;
> +
> +	range_2g = acc_z_plus - acc_z_minus;
> +	ds->accel_calib_data[2].abs_code = ABS_Z;
> +	ds->accel_calib_data[2].bias = acc_z_plus - range_2g / 2;
> +	ds->accel_calib_data[2].sens_numer = 2*DS_ACC_RES_PER_G;
> +	ds->accel_calib_data[2].sens_denom = range_2g;
> +
> +err_free:
> +	kfree(buf);
> +	return ret;
> +}
> +
>  static int dualsense_get_mac_address(struct dualsense *ds)
>  {
>  	uint8_t *buf;
> @@ -319,6 +469,7 @@  static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
>  	struct dualsense_input_report *ds_report;
>  	uint8_t battery_data, battery_capacity, charging_status, value;
>  	int battery_status;
> +	uint16_t sensor_timestamp;
>  	unsigned long flags;
>  	int i;
>
> @@ -361,6 +512,44 @@  static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
>  	input_report_key(ds->gamepad, BTN_MODE,   ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
>  	input_sync(ds->gamepad);
>
> +	/* Parse and calibrate gyroscope data. */
> +	for (i = 0; i < 3; i++) {

I think `i < ARRAY_SIZE(...)` would be better.
And I would add a `static_assert(ARRAY_SIZE(ds_report->gyro) == ARRAY_SIZE(ds->gyro_calib_data))`
somewhere around here just to be safe. Or define a new constant like `DS_GYRO_DIMS`
and use that to define the arrays. Or both. *


> +		int raw_data = (short)le16_to_cpu(ds_report->gyro[i]);
> +		int calib_data = mult_frac(ds->gyro_calib_data[i].sens_numer,
> +				raw_data - ds->gyro_calib_data[i].bias,
> +				ds->gyro_calib_data[i].sens_denom);

I believe it would be better if the second and third lines was aligned. **


> +
> +		input_report_abs(ds->sensors, ds->gyro_calib_data[i].abs_code, calib_data);
> +	}
> +
> +	/* Parse and calibrate accelerometer data. */
> +	for (i = 0; i < 3; i++) {

Same here. *


> +		int raw_data = (short)le16_to_cpu(ds_report->accel[i]);
> +		int calib_data = mult_frac(ds->accel_calib_data[i].sens_numer,
> +				raw_data - ds->accel_calib_data[i].bias,
> +				ds->accel_calib_data[i].sens_denom);

Same here. **


> +
> +		input_report_abs(ds->sensors, ds->accel_calib_data[i].abs_code, calib_data);
> +	}
> +
> +	/* Convert timestamp (in 0.33us unit) to timestamp_us */
> +	sensor_timestamp = le32_to_cpu(ds_report->sensor_timestamp);
> +	if (!ds->sensor_timestamp_initialized) {
> +		ds->sensor_timestamp_us = sensor_timestamp / 3;
> +		ds->sensor_timestamp_initialized = true;
> +	} else {
> +		uint32_t delta;
> +
> +		if (ds->prev_sensor_timestamp > sensor_timestamp)
> +			delta = (U32_MAX - ds->prev_sensor_timestamp + sensor_timestamp + 1);
> +		else
> +			delta = sensor_timestamp - ds->prev_sensor_timestamp;
> +		ds->sensor_timestamp_us += delta / 3;
> +	}
> +	ds->prev_sensor_timestamp = sensor_timestamp;
> +	input_event(ds->sensors, EV_MSC, MSC_TIMESTAMP, ds->sensor_timestamp_us);
> +	input_sync(ds->sensors);
> +
>  	for (i = 0; i < 2; i++) {
>  		bool active = (ds_report->points[i].contact & DS_TOUCH_POINT_INACTIVE) ? false : true;
>
> @@ -446,12 +635,25 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
>  	}
>  	snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds->base.mac_address);
>
> +	ret = dualsense_get_calibration_data(ds);
> +	if (ret < 0) {
> +		hid_err(hdev, "Failed to get calibration data from DualSense\n");
> +		goto err;
> +	}
> +
>  	ds->gamepad = ps_gamepad_create(hdev);
>  	if (IS_ERR(ds->gamepad)) {
>  		ret = PTR_ERR(ds->gamepad);
>  		goto err;
>  	}
>
> +	ds->sensors = ps_sensors_create(hdev, DS_ACC_RANGE, DS_ACC_RES_PER_G,
> +			DS_GYRO_RANGE, DS_GYRO_RES_PER_DEG_S);

I believe it would be better if the second line was aligned to the `h` in 'hdev'.


> +	if (IS_ERR(ds->sensors)) {
> +		ret = PTR_ERR(ds->sensors);
> +		goto err;
> +	}
> +
>  	ds->touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
>  	if (IS_ERR(ds->touchpad)) {
>  		ret = PTR_ERR(ds->touchpad);
>


[1]: https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces
[2]: https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting


Regards,
Barnabás Pőcze

Re: [PATCH v2 05/13] HID: playstation: add DualSense accelerometer and gyroscope support.

[Roderick Colenbrander]

Hi Barnabás,

Thanks for all your feedback (including other patches).

On Thu, Jan 7, 2021 at 10:52 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Hi
>
>
> 2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:
>
> > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> >
> > The DualSense features an accelerometer and gyroscope. The data is
> > embedded into the main HID input reports. Expose both sensors through
> > through a separate evdev node.
> >
> > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > [...]
> > +static struct input_dev *ps_sensors_create(struct hid_device *hdev, int accel_range, int accel_res,
> > +             int gyro_range, int gyro_res)
> > +{
> > +     struct input_dev *sensors;
> > +     int ret;
> > +
> > +     sensors = ps_allocate_input_dev(hdev, "Motion Sensors");
> > +     if (IS_ERR(sensors))
> > +             return ERR_PTR(-ENOMEM);
>
> I know that at the moment ENOMEM is the only possible error, but I believe
> `return ERR_CAST(sensors);` would be better. (Or even just `return sensors;`.)
>
>
> > +
> > +     __set_bit(INPUT_PROP_ACCELEROMETER, sensors->propbit);
> > +
> > +     /* Accelerometer */
> > +     input_set_abs_params(sensors, ABS_X, -accel_range, accel_range, 16, 0);
> > +     input_set_abs_params(sensors, ABS_Y, -accel_range, accel_range, 16, 0);
> > +     input_set_abs_params(sensors, ABS_Z, -accel_range, accel_range, 16, 0);
> > +     input_abs_set_res(sensors, ABS_X, accel_res);
> > +     input_abs_set_res(sensors, ABS_Y, accel_res);
> > +     input_abs_set_res(sensors, ABS_Z, accel_res);
> > +
> > +     /* Gyroscope */
> > +     input_set_abs_params(sensors, ABS_RX, -gyro_range, gyro_range, 16, 0);
> > +     input_set_abs_params(sensors, ABS_RY, -gyro_range, gyro_range, 16, 0);
> > +     input_set_abs_params(sensors, ABS_RZ, -gyro_range, gyro_range, 16, 0);
> > +     input_abs_set_res(sensors, ABS_RX, gyro_res);
> > +     input_abs_set_res(sensors, ABS_RY, gyro_res);
> > +     input_abs_set_res(sensors, ABS_RZ, gyro_res);
> > +
> > +     ret = input_register_device(sensors);
> > +     if (ret)
> > +             return ERR_PTR(ret);
> > +
> > +     return sensors;
> > +}
> > [...]
> > +static int dualsense_get_calibration_data(struct dualsense *ds)
> > +{
> > +     short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
> > +     short gyro_yaw_bias, gyro_yaw_plus, gyro_yaw_minus;
> > +     short gyro_roll_bias, gyro_roll_plus, gyro_roll_minus;
> > +     short gyro_speed_plus, gyro_speed_minus;
> > +     short acc_x_plus, acc_x_minus;
> > +     short acc_y_plus, acc_y_minus;
> > +     short acc_z_plus, acc_z_minus;
> > +     int speed_2x;
> > +     int range_2g;
> > +     int ret = 0;
> > +     uint8_t *buf;
> > +
> > +     buf = kzalloc(DS_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
> > +     if (!buf)
> > +             return -ENOMEM;
> > +
> > +     ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_CALIBRATION, buf,
> > +                     DS_FEATURE_REPORT_CALIBRATION_SIZE, HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>
> I think it would be better if lines were aligned. I have missed this in other patches,
> so if you decide to make this change, please do it everywhere.

What do you mean with "if lines were aligned"? You mean aligning the
DS_FEATURE.. part with ds->base.hdev?

I'm almost tempted in the future (as part of a future patch series) to
perhaps have a ps_device_get_feature_report or something like that as
there is the same code in multiple places. It can do some nicer
checking as well (including to see if the first byte is the report ID
number, which is guaranteed for DualSense). I think it is a bit much
to add now, but probably in the future also when I add DualShock 4 in
here.

>
> > +     if (ret < 0)
> > +             goto err_free;
> > +     else if (ret != DS_FEATURE_REPORT_CALIBRATION_SIZE) {
>
> As per coding style[1], please either use {} for all branches, or just drop the
> `else` and maybe add a new line:
>
> ```
> if (ret < 0)
>   goto ...
>
> if (ret != ...) {
>   ...
> }
> ```
>
>
> > +             hid_err(ds->base.hdev, "failed to retrieve DualSense calibration info\n");
>
> I think this message could be improved to better pinpoint the exact problem
> that triggered it.
>
>
> > +             ret = -EINVAL;
> > +             goto err_free;
> > +     }
> > +
> > +     gyro_pitch_bias  = get_unaligned_le16(&buf[1]);
> > +     gyro_yaw_bias    = get_unaligned_le16(&buf[3]);
> > +     gyro_roll_bias   = get_unaligned_le16(&buf[5]);
> > +     gyro_pitch_plus  = get_unaligned_le16(&buf[7]);
> > +     gyro_pitch_minus = get_unaligned_le16(&buf[9]);
> > +     gyro_yaw_plus    = get_unaligned_le16(&buf[11]);
> > +     gyro_yaw_minus   = get_unaligned_le16(&buf[13]);
> > +     gyro_roll_plus   = get_unaligned_le16(&buf[15]);
> > +     gyro_roll_minus  = get_unaligned_le16(&buf[17]);
> > +     gyro_speed_plus  = get_unaligned_le16(&buf[19]);
> > +     gyro_speed_minus = get_unaligned_le16(&buf[21]);
> > +     acc_x_plus       = get_unaligned_le16(&buf[23]);
> > +     acc_x_minus      = get_unaligned_le16(&buf[25]);
> > +     acc_y_plus       = get_unaligned_le16(&buf[27]);
> > +     acc_y_minus      = get_unaligned_le16(&buf[29]);
> > +     acc_z_plus       = get_unaligned_le16(&buf[31]);
> > +     acc_z_minus      = get_unaligned_le16(&buf[33]);
> > +
> > +     /* Set gyroscope calibration and normalization parameters.
> > +      * Data values will be normalized to 1/DS_GYRO_RES_PER_DEG_S degree/s.
> > +      */
>
> A small note, as written in [2], the preferred style of multi-line comments is different,
> so you might want to change the comments. If you decide to make this change, please
> do it everywhere.
>
>
> > +     speed_2x = (gyro_speed_plus + gyro_speed_minus);
> > +     ds->gyro_calib_data[0].abs_code = ABS_RX;
> > +     ds->gyro_calib_data[0].bias = gyro_pitch_bias;
> > +     ds->gyro_calib_data[0].sens_numer = speed_2x*DS_GYRO_RES_PER_DEG_S;
> > +     ds->gyro_calib_data[0].sens_denom = gyro_pitch_plus - gyro_pitch_minus;
> > +
> > +     ds->gyro_calib_data[1].abs_code = ABS_RY;
> > +     ds->gyro_calib_data[1].bias = gyro_yaw_bias;
> > +     ds->gyro_calib_data[1].sens_numer = speed_2x*DS_GYRO_RES_PER_DEG_S;
> > +     ds->gyro_calib_data[1].sens_denom = gyro_yaw_plus - gyro_yaw_minus;
> > +
> > +     ds->gyro_calib_data[2].abs_code = ABS_RZ;
> > +     ds->gyro_calib_data[2].bias = gyro_roll_bias;
> > +     ds->gyro_calib_data[2].sens_numer = speed_2x*DS_GYRO_RES_PER_DEG_S;
> > +     ds->gyro_calib_data[2].sens_denom = gyro_roll_plus - gyro_roll_minus;
> > +
> > +     /* Set accelerometer calibration and normalization parameters.
> > +      * Data values will be normalized to 1/DS_ACC_RES_PER_G G.
>                                                                 ^
> Minor thing, but I believe it should be 'g', not 'G'?
>
>
> > +      */
> > +     range_2g = acc_x_plus - acc_x_minus;
> > +     ds->accel_calib_data[0].abs_code = ABS_X;
> > +     ds->accel_calib_data[0].bias = acc_x_plus - range_2g / 2;
> > +     ds->accel_calib_data[0].sens_numer = 2*DS_ACC_RES_PER_G;
> > +     ds->accel_calib_data[0].sens_denom = range_2g;
> > +
> > +     range_2g = acc_y_plus - acc_y_minus;
> > +     ds->accel_calib_data[1].abs_code = ABS_Y;
> > +     ds->accel_calib_data[1].bias = acc_y_plus - range_2g / 2;
> > +     ds->accel_calib_data[1].sens_numer = 2*DS_ACC_RES_PER_G;
> > +     ds->accel_calib_data[1].sens_denom = range_2g;
> > +
> > +     range_2g = acc_z_plus - acc_z_minus;
> > +     ds->accel_calib_data[2].abs_code = ABS_Z;
> > +     ds->accel_calib_data[2].bias = acc_z_plus - range_2g / 2;
> > +     ds->accel_calib_data[2].sens_numer = 2*DS_ACC_RES_PER_G;
> > +     ds->accel_calib_data[2].sens_denom = range_2g;
> > +
> > +err_free:
> > +     kfree(buf);
> > +     return ret;
> > +}
> > +
> >  static int dualsense_get_mac_address(struct dualsense *ds)
> >  {
> >       uint8_t *buf;
> > @@ -319,6 +469,7 @@  static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
> >       struct dualsense_input_report *ds_report;
> >       uint8_t battery_data, battery_capacity, charging_status, value;
> >       int battery_status;
> > +     uint16_t sensor_timestamp;
> >       unsigned long flags;
> >       int i;
> >
> > @@ -361,6 +512,44 @@  static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
> >       input_report_key(ds->gamepad, BTN_MODE,   ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
> >       input_sync(ds->gamepad);
> >
> > +     /* Parse and calibrate gyroscope data. */
> > +     for (i = 0; i < 3; i++) {
>
> I think `i < ARRAY_SIZE(...)` would be better.
> And I would add a `static_assert(ARRAY_SIZE(ds_report->gyro) == ARRAY_SIZE(ds->gyro_calib_data))`
> somewhere around here just to be safe. Or define a new constant like `DS_GYRO_DIMS`
> and use that to define the arrays. Or both. *
>
>
> > +             int raw_data = (short)le16_to_cpu(ds_report->gyro[i]);
> > +             int calib_data = mult_frac(ds->gyro_calib_data[i].sens_numer,
> > +                             raw_data - ds->gyro_calib_data[i].bias,
> > +                             ds->gyro_calib_data[i].sens_denom);
>
> I believe it would be better if the second and third lines was aligned. **
>
>
> > +
> > +             input_report_abs(ds->sensors, ds->gyro_calib_data[i].abs_code, calib_data);
> > +     }
> > +
> > +     /* Parse and calibrate accelerometer data. */
> > +     for (i = 0; i < 3; i++) {
>
> Same here. *
>
>
> > +             int raw_data = (short)le16_to_cpu(ds_report->accel[i]);
> > +             int calib_data = mult_frac(ds->accel_calib_data[i].sens_numer,
> > +                             raw_data - ds->accel_calib_data[i].bias,
> > +                             ds->accel_calib_data[i].sens_denom);
>
> Same here. **
>
>
> > +
> > +             input_report_abs(ds->sensors, ds->accel_calib_data[i].abs_code, calib_data);
> > +     }
> > +
> > +     /* Convert timestamp (in 0.33us unit) to timestamp_us */
> > +     sensor_timestamp = le32_to_cpu(ds_report->sensor_timestamp);
> > +     if (!ds->sensor_timestamp_initialized) {
> > +             ds->sensor_timestamp_us = sensor_timestamp / 3;
> > +             ds->sensor_timestamp_initialized = true;
> > +     } else {
> > +             uint32_t delta;
> > +
> > +             if (ds->prev_sensor_timestamp > sensor_timestamp)
> > +                     delta = (U32_MAX - ds->prev_sensor_timestamp + sensor_timestamp + 1);
> > +             else
> > +                     delta = sensor_timestamp - ds->prev_sensor_timestamp;
> > +             ds->sensor_timestamp_us += delta / 3;
> > +     }
> > +     ds->prev_sensor_timestamp = sensor_timestamp;
> > +     input_event(ds->sensors, EV_MSC, MSC_TIMESTAMP, ds->sensor_timestamp_us);
> > +     input_sync(ds->sensors);
> > +
> >       for (i = 0; i < 2; i++) {
> >               bool active = (ds_report->points[i].contact & DS_TOUCH_POINT_INACTIVE) ? false : true;
> >
> > @@ -446,12 +635,25 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
> >       }
> >       snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds->base.mac_address);
> >
> > +     ret = dualsense_get_calibration_data(ds);
> > +     if (ret < 0) {
> > +             hid_err(hdev, "Failed to get calibration data from DualSense\n");
> > +             goto err;
> > +     }
> > +
> >       ds->gamepad = ps_gamepad_create(hdev);
> >       if (IS_ERR(ds->gamepad)) {
> >               ret = PTR_ERR(ds->gamepad);
> >               goto err;
> >       }
> >
> > +     ds->sensors = ps_sensors_create(hdev, DS_ACC_RANGE, DS_ACC_RES_PER_G,
> > +                     DS_GYRO_RANGE, DS_GYRO_RES_PER_DEG_S);
>
> I believe it would be better if the second line was aligned to the `h` in 'hdev'.
>
>
> > +     if (IS_ERR(ds->sensors)) {
> > +             ret = PTR_ERR(ds->sensors);
> > +             goto err;
> > +     }
> > +
> >       ds->touchpad = ps_touchpad_create(hdev, DS_TOUCHPAD_WIDTH, DS_TOUCHPAD_HEIGHT, 2);
> >       if (IS_ERR(ds->touchpad)) {
> >               ret = PTR_ERR(ds->touchpad);
> >
>
>
> [1]: https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces
> [2]: https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting
>
>
> Regards,
> Barnabás Pőcze


Regards,
Roderick Colenbrander

Re: [PATCH v2 05/13] HID: playstation: add DualSense accelerometer and gyroscope support.

[Barnabás Pőcze]

Hi


2021. január 8., péntek 7:06 keltezéssel, Roderick Colenbrander írta:

> [...]
> > > +static int dualsense_get_calibration_data(struct dualsense *ds)
> > > +{
> > > +     short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
> > > +     short gyro_yaw_bias, gyro_yaw_plus, gyro_yaw_minus;
> > > +     short gyro_roll_bias, gyro_roll_plus, gyro_roll_minus;
> > > +     short gyro_speed_plus, gyro_speed_minus;
> > > +     short acc_x_plus, acc_x_minus;
> > > +     short acc_y_plus, acc_y_minus;
> > > +     short acc_z_plus, acc_z_minus;
> > > +     int speed_2x;
> > > +     int range_2g;
> > > +     int ret = 0;
> > > +     uint8_t *buf;
> > > +
> > > +     buf = kzalloc(DS_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
> > > +     if (!buf)
> > > +             return -ENOMEM;
> > > +
> > > +     ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_CALIBRATION, buf,
> > > +                     DS_FEATURE_REPORT_CALIBRATION_SIZE, HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> >
> > I think it would be better if lines were aligned. I have missed this in other patches,
> > so if you decide to make this change, please do it everywhere.
>
> What do you mean with "if lines were aligned"? You mean aligning the
> DS_FEATURE.. part with ds->base.hdev?

Yes, exactly.


>
> I'm almost tempted in the future (as part of a future patch series) to
> perhaps have a ps_device_get_feature_report or something like that as
> there is the same code in multiple places. It can do some nicer
> checking as well (including to see if the first byte is the report ID
> number, which is guaranteed for DualSense). I think it is a bit much
> to add now, but probably in the future also when I add DualShock 4 in
> here.

I think it's a good idea to add such a function sometime.


>
> >
> > > +     if (ret < 0)
> > > +             goto err_free;
> > > +     else if (ret != DS_FEATURE_REPORT_CALIBRATION_SIZE) {
> >
> > As per coding style[1], please either use {} for all branches, or just drop the
> > `else` and maybe add a new line:
> >
> > ```
> > if (ret < 0)
> >   goto ...
> >
> > if (ret != ...) {
> >   ...
> > }
> > ```
> >
> >
> > > +             hid_err(ds->base.hdev, "failed to retrieve DualSense calibration info\n");
> >
> > I think this message could be improved to better pinpoint the exact problem
> > that triggered it.
> >
> >
> > > +             ret = -EINVAL;
> > > +             goto err_free;
> > > +     }
> [...]


Regards,
Barnabás Pőcze

Re: [PATCH v2 05/13] HID: playstation: add DualSense accelerometer and gyroscope support.

[Siarhei Vishniakou]

Hi Roderick,

Is there any way to align the sensor timestamps with the real clock on
this new device? If so, there's input_set_timestamp api [0] that could
be used for setting the timestamps of the actual input_events rather
than having to send out parallel MSC_TIMESTAMP messages. It would make
it easier for user space to process these events.

[0] https://patchwork.kernel.org/project/linux-input/patch/20190718194133.64034-1-atifniyaz@google.com/


On Fri, Jan 8, 2021 at 2:03 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Hi
>
>
> 2021. január 8., péntek 7:06 keltezéssel, Roderick Colenbrander írta:
>
> > [...]
> > > > +static int dualsense_get_calibration_data(struct dualsense *ds)
> > > > +{
> > > > +     short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
> > > > +     short gyro_yaw_bias, gyro_yaw_plus, gyro_yaw_minus;
> > > > +     short gyro_roll_bias, gyro_roll_plus, gyro_roll_minus;
> > > > +     short gyro_speed_plus, gyro_speed_minus;
> > > > +     short acc_x_plus, acc_x_minus;
> > > > +     short acc_y_plus, acc_y_minus;
> > > > +     short acc_z_plus, acc_z_minus;
> > > > +     int speed_2x;
> > > > +     int range_2g;
> > > > +     int ret = 0;
> > > > +     uint8_t *buf;
> > > > +
> > > > +     buf = kzalloc(DS_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
> > > > +     if (!buf)
> > > > +             return -ENOMEM;
> > > > +
> > > > +     ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_CALIBRATION, buf,
> > > > +                     DS_FEATURE_REPORT_CALIBRATION_SIZE, HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> > >
> > > I think it would be better if lines were aligned. I have missed this in other patches,
> > > so if you decide to make this change, please do it everywhere.
> >
> > What do you mean with "if lines were aligned"? You mean aligning the
> > DS_FEATURE.. part with ds->base.hdev?
>
> Yes, exactly.
>
>
> >
> > I'm almost tempted in the future (as part of a future patch series) to
> > perhaps have a ps_device_get_feature_report or something like that as
> > there is the same code in multiple places. It can do some nicer
> > checking as well (including to see if the first byte is the report ID
> > number, which is guaranteed for DualSense). I think it is a bit much
> > to add now, but probably in the future also when I add DualShock 4 in
> > here.
>
> I think it's a good idea to add such a function sometime.
>
>
> >
> > >
> > > > +     if (ret < 0)
> > > > +             goto err_free;
> > > > +     else if (ret != DS_FEATURE_REPORT_CALIBRATION_SIZE) {
> > >
> > > As per coding style[1], please either use {} for all branches, or just drop the
> > > `else` and maybe add a new line:
> > >
> > > ```
> > > if (ret < 0)
> > >   goto ...
> > >
> > > if (ret != ...) {
> > >   ...
> > > }
> > > ```
> > >
> > >
> > > > +             hid_err(ds->base.hdev, "failed to retrieve DualSense calibration info\n");
> > >
> > > I think this message could be improved to better pinpoint the exact problem
> > > that triggered it.
> > >
> > >
> > > > +             ret = -EINVAL;
> > > > +             goto err_free;
> > > > +     }
> > [...]
>
>
> Regards,
> Barnabás Pőcze

Re: [PATCH v2 05/13] HID: playstation: add DualSense accelerometer and gyroscope support.

[Roderick Colenbrander]

Hi Siarhei,

It might be an idea to indeed use that API. I wasn't aware of its
existence. Though I don't fully understand how it works (and how you
can guarantee alignment). Unfortunately I don't see any drivers in
upstream Linux using it. Do you happen to know of drivers using it? I
guess the might be some in Android kernel-common?

Thanks,
Roderick

On Fri, Jan 8, 2021 at 9:15 AM Siarhei Vishniakou <svv@google.com> wrote:
>
> Hi Roderick,
>
> Is there any way to align the sensor timestamps with the real clock on
> this new device? If so, there's input_set_timestamp api [0] that could
> be used for setting the timestamps of the actual input_events rather
> than having to send out parallel MSC_TIMESTAMP messages. It would make
> it easier for user space to process these events.
>
> [0] https://patchwork.kernel.org/project/linux-input/patch/20190718194133.64034-1-atifniyaz@google.com/
>
>
> On Fri, Jan 8, 2021 at 2:03 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> >
> > Hi
> >
> >
> > 2021. január 8., péntek 7:06 keltezéssel, Roderick Colenbrander írta:
> >
> > > [...]
> > > > > +static int dualsense_get_calibration_data(struct dualsense *ds)
> > > > > +{
> > > > > +     short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
> > > > > +     short gyro_yaw_bias, gyro_yaw_plus, gyro_yaw_minus;
> > > > > +     short gyro_roll_bias, gyro_roll_plus, gyro_roll_minus;
> > > > > +     short gyro_speed_plus, gyro_speed_minus;
> > > > > +     short acc_x_plus, acc_x_minus;
> > > > > +     short acc_y_plus, acc_y_minus;
> > > > > +     short acc_z_plus, acc_z_minus;
> > > > > +     int speed_2x;
> > > > > +     int range_2g;
> > > > > +     int ret = 0;
> > > > > +     uint8_t *buf;
> > > > > +
> > > > > +     buf = kzalloc(DS_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
> > > > > +     if (!buf)
> > > > > +             return -ENOMEM;
> > > > > +
> > > > > +     ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_CALIBRATION, buf,
> > > > > +                     DS_FEATURE_REPORT_CALIBRATION_SIZE, HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> > > >
> > > > I think it would be better if lines were aligned. I have missed this in other patches,
> > > > so if you decide to make this change, please do it everywhere.
> > >
> > > What do you mean with "if lines were aligned"? You mean aligning the
> > > DS_FEATURE.. part with ds->base.hdev?
> >
> > Yes, exactly.
> >
> >
> > >
> > > I'm almost tempted in the future (as part of a future patch series) to
> > > perhaps have a ps_device_get_feature_report or something like that as
> > > there is the same code in multiple places. It can do some nicer
> > > checking as well (including to see if the first byte is the report ID
> > > number, which is guaranteed for DualSense). I think it is a bit much
> > > to add now, but probably in the future also when I add DualShock 4 in
> > > here.
> >
> > I think it's a good idea to add such a function sometime.
> >
> >
> > >
> > > >
> > > > > +     if (ret < 0)
> > > > > +             goto err_free;
> > > > > +     else if (ret != DS_FEATURE_REPORT_CALIBRATION_SIZE) {
> > > >
> > > > As per coding style[1], please either use {} for all branches, or just drop the
> > > > `else` and maybe add a new line:
> > > >
> > > > ```
> > > > if (ret < 0)
> > > >   goto ...
> > > >
> > > > if (ret != ...) {
> > > >   ...
> > > > }
> > > > ```
> > > >
> > > >
> > > > > +             hid_err(ds->base.hdev, "failed to retrieve DualSense calibration info\n");
> > > >
> > > > I think this message could be improved to better pinpoint the exact problem
> > > > that triggered it.
> > > >
> > > >
> > > > > +             ret = -EINVAL;
> > > > > +             goto err_free;
> > > > > +     }
> > > [...]
> >
> >
> > Regards,
> > Barnabás Pőcze



-- 
Roderick Colenbrander
Senior Manager of Hardware & Systems Engineering
Sony Interactive Entertainment LLC
roderick.colenbrander@sony.com

Re: [PATCH v2 05/13] HID: playstation: add DualSense accelerometer and gyroscope support.

[Siarhei Vishniakou]

This api is used by some of our touch drivers to more accurately set
the timestamps of touch events. This allows us to better measure touch
latency. An example can be found in [0].

From what I remember, you call this api to apply a specific timestamp
to all of the subsequent input_events that are produced. When
input_sync happens, this timestamp is erased and you revert to the
default behaviour (acquiring a timestamp in evdev) until this api is
called again.
So if you choose to use this api, you would have to take care to only
apply it to the sensor events and not other events (unless you can
figure out the timestamps for all), as well as finding a way to align
the hardware timestamps with the wall clock.

For the touch driver case, it's easy because we are just taking the
current time at the interrupt. This still misses the portions where
the touch scanning and data preprocessing on the touch IC occurs, but
it gets us closer to the real number (for example, it helps account
for the i2c/spi data transfer time, which happens after the
interrupt).


[0] https://github.com/android-linux-stable/bluecross/blob/android-msm-bluecross-4.9/drivers/input/touchscreen/stm/fts.c#L3451

On Fri, Jan 8, 2021 at 9:54 AM Roderick Colenbrander
<roderick@gaikai.com> wrote:
>
> Hi Siarhei,
>
> It might be an idea to indeed use that API. I wasn't aware of its
> existence. Though I don't fully understand how it works (and how you
> can guarantee alignment). Unfortunately I don't see any drivers in
> upstream Linux using it. Do you happen to know of drivers using it? I
> guess the might be some in Android kernel-common?
>
> Thanks,
> Roderick
>
> On Fri, Jan 8, 2021 at 9:15 AM Siarhei Vishniakou <svv@google.com> wrote:
> >
> > Hi Roderick,
> >
> > Is there any way to align the sensor timestamps with the real clock on
> > this new device? If so, there's input_set_timestamp api [0] that could
> > be used for setting the timestamps of the actual input_events rather
> > than having to send out parallel MSC_TIMESTAMP messages. It would make
> > it easier for user space to process these events.
> >
> > [0] https://patchwork.kernel.org/project/linux-input/patch/20190718194133.64034-1-atifniyaz@google.com/
> >
> >
> > On Fri, Jan 8, 2021 at 2:03 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> > >
> > > Hi
> > >
> > >
> > > 2021. január 8., péntek 7:06 keltezéssel, Roderick Colenbrander írta:
> > >
> > > > [...]
> > > > > > +static int dualsense_get_calibration_data(struct dualsense *ds)
> > > > > > +{
> > > > > > +     short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
> > > > > > +     short gyro_yaw_bias, gyro_yaw_plus, gyro_yaw_minus;
> > > > > > +     short gyro_roll_bias, gyro_roll_plus, gyro_roll_minus;
> > > > > > +     short gyro_speed_plus, gyro_speed_minus;
> > > > > > +     short acc_x_plus, acc_x_minus;
> > > > > > +     short acc_y_plus, acc_y_minus;
> > > > > > +     short acc_z_plus, acc_z_minus;
> > > > > > +     int speed_2x;
> > > > > > +     int range_2g;
> > > > > > +     int ret = 0;
> > > > > > +     uint8_t *buf;
> > > > > > +
> > > > > > +     buf = kzalloc(DS_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
> > > > > > +     if (!buf)
> > > > > > +             return -ENOMEM;
> > > > > > +
> > > > > > +     ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_CALIBRATION, buf,
> > > > > > +                     DS_FEATURE_REPORT_CALIBRATION_SIZE, HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> > > > >
> > > > > I think it would be better if lines were aligned. I have missed this in other patches,
> > > > > so if you decide to make this change, please do it everywhere.
> > > >
> > > > What do you mean with "if lines were aligned"? You mean aligning the
> > > > DS_FEATURE.. part with ds->base.hdev?
> > >
> > > Yes, exactly.
> > >
> > >
> > > >
> > > > I'm almost tempted in the future (as part of a future patch series) to
> > > > perhaps have a ps_device_get_feature_report or something like that as
> > > > there is the same code in multiple places. It can do some nicer
> > > > checking as well (including to see if the first byte is the report ID
> > > > number, which is guaranteed for DualSense). I think it is a bit much
> > > > to add now, but probably in the future also when I add DualShock 4 in
> > > > here.
> > >
> > > I think it's a good idea to add such a function sometime.
> > >
> > >
> > > >
> > > > >
> > > > > > +     if (ret < 0)
> > > > > > +             goto err_free;
> > > > > > +     else if (ret != DS_FEATURE_REPORT_CALIBRATION_SIZE) {
> > > > >
> > > > > As per coding style[1], please either use {} for all branches, or just drop the
> > > > > `else` and maybe add a new line:
> > > > >
> > > > > ```
> > > > > if (ret < 0)
> > > > >   goto ...
> > > > >
> > > > > if (ret != ...) {
> > > > >   ...
> > > > > }
> > > > > ```
> > > > >
> > > > >
> > > > > > +             hid_err(ds->base.hdev, "failed to retrieve DualSense calibration info\n");
> > > > >
> > > > > I think this message could be improved to better pinpoint the exact problem
> > > > > that triggered it.
> > > > >
> > > > >
> > > > > > +             ret = -EINVAL;
> > > > > > +             goto err_free;
> > > > > > +     }
> > > > [...]
> > >
> > >
> > > Regards,
> > > Barnabás Pőcze
>
>
>
> --
> Roderick Colenbrander
> Senior Manager of Hardware & Systems Engineering
> Sony Interactive Entertainment LLC
> roderick.colenbrander@sony.com

Re: [PATCH v2 05/13] HID: playstation: add DualSense accelerometer and gyroscope support.

[Roderick Colenbrander]

Hi Siarhei,

Thanks for sharing the example. I see now how the API could be used.

In case of DualSense, the 'dualsense_parse_report' call is effectively
the ISR and the timestamp is derived there by the input framework. We
would like to use the hardware timestamp (as in timestamp at which the
device created the event), so we can do accurate motion tracking. In
particular in case of Bluetooth there is variation in timestamps.

We could do our own time tracking by taking an initial CLOCK_MONOTONIC
and then adding our own hardware timestamps to it. Something like:
if (!sensor_timestamp_initialized) {
    timestamp = ktime_get();
    hw_time0 = get sensor timestamp();
}

hw_delta = get_sensor_timestamp() - hw_time0;
ktime_add_ns(timestamp, hw_delta);
input_set_timestamp(sensor_dev, timestamp);

I just don't know what others would think about such an approach vs
MSC_TIMESTAMP (or we can do both).

Thanks,
Roderick

On Fri, Jan 8, 2021 at 4:11 PM Siarhei Vishniakou <svv@google.com> wrote:
>
> This api is used by some of our touch drivers to more accurately set
> the timestamps of touch events. This allows us to better measure touch
> latency. An example can be found in [0].
>
> From what I remember, you call this api to apply a specific timestamp
> to all of the subsequent input_events that are produced. When
> input_sync happens, this timestamp is erased and you revert to the
> default behaviour (acquiring a timestamp in evdev) until this api is
> called again.
> So if you choose to use this api, you would have to take care to only
> apply it to the sensor events and not other events (unless you can
> figure out the timestamps for all), as well as finding a way to align
> the hardware timestamps with the wall clock.
>
> For the touch driver case, it's easy because we are just taking the
> current time at the interrupt. This still misses the portions where
> the touch scanning and data preprocessing on the touch IC occurs, but
> it gets us closer to the real number (for example, it helps account
> for the i2c/spi data transfer time, which happens after the
> interrupt).
>
>
> [0] https://github.com/android-linux-stable/bluecross/blob/android-msm-bluecross-4.9/drivers/input/touchscreen/stm/fts.c#L3451
>
> On Fri, Jan 8, 2021 at 9:54 AM Roderick Colenbrander
> <roderick@gaikai.com> wrote:
> >
> > Hi Siarhei,
> >
> > It might be an idea to indeed use that API. I wasn't aware of its
> > existence. Though I don't fully understand how it works (and how you
> > can guarantee alignment). Unfortunately I don't see any drivers in
> > upstream Linux using it. Do you happen to know of drivers using it? I
> > guess the might be some in Android kernel-common?
> >
> > Thanks,
> > Roderick
> >
> > On Fri, Jan 8, 2021 at 9:15 AM Siarhei Vishniakou <svv@google.com> wrote:
> > >
> > > Hi Roderick,
> > >
> > > Is there any way to align the sensor timestamps with the real clock on
> > > this new device? If so, there's input_set_timestamp api [0] that could
> > > be used for setting the timestamps of the actual input_events rather
> > > than having to send out parallel MSC_TIMESTAMP messages. It would make
> > > it easier for user space to process these events.
> > >
> > > [0] https://patchwork.kernel.org/project/linux-input/patch/20190718194133.64034-1-atifniyaz@google.com/
> > >
> > >
> > > On Fri, Jan 8, 2021 at 2:03 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> > > >
> > > > Hi
> > > >
> > > >
> > > > 2021. január 8., péntek 7:06 keltezéssel, Roderick Colenbrander írta:
> > > >
> > > > > [...]
> > > > > > > +static int dualsense_get_calibration_data(struct dualsense *ds)
> > > > > > > +{
> > > > > > > +     short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
> > > > > > > +     short gyro_yaw_bias, gyro_yaw_plus, gyro_yaw_minus;
> > > > > > > +     short gyro_roll_bias, gyro_roll_plus, gyro_roll_minus;
> > > > > > > +     short gyro_speed_plus, gyro_speed_minus;
> > > > > > > +     short acc_x_plus, acc_x_minus;
> > > > > > > +     short acc_y_plus, acc_y_minus;
> > > > > > > +     short acc_z_plus, acc_z_minus;
> > > > > > > +     int speed_2x;
> > > > > > > +     int range_2g;
> > > > > > > +     int ret = 0;
> > > > > > > +     uint8_t *buf;
> > > > > > > +
> > > > > > > +     buf = kzalloc(DS_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
> > > > > > > +     if (!buf)
> > > > > > > +             return -ENOMEM;
> > > > > > > +
> > > > > > > +     ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_CALIBRATION, buf,
> > > > > > > +                     DS_FEATURE_REPORT_CALIBRATION_SIZE, HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> > > > > >
> > > > > > I think it would be better if lines were aligned. I have missed this in other patches,
> > > > > > so if you decide to make this change, please do it everywhere.
> > > > >
> > > > > What do you mean with "if lines were aligned"? You mean aligning the
> > > > > DS_FEATURE.. part with ds->base.hdev?
> > > >
> > > > Yes, exactly.
> > > >
> > > >
> > > > >
> > > > > I'm almost tempted in the future (as part of a future patch series) to
> > > > > perhaps have a ps_device_get_feature_report or something like that as
> > > > > there is the same code in multiple places. It can do some nicer
> > > > > checking as well (including to see if the first byte is the report ID
> > > > > number, which is guaranteed for DualSense). I think it is a bit much
> > > > > to add now, but probably in the future also when I add DualShock 4 in
> > > > > here.
> > > >
> > > > I think it's a good idea to add such a function sometime.
> > > >
> > > >
> > > > >
> > > > > >
> > > > > > > +     if (ret < 0)
> > > > > > > +             goto err_free;
> > > > > > > +     else if (ret != DS_FEATURE_REPORT_CALIBRATION_SIZE) {
> > > > > >
> > > > > > As per coding style[1], please either use {} for all branches, or just drop the
> > > > > > `else` and maybe add a new line:
> > > > > >
> > > > > > ```
> > > > > > if (ret < 0)
> > > > > >   goto ...
> > > > > >
> > > > > > if (ret != ...) {
> > > > > >   ...
> > > > > > }
> > > > > > ```
> > > > > >
> > > > > >
> > > > > > > +             hid_err(ds->base.hdev, "failed to retrieve DualSense calibration info\n");
> > > > > >
> > > > > > I think this message could be improved to better pinpoint the exact problem
> > > > > > that triggered it.
> > > > > >
> > > > > >
> > > > > > > +             ret = -EINVAL;
> > > > > > > +             goto err_free;
> > > > > > > +     }
> > > > > [...]
> > > >
> > > >
> > > > Regards,
> > > > Barnabás Pőcze
> >
> >
> >
> > --
> > Roderick Colenbrander
> > Senior Manager of Hardware & Systems Engineering
> > Sony Interactive Entertainment LLC
> > roderick.colenbrander@sony.com

[PATCH v2 06/13] HID: playstation: track devices in list.

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Track devices in a list, so we can detect when a device is connected
twice when using Bluetooth and USB.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/hid-playstation.c | 45 +++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 80b0dcdbfe41..91f3ed005fce 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -16,10 +16,15 @@
 
 #include "hid-ids.h"
 
+/* List of connected playstation devices. */
+static DEFINE_MUTEX(ps_devices_lock);
+static LIST_HEAD(ps_devices_list);
+
 #define HID_PLAYSTATION_VERSION_PATCH 0x8000
 
 /* Base class for playstation devices. */
 struct ps_device {
+	struct list_head list;
 	struct hid_device *hdev;
 	spinlock_t lock;
 
@@ -155,6 +160,37 @@ static const struct {int x; int y; } ps_gamepad_hat_mapping[] = {
 	{0, 0}
 };
 
+/* Add a new ps_device to ps_devices if it doesn't exist.
+ * Return error on duplicate device, which can happen if the same
+ * device is connected using both Bluetooth and USB.
+ */
+static int ps_devices_list_add(struct ps_device *dev)
+{
+	struct ps_device *entry;
+
+	mutex_lock(&ps_devices_lock);
+	list_for_each_entry(entry, &ps_devices_list, list) {
+		if (!memcmp(entry->mac_address, dev->mac_address, sizeof(dev->mac_address))) {
+			hid_err(dev->hdev, "Duplicate device found for MAC address %pMR\n",
+					dev->mac_address);
+			mutex_unlock(&ps_devices_lock);
+			return -EEXIST;
+		}
+	}
+
+	list_add_tail(&dev->list, &ps_devices_list);
+	mutex_unlock(&ps_devices_lock);
+	return 0;
+}
+
+static int ps_devices_list_remove(struct ps_device *dev)
+{
+	mutex_lock(&ps_devices_lock);
+	list_del(&dev->list);
+	mutex_unlock(&ps_devices_lock);
+	return 0;
+}
+
 static struct input_dev *ps_allocate_input_dev(struct hid_device *hdev, const char *name_suffix)
 {
 	struct input_dev *input_dev;
@@ -635,6 +671,10 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	}
 	snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds->base.mac_address);
 
+	ret = ps_devices_list_add(ps_dev);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
 	ret = dualsense_get_calibration_data(ds);
 	if (ret < 0) {
 		hid_err(hdev, "Failed to get calibration data from DualSense\n");
@@ -667,6 +707,7 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	return &ds->base;
 
 err:
+	ps_devices_list_remove(ps_dev);
 	return ERR_PTR(ret);
 }
 
@@ -724,6 +765,10 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 static void ps_remove(struct hid_device *hdev)
 {
+	struct ps_device *dev = hid_get_drvdata(hdev);
+
+	ps_devices_list_remove(dev);
+
 	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
 }
-- 
2.26.2

Re: [PATCH v2 06/13] HID: playstation: track devices in list.

[Barnabás Pőcze]

Hi


2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:

> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> Track devices in a list, so we can detect when a device is connected
> twice when using Bluetooth and USB.
>
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> [...]
> @@ -635,6 +671,10 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
>  	}
>  	snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds->base.mac_address);
>
> +	ret = ps_devices_list_add(ps_dev);
> +	if (ret < 0)

It's a minor thing, and I have already mentioned it in regards to other functions,
but I believe if `ps_devices_list_add()` can only return 0 on success, or an errno,
then `if (ret)` would be better.


> +		return ERR_PTR(ret);
> +
>  	ret = dualsense_get_calibration_data(ds);
>  	if (ret < 0) {
>  		hid_err(hdev, "Failed to get calibration data from DualSense\n");
> @@ -667,6 +707,7 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
>  	return &ds->base;
>
>  err:
> +	ps_devices_list_remove(ps_dev);
>  	return ERR_PTR(ret);
>  }
> [...]


Regards,
Barnabás Pőcze

[PATCH v2 07/13] HID: playstation: add DualSense Bluetooth support.

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

This patch adds support for the DualSense when operating in Bluetooth mode.
The device has the same behavior as the DualShock 4 in that by default it
sends a limited input report (0x1), but after requesting calibration data,
it switches to an extended input report (report 49), which adds data for
touchpad, motion sensors, battery and more.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/hid-playstation.c | 35 +++++++++++++++++++++++++++++++++++
 drivers/hid/hid-quirks.c      |  1 +
 2 files changed, 36 insertions(+)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 91f3ed005fce..552a52a50b78 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -47,6 +47,7 @@ struct ps_calibration_data {
 };
 
 #define DS_INPUT_REPORT_USB			0x01
+#define DS_INPUT_REPORT_BT			0x31
 
 #define DS_FEATURE_REPORT_CALIBRATION		5
 #define DS_FEATURE_REPORT_CALIBRATION_SIZE	41
@@ -285,6 +286,17 @@ static int ps_device_register_battery(struct ps_device *dev)
 	return 0;
 }
 
+/* Compute crc32 of HID data and compare against expected CRC. */
+static bool ps_check_crc32(uint8_t seed, uint8_t *data, size_t len, uint32_t report_crc)
+{
+	uint32_t crc;
+
+	crc = crc32_le(0xFFFFFFFF, &seed, 1);
+	crc = ~crc32_le(crc, data, len);
+
+	return crc == report_crc;
+}
+
 static struct input_dev *ps_gamepad_create(struct hid_device *hdev)
 {
 	struct input_dev *gamepad;
@@ -406,6 +418,18 @@ static int dualsense_get_calibration_data(struct dualsense *ds)
 		goto err_free;
 	}
 
+	if (ds->base.hdev->bus == BUS_BLUETOOTH) {
+		/* Last 4 bytes contains crc32 */
+		uint8_t crc_offset = DS_FEATURE_REPORT_CALIBRATION_SIZE - 4;
+		uint32_t report_crc = get_unaligned_le32(&buf[crc_offset]);
+
+		if (!ps_check_crc32(0xa3, buf, crc_offset, report_crc)) {
+			hid_err(ds->base.hdev, "DualSense calibration report CRC's check failed\n");
+			ret = -EILSEQ;
+			goto err_free;
+		}
+	}
+
 	gyro_pitch_bias  = get_unaligned_le16(&buf[1]);
 	gyro_yaw_bias    = get_unaligned_le16(&buf[3]);
 	gyro_roll_bias   = get_unaligned_le16(&buf[5]);
@@ -515,6 +539,16 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
 	 */
 	if (report->id == DS_INPUT_REPORT_USB && hdev->bus == BUS_USB) {
 		ds_report = (struct dualsense_input_report *)&data[1];
+	} else if (report->id == DS_INPUT_REPORT_BT && hdev->bus == BUS_BLUETOOTH) {
+		/* Last 4 bytes of input report contain crc32 */
+		uint32_t report_crc = get_unaligned_le32(&data[size - 4]);
+
+		if (!ps_check_crc32(0xa1, data, size - 4, report_crc)) {
+			hid_err(hdev, "DualSense input CRC's check failed, size=%d\n", size);
+			return -EILSEQ;
+		}
+
+		ds_report = (struct dualsense_input_report *)&data[2];
 	} else {
 		hid_err(hdev, "Unhandled reportID=%d\n", report->id);
 		return -1;
@@ -774,6 +808,7 @@ static void ps_remove(struct hid_device *hdev)
 }
 
 static const struct hid_device_id ps_devices[] = {
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER) },
 	{ }
 };
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 1ca46cb445be..541c8837debd 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -567,6 +567,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
 #endif
 #if IS_ENABLED(CONFIG_HID_PLAYSTATION)
 	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER) },
++	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER) },
 #endif
 #if IS_ENABLED(CONFIG_HID_PRIMAX)
 	{ HID_USB_DEVICE(USB_VENDOR_ID_PRIMAX, USB_DEVICE_ID_PRIMAX_KEYBOARD) },
-- 
2.26.2

Re: [PATCH v2 07/13] HID: playstation: add DualSense Bluetooth support.

[Barnabás Pőcze]

Hi


2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:

> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> This patch adds support for the DualSense when operating in Bluetooth mode.
> The device has the same behavior as the DualShock 4 in that by default it
> sends a limited input report (0x1), but after requesting calibration data,
> it switches to an extended input report (report 49), which adds data for
> touchpad, motion sensors, battery and more.
>
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> [...]
>  static struct input_dev *ps_gamepad_create(struct hid_device *hdev)
>  {
>  	struct input_dev *gamepad;
> @@ -406,6 +418,18 @@  static int dualsense_get_calibration_data(struct dualsense *ds)
>  		goto err_free;
>  	}
>
> +	if (ds->base.hdev->bus == BUS_BLUETOOTH) {
> +		/* Last 4 bytes contains crc32 */

Most other comments have a period at the end. I know this is a minor thing,
but I believe being consistent is better than not.

And shouldn't this CRC be checked in `dualsense_get_mac_address()`?
If so, maybe a helper function could be created that wraps the `hid_hw_raw_request()`
call?


> +		uint8_t crc_offset = DS_FEATURE_REPORT_CALIBRATION_SIZE - 4;
> +		uint32_t report_crc = get_unaligned_le32(&buf[crc_offset]);
> +
> +		if (!ps_check_crc32(0xa3, buf, crc_offset, report_crc)) {

Maybe that 0xa3 could be named?


> +			hid_err(ds->base.hdev, "DualSense calibration report CRC's check failed\n");
> +			ret = -EILSEQ;
> +			goto err_free;
> +		}
> +	}
> +
>  	gyro_pitch_bias  = get_unaligned_le16(&buf[1]);
>  	gyro_yaw_bias    = get_unaligned_le16(&buf[3]);
>  	gyro_roll_bias   = get_unaligned_le16(&buf[5]);
> @@ -515,6 +539,16 @@  static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
>  	 */
>  	if (report->id == DS_INPUT_REPORT_USB && hdev->bus == BUS_USB) {
>  		ds_report = (struct dualsense_input_report *)&data[1];
> +	} else if (report->id == DS_INPUT_REPORT_BT && hdev->bus == BUS_BLUETOOTH) {
> +		/* Last 4 bytes of input report contain crc32 */
> +		uint32_t report_crc = get_unaligned_le32(&data[size - 4]);
> +
> +		if (!ps_check_crc32(0xa1, data, size - 4, report_crc)) {

And this 0xa1 as well?


> +			hid_err(hdev, "DualSense input CRC's check failed, size=%d\n", size);
> +			return -EILSEQ;
> +		}
> +
> +		ds_report = (struct dualsense_input_report *)&data[2];
>  	} else {
>  		hid_err(hdev, "Unhandled reportID=%d\n", report->id);
>  		return -1;
> [...]


Regards,
Barnabás Pőcze

[PATCH v2 08/13] HID: playstation: add DualSense classic rumble support.

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

The DualSense features a haptics system based on voicecoil motors,
which requires PCM data (or special HID packets using Bluetooth). There
is no appropriate API yet in the Linux kernel to expose these. The
controller also provides a classic rumble feature for backwards
compatibility. Expose this classic rumble feature using the FF framework.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/Kconfig           |   8 ++
 drivers/hid/hid-playstation.c | 196 +++++++++++++++++++++++++++++++++-
 2 files changed, 202 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index ef175c1cb15c..e6c67aaa1a1a 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -863,6 +863,14 @@ config HID_PLAYSTATION
 	  its special functionalities e.g. touchpad, lights and motion
 	  sensors.
 
+config PLAYSTATION_FF
+	bool "PlayStation force feedback support"
+	depends on HID_PLAYSTATION
+	select INPUT_FF_MEMLESS
+	help
+	  Say Y here if you would like to enable force feedback support for
+	  PlayStation game controllers.
+
 config HID_PRIMAX
 	tristate "Primax non-fully HID-compliant devices"
 	depends on HID
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 552a52a50b78..36a904b2f93f 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -48,6 +48,8 @@ struct ps_calibration_data {
 
 #define DS_INPUT_REPORT_USB			0x01
 #define DS_INPUT_REPORT_BT			0x31
+#define DS_OUTPUT_REPORT_USB			0x02
+#define DS_OUTPUT_REPORT_BT			0x31
 
 #define DS_FEATURE_REPORT_CALIBRATION		5
 #define DS_FEATURE_REPORT_CALIBRATION_SIZE	41
@@ -82,6 +84,10 @@ struct ps_calibration_data {
  */
 #define DS_TOUCH_POINT_INACTIVE BIT(7)
 
+/* Flags for DualSense output report. */
+#define DS_OUTPUT_VALID_FLAG0_COMPATIBLE_VIBRATION BIT(0)
+#define DS_OUTPUT_VALID_FLAG0_HAPTICS_SELECT BIT(1)
+
 /* DualSense hardware limits */
 #define DS_ACC_RES_PER_G	8192
 #define DS_ACC_RANGE		(4*DS_ACC_RES_PER_G)
@@ -104,6 +110,15 @@ struct dualsense {
 	bool sensor_timestamp_initialized;
 	uint32_t prev_sensor_timestamp;
 	uint32_t sensor_timestamp_us;
+
+	/* Compatible rumble state */
+	bool update_rumble;
+	uint8_t motor_left;
+	uint8_t motor_right;
+
+	struct work_struct output_worker;
+	void *output_report_dmabuf;
+	uint8_t output_seq; /* Sequence number for output report. */
 };
 
 struct dualsense_touch_point {
@@ -136,6 +151,63 @@ struct dualsense_input_report {
 	uint8_t reserved4[11];
 } __packed;
 
+/* Common data between DualSense BT/USB main output report. */
+struct dualsense_output_report_common {
+	uint8_t valid_flag0;
+	uint8_t valid_flag1;
+
+	/* For DualShock 4 compatibility mode. */
+	uint8_t motor_right;
+	uint8_t motor_left;
+
+	/* Audio controls */
+	uint8_t reserved[4];
+	uint8_t mute_button_led;
+
+	uint8_t power_save_control;
+	uint8_t reserved2[28];
+
+	/* LEDs and lightbar */
+	uint8_t valid_flag2;
+	uint8_t reserved3[2];
+	uint8_t lightbar_setup;
+	uint8_t led_brightness;
+	uint8_t player_leds;
+	uint8_t lightbar_red;
+	uint8_t lightbar_green;
+	uint8_t lightbar_blue;
+} __packed;
+
+struct dualsense_output_report_bt {
+	uint8_t report_id; /* 0x31 */
+	uint8_t seq_tag;
+	uint8_t tag;
+	struct dualsense_output_report_common common;
+	uint8_t reserved[24];
+	__le32 crc32;
+} __packed;
+
+struct dualsense_output_report_usb {
+	uint8_t report_id; /* 0x02 */
+	struct dualsense_output_report_common common;
+} __packed;
+
+/* The DualSense has a main output report used to control most features. It is
+ * largely the same between Bluetooth and USB except for different headers and CRC.
+ * This structure hide the differences between the two to simplify sending output reports.
+ */
+struct dualsense_output_report {
+	uint8_t *data; /* Start of data */
+	uint8_t len; /* Size of output report */
+
+	/* Points to Bluetooth data payload in case for a Bluetooth report else NULL. */
+	struct dualsense_output_report_bt *bt;
+	/* Points to USB data payload in case for a USB report else NULL. */
+	struct dualsense_output_report_usb *usb;
+	/* Points to common section of report, so past any headers */
+	struct dualsense_output_report_common *common;
+};
+
 /* Common gamepad buttons across DualShock 3 / 4 and DualSense.
  * Note: for device with a touchpad, touchpad button is not included
  *        as it will be part of the touchpad device.
@@ -297,7 +369,8 @@ static bool ps_check_crc32(uint8_t seed, uint8_t *data, size_t len, uint32_t rep
 	return crc == report_crc;
 }
 
-static struct input_dev *ps_gamepad_create(struct hid_device *hdev)
+static struct input_dev *ps_gamepad_create(struct hid_device *hdev,
+		int (*play_effect)(struct input_dev *, void *, struct ff_effect *))
 {
 	struct input_dev *gamepad;
 	unsigned int i;
@@ -320,6 +393,13 @@ static struct input_dev *ps_gamepad_create(struct hid_device *hdev)
 	for (i = 0; i < ARRAY_SIZE(ps_gamepad_buttons); i++)
 		input_set_capability(gamepad, EV_KEY, ps_gamepad_buttons[i]);
 
+#if IS_ENABLED(CONFIG_PLAYSTATION_FF)
+	if (play_effect) {
+		input_set_capability(gamepad, EV_FF, FF_RUMBLE);
+		input_ff_create_memless(gamepad, NULL, play_effect);
+	}
+#endif
+
 	ret = input_register_device(gamepad);
 	if (ret)
 		return ERR_PTR(ret);
@@ -521,6 +601,92 @@ static int dualsense_get_mac_address(struct dualsense *ds)
 	return ret;
 }
 
+static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
+		void *buf)
+{
+	struct hid_device *hdev = ds->base.hdev;
+
+	if (hdev->bus == BUS_BLUETOOTH) {
+		struct dualsense_output_report_bt *bt = buf;
+
+		memset(bt, 0, sizeof(*bt));
+		bt->report_id = DS_OUTPUT_REPORT_BT;
+		bt->tag = 0x10; /* Magic number must be set to 0x10 */
+
+		/* Highest 4-bit is a sequence number, which needs to be increased
+		 * every report. Lowest 4-bit is tag and can be zero for now.
+		 */
+		bt->seq_tag = (ds->output_seq << 4) | 0x0;
+		if (++ds->output_seq == 15)
+			ds->output_seq = 0;
+
+		rp->data = buf;
+		rp->len = sizeof(*bt);
+		rp->bt = bt;
+		rp->usb = NULL;
+		rp->common = &bt->common;
+	} else { /* USB */
+		struct dualsense_output_report_usb *usb = buf;
+
+		memset(usb, 0, sizeof(*usb));
+		usb->report_id = DS_OUTPUT_REPORT_USB;
+
+		rp->data = buf;
+		rp->len = sizeof(*usb);
+		rp->bt = NULL;
+		rp->usb = usb;
+		rp->common = &usb->common;
+	}
+}
+
+/* Helper function to send DualSense output reports. Applies a CRC at the end of a report
+ * for Bluetooth reports.
+ */
+static void dualsense_send_output_report(struct dualsense *ds,
+		struct dualsense_output_report *report)
+{
+	struct hid_device *hdev = ds->base.hdev;
+
+	/* Bluetooth packets need to be signed with a CRC in the last 4 bytes. */
+	if (report->bt) {
+		uint32_t crc;
+		uint8_t seed = 0xA2;
+
+		crc = crc32_le(0xFFFFFFFF, &seed, 1);
+		crc = ~crc32_le(crc, report->data, report->len - 4);
+
+		report->bt->crc32 = cpu_to_le32(crc);
+	}
+
+	hid_hw_output_report(hdev, report->data, report->len);
+}
+
+static void dualsense_output_worker(struct work_struct *work)
+{
+	struct dualsense *ds = container_of(work, struct dualsense, output_worker);
+	struct dualsense_output_report report;
+	struct dualsense_output_report_common *common;
+	unsigned long flags;
+
+	dualsense_init_output_report(ds, &report, ds->output_report_dmabuf);
+	common = report.common;
+
+	spin_lock_irqsave(&ds->base.lock, flags);
+
+	if (ds->update_rumble) {
+		/* Select classic rumble style haptics and enable it. */
+		common->valid_flag0 |= DS_OUTPUT_VALID_FLAG0_HAPTICS_SELECT;
+		common->valid_flag0 |= DS_OUTPUT_VALID_FLAG0_COMPATIBLE_VIBRATION;
+		common->motor_left = ds->motor_left;
+		common->motor_right = ds->motor_right;
+		ds->update_rumble = false;
+	}
+
+	spin_unlock_irqrestore(&ds->base.lock, flags);
+
+	dualsense_send_output_report(ds, &report);
+}
+
 static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *report,
 		u8 *data, int size)
 {
@@ -676,10 +842,30 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
 	return 0;
 }
 
+static int dualsense_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect)
+{
+	struct hid_device *hdev = input_get_drvdata(dev);
+	struct dualsense *ds = hid_get_drvdata(hdev);
+	unsigned long flags;
+
+	if (effect->type != FF_RUMBLE)
+		return 0;
+
+	spin_lock_irqsave(&ds->base.lock, flags);
+	ds->update_rumble = true;
+	ds->motor_left = effect->u.rumble.strong_magnitude / 256;
+	ds->motor_right = effect->u.rumble.weak_magnitude / 256;
+	spin_unlock_irqrestore(&ds->base.lock, flags);
+
+	schedule_work(&ds->output_worker);
+	return 0;
+}
+
 static struct ps_device *dualsense_create(struct hid_device *hdev)
 {
 	struct dualsense *ds;
 	struct ps_device *ps_dev;
+	uint8_t max_output_report_size;
 	int ret;
 
 	ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL);
@@ -696,8 +882,14 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	ps_dev->battery_capacity = 100; /* initial value until parse_report. */
 	ps_dev->battery_status = POWER_SUPPLY_STATUS_UNKNOWN;
 	ps_dev->parse_report = dualsense_parse_report;
+	INIT_WORK(&ds->output_worker, dualsense_output_worker);
 	hid_set_drvdata(hdev, ds);
 
+	max_output_report_size = sizeof(struct dualsense_output_report_bt);
+	ds->output_report_dmabuf = devm_kzalloc(&hdev->dev, max_output_report_size, GFP_KERNEL);
+	if (!ds->output_report_dmabuf)
+		return ERR_PTR(-ENOMEM);
+
 	ret = dualsense_get_mac_address(ds);
 	if (ret < 0) {
 		hid_err(hdev, "Failed to get MAC address from DualSense\n");
@@ -715,7 +907,7 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 		goto err;
 	}
 
-	ds->gamepad = ps_gamepad_create(hdev);
+	ds->gamepad = ps_gamepad_create(hdev, dualsense_play_effect);
 	if (IS_ERR(ds->gamepad)) {
 		ret = PTR_ERR(ds->gamepad);
 		goto err;
-- 
2.26.2

Re: [PATCH v2 08/13] HID: playstation: add DualSense classic rumble support.

[Barnabás Pőcze]

Hi


2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:

> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> The DualSense features a haptics system based on voicecoil motors,
> which requires PCM data (or special HID packets using Bluetooth). There
> is no appropriate API yet in the Linux kernel to expose these. The
> controller also provides a classic rumble feature for backwards
> compatibility. Expose this classic rumble feature using the FF framework.
>
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index ef175c1cb15c..e6c67aaa1a1a 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -863,6 +863,14 @@  config HID_PLAYSTATION
>  	  its special functionalities e.g. touchpad, lights and motion
>  	  sensors.
>
> +config PLAYSTATION_FF

I'm wondering if HID_PLAYSTATION_FF would be a better name?


> +	bool "PlayStation force feedback support"
> +	depends on HID_PLAYSTATION
> +	select INPUT_FF_MEMLESS
> +	help
> +	  Say Y here if you would like to enable force feedback support for
> +	  PlayStation game controllers.
> +
>  config HID_PRIMAX
>  	tristate "Primax non-fully HID-compliant devices"
>  	depends on HID
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index 552a52a50b78..36a904b2f93f 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> [...]
> @@ -136,6 +151,63 @@  struct dualsense_input_report {
>  	uint8_t reserved4[11];
>  } __packed;
>
> +/* Common data between DualSense BT/USB main output report. */
> +struct dualsense_output_report_common {
> +	uint8_t valid_flag0;
> +	uint8_t valid_flag1;
> +
> +	/* For DualShock 4 compatibility mode. */
> +	uint8_t motor_right;
> +	uint8_t motor_left;
> +
> +	/* Audio controls */
> +	uint8_t reserved[4];
> +	uint8_t mute_button_led;
> +
> +	uint8_t power_save_control;
> +	uint8_t reserved2[28];
> +
> +	/* LEDs and lightbar */
> +	uint8_t valid_flag2;
> +	uint8_t reserved3[2];
> +	uint8_t lightbar_setup;
> +	uint8_t led_brightness;
> +	uint8_t player_leds;
> +	uint8_t lightbar_red;
> +	uint8_t lightbar_green;
> +	uint8_t lightbar_blue;
> +} __packed;
> +
> +struct dualsense_output_report_bt {
> +	uint8_t report_id; /* 0x31 */
> +	uint8_t seq_tag;
> +	uint8_t tag;
> +	struct dualsense_output_report_common common;
> +	uint8_t reserved[24];
> +	__le32 crc32;
> +} __packed;
> +
> +struct dualsense_output_report_usb {
> +	uint8_t report_id; /* 0x02 */
> +	struct dualsense_output_report_common common;
> +} __packed;
> +

I think it'd be good if you could add static_asserts to check the sizes of
the __packed structs at compile time.


> +/* The DualSense has a main output report used to control most features. It is
> + * largely the same between Bluetooth and USB except for different headers and CRC.
> + * This structure hide the differences between the two to simplify sending output reports.
> + */
> +struct dualsense_output_report {
> +	uint8_t *data; /* Start of data */
> +	uint8_t len; /* Size of output report */
> +
> +	/* Points to Bluetooth data payload in case for a Bluetooth report else NULL. */
> +	struct dualsense_output_report_bt *bt;
> +	/* Points to USB data payload in case for a USB report else NULL. */
> +	struct dualsense_output_report_usb *usb;
> +	/* Points to common section of report, so past any headers */
> +	struct dualsense_output_report_common *common;
> +};
> [...]
> +static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
> +		void *buf)

If the dualsense struct is already passed in, couldn't this function use
`ds->output_report_dmabuf` directly?


> +{
> +	struct hid_device *hdev = ds->base.hdev;
> +
> +	if (hdev->bus == BUS_BLUETOOTH) {
> +		struct dualsense_output_report_bt *bt = buf;
> +
> +		memset(bt, 0, sizeof(*bt));
> +		bt->report_id = DS_OUTPUT_REPORT_BT;
> +		bt->tag = 0x10; /* Magic number must be set to 0x10 */

I think it would be preferable if that 0x10 were named.


> +
> +		/* Highest 4-bit is a sequence number, which needs to be increased
> +		 * every report. Lowest 4-bit is tag and can be zero for now.
> +		 */
> +		bt->seq_tag = (ds->output_seq << 4) | 0x0;
> +		if (++ds->output_seq == 15)
> +			ds->output_seq = 0;

If I see it correctly, the maximum sequence number is 14; is that intentional?
Or am I missing something?


> +
> +		rp->data = buf;
> +		rp->len = sizeof(*bt);
> +		rp->bt = bt;
> +		rp->usb = NULL;
> +		rp->common = &bt->common;
> +	} else { /* USB */
> +		struct dualsense_output_report_usb *usb = buf;
> +
> +		memset(usb, 0, sizeof(*usb));
> +		usb->report_id = DS_OUTPUT_REPORT_USB;
> +
> +		rp->data = buf;
> +		rp->len = sizeof(*usb);
> +		rp->bt = NULL;
> +		rp->usb = usb;
> +		rp->common = &usb->common;
> +	}
> +}
> +
> +/* Helper function to send DualSense output reports. Applies a CRC at the end of a report
> + * for Bluetooth reports.
> + */
> +static void dualsense_send_output_report(struct dualsense *ds,
> +		struct dualsense_output_report *report)
> +{
> +	struct hid_device *hdev = ds->base.hdev;
> +
> +	/* Bluetooth packets need to be signed with a CRC in the last 4 bytes. */
> +	if (report->bt) {
> +		uint32_t crc;
> +		uint8_t seed = 0xA2;

Maybe this '0xA2' could be named as well? And I think it would be better if
all hexadecimal constants would either be lowercase or uppercase.


> +
> +		crc = crc32_le(0xFFFFFFFF, &seed, 1);
> +		crc = ~crc32_le(crc, report->data, report->len - 4);
> +
> +		report->bt->crc32 = cpu_to_le32(crc);
> +	}
> +
> +	hid_hw_output_report(hdev, report->data, report->len);
> +}
> [...]
>  static struct ps_device *dualsense_create(struct hid_device *hdev)
>  {
>  	struct dualsense *ds;
>  	struct ps_device *ps_dev;
> +	uint8_t max_output_report_size;
>  	int ret;
>
>  	ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL);
> @@ -696,8 +882,14 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
>  	ps_dev->battery_capacity = 100; /* initial value until parse_report. */
>  	ps_dev->battery_status = POWER_SUPPLY_STATUS_UNKNOWN;
>  	ps_dev->parse_report = dualsense_parse_report;
> +	INIT_WORK(&ds->output_worker, dualsense_output_worker);
>  	hid_set_drvdata(hdev, ds);
>
> +	max_output_report_size = sizeof(struct dualsense_output_report_bt);

I think `max(sizeof(..._bt), sizeof(..._usb))` (linux/minmax.h) would be
more expressive?


> +	ds->output_report_dmabuf = devm_kzalloc(&hdev->dev, max_output_report_size, GFP_KERNEL);
> +	if (!ds->output_report_dmabuf)
> +		return ERR_PTR(-ENOMEM);
> +
>  	ret = dualsense_get_mac_address(ds);
>  	if (ret < 0) {
>  		hid_err(hdev, "Failed to get MAC address from DualSense\n");
> @@ -715,7 +907,7 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
>  		goto err;
>  	}
>
> -	ds->gamepad = ps_gamepad_create(hdev);
> +	ds->gamepad = ps_gamepad_create(hdev, dualsense_play_effect);
>  	if (IS_ERR(ds->gamepad)) {
>  		ret = PTR_ERR(ds->gamepad);
>  		goto err;
>
>


Regards,
Barnabás Pőcze

Re: [PATCH v2 08/13] HID: playstation: add DualSense classic rumble support.

[Barnabás Pőcze]

2021. január 7., csütörtök 21:41 keltezéssel, Barnabás Pőcze írta:

> [...]
> > +struct dualsense_output_report {
> > +	uint8_t *data; /* Start of data */
> > +	uint8_t len; /* Size of output report */
> > +
> > +	/* Points to Bluetooth data payload in case for a Bluetooth report else NULL. */
> > +	struct dualsense_output_report_bt *bt;
> > +	/* Points to USB data payload in case for a USB report else NULL. */
> > +	struct dualsense_output_report_usb *usb;
> > +	/* Points to common section of report, so past any headers */
> > +	struct dualsense_output_report_common *common;
> > +};
> > [...]
> > +static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
> > +		void *buf)
>
> If the dualsense struct is already passed in, couldn't this function use
> `ds->output_report_dmabuf` directly?
> [...]

Never mind, sorry, the next patch answered this question.

Re: [PATCH v2 08/13] HID: playstation: add DualSense classic rumble support.

[Roderick Colenbrander]

Hi Barnabás,

Thanks for your comments.

On Thu, Jan 7, 2021 at 12:41 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Hi
>
>
> 2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:
>
> > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> >
> > The DualSense features a haptics system based on voicecoil motors,
> > which requires PCM data (or special HID packets using Bluetooth). There
> > is no appropriate API yet in the Linux kernel to expose these. The
> > controller also provides a classic rumble feature for backwards
> > compatibility. Expose this classic rumble feature using the FF framework.
> >
> > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> >
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index ef175c1cb15c..e6c67aaa1a1a 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -863,6 +863,14 @@  config HID_PLAYSTATION
> >         its special functionalities e.g. touchpad, lights and motion
> >         sensors.
> >
> > +config PLAYSTATION_FF
>
> I'm wondering if HID_PLAYSTATION_FF would be a better name?

I'm not sure. Other drivers use "NAME_FF". It seems that FF-only
drivers use HID_NAME_FF.

>
> > +     bool "PlayStation force feedback support"
> > +     depends on HID_PLAYSTATION
> > +     select INPUT_FF_MEMLESS
> > +     help
> > +       Say Y here if you would like to enable force feedback support for
> > +       PlayStation game controllers.
> > +
> >  config HID_PRIMAX
> >       tristate "Primax non-fully HID-compliant devices"
> >       depends on HID
> > diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> > index 552a52a50b78..36a904b2f93f 100644
> > --- a/drivers/hid/hid-playstation.c
> > +++ b/drivers/hid/hid-playstation.c
> > [...]
> > @@ -136,6 +151,63 @@  struct dualsense_input_report {
> >       uint8_t reserved4[11];
> >  } __packed;
> >
> > +/* Common data between DualSense BT/USB main output report. */
> > +struct dualsense_output_report_common {
> > +     uint8_t valid_flag0;
> > +     uint8_t valid_flag1;
> > +
> > +     /* For DualShock 4 compatibility mode. */
> > +     uint8_t motor_right;
> > +     uint8_t motor_left;
> > +
> > +     /* Audio controls */
> > +     uint8_t reserved[4];
> > +     uint8_t mute_button_led;
> > +
> > +     uint8_t power_save_control;
> > +     uint8_t reserved2[28];
> > +
> > +     /* LEDs and lightbar */
> > +     uint8_t valid_flag2;
> > +     uint8_t reserved3[2];
> > +     uint8_t lightbar_setup;
> > +     uint8_t led_brightness;
> > +     uint8_t player_leds;
> > +     uint8_t lightbar_red;
> > +     uint8_t lightbar_green;
> > +     uint8_t lightbar_blue;
> > +} __packed;
> > +
> > +struct dualsense_output_report_bt {
> > +     uint8_t report_id; /* 0x31 */
> > +     uint8_t seq_tag;
> > +     uint8_t tag;
> > +     struct dualsense_output_report_common common;
> > +     uint8_t reserved[24];
> > +     __le32 crc32;
> > +} __packed;
> > +
> > +struct dualsense_output_report_usb {
> > +     uint8_t report_id; /* 0x02 */
> > +     struct dualsense_output_report_common common;
> > +} __packed;
> > +
>
> I think it'd be good if you could add static_asserts to check the sizes of
> the __packed structs at compile time.

Good idea. Added some checks.
>
>
> > +/* The DualSense has a main output report used to control most features. It is
> > + * largely the same between Bluetooth and USB except for different headers and CRC.
> > + * This structure hide the differences between the two to simplify sending output reports.
> > + */
> > +struct dualsense_output_report {
> > +     uint8_t *data; /* Start of data */
> > +     uint8_t len; /* Size of output report */
> > +
> > +     /* Points to Bluetooth data payload in case for a Bluetooth report else NULL. */
> > +     struct dualsense_output_report_bt *bt;
> > +     /* Points to USB data payload in case for a USB report else NULL. */
> > +     struct dualsense_output_report_usb *usb;
> > +     /* Points to common section of report, so past any headers */
> > +     struct dualsense_output_report_common *common;
> > +};
> > [...]
> > +static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
> > +             void *buf)
>
> If the dualsense struct is already passed in, couldn't this function use
> `ds->output_report_dmabuf` directly?
>
>
> > +{
> > +     struct hid_device *hdev = ds->base.hdev;
> > +
> > +     if (hdev->bus == BUS_BLUETOOTH) {
> > +             struct dualsense_output_report_bt *bt = buf;
> > +
> > +             memset(bt, 0, sizeof(*bt));
> > +             bt->report_id = DS_OUTPUT_REPORT_BT;
> > +             bt->tag = 0x10; /* Magic number must be set to 0x10 */
>
> I think it would be preferable if that 0x10 were named.

Not sure yet what I want to do. I was only given this magic number and
I don't even know what it is, so even for me it is magic :)

>
> > +
> > +             /* Highest 4-bit is a sequence number, which needs to be increased
> > +              * every report. Lowest 4-bit is tag and can be zero for now.
> > +              */
> > +             bt->seq_tag = (ds->output_seq << 4) | 0x0;
> > +             if (++ds->output_seq == 15)
> > +                     ds->output_seq = 0;
>
> If I see it correctly, the maximum sequence number is 14; is that intentional?
> Or am I missing something?
>
>
> > +
> > +             rp->data = buf;
> > +             rp->len = sizeof(*bt);
> > +             rp->bt = bt;
> > +             rp->usb = NULL;
> > +             rp->common = &bt->common;
> > +     } else { /* USB */
> > +             struct dualsense_output_report_usb *usb = buf;
> > +
> > +             memset(usb, 0, sizeof(*usb));
> > +             usb->report_id = DS_OUTPUT_REPORT_USB;
> > +
> > +             rp->data = buf;
> > +             rp->len = sizeof(*usb);
> > +             rp->bt = NULL;
> > +             rp->usb = usb;
> > +             rp->common = &usb->common;
> > +     }
> > +}
> > +
> > +/* Helper function to send DualSense output reports. Applies a CRC at the end of a report
> > + * for Bluetooth reports.
> > + */
> > +static void dualsense_send_output_report(struct dualsense *ds,
> > +             struct dualsense_output_report *report)
> > +{
> > +     struct hid_device *hdev = ds->base.hdev;
> > +
> > +     /* Bluetooth packets need to be signed with a CRC in the last 4 bytes. */
> > +     if (report->bt) {
> > +             uint32_t crc;
> > +             uint8_t seed = 0xA2;
>
> Maybe this '0xA2' could be named as well? And I think it would be better if
> all hexadecimal constants would either be lowercase or uppercase.

I will see if I can find an appropriate name..

>
> > +
> > +             crc = crc32_le(0xFFFFFFFF, &seed, 1);
> > +             crc = ~crc32_le(crc, report->data, report->len - 4);
> > +
> > +             report->bt->crc32 = cpu_to_le32(crc);
> > +     }
> > +
> > +     hid_hw_output_report(hdev, report->data, report->len);
> > +}
> > [...]
> >  static struct ps_device *dualsense_create(struct hid_device *hdev)
> >  {
> >       struct dualsense *ds;
> >       struct ps_device *ps_dev;
> > +     uint8_t max_output_report_size;
> >       int ret;
> >
> >       ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL);
> > @@ -696,8 +882,14 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
> >       ps_dev->battery_capacity = 100; /* initial value until parse_report. */
> >       ps_dev->battery_status = POWER_SUPPLY_STATUS_UNKNOWN;
> >       ps_dev->parse_report = dualsense_parse_report;
> > +     INIT_WORK(&ds->output_worker, dualsense_output_worker);
> >       hid_set_drvdata(hdev, ds);
> >
> > +     max_output_report_size = sizeof(struct dualsense_output_report_bt);
>
> I think `max(sizeof(..._bt), sizeof(..._usb))` (linux/minmax.h) would be
> more expressive?
>
>
> > +     ds->output_report_dmabuf = devm_kzalloc(&hdev->dev, max_output_report_size, GFP_KERNEL);
> > +     if (!ds->output_report_dmabuf)
> > +             return ERR_PTR(-ENOMEM);
> > +
> >       ret = dualsense_get_mac_address(ds);
> >       if (ret < 0) {
> >               hid_err(hdev, "Failed to get MAC address from DualSense\n");
> > @@ -715,7 +907,7 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
> >               goto err;
> >       }
> >
> > -     ds->gamepad = ps_gamepad_create(hdev);
> > +     ds->gamepad = ps_gamepad_create(hdev, dualsense_play_effect);
> >       if (IS_ERR(ds->gamepad)) {
> >               ret = PTR_ERR(ds->gamepad);
> >               goto err;
> >
> >
>
>
> Regards,
> Barnabás Pőcze


Regards,
Roderick

[PATCH v2 09/13] HID: playstation: add DualSense lightbar support

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Expose the DualSense its RGB lightbar using the new multicolor LED
framework.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/Kconfig           |   1 +
 drivers/hid/hid-playstation.c | 117 ++++++++++++++++++++++++++++++++++
 2 files changed, 118 insertions(+)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index e6c67aaa1a1a..c80c81916f4a 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -857,6 +857,7 @@ config HID_PLAYSTATION
 	tristate "PlayStation HID Driver"
 	default !EXPERT
 	depends on HID
+	select LEDS_CLASS_MULTICOLOR
 	select POWER_SUPPLY
 	help
 	  Provides support for Sony PS5 controllers including support for
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 36a904b2f93f..3b26445acbb9 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -10,6 +10,7 @@
 #include <linux/device.h>
 #include <linux/hid.h>
 #include <linux/input/mt.h>
+#include <linux/led-class-multicolor.h>
 #include <linux/module.h>
 
 #include <asm/unaligned.h>
@@ -87,6 +88,10 @@ struct ps_calibration_data {
 /* Flags for DualSense output report. */
 #define DS_OUTPUT_VALID_FLAG0_COMPATIBLE_VIBRATION BIT(0)
 #define DS_OUTPUT_VALID_FLAG0_HAPTICS_SELECT BIT(1)
+#define DS_OUTPUT_VALID_FLAG1_LIGHTBAR_CONTROL_ENABLE BIT(2)
+#define DS_OUTPUT_VALID_FLAG1_RELEASE_LEDS BIT(3)
+#define DS_OUTPUT_VALID_FLAG2_LIGHTBAR_SETUP_CONTROL_ENABLE BIT(1)
+#define DS_OUTPUT_LIGHTBAR_SETUP_LIGHT_OUT BIT(1)
 
 /* DualSense hardware limits */
 #define DS_ACC_RES_PER_G	8192
@@ -116,6 +121,13 @@ struct dualsense {
 	uint8_t motor_left;
 	uint8_t motor_right;
 
+	/* RGB lightbar */
+	struct led_classdev_mc lightbar;
+	bool update_lightbar;
+	uint8_t lightbar_red;
+	uint8_t lightbar_green;
+	uint8_t lightbar_blue;
+
 	struct work_struct output_worker;
 	void *output_report_dmabuf;
 	uint8_t output_seq; /* Sequence number for output report. */
@@ -407,6 +419,45 @@ static struct input_dev *ps_gamepad_create(struct hid_device *hdev,
 	return gamepad;
 }
 
+/* Register a DualSense/DualShock4 RGB lightbar represented by a multicolor LED. */
+static int ps_lightbar_register(struct ps_device *ps_dev, struct led_classdev_mc *lightbar_mc_dev,
+	int (*brightness_set)(struct led_classdev *, enum led_brightness))
+{
+	struct hid_device *hdev = ps_dev->hdev;
+	struct mc_subled *mc_led_info;
+	struct led_classdev *led_cdev;
+	int ret;
+
+	mc_led_info = devm_kzalloc(&hdev->dev, 3*sizeof(*mc_led_info), GFP_KERNEL);
+	if (!mc_led_info)
+		return -ENOMEM;
+
+	mc_led_info[0].color_index = LED_COLOR_ID_RED;
+	mc_led_info[0].channel = 0;
+	mc_led_info[1].color_index = LED_COLOR_ID_GREEN;
+	mc_led_info[1].channel = 1;
+	mc_led_info[2].color_index = LED_COLOR_ID_BLUE;
+	mc_led_info[2].channel = 2;
+
+	lightbar_mc_dev->subled_info = mc_led_info;
+	lightbar_mc_dev->num_colors = 3;
+
+	led_cdev = &lightbar_mc_dev->led_cdev;
+	led_cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "playstation::%pMR::rgb",
+			ps_dev->mac_address);
+	led_cdev->brightness = 255;
+	led_cdev->max_brightness = 255;
+	led_cdev->brightness_set_blocking = brightness_set;
+
+	ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);
+	if (ret < 0) {
+		hid_err(hdev, "Cannot register multicolor LED device\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static struct input_dev *ps_sensors_create(struct hid_device *hdev, int accel_range, int accel_res,
 		int gyro_range, int gyro_res)
 {
@@ -601,6 +652,27 @@ static int dualsense_get_mac_address(struct dualsense *ds)
 	return ret;
 }
 
+static int dualsense_lightbar_set_brightness(struct led_classdev *cdev,
+	enum led_brightness brightness)
+{
+	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
+	struct hid_device *hdev = to_hid_device(cdev->dev->parent);
+	struct dualsense *ds = hid_get_drvdata(hdev);
+	unsigned long flags;
+
+	led_mc_calc_color_components(mc_cdev, brightness);
+
+	spin_lock_irqsave(&ds->base.lock, flags);
+	ds->update_lightbar = true;
+	ds->lightbar_red = mc_cdev->subled_info[0].brightness;
+	ds->lightbar_green = mc_cdev->subled_info[1].brightness;
+	ds->lightbar_blue = mc_cdev->subled_info[2].brightness;
+	spin_unlock_irqrestore(&ds->base.lock, flags);
+
+	schedule_work(&ds->output_worker);
+	return 0;
+}
+
 static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
 		void *buf)
 {
@@ -682,6 +754,15 @@ static void dualsense_output_worker(struct work_struct *work)
 		ds->update_rumble = false;
 	}
 
+	if (ds->update_lightbar) {
+		common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_LIGHTBAR_CONTROL_ENABLE;
+		common->lightbar_red = ds->lightbar_red;
+		common->lightbar_green = ds->lightbar_green;
+		common->lightbar_blue = ds->lightbar_blue;
+
+		ds->update_lightbar = false;
+	}
+
 	spin_unlock_irqrestore(&ds->base.lock, flags);
 
 	dualsense_send_output_report(ds, &report);
@@ -861,6 +942,30 @@ static int dualsense_play_effect(struct input_dev *dev, void *data, struct ff_ef
 	return 0;
 }
 
+static int dualsense_reset_leds(struct dualsense *ds)
+{
+	struct dualsense_output_report report;
+	uint8_t *buf;
+
+	buf = kzalloc(sizeof(struct dualsense_output_report_bt), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	dualsense_init_output_report(ds, &report, buf);
+	/* On Bluetooth the DualSense outputs an animation on the lightbar
+	 * during startup and maintains a color afterwards. We need to explicitly
+	 * reconfigure the lightbar before we can do any programming later on.
+	 * In USB the lightbar is not on by default, but redoing the setup there
+	 * doesn't hurt.
+	 */
+	report.common->valid_flag2 = DS_OUTPUT_VALID_FLAG2_LIGHTBAR_SETUP_CONTROL_ENABLE;
+	report.common->lightbar_setup = DS_OUTPUT_LIGHTBAR_SETUP_LIGHT_OUT; /* Fade light out. */
+	dualsense_send_output_report(ds, &report);
+
+	kfree(buf);
+	return 0;
+}
+
 static struct ps_device *dualsense_create(struct hid_device *hdev)
 {
 	struct dualsense *ds;
@@ -930,6 +1035,18 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	if (ret < 0)
 		goto err;
 
+	/* The hardware may have control over the LEDs (e.g. in Bluetooth on startup).
+	 * Reset the LEDs (lightbar, mute, player leds), so we can control them
+	 * from software.
+	 */
+	ret = dualsense_reset_leds(ds);
+	if (ret < 0)
+		goto err;
+
+	ret = ps_lightbar_register(ps_dev, &ds->lightbar, dualsense_lightbar_set_brightness);
+	if (ret < 0)
+		goto err;
+
 	return &ds->base;
 
 err:
-- 
2.26.2

Re: [PATCH v2 09/13] HID: playstation: add DualSense lightbar support

[Barnabás Pőcze]

2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:

> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> Expose the DualSense its RGB lightbar using the new multicolor LED
> framework.
>
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> [...]
> +/* Register a DualSense/DualShock4 RGB lightbar represented by a multicolor LED. */
> +static int ps_lightbar_register(struct ps_device *ps_dev, struct led_classdev_mc *lightbar_mc_dev,
> +	int (*brightness_set)(struct led_classdev *, enum led_brightness))
> +{
> +	struct hid_device *hdev = ps_dev->hdev;
> +	struct mc_subled *mc_led_info;
> +	struct led_classdev *led_cdev;
> +	int ret;
> +
> +	mc_led_info = devm_kzalloc(&hdev->dev, 3*sizeof(*mc_led_info), GFP_KERNEL);

I believe `devm_kmalloc_array()` would be more appropriate. Have you looked at it?


> +	if (!mc_led_info)
> +		return -ENOMEM;
> +
> +	mc_led_info[0].color_index = LED_COLOR_ID_RED;
> +	mc_led_info[0].channel = 0;
> +	mc_led_info[1].color_index = LED_COLOR_ID_GREEN;
> +	mc_led_info[1].channel = 1;
> +	mc_led_info[2].color_index = LED_COLOR_ID_BLUE;
> +	mc_led_info[2].channel = 2;

Just a small note, as far as I'm aware, the `channel` member is not used by
multicolor LED functions, and it's not even used in this module as far as I see.


> +
> +	lightbar_mc_dev->subled_info = mc_led_info;
> +	lightbar_mc_dev->num_colors = 3;
> +
> +	led_cdev = &lightbar_mc_dev->led_cdev;
> +	led_cdev->name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "playstation::%pMR::rgb",
> +			ps_dev->mac_address);
> +	led_cdev->brightness = 255;
> +	led_cdev->max_brightness = 255;
> +	led_cdev->brightness_set_blocking = brightness_set;
> +
> +	ret = devm_led_classdev_multicolor_register(&hdev->dev, lightbar_mc_dev);
> +	if (ret < 0) {
> +		hid_err(hdev, "Cannot register multicolor LED device\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static struct input_dev *ps_sensors_create(struct hid_device *hdev, int accel_range, int accel_res,
>  		int gyro_range, int gyro_res)
>  {
> @@ -601,6 +652,27 @@  static int dualsense_get_mac_address(struct dualsense *ds)
>  	return ret;
>  }
>
> +static int dualsense_lightbar_set_brightness(struct led_classdev *cdev,
> +	enum led_brightness brightness)
> +{
> +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> +	struct hid_device *hdev = to_hid_device(cdev->dev->parent);
> +	struct dualsense *ds = hid_get_drvdata(hdev);

I think the previous two lines could be replaced with

```
struct dualsense *ds = container_of(mc_cdev, struct dualsense, lightbar);
```


> +	unsigned long flags;
> +
> +	led_mc_calc_color_components(mc_cdev, brightness);
> +
> +	spin_lock_irqsave(&ds->base.lock, flags);
> +	ds->update_lightbar = true;
> +	ds->lightbar_red = mc_cdev->subled_info[0].brightness;
> +	ds->lightbar_green = mc_cdev->subled_info[1].brightness;
> +	ds->lightbar_blue = mc_cdev->subled_info[2].brightness;
> +	spin_unlock_irqrestore(&ds->base.lock, flags);
> +
> +	schedule_work(&ds->output_worker);
> +	return 0;
> +}
> +
>  static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
>  		void *buf)
>  {
> @@ -682,6 +754,15 @@  static void dualsense_output_worker(struct work_struct *work)
>  		ds->update_rumble = false;
>  	}
>
> +	if (ds->update_lightbar) {
> +		common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_LIGHTBAR_CONTROL_ENABLE;
> +		common->lightbar_red = ds->lightbar_red;
> +		common->lightbar_green = ds->lightbar_green;
> +		common->lightbar_blue = ds->lightbar_blue;
> +
> +		ds->update_lightbar = false;
> +	}
> +
>  	spin_unlock_irqrestore(&ds->base.lock, flags);
>
>  	dualsense_send_output_report(ds, &report);
> @@ -861,6 +942,30 @@  static int dualsense_play_effect(struct input_dev *dev, void *data, struct ff_ef
>  	return 0;
>  }
> [...]
>  static struct ps_device *dualsense_create(struct hid_device *hdev)
>  {
>  	struct dualsense *ds;
> @@ -930,6 +1035,18 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
>  	if (ret < 0)
>  		goto err;
>
> +	/* The hardware may have control over the LEDs (e.g. in Bluetooth on startup).
> +	 * Reset the LEDs (lightbar, mute, player leds), so we can control them
> +	 * from software.
> +	 */
> +	ret = dualsense_reset_leds(ds);
> +	if (ret < 0)

I believe if `dualsense_reset_leds()` can only return 0 on success, or an errno,
then `if (ret)` would be better.


> +		goto err;
> +
> +	ret = ps_lightbar_register(ps_dev, &ds->lightbar, dualsense_lightbar_set_brightness);
> +	if (ret < 0)
> [...]

Same here.


Regards,
Barnabás Pőcze

[PATCH v2 10/13] HID: playstation: add microphone mute support for DualSense.

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

The DualSense controller has a built-in microphone exposed as an
audio device over USB (or HID using Bluetooth). A dedicated
button on the controller handles mute, but software has to configure
the device to mute the audio stream.

This patch captures the mute button and schedules an output report
to mute/unmute the audio stream as well as toggle the mute LED.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/Kconfig           |   1 +
 drivers/hid/hid-playstation.c | 102 ++++++++++++++++++++++++++++++++++
 2 files changed, 103 insertions(+)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index c80c81916f4a..9b1803f8f935 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -857,6 +857,7 @@ config HID_PLAYSTATION
 	tristate "PlayStation HID Driver"
 	default !EXPERT
 	depends on HID
+	select LEDS_CLASS
 	select LEDS_CLASS_MULTICOLOR
 	select POWER_SUPPLY
 	help
diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 3b26445acbb9..ebf8a6550308 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -10,6 +10,7 @@
 #include <linux/device.h>
 #include <linux/hid.h>
 #include <linux/input/mt.h>
+#include <linux/leds.h>
 #include <linux/led-class-multicolor.h>
 #include <linux/module.h>
 
@@ -47,6 +48,12 @@ struct ps_calibration_data {
 	int sens_denom;
 };
 
+struct ps_led_info {
+	const char *name;
+	enum led_brightness (*brightness_get)(struct led_classdev *cdev);
+	void (*brightness_set)(struct led_classdev *cdev, enum led_brightness);
+};
+
 #define DS_INPUT_REPORT_USB			0x01
 #define DS_INPUT_REPORT_BT			0x31
 #define DS_OUTPUT_REPORT_USB			0x02
@@ -73,6 +80,7 @@ struct ps_calibration_data {
 #define DS_BUTTONS1_R3		BIT(7)
 #define DS_BUTTONS2_PS_HOME	BIT(0)
 #define DS_BUTTONS2_TOUCHPAD	BIT(1)
+#define DS_BUTTONS2_MIC_MUTE	BIT(2)
 
 /* Status field of DualSense input report. */
 #define DS_STATUS_BATTERY_CAPACITY	GENMASK(3, 0)
@@ -88,9 +96,12 @@ struct ps_calibration_data {
 /* Flags for DualSense output report. */
 #define DS_OUTPUT_VALID_FLAG0_COMPATIBLE_VIBRATION BIT(0)
 #define DS_OUTPUT_VALID_FLAG0_HAPTICS_SELECT BIT(1)
+#define DS_OUTPUT_VALID_FLAG1_MIC_MUTE_LED_CONTROL_ENABLE BIT(0)
+#define DS_OUTPUT_VALID_FLAG1_POWER_SAVE_CONTROL_ENABLE BIT(1)
 #define DS_OUTPUT_VALID_FLAG1_LIGHTBAR_CONTROL_ENABLE BIT(2)
 #define DS_OUTPUT_VALID_FLAG1_RELEASE_LEDS BIT(3)
 #define DS_OUTPUT_VALID_FLAG2_LIGHTBAR_SETUP_CONTROL_ENABLE BIT(1)
+#define DS_OUTPUT_POWER_SAVE_CONTROL_MIC_MUTE BIT(4)
 #define DS_OUTPUT_LIGHTBAR_SETUP_LIGHT_OUT BIT(1)
 
 /* DualSense hardware limits */
@@ -128,6 +139,12 @@ struct dualsense {
 	uint8_t lightbar_green;
 	uint8_t lightbar_blue;
 
+	/* Microphone */
+	bool update_mic_mute;
+	bool mic_muted;
+	bool last_btn_mic_state;
+	struct led_classdev mute_led;
+
 	struct work_struct output_worker;
 	void *output_report_dmabuf;
 	uint8_t output_seq; /* Sequence number for output report. */
@@ -419,6 +436,32 @@ static struct input_dev *ps_gamepad_create(struct hid_device *hdev,
 	return gamepad;
 }
 
+static int ps_led_register(struct ps_device *ps_dev, struct led_classdev *led,
+		struct ps_led_info *led_info)
+{
+	int ret;
+
+	led->name = devm_kasprintf(&ps_dev->hdev->dev, GFP_KERNEL,
+			"playstation::%pMR::%s", ps_dev->mac_address, led_info->name);
+
+	if (!led->name)
+		return -ENOMEM;
+
+	led->brightness = 0;
+	led->max_brightness = 1;
+	led->flags = LED_CORE_SUSPENDRESUME;
+	led->brightness_get = led_info->brightness_get;
+	led->brightness_set = led_info->brightness_set;
+
+	ret = devm_led_classdev_register(&ps_dev->hdev->dev, led);
+	if (ret) {
+		hid_err(ps_dev->hdev, "Failed to register LED %s: %d\n", led_info->name, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 /* Register a DualSense/DualShock4 RGB lightbar represented by a multicolor LED. */
 static int ps_lightbar_register(struct ps_device *ps_dev, struct led_classdev_mc *lightbar_mc_dev,
 	int (*brightness_set)(struct led_classdev *, enum led_brightness))
@@ -673,6 +716,20 @@ static int dualsense_lightbar_set_brightness(struct led_classdev *cdev,
 	return 0;
 }
 
+static enum led_brightness dualsense_mute_led_get_brightness(struct led_classdev *led)
+{
+	struct hid_device *hdev = to_hid_device(led->dev->parent);
+	struct dualsense *ds = hid_get_drvdata(hdev);
+
+	return ds->mic_muted;
+}
+
+/* The mute LED is treated as read-only. This set call prevents ENOTSUP errors e.g. on unload. */
+static void dualsense_mute_led_set_brightness(struct led_classdev *led, enum led_brightness value)
+{
+
+}
+
 static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
 		void *buf)
 {
@@ -763,6 +820,26 @@ static void dualsense_output_worker(struct work_struct *work)
 		ds->update_lightbar = false;
 	}
 
+	if (ds->update_mic_mute) {
+		if (ds->mic_muted) {
+			common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_MIC_MUTE_LED_CONTROL_ENABLE;
+			common->mute_button_led = 1; /* Enable mute LED. */
+
+			/* Disable microphone */
+			common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_POWER_SAVE_CONTROL_ENABLE;
+			common->power_save_control |= DS_OUTPUT_POWER_SAVE_CONTROL_MIC_MUTE;
+		} else {
+			common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_MIC_MUTE_LED_CONTROL_ENABLE;
+			common->mute_button_led = 0; /* Disable mute LED. */
+
+			/* Enable microphone */
+			common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_POWER_SAVE_CONTROL_ENABLE;
+			common->power_save_control &= ~DS_OUTPUT_POWER_SAVE_CONTROL_MIC_MUTE;
+		}
+
+		ds->update_mic_mute = false;
+	}
+
 	spin_unlock_irqrestore(&ds->base.lock, flags);
 
 	dualsense_send_output_report(ds, &report);
@@ -777,6 +854,7 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
 	uint8_t battery_data, battery_capacity, charging_status, value;
 	int battery_status;
 	uint16_t sensor_timestamp;
+	bool btn_mic_state;
 	unsigned long flags;
 	int i;
 
@@ -829,6 +907,22 @@ static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
 	input_report_key(ds->gamepad, BTN_MODE,   ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
 	input_sync(ds->gamepad);
 
+	/* The DualSense has an internal microphone, which can be muted through a mute button
+	 * on the device. The driver expected to read the button state and program the device
+	 * to mute/unmute audio at the hardware level.
+	 */
+	btn_mic_state = !!(ds_report->buttons[2] & DS_BUTTONS2_MIC_MUTE);
+	if (btn_mic_state && !ds->last_btn_mic_state) {
+		spin_lock_irqsave(&ps_dev->lock, flags);
+		ds->update_mic_mute = true;
+		ds->mic_muted = !ds->mic_muted; /* toggle */
+		spin_unlock_irqrestore(&ps_dev->lock, flags);
+
+		/* Schedule updating of microphone state at hardware level. */
+		schedule_work(&ds->output_worker);
+	}
+	ds->last_btn_mic_state = btn_mic_state;
+
 	/* Parse and calibrate gyroscope data. */
 	for (i = 0; i < 3; i++) {
 		int raw_data = (short)le16_to_cpu(ds_report->gyro[i]);
@@ -973,6 +1067,10 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	uint8_t max_output_report_size;
 	int ret;
 
+	struct ps_led_info mute_led_info = {
+		"micmute", dualsense_mute_led_get_brightness, dualsense_mute_led_set_brightness
+	};
+
 	ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL);
 	if (!ds)
 		return ERR_PTR(-ENOMEM);
@@ -1047,6 +1145,10 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	if (ret < 0)
 		goto err;
 
+	ret = ps_led_register(ps_dev, &ds->mute_led, &mute_led_info);
+	if (ret < 0)
+		goto err;
+
 	return &ds->base;
 
 err:
-- 
2.26.2

Re: [PATCH v2 10/13] HID: playstation: add microphone mute support for DualSense.

[Barnabás Pőcze]

Hi


2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:

> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> The DualSense controller has a built-in microphone exposed as an
> audio device over USB (or HID using Bluetooth). A dedicated
> button on the controller handles mute, but software has to configure
> the device to mute the audio stream.
>
> This patch captures the mute button and schedules an output report
> to mute/unmute the audio stream as well as toggle the mute LED.
>
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index c80c81916f4a..9b1803f8f935 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -857,6 +857,7 @@  config HID_PLAYSTATION
>  	tristate "PlayStation HID Driver"
>  	default !EXPERT
>  	depends on HID

I think maybe "select NEW_LEDS" should be here as well?


> +	select LEDS_CLASS
>  	select LEDS_CLASS_MULTICOLOR
>  	select POWER_SUPPLY
>  	help
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index 3b26445acbb9..ebf8a6550308 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> [...]
> +static int ps_led_register(struct ps_device *ps_dev, struct led_classdev *led,
> +		struct ps_led_info *led_info)
> +{
> +	int ret;
> +
> +	led->name = devm_kasprintf(&ps_dev->hdev->dev, GFP_KERNEL,
> +			"playstation::%pMR::%s", ps_dev->mac_address, led_info->name);
> +
> +	if (!led->name)
> +		return -ENOMEM;
> +
> +	led->brightness = 0;
> +	led->max_brightness = 1;
> +	led->flags = LED_CORE_SUSPENDRESUME;
> +	led->brightness_get = led_info->brightness_get;
> +	led->brightness_set = led_info->brightness_set;
> +
> +	ret = devm_led_classdev_register(&ps_dev->hdev->dev, led);
> +	if (ret) {
> +		hid_err(ps_dev->hdev, "Failed to register LED %s: %d\n", led_info->name, ret);

Another inconsistency with log messages is that sometimes the error code is printed,
sometimes it is not. I believe it would be preferable if there was no incosistency.


> +		return ret;
> +	}
> +
> +	return 0;
> +}
> [...]
> +static enum led_brightness dualsense_mute_led_get_brightness(struct led_classdev *led)
> +{
> +	struct hid_device *hdev = to_hid_device(led->dev->parent);
> +	struct dualsense *ds = hid_get_drvdata(hdev);

I think these two lines could be replaced with:

```
struct dualsense *ds = container_of(led, struct dualsense, mute_led);
```


> +
> +	return ds->mic_muted;
> +}
> [...]
>  static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
>  		void *buf)
>  {
> @@ -763,6 +820,26 @@  static void dualsense_output_worker(struct work_struct *work)
>  		ds->update_lightbar = false;
>  	}
>
> +	if (ds->update_mic_mute) {
> +		if (ds->mic_muted) {
> +			common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_MIC_MUTE_LED_CONTROL_ENABLE;
> +			common->mute_button_led = 1; /* Enable mute LED. */
> +
> +			/* Disable microphone */
> +			common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_POWER_SAVE_CONTROL_ENABLE;
> +			common->power_save_control |= DS_OUTPUT_POWER_SAVE_CONTROL_MIC_MUTE;
> +		} else {
> +			common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_MIC_MUTE_LED_CONTROL_ENABLE;
> +			common->mute_button_led = 0; /* Disable mute LED. */
> +
> +			/* Enable microphone */
> +			common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_POWER_SAVE_CONTROL_ENABLE;
> +			common->power_save_control &= ~DS_OUTPUT_POWER_SAVE_CONTROL_MIC_MUTE;
> +		}
> +

I'd put the first two lines (or at least the first line) outside the conditional.


> +		ds->update_mic_mute = false;
> +	}
> +
>  	spin_unlock_irqrestore(&ds->base.lock, flags);
>
>  	dualsense_send_output_report(ds, &report);
> @@ -777,6 +854,7 @@  static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
>  	uint8_t battery_data, battery_capacity, charging_status, value;
>  	int battery_status;
>  	uint16_t sensor_timestamp;
> +	bool btn_mic_state;
>  	unsigned long flags;
>  	int i;
>
> @@ -829,6 +907,22 @@  static int dualsense_parse_report(struct ps_device *ps_dev, struct hid_report *r
>  	input_report_key(ds->gamepad, BTN_MODE,   ds_report->buttons[2] & DS_BUTTONS2_PS_HOME);
>  	input_sync(ds->gamepad);
>
> +	/* The DualSense has an internal microphone, which can be muted through a mute button
> +	 * on the device. The driver expected to read the button state and program the device
> [...]

"The driver is expected" ?


Regards,
Barnabás Pőcze

[PATCH v2 11/13] HID: playstation: add DualSense player LEDs support.

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

The DualSense features 5 player LEDs below its touchpad, which are
meant as player id indications. This patch exposes the player LEDs
as individual LEDs.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/hid-playstation.c | 60 ++++++++++++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index ebf8a6550308..183f011f081b 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -100,6 +100,7 @@ struct ps_led_info {
 #define DS_OUTPUT_VALID_FLAG1_POWER_SAVE_CONTROL_ENABLE BIT(1)
 #define DS_OUTPUT_VALID_FLAG1_LIGHTBAR_CONTROL_ENABLE BIT(2)
 #define DS_OUTPUT_VALID_FLAG1_RELEASE_LEDS BIT(3)
+#define DS_OUTPUT_VALID_FLAG1_PLAYER_INDICATOR_CONTROL_ENABLE BIT(4)
 #define DS_OUTPUT_VALID_FLAG2_LIGHTBAR_SETUP_CONTROL_ENABLE BIT(1)
 #define DS_OUTPUT_POWER_SAVE_CONTROL_MIC_MUTE BIT(4)
 #define DS_OUTPUT_LIGHTBAR_SETUP_LIGHT_OUT BIT(1)
@@ -145,6 +146,11 @@ struct dualsense {
 	bool last_btn_mic_state;
 	struct led_classdev mute_led;
 
+	/* Player leds */
+	bool update_player_leds;
+	uint8_t player_leds_state;
+	struct led_classdev player_leds[5];
+
 	struct work_struct output_worker;
 	void *output_report_dmabuf;
 	uint8_t output_seq; /* Sequence number for output report. */
@@ -730,6 +736,35 @@ static void dualsense_mute_led_set_brightness(struct led_classdev *led, enum led
 
 }
 
+static enum led_brightness dualsense_player_led_get_brightness(struct led_classdev *led)
+{
+	struct hid_device *hdev = to_hid_device(led->dev->parent);
+	struct dualsense *ds = hid_get_drvdata(hdev);
+
+	return !!(ds->player_leds_state & BIT(led - ds->player_leds));
+}
+
+static void dualsense_player_led_set_brightness(struct led_classdev *led, enum led_brightness value)
+{
+	struct hid_device *hdev = to_hid_device(led->dev->parent);
+	struct dualsense *ds = hid_get_drvdata(hdev);
+	unsigned long flags;
+	unsigned int led_index;
+
+	spin_lock_irqsave(&ds->base.lock, flags);
+
+	led_index = led - ds->player_leds;
+	if (value == LED_OFF)
+		ds->player_leds_state &= ~(1 << led_index);
+	else
+		ds->player_leds_state |= (1 << led_index);
+
+	ds->update_player_leds = true;
+	spin_unlock_irqrestore(&ds->base.lock, flags);
+
+	schedule_work(&ds->output_worker);
+}
+
 static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
 		void *buf)
 {
@@ -820,6 +855,13 @@ static void dualsense_output_worker(struct work_struct *work)
 		ds->update_lightbar = false;
 	}
 
+	if (ds->update_player_leds) {
+		common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_PLAYER_INDICATOR_CONTROL_ENABLE;
+		common->player_leds = ds->player_leds_state;
+
+		ds->update_player_leds = false;
+	}
+
 	if (ds->update_mic_mute) {
 		if (ds->mic_muted) {
 			common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_MIC_MUTE_LED_CONTROL_ENABLE;
@@ -1065,12 +1107,20 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	struct dualsense *ds;
 	struct ps_device *ps_dev;
 	uint8_t max_output_report_size;
-	int ret;
+	int i, ret;
 
 	struct ps_led_info mute_led_info = {
 		"micmute", dualsense_mute_led_get_brightness, dualsense_mute_led_set_brightness
 	};
 
+	struct ps_led_info player_leds_info[] = {
+		{ "led1", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness },
+		{ "led2", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness },
+		{ "led3", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness },
+		{ "led4", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness },
+		{ "led5", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness }
+	};
+
 	ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL);
 	if (!ds)
 		return ERR_PTR(-ENOMEM);
@@ -1149,6 +1199,14 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	if (ret < 0)
 		goto err;
 
+	for (i = 0; i < ARRAY_SIZE(player_leds_info); i++) {
+		struct ps_led_info *led_info = &player_leds_info[i];
+
+		ret = ps_led_register(ps_dev, &ds->player_leds[i], led_info);
+		if (ret < 0)
+			goto err;
+	}
+
 	return &ds->base;
 
 err:
-- 
2.26.2

Re: [PATCH v2 11/13] HID: playstation: add DualSense player LEDs support.

[Barnabás Pőcze]

Hi


2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:

> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> The DualSense features 5 player LEDs below its touchpad, which are
> meant as player id indications. This patch exposes the player LEDs
> as individual LEDs.
>
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index ebf8a6550308..183f011f081b 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> [...]
> +static enum led_brightness dualsense_player_led_get_brightness(struct led_classdev *led)
> +{
> +	struct hid_device *hdev = to_hid_device(led->dev->parent);
> +	struct dualsense *ds = hid_get_drvdata(hdev);
> +
> +	return !!(ds->player_leds_state & BIT(led - ds->player_leds));
> +}
> +
> +static void dualsense_player_led_set_brightness(struct led_classdev *led, enum led_brightness value)
> +{
> +	struct hid_device *hdev = to_hid_device(led->dev->parent);
> +	struct dualsense *ds = hid_get_drvdata(hdev);
> +	unsigned long flags;
> +	unsigned int led_index;
> +
> +	spin_lock_irqsave(&ds->base.lock, flags);
> +
> +	led_index = led - ds->player_leds;
> +	if (value == LED_OFF)
> +		ds->player_leds_state &= ~(1 << led_index);
> +	else
> +		ds->player_leds_state |= (1 << led_index);

A minor thing: I think `BIT(led_index)` should be used here.


> +
> +	ds->update_player_leds = true;
> +	spin_unlock_irqrestore(&ds->base.lock, flags);
> +
> +	schedule_work(&ds->output_worker);
> +}
> +
>  static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
>  		void *buf)
>  {
> @@ -820,6 +855,13 @@  static void dualsense_output_worker(struct work_struct *work)
>  		ds->update_lightbar = false;
>  	}
>
> +	if (ds->update_player_leds) {
> +		common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_PLAYER_INDICATOR_CONTROL_ENABLE;
> +		common->player_leds = ds->player_leds_state;
> +
> +		ds->update_player_leds = false;
> +	}
> +
>  	if (ds->update_mic_mute) {
>  		if (ds->mic_muted) {
>  			common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_MIC_MUTE_LED_CONTROL_ENABLE;
> @@ -1065,12 +1107,20 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
>  	struct dualsense *ds;
>  	struct ps_device *ps_dev;
>  	uint8_t max_output_report_size;
> -	int ret;
> +	int i, ret;
>
>  	struct ps_led_info mute_led_info = {
>  		"micmute", dualsense_mute_led_get_brightness, dualsense_mute_led_set_brightness
>  	};
>
> +	struct ps_led_info player_leds_info[] = {
> +		{ "led1", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness },
> +		{ "led2", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness },
> +		{ "led3", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness },
> +		{ "led4", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness },
> +		{ "led5", dualsense_player_led_get_brightness, dualsense_player_led_set_brightness }
> +	};

I think it would make sense to make `player_leds_info` and even `mute_led_info`
`const` and `static`.


> +
>  	ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL);
>  	if (!ds)
>  		return ERR_PTR(-ENOMEM);
> @@ -1149,6 +1199,14 @@  static struct ps_device *dualsense_create(struct hid_device *hdev)
>  	if (ret < 0)
>  		goto err;
>
> +	for (i = 0; i < ARRAY_SIZE(player_leds_info); i++) {
> +		struct ps_led_info *led_info = &player_leds_info[i];
> +
> +		ret = ps_led_register(ps_dev, &ds->player_leds[i], led_info);
> +		if (ret < 0)
> +			goto err;
> +	}
> +
>  	return &ds->base;
>
>  err:


Regards,
Barnabás Pőcze

[PATCH v2 12/13] HID: playstation: DualSense set LEDs to default player id.

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Add a ID allocator to assign player ids to ps_device instances.
Utilize the player id to set a default color on the DualSense its
player LED strip.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/hid-playstation.c | 69 ++++++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 183f011f081b..1a95c81da8a3 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -9,6 +9,7 @@
 #include <linux/crc32.h>
 #include <linux/device.h>
 #include <linux/hid.h>
+#include <linux/idr.h>
 #include <linux/input/mt.h>
 #include <linux/leds.h>
 #include <linux/led-class-multicolor.h>
@@ -22,6 +23,8 @@
 static DEFINE_MUTEX(ps_devices_lock);
 static LIST_HEAD(ps_devices_list);
 
+static DEFINE_IDA(ps_player_id_allocator);
+
 #define HID_PLAYSTATION_VERSION_PATCH 0x8000
 
 /* Base class for playstation devices. */
@@ -30,6 +33,8 @@ struct ps_device {
 	struct hid_device *hdev;
 	spinlock_t lock;
 
+	uint32_t player_id;
+
 	struct power_supply_desc battery_desc;
 	struct power_supply *battery;
 	uint8_t battery_capacity;
@@ -299,6 +304,24 @@ static int ps_devices_list_remove(struct ps_device *dev)
 	return 0;
 }
 
+static int ps_device_set_player_id(struct ps_device *dev)
+{
+	int ret = ida_alloc(&ps_player_id_allocator, GFP_KERNEL);
+
+	if (ret < 0)
+		return ret;
+
+	dev->player_id = ret;
+	return 0;
+}
+
+static void ps_device_release_player_id(struct ps_device *dev)
+{
+	ida_free(&ps_player_id_allocator, dev->player_id);
+
+	dev->player_id = -1;
+}
+
 static struct input_dev *ps_allocate_input_dev(struct hid_device *hdev, const char *name_suffix)
 {
 	struct input_dev *input_dev;
@@ -1102,6 +1125,28 @@ static int dualsense_reset_leds(struct dualsense *ds)
 	return 0;
 }
 
+static void dualsense_set_player_leds(struct dualsense *ds)
+{
+	/* The DualSense controller has a row of 5 LEDs used for player ids.
+	 * Behavior on the PlayStation 5 console is to center the player id
+	 * across the LEDs, so e.g. player 1 would be "--x--" with x being 'on'.
+	 * Follow a similar mapping here.
+	 */
+	int player_ids[5] = {
+		BIT(2),
+		BIT(3) | BIT(1),
+		BIT(4) | BIT(2) | BIT(0),
+		BIT(4) | BIT(3) | BIT(1) | BIT(0),
+		BIT(4) | BIT(3) | BIT(2) | BIT(1) | BIT(0)
+	};
+
+	uint8_t player_id = ds->base.player_id % 5;
+
+	ds->update_player_leds = true;
+	ds->player_leds_state = player_ids[player_id];
+	schedule_work(&ds->output_worker);
+}
+
 static struct ps_device *dualsense_create(struct hid_device *hdev)
 {
 	struct dualsense *ds;
@@ -1207,6 +1252,15 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 			goto err;
 	}
 
+	ret = ps_device_set_player_id(ps_dev);
+	if (ret < 0) {
+		hid_err(hdev, "Failed to assign player id for DualSense\n");
+		goto err;
+	}
+
+	/* Set player LEDs to our player id. */
+	dualsense_set_player_leds(ds);
+
 	return &ds->base;
 
 err:
@@ -1271,6 +1325,7 @@ static void ps_remove(struct hid_device *hdev)
 	struct ps_device *dev = hid_get_drvdata(hdev);
 
 	ps_devices_list_remove(dev);
+	ps_device_release_player_id(dev);
 
 	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
@@ -1291,7 +1346,19 @@ static struct hid_driver ps_driver = {
 	.raw_event        = ps_raw_event,
 };
 
-module_hid_driver(ps_driver);
+static int __init ps_init(void)
+{
+	return hid_register_driver(&ps_driver);
+}
+
+static void __exit ps_exit(void)
+{
+	hid_unregister_driver(&ps_driver);
+	ida_destroy(&ps_player_id_allocator);
+}
+
+module_init(ps_init);
+module_exit(ps_exit);
 
 MODULE_AUTHOR("Sony Interactive Entertainment");
 MODULE_DESCRIPTION("HID Driver for PlayStation peripherals.");
-- 
2.26.2

Re: [PATCH v2 12/13] HID: playstation: DualSense set LEDs to default player id.

[Barnabás Pőcze]

Hi


2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:

> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> Add a ID allocator to assign player ids to ps_device instances.
> Utilize the player id to set a default color on the DualSense its
> player LED strip.
>
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
>
> [...]
> +static void ps_device_release_player_id(struct ps_device *dev)
> +{
> +	ida_free(&ps_player_id_allocator, dev->player_id);
> +
> +	dev->player_id = -1;

A minor thing, but I believe U32_MAX would be better here. I'd avoid
(especially) negative magic numbers for an unsigned value. You could even
  #define PS_PLAYER_ID_INVALID U32_MAX
or something similar.


> +}
> +
>  static struct input_dev *ps_allocate_input_dev(struct hid_device *hdev, const char *name_suffix)
>  {
>  	struct input_dev *input_dev;
> @@ -1102,6 +1125,28 @@  static int dualsense_reset_leds(struct dualsense *ds)
>  	return 0;
>  }
>
> +static void dualsense_set_player_leds(struct dualsense *ds)
> +{
> +	/* The DualSense controller has a row of 5 LEDs used for player ids.
> +	 * Behavior on the PlayStation 5 console is to center the player id
> +	 * across the LEDs, so e.g. player 1 would be "--x--" with x being 'on'.
> +	 * Follow a similar mapping here.
> +	 */
> +	int player_ids[5] = {
> +		BIT(2),
> +		BIT(3) | BIT(1),
> +		BIT(4) | BIT(2) | BIT(0),
> +		BIT(4) | BIT(3) | BIT(1) | BIT(0),
> +		BIT(4) | BIT(3) | BIT(2) | BIT(1) | BIT(0)
> +	};
> +
> +	uint8_t player_id = ds->base.player_id % 5;

I'd write `ds->base.player_id % ARRAY_SIZE(player_ids)` here.


> +
> +	ds->update_player_leds = true;
> +	ds->player_leds_state = player_ids[player_id];
> +	schedule_work(&ds->output_worker);
> +}
> [...]


Regards,
Barnabás Pőcze

[PATCH v2 13/13] HID: playstation: report DualSense hardware and firmware version.

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Retrieve DualSense hardware and firmware information using a vendor
specific feature report. Report the data through sysfs and also
report using hid_info as there can be signficant differences between
versions.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/hid-playstation.c | 84 +++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 1a95c81da8a3..8440af6d6cd7 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -41,6 +41,8 @@ struct ps_device {
 	int battery_status;
 
 	uint8_t mac_address[6];
+	uint32_t hw_version;
+	uint32_t fw_version;
 
 	int (*parse_report)(struct ps_device *dev, struct hid_report *report, u8 *data, int size);
 };
@@ -68,6 +70,8 @@ struct ps_led_info {
 #define DS_FEATURE_REPORT_CALIBRATION_SIZE	41
 #define DS_FEATURE_REPORT_PAIRING_INFO		9
 #define DS_FEATURE_REPORT_PAIRING_INFO_SIZE	19
+#define DS_FEATURE_REPORT_FIRMWARE_INFO		32
+#define DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE	64
 
 /* Button masks for DualSense input report. */
 #define DS_BUTTONS0_HAT_SWITCH	GENMASK(3, 0)
@@ -593,6 +597,40 @@ static struct input_dev *ps_touchpad_create(struct hid_device *hdev, int width,
 	return touchpad;
 }
 
+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 sysfs_emit(buf, "0x%08x\n", ps_dev->fw_version);
+}
+
+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 sysfs_emit(buf, "0x%08x\n", ps_dev->hw_version);
+}
+
+static DEVICE_ATTR(hardware_version, 0444, ps_show_hardware_version, NULL);
+
+static struct attribute *ps_device_attributes[] = {
+	&dev_attr_firmware_version.attr,
+	&dev_attr_hardware_version.attr,
+	NULL
+};
+
+static const struct attribute_group ps_device_attribute_group = {
+	.attrs = ps_device_attributes,
+};
+
 static int dualsense_get_calibration_data(struct dualsense *ds)
 {
 	short gyro_pitch_bias, gyro_pitch_plus, gyro_pitch_minus;
@@ -696,6 +734,34 @@ static int dualsense_get_calibration_data(struct dualsense *ds)
 	return ret;
 }
 
+static int dualsense_get_firmware_info(struct dualsense *ds)
+{
+	uint8_t *buf;
+	int ret;
+
+	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;
+	else if (ret != DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE) {
+		hid_err(ds->base.hdev, "failed to retrieve DualSense firmware info\n");
+		ret = -EINVAL;
+		goto err_free;
+	}
+
+	ds->base.hw_version = get_unaligned_le32(&buf[24]);
+	ds->base.fw_version = get_unaligned_le32(&buf[28]);
+
+err_free:
+	kfree(buf);
+	return ret;
+}
+
 static int dualsense_get_mac_address(struct dualsense *ds)
 {
 	uint8_t *buf;
@@ -1195,6 +1261,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(ps_dev);
 	if (ret < 0)
 		return ERR_PTR(ret);
@@ -1261,6 +1333,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 &ds->base;
 
 err:
@@ -1311,6 +1389,12 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		}
 	}
 
+	ret = devm_device_add_group(&hdev->dev, &ps_device_attribute_group);
+	if (ret < 0) {
+		hid_err(hdev, "Failed to register sysfs nodes.\n");
+		goto err_close;
+	}
+
 	return ret;
 
 err_close:
-- 
2.26.2

Re: [PATCH v2 13/13] HID: playstation: report DualSense hardware and firmware version.

[Barnabás Pőcze]

Hi


2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:

> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index 1a95c81da8a3..8440af6d6cd7 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> [...]
> +static int dualsense_get_firmware_info(struct dualsense *ds)
> +{
> +	uint8_t *buf;
> +	int ret;
> +
> +	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;
> +	else if (ret != DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE) {

As per coding style[1], please either use {} for all branches, or just drop the
`else` and maybe add a new line:

```
if (ret < 0)
  goto ...

if (ret != ...) {
  ...
}
```

> +		hid_err(ds->base.hdev, "failed to retrieve DualSense firmware info\n");
> +		ret = -EINVAL;
> +		goto err_free;
> +	}

Shouldn't the CRC be validated here when using Bluetooth? Or there is none?


> +
> +	ds->base.hw_version = get_unaligned_le32(&buf[24]);
> +	ds->base.fw_version = get_unaligned_le32(&buf[28]);
> +
> +err_free:
> +	kfree(buf);
> +	return ret;
> +}
> +
>  static int dualsense_get_mac_address(struct dualsense *ds)
>  {
>  	uint8_t *buf;
> @@ -1195,6 +1261,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(ps_dev);
>  	if (ret < 0)
>  		return ERR_PTR(ret);
> @@ -1261,6 +1333,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",

Maybe the format could be same as in the device attributes (0x%08x)?


> +			ds->base.hw_version, ds->base.fw_version);
> +
>  	return &ds->base;
>
>  err:
> @@ -1311,6 +1389,12 @@  static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		}
>  	}
>
> +	ret = devm_device_add_group(&hdev->dev, &ps_device_attribute_group);
> +	if (ret < 0) {
> +		hid_err(hdev, "Failed to register sysfs nodes.\n");
> +		goto err_close;
> +	}
> +
>  	return ret;
>
>  err_close:
>


[1]: https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces


Regards,
Barnabás Pőcze

Re: [PATCH v2 13/13] HID: playstation: report DualSense hardware and firmware version.

[Roderick Colenbrander]

Hi Barnabás,

A couple of places lacked MAC address checks (some of these reports I
didn't have datasheets on). In the end I decided to make a new helper
function as there is so much common nasty code. It also exposed a few
tiny bugs as some reports were an incorrect size (not critical as the
data wasn't used). It is a lot simpler now with more and better
checking.

static int ps_get_report(struct hid_device *hdev, uint8_t report_id,
uint8_t *buf, size_t size)
{
    int ret;

    ret = hid_hw_raw_request(hdev, report_id, buf, size,
HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
    if (ret < 0) {
        hid_err(hdev, "Failed to retrieve feature report %d,
ret=%d\n", report_id, ret);
        return ret;
    }

    if (ret != size) {
        hid_err(hdev, "Invalid byte count transferred, expected %zu
got %d\n", size, ret);
        return -EINVAL;
    }

    if (buf[0] != report_id) {
        hid_err(hdev, "Incorrect reportID received, expected %d got
%d\n", report_id, buf[0]);
        return -EINVAL;
    }

    if (hdev->bus == BUS_BLUETOOTH) {
        /* Last 4 bytes contains crc32. */
        uint8_t crc_offset = size - 4;
        uint32_t report_crc = get_unaligned_le32(&buf[crc_offset]);

        if (!ps_check_crc32(PS_FEATURE_CRC32_SEED, buf, crc_offset,
report_crc)) {
            hid_err(hdev, "CRC check failed for reportID=%d\n", report_id);
            return -EILSEQ;
        }
    }

    return 0;
}

On Thu, Jan 7, 2021 at 2:26 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Hi
>
>
> 2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:
>
> > diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> > index 1a95c81da8a3..8440af6d6cd7 100644
> > --- a/drivers/hid/hid-playstation.c
> > +++ b/drivers/hid/hid-playstation.c
> > [...]
> > +static int dualsense_get_firmware_info(struct dualsense *ds)
> > +{
> > +     uint8_t *buf;
> > +     int ret;
> > +
> > +     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;
> > +     else if (ret != DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE) {
>
> As per coding style[1], please either use {} for all branches, or just drop the
> `else` and maybe add a new line:
>
> ```
> if (ret < 0)
>   goto ...
>
> if (ret != ...) {
>   ...
> }
> ```
>
> > +             hid_err(ds->base.hdev, "failed to retrieve DualSense firmware info\n");
> > +             ret = -EINVAL;
> > +             goto err_free;
> > +     }
>
> Shouldn't the CRC be validated here when using Bluetooth? Or there is none?
>
>
> > +
> > +     ds->base.hw_version = get_unaligned_le32(&buf[24]);
> > +     ds->base.fw_version = get_unaligned_le32(&buf[28]);
> > +
> > +err_free:
> > +     kfree(buf);
> > +     return ret;
> > +}
> > +
> >  static int dualsense_get_mac_address(struct dualsense *ds)
> >  {
> >       uint8_t *buf;
> > @@ -1195,6 +1261,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(ps_dev);
> >       if (ret < 0)
> >               return ERR_PTR(ret);
> > @@ -1261,6 +1333,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",
>
> Maybe the format could be same as in the device attributes (0x%08x)?
>
>
> > +                     ds->base.hw_version, ds->base.fw_version);
> > +
> >       return &ds->base;
> >
> >  err:
> > @@ -1311,6 +1389,12 @@  static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >               }
> >       }
> >
> > +     ret = devm_device_add_group(&hdev->dev, &ps_device_attribute_group);
> > +     if (ret < 0) {
> > +             hid_err(hdev, "Failed to register sysfs nodes.\n");
> > +             goto err_close;
> > +     }
> > +
> >       return ret;
> >
> >  err_close:
> >
>
>
> [1]: https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces
>
>
> Regards,
> Barnabás Pőcze



-- 
Roderick Colenbrander
Senior Manager of Hardware & Systems Engineering
Sony Interactive Entertainment LLC
roderick.colenbrander@sony.com