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

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

[Roderick Colenbrander]

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

Hi,

I am pleased to share a new Linux driver for the PlayStation 5 'DualSense'
game controller. The driver supports the DualSense in both Bluetooth
and USB modes. Most controller features are supported including LEDs,
Touchpad, Motion Sensors and Rumble.

DualSense supported is implemented in a new 'hid-playstation' driver, which
will be used for peripherals by 'Sony Interactive Entertainment' (PlayStation).
Hid-sony will be used for devices for the larger Sony Group. We intend to
migrate existing devices over time gradually to hid-playstation. We do not
want to cause any regressions and maintain quality. As such moving forward,
unit tests are important and we started by providing these through 'hid-tools'
including DualSense.

The Linux driver exposes DualSense functionality as a 'compositive device'
similar to DualShock 4 in hid-sony, spanning multiple frameworks. First,
it exposes 3 evdev nodes for respectively the 'gamepad', 'touchpad' and
'motion sensors'. The FF framework is used to provide basic rumble features.
The leds-class is used to implement the Player indicator LEDs below the
DualSense's touchpad, while the new 'leds-class-multicolor' is used for
the lightbars next to the touchpad.

Not yet supported are new unique features introduced by the DualSense
such as Adaptive Triggers and the VCM based Haptics. These features require
a large amount of data and complex data structures. It is not clear how to
expose these. The current Evdev and FF frameworks are too limiting. We hope
to have a dialog on how to expose these over time in a generic way.

Enjoy the new DualSense driver and let us know if you have any questions
or feedback.

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.

 drivers/hid/Kconfig           |   20 +
 drivers/hid/Makefile          |    1 +
 drivers/hid/hid-ids.h         |    1 +
 drivers/hid/hid-playstation.c | 1416 +++++++++++++++++++++++++++++++++
 drivers/hid/hid-quirks.c      |    4 +
 5 files changed, 1442 insertions(+)
 create mode 100644 drivers/hid/hid-playstation.c

-- 
2.26.2

[PATCH 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 | 317 ++++++++++++++++++++++++++++++++++
 drivers/hid/hid-quirks.c      |   3 +
 6 files changed, 337 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..8dbd0ae7e082
--- /dev/null
+++ b/drivers/hid/hid-playstation.c
@@ -0,0 +1,317 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *  HID driver for Sony DualSense(TM) controller.
+ *
+ *  Copyright (c) 2020 Sony Interactive Entertainment
+ */
+
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/input/mt.h>
+#include <linux/module.h>
+#include <linux/crc32.h>
+#include <asm/unaligned.h>
+
+#include "hid-ids.h"
+
+/* 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 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 = (struct dualsense *)ps_dev;
+	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 |= 0x8000;
+
+	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 (struct ps_device *)ds;
+
+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;
+		}
+	} else {
+		hid_err(hdev, "Unhandled device\n");
+		ret = -EINVAL;
+		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),
+		.driver_data = 0 },
+};
+
+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_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

[PATCH 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 | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 8dbd0ae7e082..88823a7ba1a0 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -17,12 +17,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)
@@ -127,6 +131,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;
@@ -157,6 +162,29 @@ 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;
+
+	/* 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)
 {
@@ -225,6 +253,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

[PATCH 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 | 122 +++++++++++++++++++++++++++++++++-
 2 files changed, 122 insertions(+), 1 deletion(-)

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 88823a7ba1a0..598d262e2a08 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -17,6 +17,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);
@@ -44,6 +51,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;
@@ -131,6 +143,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)
 {
@@ -191,7 +268,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 = (struct dualsense *)ps_dev;
 	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
@@ -232,6 +311,41 @@ 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;
 }
 
@@ -250,6 +364,8 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	hdev->version |= 0x8000;
 
 	ds->base.hdev = hdev;
+	ds->base.battery_capacity = 100; /* initial value until parse_report. */
+	ds->base.battery_status = POWER_SUPPLY_STATUS_UNKNOWN;
 	ds->base.parse_report = dualsense_parse_report;
 	hid_set_drvdata(hdev, ds);
 
@@ -266,6 +382,10 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 		goto err;
 	}
 
+	ret = ps_device_register_battery((struct ps_device *)ds);
+	if (ret < 0)
+		goto err;
+
 	return (struct ps_device *)ds;
 
 err:
-- 
2.26.2

[PATCH 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 | 59 +++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 598d262e2a08..7e622b02ee30 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -56,9 +56,14 @@ struct ps_device {
 #define DS_STATUS_CHARGING		GENMASK(7, 4)
 #define DS_STATUS_CHARGING_SHIFT	4
 
+/* 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 {
@@ -239,6 +244,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,
+		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, 0, 0);
+	input_set_abs_params(touchpad, ABS_MT_POSITION_Y, 0, height, 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;
@@ -271,6 +304,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
@@ -311,6 +345,25 @@ 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);
 
+	input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
+	for (i = 0; i < 2; i++) {
+		bool active = (ds_report->points[i].contact & 0x80) ? 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;
 
@@ -382,6 +435,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((struct ps_device *)ds);
 	if (ret < 0)
 		goto err;
-- 
2.26.2

[PATCH 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 | 197 ++++++++++++++++++++++++++++++++++
 1 file changed, 197 insertions(+)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 7e622b02ee30..ea64f73e0b84 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -29,8 +29,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
 
@@ -57,13 +67,27 @@ struct ps_device {
 #define DS_STATUS_CHARGING_SHIFT	4
 
 /* 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 {
@@ -244,6 +268,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,
 		int num_contacts)
 {
@@ -272,6 +331,92 @@ 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;
+
+	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;
@@ -303,6 +448,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;
 
@@ -345,6 +491,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);
+
 	input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
 	for (i = 0; i < 2; i++) {
 		bool active = (ds_report->points[i].contact & 0x80) ? false : true;
@@ -429,12 +613,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

[PATCH 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 ea64f73e0b84..b0c0286cfd2b 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -14,8 +14,13 @@
 
 #include "hid-ids.h"
 
+/* List of connected playstation devices. */
+static DEFINE_MUTEX(ps_devices_lock);
+static LIST_HEAD(ps_devices_list);
+
 /* Base class for playstation devices. */
 struct ps_device {
+	struct list_head list;
 	struct hid_device *hdev;
 	spinlock_t lock;
 
@@ -145,6 +150,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;
@@ -613,6 +649,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((struct ps_device *)ds);
+	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");
@@ -645,6 +685,7 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	return (struct ps_device *)ds;
 
 err:
+	ps_devices_list_remove((struct ps_device *)ds);
 	return ERR_PTR(ret);
 }
 
@@ -706,6 +747,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

[PATCH 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 | 36 +++++++++++++++++++++++++++++++++++
 drivers/hid/hid-quirks.c      |  1 +
 2 files changed, 37 insertions(+)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index b0c0286cfd2b..042fa8d2627d 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -43,6 +43,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
@@ -274,6 +275,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;
@@ -390,6 +402,18 @@ static int dualsense_get_calibration_data(struct dualsense *ds)
 	if (ret < 0)
 		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]);
@@ -494,6 +518,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;
@@ -756,6 +790,8 @@ 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),
+		.driver_data = 0 },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER),
 		.driver_data = 0 },
 };
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

[PATCH 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 042fa8d2627d..0b62bcb28d8a 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -44,6 +44,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
@@ -72,6 +74,10 @@ struct ps_calibration_data {
 #define DS_STATUS_CHARGING		GENMASK(7, 4)
 #define DS_STATUS_CHARGING_SHIFT	4
 
+/* 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)
@@ -94,6 +100,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 {
@@ -126,6 +141,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.
@@ -286,7 +358,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;
@@ -309,6 +382,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);
@@ -500,6 +580,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)
 {
@@ -656,9 +822,29 @@ 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;
+	uint8_t max_output_report_size;
 	int ret;
 
 	ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL);
@@ -674,8 +860,14 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	ds->base.battery_capacity = 100; /* initial value until parse_report. */
 	ds->base.battery_status = POWER_SUPPLY_STATUS_UNKNOWN;
 	ds->base.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");
@@ -693,7 +885,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

[PATCH 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 | 124 ++++++++++++++++++++++++++++++++++
 2 files changed, 125 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 0b62bcb28d8a..f8cf82a27d43 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -8,6 +8,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 <linux/crc32.h>
 #include <asm/unaligned.h>
@@ -77,6 +78,9 @@ 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)
 
 /* DualSense hardware limits */
 #define DS_ACC_RES_PER_G	8192
@@ -106,6 +110,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. */
@@ -396,6 +407,50 @@ static struct input_dev *ps_gamepad_create(struct hid_device *hdev,
 	return gamepad;
 }
 
+/* Create a DualSense/DualShock4 RGB lightbar represented by a multicolor LED. */
+static struct led_classdev_mc *ps_lightbar_create(struct ps_device *ps_dev,
+	int (*brightness_set)(struct led_classdev *, enum led_brightness))
+{
+	struct hid_device *hdev = ps_dev->hdev;
+	struct led_classdev_mc *lightbar_mc_dev;
+	struct mc_subled *mc_led_info;
+	struct led_classdev *led_cdev;
+	int ret;
+
+	lightbar_mc_dev = devm_kzalloc(&hdev->dev, sizeof(*lightbar_mc_dev), GFP_KERNEL);
+	if (!lightbar_mc_dev)
+		return ERR_PTR(-ENOMEM);
+
+	mc_led_info = devm_kzalloc(&hdev->dev, 3*sizeof(*mc_led_info), GFP_KERNEL);
+	if (!mc_led_info)
+		return ERR_PTR(-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 ERR_PTR(ret);
+	}
+
+	return lightbar_mc_dev;
+}
+
 static struct input_dev *ps_sensors_create(struct hid_device *hdev, int accel_range, int accel_res,
 		int gyro_range, int gyro_res)
 {
@@ -580,6 +635,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)
 {
@@ -661,6 +737,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);
@@ -841,6 +926,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 = 2; /* 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;
@@ -908,6 +1017,21 @@ 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;
+
+	ds->lightbar = ps_lightbar_create((struct ps_device *)ds,
+			dualsense_lightbar_set_brightness);
+	if (IS_ERR(ds->lightbar)) {
+		ret = PTR_ERR(ds->lightbar);
+		goto err;
+	}
+
 	return (struct ps_device *)ds;
 
 err:
-- 
2.26.2

[PATCH 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 | 94 +++++++++++++++++++++++++++++++++++
 2 files changed, 95 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 f8cf82a27d43..ad8eedd3d2bf 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -8,6 +8,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>
 #include <linux/crc32.h>
@@ -43,6 +44,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
@@ -69,6 +76,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)
@@ -78,9 +86,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)
 
 /* DualSense hardware limits */
 #define DS_ACC_RES_PER_G	8192
@@ -117,6 +128,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. */
@@ -407,6 +424,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;
+}
+
 /* Create a DualSense/DualShock4 RGB lightbar represented by a multicolor LED. */
 static struct led_classdev_mc *ps_lightbar_create(struct ps_device *ps_dev,
 	int (*brightness_set)(struct led_classdev *, enum led_brightness))
@@ -656,6 +699,14 @@ 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;
+}
+
 static void dualsense_init_output_report(struct dualsense *ds, struct dualsense_output_report *rp,
 		void *buf)
 {
@@ -746,6 +797,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);
@@ -760,6 +831,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;
 
@@ -812,6 +884,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]);
@@ -956,6 +1044,8 @@ 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, NULL };
+
 	ds = devm_kzalloc(&hdev->dev, sizeof(*ds), GFP_KERNEL);
 	if (!ds)
 		return ERR_PTR(-ENOMEM);
@@ -1032,6 +1122,10 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 		goto err;
 	}
 
+	ret = ps_led_register((struct ps_device *)ds, &ds->mute_led, &mute_led_info);
+	if (ret < 0)
+		goto err;
+
 	return (struct ps_device *)ds;
 
 err:
-- 
2.26.2

[PATCH 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 | 64 ++++++++++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index ad8eedd3d2bf..384449e3095d 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -90,6 +90,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)
 
@@ -134,6 +135,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. */
@@ -707,6 +713,39 @@ static enum led_brightness dualsense_mute_led_get_brightness(struct led_classdev
 	return ds->mic_muted;
 }
 
+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);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++) {
+		if (&ds->player_leds[i] == led)
+			return !!(ds->player_leds_state & BIT(i));
+	}
+
+	return LED_OFF;
+}
+
+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);
+	uint8_t player_leds_state = 0;
+	unsigned long flags;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++)
+		player_leds_state |= (ds->player_leds[i].brightness << i);
+
+	spin_lock_irqsave(&ds->base.lock, flags);
+	ds->player_leds_state = player_leds_state;
+	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)
 {
@@ -797,6 +836,13 @@ static void dualsense_output_worker(struct work_struct *work)
 		ds->update_lightbar = false;
 	}
 
+	if (ds->update_player_leds) {
+		r->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_PLAYER_INDICATOR_CONTROL_ENABLE;
+		r->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;
@@ -1042,10 +1088,18 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 {
 	struct dualsense *ds;
 	uint8_t max_output_report_size;
-	int ret;
+	int i, ret;
 
 	struct ps_led_info mute_led_info = { "micmute", dualsense_mute_led_get_brightness, NULL };
 
+	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);
@@ -1126,6 +1180,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((struct ps_device *)ds, &ds->player_leds[i], led_info);
+		if (ret < 0)
+			goto err;
+	}
+
 	return (struct ps_device *)ds;
 
 err:
-- 
2.26.2

[PATCH 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 | 58 +++++++++++++++++++++++++++++++++--
 1 file changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index 384449e3095d..a55375ac79a9 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -20,12 +20,16 @@
 static DEFINE_MUTEX(ps_devices_lock);
 static LIST_HEAD(ps_devices_list);
 
+static DEFINE_IDA(ps_player_id_allocator);
+
 /* Base class for playstation devices. */
 struct ps_device {
 	struct list_head list;
 	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;
@@ -288,6 +292,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;
@@ -837,8 +859,8 @@ static void dualsense_output_worker(struct work_struct *work)
 	}
 
 	if (ds->update_player_leds) {
-		r->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_PLAYER_INDICATOR_CONTROL_ENABLE;
-		r->player_leds = ds->player_leds_state;
+		common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_PLAYER_INDICATOR_CONTROL_ENABLE;
+		common->player_leds = ds->player_leds_state;
 
 		ds->update_player_leds = false;
 	}
@@ -1084,6 +1106,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;
@@ -1188,6 +1232,15 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 			goto err;
 	}
 
+	ret = ps_device_set_player_id((struct ps_device *)ds);
+	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 (struct ps_device *)ds;
 
 err:
@@ -1256,6 +1309,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);
-- 
2.26.2

[PATCH 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 | 81 +++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
index a55375ac79a9..2f989da906f3 100644
--- a/drivers/hid/hid-playstation.c
+++ b/drivers/hid/hid-playstation.c
@@ -36,6 +36,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);
 };
@@ -63,6 +65,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)
@@ -585,6 +589,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 snprintf(buf, PAGE_SIZE, "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 snprintf(buf, PAGE_SIZE, "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;
@@ -683,6 +721,29 @@ 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 = 0;
+
+	buf = kzalloc(DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_FIRMWARE_INFO, buf,
+			DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE, HID_FEATURE_REPORT,
+			HID_REQ_GET_REPORT);
+	if (ret < 0)
+		goto err_free;
+
+	ds->base.hw_version = get_unaligned_le32(&buf[24]);
+	ds->base.fw_version = get_unaligned_le32(&buf[28]);
+
+err_free:
+	kfree(buf);
+	return ret;
+}
+
 static int dualsense_get_mac_address(struct dualsense *ds)
 {
 	uint8_t *buf;
@@ -1172,6 +1233,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	}
 	snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds->base.mac_address);
 
+	ret = dualsense_get_firmware_info(ds);
+	if (ret < 0) {
+		hid_err(hdev, "Failed to get firmware info from DualSense\n");
+		return ERR_PTR(ret);
+	}
+
 	ret = ps_devices_list_add((struct ps_device *)ds);
 	if (ret < 0)
 		return ERR_PTR(ret);
@@ -1241,6 +1308,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
 	/* Set player LEDs to our player id. */
 	dualsense_set_player_leds(ds);
 
+	/* Reporting hardware and firmware is important as there are frequent updates, which
+	 * can change behavior.
+	 */
+	hid_info(hdev, "Registered DualSense controller hw_version=%x fw_version=%x\n",
+			ds->base.hw_version, ds->base.fw_version);
+
 	return (struct ps_device *)ds;
 
 err:
@@ -1295,6 +1368,12 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		goto err_close;
 	}
 
+	ret = sysfs_create_group(&hdev->dev.kobj, &ps_device_attribute_group);
+	if (ret < 0) {
+		hid_err(hdev, "Failed to register sysfs nodes.\n");
+		goto err_close;
+	}
+
 	return ret;
 
 err_close:
@@ -1313,6 +1392,8 @@ static void ps_remove(struct hid_device *hdev)
 
 	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
+
+	sysfs_remove_group(&hdev->dev.kobj, &ps_device_attribute_group);
 }
 
 static const struct hid_device_id ps_devices[] = {
-- 
2.26.2

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

[Bastien Nocera]

Hey Roderick,

On Fri, 2020-12-18 at 22:23 -0800, Roderick Colenbrander wrote:
> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> 
> Hi,
> 
> I am pleased to share a new Linux driver for the PlayStation 5
> 'DualSense'
> game controller. The driver supports the DualSense in both Bluetooth
> and USB modes. Most controller features are supported including LEDs,
> Touchpad, Motion Sensors and Rumble.

Excellent, this is a nice early holiday present :)
I've just received a DualSense controller so I'll try to test this in
the coming days.

> DualSense supported is implemented in a new 'hid-playstation' driver,

"hid-sony-playstation"? Not sure how much the input/hid tree
maintainers will want to force this, but the same problem is going to
show up for the hid-based XBox controller driver (hid-xbox, or hid-
microsoft-xbox?) as hid-microsoft is as busy as hid-sony.

> which
> will be used for peripherals by 'Sony Interactive Entertainment'
> (PlayStation).
> Hid-sony will be used for devices for the larger Sony Group. We
> intend to
> migrate existing devices over time gradually to hid-playstation. We
> do not
> want to cause any regressions and maintain quality. As such moving
> forward,
> unit tests are important and we started by providing these through
> 'hid-tools'
> including DualSense.

I know it's not your job to handle those, but be careful with not
breaking the clone controllers. Plenty of folks use them, for better or
for worse.

> 
> The Linux driver exposes DualSense functionality as a 'compositive
> device'
> similar to DualShock 4 in hid-sony, spanning multiple frameworks.
> First,
> it exposes 3 evdev nodes for respectively the 'gamepad', 'touchpad'
> and
> 'motion sensors'. The FF framework is used to provide basic rumble
> features.
> The leds-class is used to implement the Player indicator LEDs below
> the
> DualSense's touchpad, while the new 'leds-class-multicolor' is used
> for
> the lightbars next to the touchpad.
> 
> Not yet supported are new unique features introduced by the DualSense
> such as Adaptive Triggers and the VCM based Haptics. These features
> require
> a large amount of data and complex data structures. It is not clear
> how to
> expose these. The current Evdev and FF frameworks are too limiting.
> We hope
> to have a dialog on how to expose these over time in a generic way.
> 
> Enjoy the new DualSense driver and let us know if you have any
> questions
> or feedback.

Is the audio jack on the controller already supported, or would that be
part of the features that aren't supported yet?

Do you think you might be able to share the information regarding how
the cable pairing is done, so we can add this support to bluez? I'm
fine with only sharing the implementation if you prefer to give me the
details privately (or through my employer).

Cheers

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

[Roderick.Colenbrander]

Hey Bastian,

> From: Bastien Nocera <hadess@hadess.net>
> Sent: Saturday, December 19, 2020 12:38 AM
> To: roderick@gaikai.com <roderick@gaikai.com>; Jiri Kosina <jikos@kernel.org>; Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: linux-input@vger.kernel.org <linux-input@vger.kernel.org>; Chris Ye <lzye@google.com>; Colenbrander, Roderick <Roderick.Colenbrander@sony.com>
> Subject: Re: [PATCH 00/13] HID: new driver for PS5 'DualSense' controller 
>  
> Hey Roderick,
> 
> On Fri, 2020-12-18 at 22:23 -0800, Roderick Colenbrander wrote:
> > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > 
> > Hi,
> > 
> > I am pleased to share a new Linux driver for the PlayStation 5
> > 'DualSense'
> > game controller. The driver supports the DualSense in both Bluetooth
> > and USB modes. Most controller features are supported including LEDs,
> > Touchpad, Motion Sensors and Rumble.

> Excellent, this is a nice early holiday present :)
> I've just received a DualSense controller so I'll try to test this in
> the coming days.

> > DualSense supported is implemented in a new 'hid-playstation' driver,

> "hid-sony-playstation"? Not sure how much the input/hid tree
> maintainers will want to force this, but the same problem is going to
> show up for the hid-based XBox controller driver (hid-xbox, or hid-
> microsoft-xbox?) as hid-microsoft is as busy as hid-sony.

I had some offline conversation prior with Jiri and Benjamin. I wanted to avoid keeping PlayStation devices in hid-sony as Sony is a large group of companies and as 'PlayStation' we don't have any authority over those devices. When discussing we felt that hid-playstation was the best name. Technically the PlayStation company is "Sony Interactive Entertainment", but a a "hid-sie" wouldn't see any name recognition.

Other crowded drivers at some point could use a split as well as having too many devices in there can be a problem. Not just for maintenance, but also for device integration (e.g. phones, TVs,..), though not a main worry for the open kernel. Just for context in such embedded devices you may only want to pull in the 'game controller' driver and not the full weight of a hid-microsoft or a hid-sony. In case of hid-sony for example there is support for TV remotes as well, which cause collisions with whatever proprietary user/kernel mode TV remote support the device already used.

> > which
> > will be used for peripherals by 'Sony Interactive Entertainment'
> > (PlayStation).
> > Hid-sony will be used for devices for the larger Sony Group. We
> > intend to
> > migrate existing devices over time gradually to hid-playstation. We
> > do not
> > want to cause any regressions and maintain quality. As such moving
> > forward,
> > unit tests are important and we started by providing these through
> > 'hid-tools'
> including DualSense.

> I know it's not your job to handle those, but be careful with not
> breaking the clone controllers. Plenty of folks use them, for better or
> for worse.

Support for DualShock 4 is easy to migrate, but I'm really, really nervous about the DualShock 3 generation with many buggy clones. Some of it is due to broken HID reports (even though the practical reports must use the same byte layout as the DualShock 3). Not using the HID parser and parsing raw reports directly may mitigate some of that (the HID report is fully with vendor specific stuff anyway). But yeah it is a pain and I don't know if I want to dare trying DualShock 3... but will see where that goes. Normally it is tricky for us to support clone devices, but if this migration happened we would have to pay attention to it and perhaps get these devices on Ebay.

> > 
> > The Linux driver exposes DualSense functionality as a 'compositive
> > device'
> > similar to DualShock 4 in hid-sony, spanning multiple frameworks.
> > First,
> > it exposes 3 evdev nodes for respectively the 'gamepad', 'touchpad'
> > and
> > 'motion sensors'. The FF framework is used to provide basic rumble
> > features.
> > The leds-class is used to implement the Player indicator LEDs below
> > the
> > DualSense's touchpad, while the new 'leds-class-multicolor' is used
> > for
> > the lightbars next to the touchpad.
> > 
> > Not yet supported are new unique features introduced by the DualSense
> > such as Adaptive Triggers and the VCM based Haptics. These features
> > require
> > a large amount of data and complex data structures. It is not clear
> > how to
> > expose these. The current Evdev and FF frameworks are too limiting.
> > We hope
> > to have a dialog on how to expose these over time in a generic way.
> > 
> > Enjoy the new DualSense driver and let us know if you have any
> > questions
> > or feedback.

> Is the audio jack on the controller already supported, or would that be
> part of the features that aren't supported yet?

The answer is yes and no. In case of USB, there is some level of audio support, though I am not yet sure if I like it.

Basically the VCM haptics, microphone and speaker appears as a USB audio device. It means they kind of work. Though some of the audio control features are HID managed, so they are not there (I haven't seen what audio controls are native exposed through USB). I don't know if the headphone jack currently works or if that needs some MUXing logic through HID.

Bluetooth is not supported at all. It is some custom protocol over HID of which I don't know the details.

Provided we can contribute such support in the future, it is beyond complicated. Just in the 'simple' USB audio case, some interop might be needed with the USB audio drivers for e.g. audio controls. Also the new VCM haptics seem to be reported as audio channels on this device as well. Should haptics really be audio? Or some new interface (a hypothetical EV_FF2)?

> Do you think you might be able to share the information regarding how
> the cable pairing is done, so we can add this support to bluez? I'm
> fine with only sharing the implementation if you prefer to give me the
> details privately (or through my employer).

> Cheers

Thanks,
Roderick

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

[Samuel Čavoj]

Hello,

thank you for this driver. It makes me really happy to see an official
one. Just a small thing I noticed while reading through it:


On 18.12.2020 22:23, Roderick Colenbrander wrote:
> @@ -311,6 +345,25 @@ 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);
>  
> +	input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);

The above line is duplicated right before the input_sync (marked). Is
there any reason for this or is it accidental?

> +	for (i = 0; i < 2; i++) {
> +		bool active = (ds_report->points[i].contact & 0x80) ? 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);

Right here.

> +	input_sync(ds->touchpad);
> +

Regards,
Samuel

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

[Samuel Čavoj]

Hi,

I noticed that the `value` argument is not at all used in the
player_led_set_brightness function.

On 18.12.2020 22:23, Roderick Colenbrander wrote:
> +
> +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);
> +	uint8_t player_leds_state = 0;
> +	unsigned long flags;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++)
> +		player_leds_state |= (ds->player_leds[i].brightness << i);

Is it guaranteed at this point, that the led->brightness is set to the
new value? I'm unfamiliar with the led subsystem, but skimming other
drivers I found that they update the device based on the value of the
`value` argument.

> +
> +	spin_lock_irqsave(&ds->base.lock, flags);
> +	ds->player_leds_state = player_leds_state;
> +	ds->update_player_leds = true;
> +	spin_unlock_irqrestore(&ds->base.lock, flags);
> +
> +	schedule_work(&ds->output_worker);
> +}

Reading led-core.c, I found that led_set_brightness_{nosleep,sync} do
set the brightness attribute, but the _nopm one does not and it is
exported, although it is not used anywhere other than led-core.c and
led-class.c.

However, I find the usage in led_classdev_suspend and _resume
interesting. In suspend, set_brightness_nopm is called with a value of
0, which should turn off the LED while retaining the value of the
brightness attribute, which is later recalled in _resume. I assume the
intended behaviour is the LED to turn off when suspending and return to
its original state on resume, without overwriting the attribute.

Assuming that, the "value" argument passed to dualsense_player_led_set_brightness
can be different from led->brightness *on purpose* and should be used
instead.

I would write something along these lines:

for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++) {
	if (&ds->player_leds[i] == led) {
		if (value == LED_OFF)
			player_leds_state &= ~(1 << i);
		else
			player_leds_state |= (1 << i);
		break;
	}
}

This is just me hypothesizing though, could anyone clear this up for
me? Thank you very much.

Regards,
Samuel

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

[Roderick Colenbrander]

Hi Samuel,

Thanks for your review.


On Fri, Dec 25, 2020 at 6:14 PM Samuel Čavoj <samuel@cavoj.net> wrote:
>
> Hello,
>
> thank you for this driver. It makes me really happy to see an official
> one. Just a small thing I noticed while reading through it:
>
>
> On 18.12.2020 22:23, Roderick Colenbrander wrote:
> > @@ -311,6 +345,25 @@ 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);
> >
> > +     input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
>
> The above line is duplicated right before the input_sync (marked). Is
> there any reason for this or is it accidental?

Good catch. It was purely accidental as the code went through many
rebases and code shuffling. So a little artifact..

>
> > +     for (i = 0; i < 2; i++) {
> > +             bool active = (ds_report->points[i].contact & 0x80) ? 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);
>
> Right here.

I will probably keep this one and take the other one out as this is
next to the other touchpad code.

>
> > +     input_sync(ds->touchpad);
> > +
>
> Regards,
> Samuel

Thanks,
Roderick

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

[Roderick Colenbrander]

Hi Samuel,

Thanks for your review!

On Sat, Dec 26, 2020 at 11:27 AM Samuel Čavoj <samuel@cavoj.net> wrote:
>
> Hi,
>
> I noticed that the `value` argument is not at all used in the
> player_led_set_brightness function.
>
> On 18.12.2020 22:23, Roderick Colenbrander wrote:
> > +
> > +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);
> > +     uint8_t player_leds_state = 0;
> > +     unsigned long flags;
> > +     int i;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++)
> > +             player_leds_state |= (ds->player_leds[i].brightness << i);
>
> Is it guaranteed at this point, that the led->brightness is set to the
> new value? I'm unfamiliar with the led subsystem, but skimming other
> drivers I found that they update the device based on the value of the
> `value` argument.

See comments below. Normally yes the brightness is already updated,
but as mentioned below it might still be best to change the code.

>
> > +
> > +     spin_lock_irqsave(&ds->base.lock, flags);
> > +     ds->player_leds_state = player_leds_state;
> > +     ds->update_player_leds = true;
> > +     spin_unlock_irqrestore(&ds->base.lock, flags);
> > +
> > +     schedule_work(&ds->output_worker);
> > +}
>
> Reading led-core.c, I found that led_set_brightness_{nosleep,sync} do
> set the brightness attribute, but the _nopm one does not and it is
> exported, although it is not used anywhere other than led-core.c and
> led-class.c.
>
> However, I find the usage in led_classdev_suspend and _resume
> interesting. In suspend, set_brightness_nopm is called with a value of
> 0, which should turn off the LED while retaining the value of the
> brightness attribute, which is later recalled in _resume. I assume the
> intended behaviour is the LED to turn off when suspending and return to
> its original state on resume, without overwriting the attribute.
>
> Assuming that, the "value" argument passed to dualsense_player_led_set_brightness
> can be different from led->brightness *on purpose* and should be used
> instead.
>
> I would write something along these lines:
>
> for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++) {
>         if (&ds->player_leds[i] == led) {
>                 if (value == LED_OFF)
>                         player_leds_state &= ~(1 << i);
>                 else
>                         player_leds_state |= (1 << i);
>                 break;
>         }
> }
>
> This is just me hypothesizing though, could anyone clear this up for
> me? Thank you very much.

Thanks for your deep analysis. While writing the code I already found
it strange there were 2 layers of bookkeeping. As you point out this
is likely due to the power management logic.

The LEDs are created by 'ps_led_register' and it is setting:
    led->flags = LED_CORE_SUSPENDRESUME;

So those code paths would indeed be triggered provided
'CONFIG_PM_SLEEP' is set. So it is probably safest for me to change
that for-loop anyway. I had hoped to skip the 'find the LED logic' (it
just feels ugly somehow, but not a big deal). Let me make this quick
change. I will just wait a few more days on potential other feedback
from the list before submitting a 'v2'. So far nothing major, but just
these few small things :)


>
> Regards,
> Samuel

Thanks,
Roderick


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

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

[Samuel Čavoj]

Hi Roderick,

this one is very much just a nitpick, yet not completely pointless.

On 18.12.2020 22:23, Roderick Colenbrander wrote:
> @@ -837,8 +859,8 @@ static void dualsense_output_worker(struct work_struct *work)
>  	}
>  
>  	if (ds->update_player_leds) {
> -		r->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_PLAYER_INDICATOR_CONTROL_ENABLE;
> -		r->player_leds = ds->player_leds_state;
> +		common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_PLAYER_INDICATOR_CONTROL_ENABLE;
> +		common->player_leds = ds->player_leds_state;

This change should be merged into the previous patch, as it doesn't
compile without it (typo, I suppose). Could lead to annoyment during
git bisect and all that.

Regards,
Samuel

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

[Barnabás Pőcze]

Hi


2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:

> [...]
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index ad8eedd3d2bf..384449e3095d 100644
> [...]
> +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);
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++) {
> +		if (&ds->player_leds[i] == led)
> +			return !!(ds->player_leds_state & BIT(i));
> +	}

Is there any reason why

```
  !!(ds->player_leds_state & BIT( led - ds->player_leds ))
```

or something similar is not used? Or am I missing something that prevents this
from working?

Furthermore, I don't quite see the purpose of this function. The LED core
can handle if no brightness_get() callback is provided. And since this
function returns just a cached value, I fail to see how it is different from
the default behaviour of the LED core, which is returning the last brightness
value. Am I missing something?


> +
> +	return LED_OFF;
> +}
> +
> +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);
> +	uint8_t player_leds_state = 0;
> +	unsigned long flags;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++)
> +		player_leds_state |= (ds->player_leds[i].brightness << i);
> +

I believe this could be simplified as well using the fact that
`led - ds->player_leds` gives the index of the LED.


> +	spin_lock_irqsave(&ds->base.lock, flags);
> +	ds->player_leds_state = player_leds_state;
> +	ds->update_player_leds = true;
> +	spin_unlock_irqrestore(&ds->base.lock, flags);
> +
> +	schedule_work(&ds->output_worker);
> +}
> [...]


Regards,
Barnabás Pőcze

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

[Barnabás Pőcze]

Hi


2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:

> [...]
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index 0b62bcb28d8a..f8cf82a27d43 100644
> [...]
> +/* Create a DualSense/DualShock4 RGB lightbar represented by a multicolor LED. */
> +static struct led_classdev_mc *ps_lightbar_create(struct ps_device *ps_dev,
> +	int (*brightness_set)(struct led_classdev *, enum led_brightness))
> +{
> +	struct hid_device *hdev = ps_dev->hdev;
> +	struct led_classdev_mc *lightbar_mc_dev;
> +	struct mc_subled *mc_led_info;
> +	struct led_classdev *led_cdev;
> +	int ret;
> +
> +	lightbar_mc_dev = devm_kzalloc(&hdev->dev, sizeof(*lightbar_mc_dev), GFP_KERNEL);
> +	if (!lightbar_mc_dev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mc_led_info = devm_kzalloc(&hdev->dev, 3*sizeof(*mc_led_info), GFP_KERNEL);
> +	if (!mc_led_info)
> +		return ERR_PTR(-ENOMEM);
> +

Is there any reason why these are dynamically allocated?


> +	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);

I guess the double colons are used because the MAC address has ':' in it; but
as far as I know this doesn't follow the naming scheme for LED devices, so I'm
not sure if this is the best way to go about it.


> +	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 ERR_PTR(ret);
> +	}
> +
> +	return lightbar_mc_dev;
> +}
> [...]
> +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 = 2; /* Fade light out. */

Maybe it'd be better to name that '2'?


> +	dualsense_send_output_report(ds, &report);
> +
> +	kfree(buf);
> +	return 0;
> +}
> [...]


Regards,
Barnabás Pőcze

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

[Barnabás Pőcze]

Hi


2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:

> [...]
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> new file mode 100644
> index 000000000000..8dbd0ae7e082
> --- /dev/null
> +++ b/drivers/hid/hid-playstation.c
> @@ -0,0 +1,317 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *  HID driver for Sony DualSense(TM) controller.
> + *
> + *  Copyright (c) 2020 Sony Interactive Entertainment
> + */
> +
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/input/mt.h>
> +#include <linux/module.h>
> +#include <linux/crc32.h>
> +#include <asm/unaligned.h>
> +

I believe it would be preferable to keep the list of includes lexicographically
sorted.


> +#include "hid-ids.h"
> +
> +/* 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)

I believe it would be preferable to explicitly include everything you need
and not to count on other headers indirectly including it for you. In this
case `linux/bits.h` is not included.


> [...]
> +/* 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 int ps_gamepad_buttons[] = {

Any reason why it's not `const`?


> +	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 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;
> +

As per [1], please use braces around the body of the `else`.


> +	input_set_drvdata(input_dev, hdev);
> +
> +	return input_dev;
> +}
> [...]
> +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 = (struct dualsense *)ps_dev;

I believe `container_of(ps_dev, struct dualsense, base)` would be preferable here
(and everywhere this pattern emerges).


> +	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);

Possibly this could be replaced with a loop? I have something like this in mind:

```
struct ps_gamepad_button {
  unsigned int code;
  uint8_t button_idx;
  uint8_t mask;
} ps_gamepad_buttons[] = {...};

for (...) {
  struct ps_gamepad_button *b = ...;
  input_report_key(...);
}
```

Or is there any reason why the unrolled version is preffered that I'm missing?


> +	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 |= 0x8000;

Maybe that '0x8000' could be given a name?


> +
> +	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 (struct ps_device *)ds;

I believe using `&ds->base` instead of `(struct ps_device *)ds` would be somewhat
better as it does not subvert the type system as much.


> +
> +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;
> +		}
> +	} else {

When would this be possible?


> +		hid_err(hdev, "Unhandled device\n");
> +		ret = -EINVAL;

Assuming it's possible, I believe `-ENODEV` is a better error code here.


> +		goto err_close;
> +	}
> +
> +	return ret;
> +
> +err_close:
> +	hid_hw_close(hdev);
> +err_stop:
> +	hid_hw_stop(hdev);
> +	return ret;
> +}
> [...]
> +static const struct hid_device_id ps_devices[] = {
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER),
> +		.driver_data = 0 },
> [...]

`.driver_data` would be initialized to zero anyways, why is it necessary to do
so explicitly?


[1]: https://www.kernel.org/doc/html/latest/process/coding-style.html


Regards,
Barnabás Pőcze

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

[Barnabás Pőcze]

Hi


2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:

> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index a55375ac79a9..2f989da906f3 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> [...]
> +static ssize_t ps_show_firmware_version(struct device *dev,
> +				struct device_attribute
> +				*attr, char *buf)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct ps_device *ps_dev = hid_get_drvdata(hdev);
> +
> +	return snprintf(buf, PAGE_SIZE, "0x%08x\n", ps_dev->fw_version);

`sysfs_emit()` is preferred over *printf().


> +}
> +
> +static DEVICE_ATTR(firmware_version, 0444, ps_show_firmware_version, NULL);
> +
> +static ssize_t ps_show_hardware_version(struct device *dev,
> +				struct device_attribute
> +				*attr, char *buf)
> +{
> +	struct hid_device *hdev = to_hid_device(dev);
> +	struct ps_device *ps_dev = hid_get_drvdata(hdev);
> +
> +	return snprintf(buf, PAGE_SIZE, "0x%08x\n", ps_dev->hw_version);

Same here.


> +}
> [...]
> +static int dualsense_get_firmware_info(struct dualsense *ds)
> +{
> +	uint8_t *buf;
> +	int ret = 0;

Is there any reason it needs to be initialized?


> +
> +	buf = kzalloc(DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_FIRMWARE_INFO, buf,
> +			DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE, HID_FEATURE_REPORT,
> +			HID_REQ_GET_REPORT);
> +	if (ret < 0)
> +		goto err_free;
> +
> +	ds->base.hw_version = get_unaligned_le32(&buf[24]);
> +	ds->base.fw_version = get_unaligned_le32(&buf[28]);

Shouldn't the size of the reply be checked?


> +
> +err_free:
> +	kfree(buf);
> +	return ret;
> +}
> +
>  static int dualsense_get_mac_address(struct dualsense *ds)
>  {
>  	uint8_t *buf;
> @@ -1172,6 +1233,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>  	}
>  	snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds->base.mac_address);
>
> +	ret = dualsense_get_firmware_info(ds);
> +	if (ret < 0) {
> +		hid_err(hdev, "Failed to get firmware info from DualSense\n");
> +		return ERR_PTR(ret);
> +	}
> +
>  	ret = ps_devices_list_add((struct ps_device *)ds);
>  	if (ret < 0)
>  		return ERR_PTR(ret);
> @@ -1241,6 +1308,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>  	/* Set player LEDs to our player id. */
>  	dualsense_set_player_leds(ds);
>
> +	/* Reporting hardware and firmware is important as there are frequent updates, which
> +	 * can change behavior.
> +	 */
> +	hid_info(hdev, "Registered DualSense controller hw_version=%x fw_version=%x\n",
> +			ds->base.hw_version, ds->base.fw_version);
> +
>  	return (struct ps_device *)ds;
>
>  err:
> @@ -1295,6 +1368,12 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		goto err_close;
>  	}
>
> +	ret = sysfs_create_group(&hdev->dev.kobj, &ps_device_attribute_group);

It's a minor thing, but I think `device_{add,remove}_group()` would be better
here in the sense that it expresses the fact that the group is added to a device,
not just any object better.


> +	if (ret < 0) {
> +		hid_err(hdev, "Failed to register sysfs nodes.\n");
> +		goto err_close;
> +	}
> +
>  	return ret;
>
>  err_close:
> @@ -1313,6 +1392,8 @@ static void ps_remove(struct hid_device *hdev)
>
>  	hid_hw_close(hdev);
>  	hid_hw_stop(hdev);
> +
> +	sysfs_remove_group(&hdev->dev.kobj, &ps_device_attribute_group);
>  }
>
>  static const struct hid_device_id ps_devices[] = {
> --
> 2.26.2


Regards,
Barnabás Pőcze

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

[Roderick Colenbrander]

Hi Barnabás,

Thanks for you great feedback.

On Sun, Dec 27, 2020 at 9:06 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Hi
>
>
> 2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:
>
> > diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> > index a55375ac79a9..2f989da906f3 100644
> > --- a/drivers/hid/hid-playstation.c
> > +++ b/drivers/hid/hid-playstation.c
> > [...]
> > +static ssize_t ps_show_firmware_version(struct device *dev,
> > +                             struct device_attribute
> > +                             *attr, char *buf)
> > +{
> > +     struct hid_device *hdev = to_hid_device(dev);
> > +     struct ps_device *ps_dev = hid_get_drvdata(hdev);
> > +
> > +     return snprintf(buf, PAGE_SIZE, "0x%08x\n", ps_dev->fw_version);
>
> `sysfs_emit()` is preferred over *printf().

Thanks, I wasn't aware of its existence. Looks like it was added
recently, but yeah it is a lot nicer.

>
> > +}
> > +
> > +static DEVICE_ATTR(firmware_version, 0444, ps_show_firmware_version, NULL);
> > +
> > +static ssize_t ps_show_hardware_version(struct device *dev,
> > +                             struct device_attribute
> > +                             *attr, char *buf)
> > +{
> > +     struct hid_device *hdev = to_hid_device(dev);
> > +     struct ps_device *ps_dev = hid_get_drvdata(hdev);
> > +
> > +     return snprintf(buf, PAGE_SIZE, "0x%08x\n", ps_dev->hw_version);
>
> Same here.
>
>
> > +}
> > [...]
> > +static int dualsense_get_firmware_info(struct dualsense *ds)
> > +{
> > +     uint8_t *buf;
> > +     int ret = 0;
>
> Is there any reason it needs to be initialized?
>
>
> > +
> > +     buf = kzalloc(DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE, GFP_KERNEL);
> > +     if (!buf)
> > +             return -ENOMEM;
> > +
> > +     ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_FIRMWARE_INFO, buf,
> > +                     DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE, HID_FEATURE_REPORT,
> > +                     HID_REQ_GET_REPORT);
> > +     if (ret < 0)
> > +             goto err_free;
> > +
> > +     ds->base.hw_version = get_unaligned_le32(&buf[24]);
> > +     ds->base.fw_version = get_unaligned_le32(&buf[28]);
>
> Shouldn't the size of the reply be checked?

Good point, I added a check and returning -EINVAL now in case of a
size mismatch.

>
> > +
> > +err_free:
> > +     kfree(buf);
> > +     return ret;
> > +}
> > +
> >  static int dualsense_get_mac_address(struct dualsense *ds)
> >  {
> >       uint8_t *buf;
> > @@ -1172,6 +1233,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
> >       }
> >       snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds->base.mac_address);
> >
> > +     ret = dualsense_get_firmware_info(ds);
> > +     if (ret < 0) {
> > +             hid_err(hdev, "Failed to get firmware info from DualSense\n");
> > +             return ERR_PTR(ret);
> > +     }
> > +
> >       ret = ps_devices_list_add((struct ps_device *)ds);
> >       if (ret < 0)
> >               return ERR_PTR(ret);
> > @@ -1241,6 +1308,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
> >       /* Set player LEDs to our player id. */
> >       dualsense_set_player_leds(ds);
> >
> > +     /* Reporting hardware and firmware is important as there are frequent updates, which
> > +      * can change behavior.
> > +      */
> > +     hid_info(hdev, "Registered DualSense controller hw_version=%x fw_version=%x\n",
> > +                     ds->base.hw_version, ds->base.fw_version);
> > +
> >       return (struct ps_device *)ds;
> >
> >  err:
> > @@ -1295,6 +1368,12 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >               goto err_close;
> >       }
> >
> > +     ret = sysfs_create_group(&hdev->dev.kobj, &ps_device_attribute_group);
>
> It's a minor thing, but I think `device_{add,remove}_group()` would be better
> here in the sense that it expresses the fact that the group is added to a device,
> not just any object better.

Agreed, that's nicer I wasn't aware of it. I try to follow what other
hid drivers do and they all used the kobj directly, which honestly
felt nasty. Will change it to this.

>
> > +     if (ret < 0) {
> > +             hid_err(hdev, "Failed to register sysfs nodes.\n");
> > +             goto err_close;
> > +     }
> > +
> >       return ret;
> >
> >  err_close:
> > @@ -1313,6 +1392,8 @@ static void ps_remove(struct hid_device *hdev)
> >
> >       hid_hw_close(hdev);
> >       hid_hw_stop(hdev);
> > +
> > +     sysfs_remove_group(&hdev->dev.kobj, &ps_device_attribute_group);
> >  }
> >
> >  static const struct hid_device_id ps_devices[] = {
> > --
> > 2.26.2
>
>
> Regards,
> Barnabás Pőcze

Thanks,
Roderick

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

[Roderick Colenbrander]

On Sun, Dec 27, 2020 at 2:21 PM Roderick Colenbrander
<roderick@gaikai.com> wrote:
>
> Hi Barnabás,
>
> Thanks for you great feedback.
>
> On Sun, Dec 27, 2020 at 9:06 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> >
> > Hi
> >
> >
> > 2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:
> >
> > > diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> > > index a55375ac79a9..2f989da906f3 100644
> > > --- a/drivers/hid/hid-playstation.c
> > > +++ b/drivers/hid/hid-playstation.c
> > > [...]
> > > +static ssize_t ps_show_firmware_version(struct device *dev,
> > > +                             struct device_attribute
> > > +                             *attr, char *buf)
> > > +{
> > > +     struct hid_device *hdev = to_hid_device(dev);
> > > +     struct ps_device *ps_dev = hid_get_drvdata(hdev);
> > > +
> > > +     return snprintf(buf, PAGE_SIZE, "0x%08x\n", ps_dev->fw_version);
> >
> > `sysfs_emit()` is preferred over *printf().
>
> Thanks, I wasn't aware of its existence. Looks like it was added
> recently, but yeah it is a lot nicer.
>
> >
> > > +}
> > > +
> > > +static DEVICE_ATTR(firmware_version, 0444, ps_show_firmware_version, NULL);
> > > +
> > > +static ssize_t ps_show_hardware_version(struct device *dev,
> > > +                             struct device_attribute
> > > +                             *attr, char *buf)
> > > +{
> > > +     struct hid_device *hdev = to_hid_device(dev);
> > > +     struct ps_device *ps_dev = hid_get_drvdata(hdev);
> > > +
> > > +     return snprintf(buf, PAGE_SIZE, "0x%08x\n", ps_dev->hw_version);
> >
> > Same here.
> >
> >
> > > +}
> > > [...]
> > > +static int dualsense_get_firmware_info(struct dualsense *ds)
> > > +{
> > > +     uint8_t *buf;
> > > +     int ret = 0;
> >
> > Is there any reason it needs to be initialized?
> >
> >
> > > +
> > > +     buf = kzalloc(DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE, GFP_KERNEL);
> > > +     if (!buf)
> > > +             return -ENOMEM;
> > > +
> > > +     ret = hid_hw_raw_request(ds->base.hdev, DS_FEATURE_REPORT_FIRMWARE_INFO, buf,
> > > +                     DS_FEATURE_REPORT_FIRMWARE_INFO_SIZE, HID_FEATURE_REPORT,
> > > +                     HID_REQ_GET_REPORT);
> > > +     if (ret < 0)
> > > +             goto err_free;
> > > +
> > > +     ds->base.hw_version = get_unaligned_le32(&buf[24]);
> > > +     ds->base.fw_version = get_unaligned_le32(&buf[28]);
> >
> > Shouldn't the size of the reply be checked?
>
> Good point, I added a check and returning -EINVAL now in case of a
> size mismatch.
>
> >
> > > +
> > > +err_free:
> > > +     kfree(buf);
> > > +     return ret;
> > > +}
> > > +
> > >  static int dualsense_get_mac_address(struct dualsense *ds)
> > >  {
> > >       uint8_t *buf;
> > > @@ -1172,6 +1233,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
> > >       }
> > >       snprintf(hdev->uniq, sizeof(hdev->uniq), "%pMR", ds->base.mac_address);
> > >
> > > +     ret = dualsense_get_firmware_info(ds);
> > > +     if (ret < 0) {
> > > +             hid_err(hdev, "Failed to get firmware info from DualSense\n");
> > > +             return ERR_PTR(ret);
> > > +     }
> > > +
> > >       ret = ps_devices_list_add((struct ps_device *)ds);
> > >       if (ret < 0)
> > >               return ERR_PTR(ret);
> > > @@ -1241,6 +1308,12 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
> > >       /* Set player LEDs to our player id. */
> > >       dualsense_set_player_leds(ds);
> > >
> > > +     /* Reporting hardware and firmware is important as there are frequent updates, which
> > > +      * can change behavior.
> > > +      */
> > > +     hid_info(hdev, "Registered DualSense controller hw_version=%x fw_version=%x\n",
> > > +                     ds->base.hw_version, ds->base.fw_version);
> > > +
> > >       return (struct ps_device *)ds;
> > >
> > >  err:
> > > @@ -1295,6 +1368,12 @@ static int ps_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > >               goto err_close;
> > >       }
> > >
> > > +     ret = sysfs_create_group(&hdev->dev.kobj, &ps_device_attribute_group);
> >
> > It's a minor thing, but I think `device_{add,remove}_group()` would be better
> > here in the sense that it expresses the fact that the group is added to a device,
> > not just any object better.
>
> Agreed, that's nicer I wasn't aware of it. I try to follow what other
> hid drivers do and they all used the kobj directly, which honestly
> felt nasty. Will change it to this.
>

Actually devm_device_add_group seems to be even nicer. Surprisingly it
isn't widely used yet.

Roderick

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

[Barnabás Pőcze]

2020. december 27., vasárnap 23:27 keltezéssel, Roderick Colenbrander írta:

> [...]
> > > > -       ret = sysfs_create_group(&hdev->dev.kobj, &ps_device_attribute_group);
> > > >
> > > >
> > >
> > > It's a minor thing, but I think `device_{add,remove}_group()` would be better
> > > here in the sense that it expresses the fact that the group is added to a device,
> > > not just any object better.
> >
> > Agreed, that's nicer I wasn't aware of it. I try to follow what other
> > hid drivers do and they all used the kobj directly, which honestly
> > felt nasty. Will change it to this.
>
> Actually devm_device_add_group seems to be even nicer. Surprisingly it
> isn't widely used yet.
>
> Roderick


Well, indeed, although I believe that shouldn't be used here. Consider
what happens if the hid-playstation module is unloaded. The attributes
of the HID device will not be unregistered, but the backing functions/etc.
are unloaded, so reading/writing them will have undesirable effects - I imagine.
So in either case, you'll need to use `[devm_]device_remove_group()`, and for
that reason I think using the devm_* variant is less efficient.
Please note, that I am not 100% sure this hypothesis is correct, but I'm pretty sure.


Regards,
Barnabás Pőcze

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

[Roderick Colenbrander]

Hi Barnabás,

Thanks for your review.

On Sun, Dec 27, 2020 at 8:24 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Hi
>
>
> 2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:
>
> > [...]
> > diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> > new file mode 100644
> > index 000000000000..8dbd0ae7e082
> > --- /dev/null
> > +++ b/drivers/hid/hid-playstation.c
> > @@ -0,0 +1,317 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + *  HID driver for Sony DualSense(TM) controller.
> > + *
> > + *  Copyright (c) 2020 Sony Interactive Entertainment
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/hid.h>
> > +#include <linux/input/mt.h>
> > +#include <linux/module.h>
> > +#include <linux/crc32.h>
> > +#include <asm/unaligned.h>
> > +
>
> I believe it would be preferable to keep the list of includes lexicographically
> sorted.
>
>
> > +#include "hid-ids.h"
> > +
> > +/* 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)
>
> I believe it would be preferable to explicitly include everything you need
> and not to count on other headers indirectly including it for you. In this
> case `linux/bits.h` is not included.
>
>
> > [...]
> > +/* 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 int ps_gamepad_buttons[] = {
>
> Any reason why it's not `const`?

It should be const.

>
>
> > +     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 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;
> > +
>
> As per [1], please use braces around the body of the `else`.
>
>
> > +     input_set_drvdata(input_dev, hdev);
> > +
> > +     return input_dev;
> > +}
> > [...]
> > +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 = (struct dualsense *)ps_dev;
>
> I believe `container_of(ps_dev, struct dualsense, base)` would be preferable here
> (and everywhere this pattern emerges).

Agreed.

>
> > +     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);
>
> Possibly this could be replaced with a loop? I have something like this in mind:
>
> ```
> struct ps_gamepad_button {
>   unsigned int code;
>   uint8_t button_idx;
>   uint8_t mask;
> } ps_gamepad_buttons[] = {...};
>
> for (...) {
>   struct ps_gamepad_button *b = ...;
>   input_report_key(...);
> }
> ```
>
> Or is there any reason why the unrolled version is preffered that I'm missing?

It can be done from a loop. Main reason for unrolled was that it is
actually less code and potentially a tiny bit faster, but I bet a
compiler would have unrolled it anyway. I don't know what I want to do
here. Being explicit feels nice (other drivers do something similar).

>
> > +     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 |= 0x8000;
>
> Maybe that '0x8000' could be given a name?

Will do so. Calling it for now 'HID_PLAYSTATION_VERSION_PATCH' or
something like that.

>
>
> > +
> > +     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 (struct ps_device *)ds;
>
> I believe using `&ds->base` instead of `(struct ps_device *)ds` would be somewhat
> better as it does not subvert the type system as much.

Thanks, yeah that's cleaner.

>
>
> > +
> > +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;
> > +             }
> > +     } else {
>
> When would this be possible?

It isn't possible right now. A colleague really wanted me to add (I
originally didn't have it in an internal build), but I don't mind
taking it out.

>
>
> > +             hid_err(hdev, "Unhandled device\n");
> > +             ret = -EINVAL;
>
> Assuming it's possible, I believe `-ENODEV` is a better error code here.
>
>
> > +             goto err_close;
> > +     }
> > +
> > +     return ret;
> > +
> > +err_close:
> > +     hid_hw_close(hdev);
> > +err_stop:
> > +     hid_hw_stop(hdev);
> > +     return ret;
> > +}
> > [...]
> > +static const struct hid_device_id ps_devices[] = {
> > +     { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER),
> > +             .driver_data = 0 },
> > [...]
>
> `.driver_data` would be initialized to zero anyways, why is it necessary to do
> so explicitly?

It is not needed.

>
>
> [1]: https://www.kernel.org/doc/html/latest/process/coding-style.html
>
>
> Regards,
> Barnabás Pőcze

Regards,
Roderick

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

[Roderick Colenbrander]

On Sat, Dec 26, 2020 at 4:08 PM Samuel Čavoj <sammko@sammserver.com> wrote:
>
> Hi Roderick,
>
> this one is very much just a nitpick, yet not completely pointless.
>
> On 18.12.2020 22:23, Roderick Colenbrander wrote:
> > @@ -837,8 +859,8 @@ static void dualsense_output_worker(struct work_struct *work)
> >       }
> >
> >       if (ds->update_player_leds) {
> > -             r->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_PLAYER_INDICATOR_CONTROL_ENABLE;
> > -             r->player_leds = ds->player_leds_state;
> > +             common->valid_flag1 |= DS_OUTPUT_VALID_FLAG1_PLAYER_INDICATOR_CONTROL_ENABLE;
> > +             common->player_leds = ds->player_leds_state;
>
> This change should be merged into the previous patch, as it doesn't
> compile without it (typo, I suppose). Could lead to annoyment during
> git bisect and all that.
>

Yep that doesn't belong here. Rebase artifact and I didn't compile
that particular patch after the fact apparently... Will run a 'rebase
-x' with a compile script on the next revision to make sure nothing
got messed up...

Thanks,
Roderick

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

[Roderick Colenbrander]

Hi Barnabás,

Thanks for your review.

On Sun, Dec 27, 2020 at 6:41 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Hi
>
>
> 2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:
>
> > [...]
> > diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> > index 0b62bcb28d8a..f8cf82a27d43 100644
> > [...]
> > +/* Create a DualSense/DualShock4 RGB lightbar represented by a multicolor LED. */
> > +static struct led_classdev_mc *ps_lightbar_create(struct ps_device *ps_dev,
> > +     int (*brightness_set)(struct led_classdev *, enum led_brightness))
> > +{
> > +     struct hid_device *hdev = ps_dev->hdev;
> > +     struct led_classdev_mc *lightbar_mc_dev;
> > +     struct mc_subled *mc_led_info;
> > +     struct led_classdev *led_cdev;
> > +     int ret;
> > +
> > +     lightbar_mc_dev = devm_kzalloc(&hdev->dev, sizeof(*lightbar_mc_dev), GFP_KERNEL);
> > +     if (!lightbar_mc_dev)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     mc_led_info = devm_kzalloc(&hdev->dev, 3*sizeof(*mc_led_info), GFP_KERNEL);
> > +     if (!mc_led_info)
> > +             return ERR_PTR(-ENOMEM);
> > +
>
> Is there any reason why these are dynamically allocated?

No particular reason. I should probably at least not dynamically
allocate 'mc_dev' and pass it in similar to regular LED registration
(previously I had my regular LEDs dynamically allocated). The
mc_led_info I will probably keep dynamic. It feels a bit nasty to have
the caller be aware of these internal details.

>
>
> > +     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);
>
> I guess the double colons are used because the MAC address has ':' in it; but
> as far as I know this doesn't follow the naming scheme for LED devices, so I'm
> not sure if this is the best way to go about it.

Actually it was Benjamin who suggested this type of naming. He wasn't
a fan of the previous hid-sony device naming (neither was I). This was
the main idea so far.

>
> > +     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 ERR_PTR(ret);
> > +     }
> > +
> > +     return lightbar_mc_dev;
> > +}
> > [...]
> > +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 = 2; /* Fade light out. */
>
> Maybe it'd be better to name that '2'?

Will document this one.

>
>
> > +     dualsense_send_output_report(ds, &report);
> > +
> > +     kfree(buf);
> > +     return 0;
> > +}
> > [...]
>
>
> Regards,
> Barnabás Pőcze

Thanks,
Roderick

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

[Roderick Colenbrander]

Hi Barnabás,

On Sun, Dec 27, 2020 at 6:27 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Hi
>
>
> 2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:
>
> > [...]
> > diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> > index ad8eedd3d2bf..384449e3095d 100644
> > [...]
> > +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);
> > +     int i;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++) {
> > +             if (&ds->player_leds[i] == led)
> > +                     return !!(ds->player_leds_state & BIT(i));
> > +     }
>
> Is there any reason why
>
> ```
>   !!(ds->player_leds_state & BIT( led - ds->player_leds ))
> ```
>
> or something similar is not used? Or am I missing something that prevents this
> from working?

I think this pointer math would work and need to give it a try. Hadn't
considered it

> Furthermore, I don't quite see the purpose of this function. The LED core
> can handle if no brightness_get() callback is provided. And since this
> function returns just a cached value, I fail to see how it is different from
> the default behaviour of the LED core, which is returning the last brightness
> value. Am I missing something?

Not all values may get set through sysfs. For example in the next
patch (12/13) the driver sets a default player LED mask value directly
and may set e.g. 0x1f or so. This could use the LED APIs, but the LED
framework doesn't have any group LED support (besides the new
multicolor class) and as such would get scheduled across multiple
output reports.

>
> > +
> > +     return LED_OFF;
> > +}
> > +
> > +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);
> > +     uint8_t player_leds_state = 0;
> > +     unsigned long flags;
> > +     int i;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(ds->player_leds); i++)
> > +             player_leds_state |= (ds->player_leds[i].brightness << i);
> > +
>
> I believe this could be simplified as well using the fact that
> `led - ds->player_leds` gives the index of the LED.

I will give this a try as well, thanks.

>
> > +     spin_lock_irqsave(&ds->base.lock, flags);
> > +     ds->player_leds_state = player_leds_state;
> > +     ds->update_player_leds = true;
> > +     spin_unlock_irqrestore(&ds->base.lock, flags);
> > +
> > +     schedule_work(&ds->output_worker);
> > +}
> > [...]
>
>
> Regards,
> Barnabás Pőcze

Thanks,
Roderick

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

[Roderick Colenbrander]

On Sun, Dec 27, 2020 at 2:38 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> 2020. december 27., vasárnap 23:27 keltezéssel, Roderick Colenbrander írta:
>
> > [...]
> > > > > -       ret = sysfs_create_group(&hdev->dev.kobj, &ps_device_attribute_group);
> > > > >
> > > > >
> > > >
> > > > It's a minor thing, but I think `device_{add,remove}_group()` would be better
> > > > here in the sense that it expresses the fact that the group is added to a device,
> > > > not just any object better.
> > >
> > > Agreed, that's nicer I wasn't aware of it. I try to follow what other
> > > hid drivers do and they all used the kobj directly, which honestly
> > > felt nasty. Will change it to this.
> >
> > Actually devm_device_add_group seems to be even nicer. Surprisingly it
> > isn't widely used yet.
> >
> > Roderick
>
>
> Well, indeed, although I believe that shouldn't be used here. Consider
> what happens if the hid-playstation module is unloaded. The attributes
> of the HID device will not be unregistered, but the backing functions/etc.
> are unloaded, so reading/writing them will have undesirable effects - I imagine.
> So in either case, you'll need to use `[devm_]device_remove_group()`, and for
> that reason I think using the devm_* variant is less efficient.
> Please note, that I am not 100% sure this hypothesis is correct, but I'm pretty sure.
>
>
> Regards,
> Barnabás Pőcze

I did some more digging into 'devm_device_add_group' as I was curious.
It is widely used for touchscreen drivers apparently and some other
devices and generally used from 'probe' as you would expect. None of
the drivers I found call devm_device_remove_group. Though, none of the
drivers use HID.

I tried using the call and it seems to work fine even after driver
unloads/reloads without a 'devm_device_remove_group' call. I don't
believe any sysfs entries are kept around (also based on watching the
contents of the sysfs directory for the device). If they were I'm sure
the kernel would have thrown some errors during a future
'devm_device_add_group' call as you know sysfs gets quite unhappy if
you added a duplicate node.

This makes me believe it is getting cleaned up, but I'm not sure how.
I suspect it happens when the HID driver is unregistered
(hid_unregister_driver) from the bus, which follows a bus rescan. When
the driver is removed, device_driver_detach is called, which triggers
a lot of cleanup logic in 'device_driver_release_internal'. I haven't
traced this call, but I think its call 'devres_release_all(dev)' is
what is doing the magic.

Any thoughts?

Regards,
Roderick

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

[Barnabás Pőcze]

2020. december 28., hétfő 23:45 keltezéssel, Roderick Colenbrander írta:

> On Sun, Dec 27, 2020 at 2:38 PM Barnabás Pőcze pobrn@protonmail.com wrote:
>
> > 2020.  december 27., vasárnap 23:27 keltezéssel, Roderick Colenbrander írta:
> >
> > > [...]
> > >
> > > > > > -         ret = sysfs_create_group(&hdev->dev.kobj, &ps_device_attribute_group);
> > > > > >
> > > > > >
> > > > >
> > > > > It's a minor thing, but I think `device_{add,remove}_group()` would be better
> > > > > here in the sense that it expresses the fact that the group is added to a device,
> > > > > not just any object better.
> > > >
> > > > Agreed, that's nicer I wasn't aware of it. I try to follow what other
> > > > hid drivers do and they all used the kobj directly, which honestly
> > > > felt nasty. Will change it to this.
> > >
> > > Actually devm_device_add_group seems to be even nicer. Surprisingly it
> > > isn't widely used yet.
> > > Roderick
> >
> > Well, indeed, although I believe that shouldn't be used here. Consider
> > what happens if the hid-playstation module is unloaded. The attributes
> > of the HID device will not be unregistered, but the backing functions/etc.
> > are unloaded, so reading/writing them will have undesirable effects - I imagine.
> > So in either case, you'll need to use `[devm_]device_remove_group()`, and for
> > that reason I think using the devm_* variant is less efficient.
> > Please note, that I am not 100% sure this hypothesis is correct, but I'm pretty sure.
> > Regards,
> > Barnabás Pőcze
>
> I did some more digging into 'devm_device_add_group' as I was curious.
> It is widely used for touchscreen drivers apparently and some other
> devices and generally used from 'probe' as you would expect. None of
> the drivers I found call devm_device_remove_group. Though, none of the
> drivers use HID.
>
> I tried using the call and it seems to work fine even after driver
> unloads/reloads without a 'devm_device_remove_group' call. I don't
> believe any sysfs entries are kept around (also based on watching the
> contents of the sysfs directory for the device). If they were I'm sure
> the kernel would have thrown some errors during a future
> 'devm_device_add_group' call as you know sysfs gets quite unhappy if
> you added a duplicate node.
>
> This makes me believe it is getting cleaned up, but I'm not sure how.
> I suspect it happens when the HID driver is unregistered
> (hid_unregister_driver) from the bus, which follows a bus rescan. When
> the driver is removed, device_driver_detach is called, which triggers
> a lot of cleanup logic in 'device_driver_release_internal'. I haven't
> traced this call, but I think its call 'devres_release_all(dev)' is
> what is doing the magic.
>
> Any thoughts?


I also did some tests and it seems you're right, I was under the impression
that "device managed" resources are tied to the lifetime of the device,
and are not released when the driver unbinds, but this assumptions seems
to be false and device managed resources are, in fact, released when a driver
detaches so sorry for making you take this detour into investigating it.


Regards,
Barnabás Pőcze

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

[Barnabás Pőcze]

2020. december 28., hétfő 23:02 keltezéssel, Roderick Colenbrander írta:

> [...]
> > Furthermore, I don't quite see the purpose of this function. The LED core
> > can handle if no brightness_get() callback is provided. And since this
> > function returns just a cached value, I fail to see how it is different from
> > the default behaviour of the LED core, which is returning the last brightness
> > value. Am I missing something?
>
> Not all values may get set through sysfs. For example in the next
> patch (12/13) the driver sets a default player LED mask value directly
> and may set e.g. 0x1f or so. This could use the LED APIs, but the LED
> framework doesn't have any group LED support (besides the new
> multicolor class) and as such would get scheduled across multiple
> output reports.
> [...]

You're right, I've missed that.

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

[Barnabás Pőcze]

2020. december 28., hétfő 22:26 keltezéssel, Roderick Colenbrander írta:

> [...]
> > > +/* Create a DualSense/DualShock4 RGB lightbar represented by a multicolor LED. */
> > > +static struct led_classdev_mc *ps_lightbar_create(struct ps_device *ps_dev,
> > > +     int (*brightness_set)(struct led_classdev *, enum led_brightness))
> > > +{
> > > +     struct hid_device *hdev = ps_dev->hdev;
> > > +     struct led_classdev_mc *lightbar_mc_dev;
> > > +     struct mc_subled *mc_led_info;
> > > +     struct led_classdev *led_cdev;
> > > +     int ret;
> > > +
> > > +     lightbar_mc_dev = devm_kzalloc(&hdev->dev, sizeof(*lightbar_mc_dev), GFP_KERNEL);
> > > +     if (!lightbar_mc_dev)
> > > +             return ERR_PTR(-ENOMEM);
> > > +
> > > +     mc_led_info = devm_kzalloc(&hdev->dev, 3*sizeof(*mc_led_info), GFP_KERNEL);
> > > +     if (!mc_led_info)
> > > +             return ERR_PTR(-ENOMEM);
> > > +
> >
> > Is there any reason why these are dynamically allocated?
>
> No particular reason. I should probably at least not dynamically
> allocate 'mc_dev' and pass it in similar to regular LED registration
> (previously I had my regular LEDs dynamically allocated). The
> mc_led_info I will probably keep dynamic. It feels a bit nasty to have
> the caller be aware of these internal details.
> [...]

Could you please elaborate what you mean by "It feels a bit nasty to have
the caller be aware of these internal details."? I don't think I fully understand
what you're referring to.

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

[Barnabás Pőcze]

2020. december 28., hétfő 0:04 keltezéssel, Roderick Colenbrander írta:

> [...]
> > > +     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);
> >
> > Possibly this could be replaced with a loop? I have something like this in mind:
> >
> > ```
> > struct ps_gamepad_button {
> >   unsigned int code;
> >   uint8_t button_idx;
> >   uint8_t mask;
> > } ps_gamepad_buttons[] = {...};
> >
> > for (...) {
> >   struct ps_gamepad_button *b = ...;
> >   input_report_key(...);
> > }
> > ```
> >
> > Or is there any reason why the unrolled version is preffered that I'm missing?
>
> It can be done from a loop. Main reason for unrolled was that it is
> actually less code and potentially a tiny bit faster, but I bet a
> compiler would have unrolled it anyway. I don't know what I want to do
> here. Being explicit feels nice (other drivers do something similar).
> [...]

I agree that the compiler would've probably unrolled it. I'd personally consider
the loop version to be shorter as I'd not consider the static array "code" -
it's really just data initialization. Anyways, may I suggest then to align the
parameters so that the given parameter of each call starts in the same column?
I think it helps readability a good deal.

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

[Barnabás Pőcze]

Hi


2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:

> [...]
> +static struct input_dev *ps_touchpad_create(struct hid_device *hdev, int width, int height,
> +		int num_contacts)

Very minor thing, but `input_mt_init_slots()` takes an `unsigned int`, so
wouldn't it be better if `num_contacts` was an `unsigned int`?


> +{
> +	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, 0, 0);
> +	input_set_abs_params(touchpad, ABS_MT_POSITION_Y, 0, height, 0, 0);

Shouldn't it be `width - 1` and `height - 1`? Or what am I missing?


> +
> +	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;
> @@ -271,6 +304,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
> @@ -311,6 +345,25 @@ 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);
>
> +	input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
> +	for (i = 0; i < 2; i++) {
> +		bool active = (ds_report->points[i].contact & 0x80) ? false : true;
> [...]

I believe it'd be preferable to give a name to that 0x80.


Regards,
Barnabás Pőcze

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

[Roderick Colenbrander]

On Tue, Dec 29, 2020 at 10:59 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> 2020. december 28., hétfő 22:26 keltezéssel, Roderick Colenbrander írta:
>
> > [...]
> > > > +/* Create a DualSense/DualShock4 RGB lightbar represented by a multicolor LED. */
> > > > +static struct led_classdev_mc *ps_lightbar_create(struct ps_device *ps_dev,
> > > > +     int (*brightness_set)(struct led_classdev *, enum led_brightness))
> > > > +{
> > > > +     struct hid_device *hdev = ps_dev->hdev;
> > > > +     struct led_classdev_mc *lightbar_mc_dev;
> > > > +     struct mc_subled *mc_led_info;
> > > > +     struct led_classdev *led_cdev;
> > > > +     int ret;
> > > > +
> > > > +     lightbar_mc_dev = devm_kzalloc(&hdev->dev, sizeof(*lightbar_mc_dev), GFP_KERNEL);
> > > > +     if (!lightbar_mc_dev)
> > > > +             return ERR_PTR(-ENOMEM);
> > > > +
> > > > +     mc_led_info = devm_kzalloc(&hdev->dev, 3*sizeof(*mc_led_info), GFP_KERNEL);
> > > > +     if (!mc_led_info)
> > > > +             return ERR_PTR(-ENOMEM);
> > > > +
> > >
> > > Is there any reason why these are dynamically allocated?
> >
> > No particular reason. I should probably at least not dynamically
> > allocate 'mc_dev' and pass it in similar to regular LED registration
> > (previously I had my regular LEDs dynamically allocated). The
> > mc_led_info I will probably keep dynamic. It feels a bit nasty to have
> > the caller be aware of these internal details.
> > [...]
>
> Could you please elaborate what you mean by "It feels a bit nasty to have
> the caller be aware of these internal details."? I don't think I fully understand
> what you're referring to.
>

Maybe I misunderstood your original comment. The question was why both
'lightbar_mc_dev' and 'mc_led_info' were dynamically allocated. I
interpreted it as getting rid of some of the dynamic allocation as
some wasn't needed, but not sure what you had in mind. The code now
looks like:

struct dualsense {
...
        /* RGB lightbar */
        struct led_classdev_mc lightbar; (not a pointer anymore)
}

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;
...

In here I kept 'mc_led_info' as dynamically allocated. I didn't think
it made sense to have the caller know about it. What was your original
idea?

Regards,
Roderick

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

[Barnabás Pőcze]

2020. december 29., kedd 20:54 keltezéssel, Roderick Colenbrander írta:

> On Tue, Dec 29, 2020 at 10:59 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> >
> > 2020. december 28., hétfő 22:26 keltezéssel, Roderick Colenbrander írta:
> >
> > > [...]
> > > > > +/* Create a DualSense/DualShock4 RGB lightbar represented by a multicolor LED. */
> > > > > +static struct led_classdev_mc *ps_lightbar_create(struct ps_device *ps_dev,
> > > > > +     int (*brightness_set)(struct led_classdev *, enum led_brightness))
> > > > > +{
> > > > > +     struct hid_device *hdev = ps_dev->hdev;
> > > > > +     struct led_classdev_mc *lightbar_mc_dev;
> > > > > +     struct mc_subled *mc_led_info;
> > > > > +     struct led_classdev *led_cdev;
> > > > > +     int ret;
> > > > > +
> > > > > +     lightbar_mc_dev = devm_kzalloc(&hdev->dev, sizeof(*lightbar_mc_dev), GFP_KERNEL);
> > > > > +     if (!lightbar_mc_dev)
> > > > > +             return ERR_PTR(-ENOMEM);
> > > > > +
> > > > > +     mc_led_info = devm_kzalloc(&hdev->dev, 3*sizeof(*mc_led_info), GFP_KERNEL);
> > > > > +     if (!mc_led_info)
> > > > > +             return ERR_PTR(-ENOMEM);
> > > > > +
> > > >
> > > > Is there any reason why these are dynamically allocated?
> > >
> > > No particular reason. I should probably at least not dynamically
> > > allocate 'mc_dev' and pass it in similar to regular LED registration
> > > (previously I had my regular LEDs dynamically allocated). The
> > > mc_led_info I will probably keep dynamic. It feels a bit nasty to have
> > > the caller be aware of these internal details.
> > > [...]
> >
> > Could you please elaborate what you mean by "It feels a bit nasty to have
> > the caller be aware of these internal details."? I don't think I fully understand
> > what you're referring to.
> >
>
> Maybe I misunderstood your original comment. The question was why both
> 'lightbar_mc_dev' and 'mc_led_info' were dynamically allocated. I
> interpreted it as getting rid of some of the dynamic allocation as
> some wasn't needed, but not sure what you had in mind. The code now
> looks like:
>
> struct dualsense {
> ...
>         /* RGB lightbar */
>         struct led_classdev_mc lightbar; (not a pointer anymore)
> }
>
> 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;
> ...
>
> In here I kept 'mc_led_info' as dynamically allocated. I didn't think
> it made sense to have the caller know about it. What was your original
> idea?
> [...]

Thanks, I thought you meant something different, but this clears it up. By the way,
my original idea is really the simplest: have a `struct mc_subled[3]` in the dualsense
struct in addition to the multicolor LED, thus no dynamic allocation (apart from
allocating the dualsense struct) is necessary.

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

[Roderick Colenbrander]

Hi Barnabás,

On Tue, Dec 29, 2020 at 11:49 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Hi
>
>
> 2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:
>
> > [...]
> > +static struct input_dev *ps_touchpad_create(struct hid_device *hdev, int width, int height,
> > +             int num_contacts)
>
> Very minor thing, but `input_mt_init_slots()` takes an `unsigned int`, so
> wouldn't it be better if `num_contacts` was an `unsigned int`?

Agreed.

>
> > +{
> > +     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, 0, 0);
> > +     input_set_abs_params(touchpad, ABS_MT_POSITION_Y, 0, height, 0, 0);
>
> Shouldn't it be `width - 1` and `height - 1`? Or what am I missing?

I suspect that's what it should be. The docs aren't very clear on that
and after glancing around other drivers (in particular
input/touchscreen) many seem to be off by 1 as well. I think it should
indeed be 'width - 1' as also related axes are created through
input_mt_init_slots like ABS_X and ABS_Y, which inherit the same
dimensions and which would also be off by 1. So yeah, good catch.

> > +
> > +     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;
> > @@ -271,6 +304,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
> > @@ -311,6 +345,25 @@ 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);
> >
> > +     input_report_key(ds->touchpad, BTN_LEFT, ds_report->buttons[2] & DS_BUTTONS2_TOUCHPAD);
> > +     for (i = 0; i < 2; i++) {
> > +             bool active = (ds_report->points[i].contact & 0x80) ? false : true;
> > [...]
>
> I believe it'd be preferable to give a name to that 0x80.

Will do.

>
> Regards,
> Barnabás Pőcze

Regards,
Roderick

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

[Barnabás Pőcze]

Hi


2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:

> [...]
> +static const struct hid_device_id ps_devices[] = {
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER),
> +		.driver_data = 0 },
> +};
> [...]

I have come across an AUR package [1], and looking at the modifications applied
there (work-without-modifying-hid-quirks.patch), I think should be a terminating
entry in this array. And maybe `MODULE_DEVICE_TABLE()` is also missing?

[1]: https://aur.archlinux.org/packages/hid-playstation-dkms/


Regards,
Barnabás Pőcze

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

[Roderick Colenbrander]

Hi Barnabás,

On Wed, Dec 30, 2020 at 4:08 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Hi
>
>
> 2020. december 19., szombat 7:23 keltezéssel, Roderick Colenbrander írta:
>
> > [...]
> > +static const struct hid_device_id ps_devices[] = {
> > +     { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS5_CONTROLLER),
> > +             .driver_data = 0 },
> > +};
> > [...]
>
> I have come across an AUR package [1], and looking at the modifications applied
> there (work-without-modifying-hid-quirks.patch), I think should be a terminating
> entry in this array. And maybe `MODULE_DEVICE_TABLE()` is also missing?
>
> [1]: https://aur.archlinux.org/packages/hid-playstation-dkms/
>
>
> Regards,
> Barnabás Pőcze


Good catch, that's indeed missing. I wonder how I didn't stumble on
that (I used an unmodified kernel here and loaded it out of tree). In
any case I will add it and use it in revision 2.

Regards,
Roderick