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

["Barnabás Pőcze" <pobrn@protonmail.com>]

Hi


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

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

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


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

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


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

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


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

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


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

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


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

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


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

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


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


Regards,
Barnabás Pőcze