Hi Barnabás,
Thanks for your comments.
On Thu, Jan 7, 2021 at 12:41 PM Barnabás Pőcze <pobrn@xxxxxxxxxxxxxx> wrote:
>
> 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?
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
