Re: [PATCH 4/5] HID: sony: Send ds4 output reports on output end-point

[Roderick Colenbrander <roderick@gaikai.com>]

On Wed, Oct 5, 2016 at 9:54 AM, Frank Praznik <frank.praznik@gmail.com> wrote:
>
>> On Oct 5, 2016, at 04:31, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
>>
>> [adding Frank, Simon and Antonio as they are the main developpers of
>> hid-sony]
>>
>> On Oct 04 2016 or thereabouts, Roderick Colenbrander wrote:
>>> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
>>>
>>> Add a CRC value to each output report. This removes the need for the
>>> 'no output reports on interrupt end-point' quirk.
>>>
>>> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
>>> ---
>>> drivers/hid/hid-sony.c | 20 +++++++++++---------
>>> 1 file changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
>>> index 34988ce..24f7d19 100644
>>> --- a/drivers/hid/hid-sony.c
>>> +++ b/drivers/hid/hid-sony.c
>>> @@ -1895,7 +1895,7 @@ static void dualshock4_send_output_report(struct sony_sc *sc)
>>>      } else {
>>>              memset(buf, 0, DS4_OUTPUT_REPORT_0x11_SIZE);
>>>              buf[0] = 0x11;
>>> -            buf[1] = 0x80;
>>> +            buf[1] = 0xC0; /* HID + CRC */
>>>              buf[3] = 0x0F;
>>>              offset = 6;
>>>      }
>>> @@ -1922,9 +1922,16 @@ static void dualshock4_send_output_report(struct sony_sc *sc)
>>>
>>>      if (sc->quirks & DUALSHOCK4_CONTROLLER_USB)
>>>              hid_hw_output_report(hdev, buf, DS4_OUTPUT_REPORT_0x05_SIZE);
>>> -    else
>>> -            hid_hw_raw_request(hdev, 0x11, buf, DS4_OUTPUT_REPORT_0x11_SIZE,
>>> -                            HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
>>> +    else {
>>> +            /* CRC generation */
>>> +            u8 bthdr = 0xA2;
>>> +            u32 crc;
>>> +
>>> +            crc = crc32_le(0xFFFFFFFF, &bthdr, 1);
>>> +            crc = ~crc32_le(crc, buf, DS4_OUTPUT_REPORT_0x11_SIZE-4);
>>> +            put_unaligned_le32(crc, &buf[74]);
>>> +            hid_hw_output_report(hdev, buf, DS4_OUTPUT_REPORT_0x11_SIZE);
>>> +    }
>>
>> nitpicking:
>> hid_hw_output_report(hdev, buf, DS4_OUTPUT_REPORT_0x11_SIZE); could be
>> factorized out if you add the crc only for BT devices.
>>
>>> }
>>>
>>> static void motion_send_output_report(struct sony_sc *sc)
>>> @@ -2378,11 +2385,6 @@ static int sony_input_configured(struct hid_device *hdev,
>>>              sony_init_output_report(sc, sixaxis_send_output_report);
>>>      } else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
>>>              if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) {
>>> -                    /*
>>> -                     * The DualShock 4 wants output reports sent on the ctrl
>>> -                     * endpoint when connected via Bluetooth.
>>> -                     */
>>> -                    hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP;
>>
>> The purpose of this quirk is only to have hidraw behaving like legacy
>> when the HID low-level transport has been worked on.
>> So basically, this might potentially break userspace as the CRC doesn't
>> get added automatically by the driver when talking to the device through
>> hidraw.
>>
>> Frank, Simon, Antonio, are there any userspace application that need to
>> communicate over hidraw to the controllers or is it entirely done in the
>> kernel now?
>>
>> Cheers,
>> Benjamin
>
> The best answer I can give is "not to my knowledge”.  Rumble and LEDs have standard
> kernel interfaces but obviously we can’t guarantee 100% that nobody is using hidraw
> directly.  Commercial games and engines aren't running as root and thus can’t use
> hidraw so I think we can safely say that no major commercial software will be broken
> by this.
>
> There are people who have had to alter the reporting rate of the controller for use with
> slow embedded processors.  With this change can the rate-control value be altered
> from the full-speed value of C0?
>
> I guess that in these corner cases people can revert it to the old behavior themselves if
> necessary since they have to customize the module anyways.
>
> Regards,
> Frank
>

To be honest I'm not sure how the rate control works. I think it is
handled outside the driver more by the bluetooth layer, but I'm not
sure. At least those few upper bits we changed don't seem to be
related to the speed.

Thanks,
Roderick