Hi
2021. január 2., szombat 23:31 keltezéssel, Roderick Colenbrander írta:
> From: Roderick Colenbrander <roderick.colenbrander@xxxxxxxx>
>
> 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@xxxxxxxx>
>
> 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
