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

[Roderick Colenbrander <roderick@gaikai.com>]

Hi Barnabás,

Thanks for your comments.

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

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

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

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

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

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

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

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


Regards,
Roderick