[PATCH 0/5] HID: sony: game controller updates

[PATCH 0/5] HID: sony: game controller updates

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Hi all,

This patch series contains a couple patches to improve ds4 support.
The first two patches were sent earlier, but I was asked to rebase
against a different branch.

At time of the first submission, the second patch was not yet needed,
because there was no report id collision yet. This collission starts
happening as of patch 3, which adds CRC checking for input reports.
The other patches add crc checking for output report, which allows
getting rid of an ugly quirk. The last patch improves touchpad support.

Thanks,
Roderick

Roderick Colenbrander (5):
  HID: sony: Fix race condition in sony_probe
  HID: sony: Adjust HID report size name definitions
  HID: sony: Perform CRC check on bluetooth input packets
  HID: sony: Send ds4 output reports on output end-point
  HID: sony: Handle multiple touch events input record

 drivers/hid/hid-sony.c | 259 ++++++++++++++++++++++++++++---------------------
 1 file changed, 151 insertions(+), 108 deletions(-)

-- 
2.7.4

[PATCH 1/5] HID: sony: Fix race condition in sony_probe

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Early on the sony_probe function calls hid_hw_start to start the hardware.
Afterwards it issues some hardware requests, initializes other functionality
like Force Feedback, power classes and others. However by the time
hid_hw_start returns, the device nodes have already been created, which leads
to a race condition by user space applications which may detect the device
prior to completion of initialization. We have observed this problem many
times, this patch fixes the problem.

This patch moves most of sony_probe to sony_input_configured, which is called
prior to device registration. This fixes the race condition and the same
approach is used in other HID drivers.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/hid-sony.c | 117 ++++++++++++++++++++++++-------------------------
 1 file changed, 58 insertions(+), 59 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index b0bb99a..afa8219 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1387,28 +1387,6 @@ static int sony_register_touchpad(struct hid_input *hi, int touch_count,
 	return 0;
 }
 
-static int sony_input_configured(struct hid_device *hdev,
-					struct hid_input *hidinput)
-{
-	struct sony_sc *sc = hid_get_drvdata(hdev);
-	int ret;
-
-	/*
-	 * The Dualshock 4 touchpad supports 2 touches and has a
-	 * resolution of 1920x942 (44.86 dots/mm).
-	 */
-	if (sc->quirks & DUALSHOCK4_CONTROLLER) {
-		ret = sony_register_touchpad(hidinput, 2, 1920, 942);
-		if (ret) {
-			hid_err(sc->hdev,
-				"Unable to initialize multi-touch slots: %d\n",
-				ret);
-			return ret;
-		}
-	}
-
-	return 0;
-}
 
 /*
  * Sending HID_REQ_GET_REPORT changes the operation mode of the ps3 controller
@@ -2329,45 +2307,12 @@ static inline void sony_cancel_work_sync(struct sony_sc *sc)
 		cancel_work_sync(&sc->state_worker);
 }
 
-static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
+static int sony_input_configured(struct hid_device *hdev,
+					struct hid_input *hidinput)
 {
-	int ret;
+	struct sony_sc *sc = hid_get_drvdata(hdev);
 	int append_dev_id;
-	unsigned long quirks = id->driver_data;
-	struct sony_sc *sc;
-	unsigned int connect_mask = HID_CONNECT_DEFAULT;
-
-	if (!strcmp(hdev->name, "FutureMax Dance Mat"))
-		quirks |= FUTUREMAX_DANCE_MAT;
-
-	sc = devm_kzalloc(&hdev->dev, sizeof(*sc), GFP_KERNEL);
-	if (sc == NULL) {
-		hid_err(hdev, "can't alloc sony descriptor\n");
-		return -ENOMEM;
-	}
-
-	spin_lock_init(&sc->lock);
-
-	sc->quirks = quirks;
-	hid_set_drvdata(hdev, sc);
-	sc->hdev = hdev;
-
-	ret = hid_parse(hdev);
-	if (ret) {
-		hid_err(hdev, "parse failed\n");
-		return ret;
-	}
-
-	if (sc->quirks & VAIO_RDESC_CONSTANT)
-		connect_mask |= HID_CONNECT_HIDDEV_FORCE;
-	else if (sc->quirks & SIXAXIS_CONTROLLER)
-		connect_mask |= HID_CONNECT_HIDDEV_FORCE;
-
-	ret = hid_hw_start(hdev, connect_mask);
-	if (ret) {
-		hid_err(hdev, "hw start failed\n");
-		return ret;
-	}
+	int ret;
 
 	ret = sony_set_device_id(sc);
 	if (ret < 0) {
@@ -2427,6 +2372,18 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 			}
 		}
 
+		/*
+		 * The Dualshock 4 touchpad supports 2 touches and has a
+		 * resolution of 1920x942 (44.86 dots/mm).
+		 */
+		ret = sony_register_touchpad(hidinput, 2, 1920, 942);
+		if (ret) {
+			hid_err(sc->hdev,
+			"Unable to initialize multi-touch slots: %d\n",
+			ret);
+			return ret;
+		}
+
 		sony_init_output_report(sc, dualshock4_send_output_report);
 	} else if (sc->quirks & MOTION_CONTROLLER) {
 		sony_init_output_report(sc, motion_send_output_report);
@@ -2482,6 +2439,48 @@ err_stop:
 	return ret;
 }
 
+static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+	int ret;
+	unsigned long quirks = id->driver_data;
+	struct sony_sc *sc;
+	unsigned int connect_mask = HID_CONNECT_DEFAULT;
+
+	if (!strcmp(hdev->name, "FutureMax Dance Mat"))
+		quirks |= FUTUREMAX_DANCE_MAT;
+
+	sc = devm_kzalloc(&hdev->dev, sizeof(*sc), GFP_KERNEL);
+	if (sc == NULL) {
+		hid_err(hdev, "can't alloc sony descriptor\n");
+		return -ENOMEM;
+	}
+
+	spin_lock_init(&sc->lock);
+
+	sc->quirks = quirks;
+	hid_set_drvdata(hdev, sc);
+	sc->hdev = hdev;
+
+	ret = hid_parse(hdev);
+	if (ret) {
+		hid_err(hdev, "parse failed\n");
+		return ret;
+	}
+
+	if (sc->quirks & VAIO_RDESC_CONSTANT)
+		connect_mask |= HID_CONNECT_HIDDEV_FORCE;
+	else if (sc->quirks & SIXAXIS_CONTROLLER)
+		connect_mask |= HID_CONNECT_HIDDEV_FORCE;
+
+	ret = hid_hw_start(hdev, connect_mask);
+	if (ret) {
+		hid_err(hdev, "hw start failed\n");
+		return ret;
+	}
+
+	return ret;
+}
+
 static void sony_remove(struct hid_device *hdev)
 {
 	struct sony_sc *sc = hid_get_drvdata(hdev);
-- 
2.7.4

[PATCH 2/5] HID: sony: Adjust HID report size name definitions

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Put the report type (feature / output) in the report size definitions.
This prevents name collisions later on for other different reports, which use
the same report id, but have a different size.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/hid-sony.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index afa8219..43bb24c 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1019,10 +1019,10 @@ struct motion_output_report_02 {
 	u8 rumble;
 };
 
-#define DS4_REPORT_0x02_SIZE 37
-#define DS4_REPORT_0x05_SIZE 32
-#define DS4_REPORT_0x11_SIZE 78
-#define DS4_REPORT_0x81_SIZE 7
+#define DS4_FEATURE_REPORT_0x02_SIZE 37
+#define DS4_FEATURE_REPORT_0x81_SIZE 7
+#define DS4_OUTPUT_REPORT_0x05_SIZE 32
+#define DS4_OUTPUT_REPORT_0x11_SIZE 78
 #define SIXAXIS_REPORT_0xF2_SIZE 17
 #define SIXAXIS_REPORT_0xF5_SIZE 8
 #define MOTION_REPORT_0x02_SIZE 49
@@ -1461,11 +1461,11 @@ static int dualshock4_set_operational_bt(struct hid_device *hdev)
 	u8 *buf;
 	int ret;
 
-	buf = kmalloc(DS4_REPORT_0x02_SIZE, GFP_KERNEL);
+	buf = kmalloc(DS4_FEATURE_REPORT_0x02_SIZE, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
-	ret = hid_hw_raw_request(hdev, 0x02, buf, DS4_REPORT_0x02_SIZE,
+	ret = hid_hw_raw_request(hdev, 0x02, buf, DS4_FEATURE_REPORT_0x02_SIZE,
 				HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
 
 	kfree(buf);
@@ -1870,12 +1870,12 @@ static void dualshock4_send_output_report(struct sony_sc *sc)
 	 * 0xD0 - 66hz
 	 */
 	if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
-		memset(buf, 0, DS4_REPORT_0x05_SIZE);
+		memset(buf, 0, DS4_OUTPUT_REPORT_0x05_SIZE);
 		buf[0] = 0x05;
 		buf[1] = 0xFF;
 		offset = 4;
 	} else {
-		memset(buf, 0, DS4_REPORT_0x11_SIZE);
+		memset(buf, 0, DS4_OUTPUT_REPORT_0x11_SIZE);
 		buf[0] = 0x11;
 		buf[1] = 0x80;
 		buf[3] = 0x0F;
@@ -1903,9 +1903,9 @@ static void dualshock4_send_output_report(struct sony_sc *sc)
 	buf[offset++] = sc->led_delay_off[3];
 
 	if (sc->quirks & DUALSHOCK4_CONTROLLER_USB)
-		hid_hw_output_report(hdev, buf, DS4_REPORT_0x05_SIZE);
+		hid_hw_output_report(hdev, buf, DS4_OUTPUT_REPORT_0x05_SIZE);
 	else
-		hid_hw_raw_request(hdev, 0x11, buf, DS4_REPORT_0x11_SIZE,
+		hid_hw_raw_request(hdev, 0x11, buf, DS4_OUTPUT_REPORT_0x11_SIZE,
 				HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
 }
 
@@ -1950,10 +1950,10 @@ static int sony_allocate_output_report(struct sony_sc *sc)
 			kmalloc(sizeof(union sixaxis_output_report_01),
 				GFP_KERNEL);
 	else if (sc->quirks & DUALSHOCK4_CONTROLLER_BT)
-		sc->output_report_dmabuf = kmalloc(DS4_REPORT_0x11_SIZE,
+		sc->output_report_dmabuf = kmalloc(DS4_OUTPUT_REPORT_0x11_SIZE,
 						GFP_KERNEL);
 	else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB)
-		sc->output_report_dmabuf = kmalloc(DS4_REPORT_0x05_SIZE,
+		sc->output_report_dmabuf = kmalloc(DS4_OUTPUT_REPORT_0x05_SIZE,
 						GFP_KERNEL);
 	else if (sc->quirks & MOTION_CONTROLLER)
 		sc->output_report_dmabuf = kmalloc(MOTION_REPORT_0x02_SIZE,
@@ -2198,7 +2198,7 @@ static int sony_check_add(struct sony_sc *sc)
 			return 0;
 		}
 	} else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
-		buf = kmalloc(DS4_REPORT_0x81_SIZE, GFP_KERNEL);
+		buf = kmalloc(DS4_FEATURE_REPORT_0x81_SIZE, GFP_KERNEL);
 		if (!buf)
 			return -ENOMEM;
 
@@ -2208,10 +2208,10 @@ static int sony_check_add(struct sony_sc *sc)
 		 * offset 1.
 		 */
 		ret = hid_hw_raw_request(sc->hdev, 0x81, buf,
-				DS4_REPORT_0x81_SIZE, HID_FEATURE_REPORT,
+				DS4_FEATURE_REPORT_0x81_SIZE, HID_FEATURE_REPORT,
 				HID_REQ_GET_REPORT);
 
-		if (ret != DS4_REPORT_0x81_SIZE) {
+		if (ret != DS4_FEATURE_REPORT_0x81_SIZE) {
 			hid_err(sc->hdev, "failed to retrieve feature report 0x81 with the DualShock 4 MAC address\n");
 			ret = ret < 0 ? ret : -EINVAL;
 			goto out_free;
-- 
2.7.4

[PATCH 3/5] HID: sony: Perform CRC check on bluetooth input packets

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/hid-sony.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 43bb24c..34988ce 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -36,6 +36,8 @@
 #include <linux/list.h>
 #include <linux/idr.h>
 #include <linux/input/mt.h>
+#include <linux/crc32.h>
+#include <asm/unaligned.h>
 
 #include "hid-ids.h"
 
@@ -1021,6 +1023,7 @@ struct motion_output_report_02 {
 
 #define DS4_FEATURE_REPORT_0x02_SIZE 37
 #define DS4_FEATURE_REPORT_0x81_SIZE 7
+#define DS4_INPUT_REPORT_0x11_SIZE 78
 #define DS4_OUTPUT_REPORT_0x05_SIZE 32
 #define DS4_OUTPUT_REPORT_0x11_SIZE 78
 #define SIXAXIS_REPORT_0xF2_SIZE 17
@@ -1324,6 +1327,21 @@ static int sony_raw_event(struct hid_device *hdev, struct hid_report *report,
 	} else if (((sc->quirks & DUALSHOCK4_CONTROLLER_USB) && rd[0] == 0x01 &&
 			size == 64) || ((sc->quirks & DUALSHOCK4_CONTROLLER_BT)
 			&& rd[0] == 0x11 && size == 78)) {
+		if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) {
+			/* CRC check */
+			u8 bthdr = 0xA1;
+			u32 crc;
+			u32 report_crc;
+
+			crc = crc32_le(0xFFFFFFFF, &bthdr, 1);
+			crc = ~crc32_le(crc, rd, DS4_INPUT_REPORT_0x11_SIZE-4);
+			report_crc = get_unaligned_le32(&rd[DS4_INPUT_REPORT_0x11_SIZE-4]);
+			if (crc != report_crc) {
+				hid_dbg(sc->hdev, "DualShock 4 input report's CRC check failed, received crc 0x%0x != 0x%0x\n",
+					report_crc, crc);
+				return -EILSEQ;
+			}
+		}
 		dualshock4_parse_report(sc, rd, size);
 	}
 
-- 
2.7.4

Re: [PATCH 3/5] HID: sony: Perform CRC check on bluetooth input packets

[Benjamin Tissoires]

Hi Roderick,

Thanks for the fast respin.

On Oct 04 2016 or thereabouts, Roderick Colenbrander wrote:
> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> 
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> ---
>  drivers/hid/hid-sony.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 43bb24c..34988ce 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -36,6 +36,8 @@
>  #include <linux/list.h>
>  #include <linux/idr.h>
>  #include <linux/input/mt.h>
> +#include <linux/crc32.h>
> +#include <asm/unaligned.h>
>  
>  #include "hid-ids.h"
>  
> @@ -1021,6 +1023,7 @@ struct motion_output_report_02 {
>  
>  #define DS4_FEATURE_REPORT_0x02_SIZE 37
>  #define DS4_FEATURE_REPORT_0x81_SIZE 7
> +#define DS4_INPUT_REPORT_0x11_SIZE 78

Just nitpicking, keeping the old define would have not changed a bit,
given that input and output reports on 0x11 are the same :)

Anyway, the 3 first patches are:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

(you can add this to the patches in case of a resubmission, that's
easier to track which patches are reviewed and which are not).

I have a few comments on the 2 next.

Cheers,
Benjamin

>  #define DS4_OUTPUT_REPORT_0x05_SIZE 32
>  #define DS4_OUTPUT_REPORT_0x11_SIZE 78
>  #define SIXAXIS_REPORT_0xF2_SIZE 17
> @@ -1324,6 +1327,21 @@ static int sony_raw_event(struct hid_device *hdev, struct hid_report *report,
>  	} else if (((sc->quirks & DUALSHOCK4_CONTROLLER_USB) && rd[0] == 0x01 &&
>  			size == 64) || ((sc->quirks & DUALSHOCK4_CONTROLLER_BT)
>  			&& rd[0] == 0x11 && size == 78)) {
> +		if (sc->quirks & DUALSHOCK4_CONTROLLER_BT) {
> +			/* CRC check */
> +			u8 bthdr = 0xA1;
> +			u32 crc;
> +			u32 report_crc;
> +
> +			crc = crc32_le(0xFFFFFFFF, &bthdr, 1);
> +			crc = ~crc32_le(crc, rd, DS4_INPUT_REPORT_0x11_SIZE-4);
> +			report_crc = get_unaligned_le32(&rd[DS4_INPUT_REPORT_0x11_SIZE-4]);
> +			if (crc != report_crc) {
> +				hid_dbg(sc->hdev, "DualShock 4 input report's CRC check failed, received crc 0x%0x != 0x%0x\n",
> +					report_crc, crc);
> +				return -EILSEQ;
> +			}
> +		}
>  		dualshock4_parse_report(sc, rd, size);
>  	}
>  
> -- 
> 2.7.4
> 

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

[Roderick Colenbrander]

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);
+	}
 }
 
 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;
 			ret = dualshock4_set_operational_bt(hdev);
 			if (ret < 0) {
 				hid_err(hdev, "failed to set the Dualshock 4 operational mode\n");
-- 
2.7.4

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

[Benjamin Tissoires]

[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

>  			ret = dualshock4_set_operational_bt(hdev);
>  			if (ret < 0) {
>  				hid_err(hdev, "failed to set the Dualshock 4 operational mode\n");
> -- 
> 2.7.4
> 

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

[Frank Praznik]

> 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

> 
>> 			ret = dualshock4_set_operational_bt(hdev);
>> 			if (ret < 0) {
>> 				hid_err(hdev, "failed to set the Dualshock 4 operational mode\n");
>> -- 
>> 2.7.4
>> 

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

[Simon Wood]

On Wed, October 5, 2016 10:54 am, Frank Praznik wrote:
> 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 a couple of projects I know of, but I am unable to say whether
they are still active or whether proposed changes will affect them...
https://thp.io/2010/psmove/
http://moveonpc.blogspot.ca/
https://github.com/chrippa/ds4drv

Audio output is one area of the DS4 which is not supported in kernel,
maybe some renewed interest can get this working.

I agree with Frank that we should move forward, if something gets broken
we can work a better solution.
Simon.

acked-by: Simon Wood <simon@mungewell.org>

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

[Frank Praznik]

> On Oct 5, 2016, at 13:35, Simon Wood <simon@mungewell.org> wrote:
> 
> On Wed, October 5, 2016 10:54 am, Frank Praznik wrote:
>> 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 a couple of projects I know of, but I am unable to say whether
> they are still active or whether proposed changes will affect them...
> https://thp.io/2010/psmove/
> http://moveonpc.blogspot.ca/
> https://github.com/chrippa/ds4drv
> 
> Audio output is one area of the DS4 which is not supported in kernel,
> maybe some renewed interest can get this working.
> 
> I agree with Frank that we should move forward, if something gets broken
> we can work a better solution.
> Simon.
> 
> acked-by: Simon Wood <simon@mungewell.org>
> 
> 
> 

This change only affects sending rumble/LED output reports to the DS4, so hidraw input reports and sending/receiving feature reports will be unchanged as will anything not in the DS4 codepath (ie. Move controllers).  It looks like ds4drv has both a hidraw backend and one which uses raw Bluetooth sockets.  The raw socket backend should be unaffected, but the hidraw backend will probably have issues.  It should be trivial to patch it to append the CRC to the raw data packet like the kernel module does though.  It also looks like hidraw isn’t the default Bluetooth backend which reduces the odds that it will be an issue even further.

I’m definitely in favor of moving toward more correct behavior as long as the ds4drv project is given a warning to start sending CRC-ed output reports.

acked-by: Frank Praznik <frank.praznik@gmail.com>

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

[Benjamin Tissoires]

On Oct 05 2016 or thereabouts, Frank Praznik wrote:
> 
> > On Oct 5, 2016, at 13:35, Simon Wood <simon@mungewell.org> wrote:
> > 
> > On Wed, October 5, 2016 10:54 am, Frank Praznik wrote:
> >> 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 a couple of projects I know of, but I am unable to say whether
> > they are still active or whether proposed changes will affect them...
> > https://thp.io/2010/psmove/
> > http://moveonpc.blogspot.ca/
> > https://github.com/chrippa/ds4drv
> > 
> > Audio output is one area of the DS4 which is not supported in kernel,
> > maybe some renewed interest can get this working.
> > 
> > I agree with Frank that we should move forward, if something gets broken
> > we can work a better solution.
> > Simon.
> > 
> > acked-by: Simon Wood <simon@mungewell.org>
> > 
> > 
> > 
> 
> This change only affects sending rumble/LED output reports to the DS4, so hidraw input reports and sending/receiving feature reports will be unchanged as will anything not in the DS4 codepath (ie. Move controllers).  It looks like ds4drv has both a hidraw backend and one which uses raw Bluetooth sockets.  The raw socket backend should be unaffected, but the hidraw backend will probably have issues.  It should be trivial to patch it to append the CRC to the raw data packet like the kernel module does though.  It also looks like hidraw isn’t the default Bluetooth backend which reduces the odds that it will be an issue even further.
> 
> I’m definitely in favor of moving toward more correct behavior as long as the ds4drv project is given a warning to start sending CRC-ed output reports.
> 
> acked-by: Frank Praznik <frank.praznik@gmail.com>

Then:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
For the user-space change: Acked-by me too.

Cheers,
Benjamin

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

[Roderick Colenbrander]

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

[PATCH 5/5] HID: sony: Handle multiple touch events input record

[Roderick Colenbrander]

From: Roderick Colenbrander <roderick.colenbrander@sony.com>

Read the touch history field in the HID descriptor and use this value
to determine how many touch events to read from the report. As part
of this patch, we did a first attempt of making the offset calculation
code less magical.

Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
---
 drivers/hid/hid-sony.c | 76 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 50 insertions(+), 26 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 24f7d19..2387aaf 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1030,6 +1030,12 @@ struct motion_output_report_02 {
 #define SIXAXIS_REPORT_0xF5_SIZE 8
 #define MOTION_REPORT_0x02_SIZE 49
 
+/* Offsets relative to USB input report (0x1). Bluetooth (0x11) requires an
+ * additional +2.
+ */
+#define DS4_INPUT_REPORT_BATTERY_OFFSET  30
+#define DS4_INPUT_REPORT_TOUCHPAD_OFFSET 33
+
 static DEFINE_SPINLOCK(sony_dev_list_lock);
 static LIST_HEAD(sony_device_list);
 static DEFINE_IDA(sony_device_id_allocator);
@@ -1226,19 +1232,17 @@ static void dualshock4_parse_report(struct sony_sc *sc, u8 *rd, int size)
 						struct hid_input, list);
 	struct input_dev *input_dev = hidinput->input;
 	unsigned long flags;
-	int n, offset;
+	int n, m, offset, num_touch_data, max_touch_data;
 	u8 cable_state, battery_capacity, battery_charging;
 
-	/*
-	 * Battery and touchpad data starts at byte 30 in the USB report and
-	 * 32 in Bluetooth report.
-	 */
-	offset = (sc->quirks & DUALSHOCK4_CONTROLLER_USB) ? 30 : 32;
+	/* When using Bluetooth the header is 2 bytes longer, so skip these. */
+	int data_offset = (sc->quirks & DUALSHOCK4_CONTROLLER_USB) ? 0 : 2;
 
 	/*
-	 * The lower 4 bits of byte 30 contain the battery level
+	 * The lower 4 bits of byte 30 (or 32 for BT) contain the battery level
 	 * and the 5th bit contains the USB cable state.
 	 */
+	offset = data_offset + DS4_INPUT_REPORT_BATTERY_OFFSET;
 	cable_state = (rd[offset] >> 4) & 0x01;
 	battery_capacity = rd[offset] & 0x0F;
 
@@ -1265,30 +1269,50 @@ static void dualshock4_parse_report(struct sony_sc *sc, u8 *rd, int size)
 	sc->battery_charging = battery_charging;
 	spin_unlock_irqrestore(&sc->lock, flags);
 
-	offset += 5;
-
 	/*
-	 * The Dualshock 4 multi-touch trackpad data starts at offset 35 on USB
-	 * and 37 on Bluetooth.
-	 * The first 7 bits of the first byte is a counter and bit 8 is a touch
-	 * indicator that is 0 when pressed and 1 when not pressed.
-	 * The next 3 bytes are two 12 bit touch coordinates, X and Y.
-	 * The data for the second touch is in the same format and immediatly
-	 * follows the data for the first.
+	 * The Dualshock 4 multi-touch trackpad data starts at offset 33 on USB
+	 * and 35 on Bluetooth.
+	 * The first byte indicates the number of touch data in the report.
+	 * Trackpad data starts 2 bytes later (e.g. 35 for USB).
 	 */
-	for (n = 0; n < 2; n++) {
-		u16 x, y;
+	offset = data_offset + DS4_INPUT_REPORT_TOUCHPAD_OFFSET;
+	max_touch_data = (sc->quirks & DUALSHOCK4_CONTROLLER_USB) ? 3 : 4;
+	if (rd[offset] > 0 && rd[offset] <= max_touch_data)
+		num_touch_data = rd[offset];
+	else
+		num_touch_data = 1;
+	offset += 1;
 
-		x = rd[offset+1] | ((rd[offset+2] & 0xF) << 8);
-		y = ((rd[offset+2] & 0xF0) >> 4) | (rd[offset+3] << 4);
+	for (m = 0; m < num_touch_data; m++) {
+		/* Skip past timestamp */
+		offset += 1;
 
-		input_mt_slot(input_dev, n);
-		input_mt_report_slot_state(input_dev, MT_TOOL_FINGER,
-					!(rd[offset] >> 7));
-		input_report_abs(input_dev, ABS_MT_POSITION_X, x);
-		input_report_abs(input_dev, ABS_MT_POSITION_Y, y);
+		/*
+		 * The first 7 bits of the first byte is a counter and bit 8 is
+		 * a touch indicator that is 0 when pressed and 1 when not
+		 * pressed.
+		 * The next 3 bytes are two 12 bit touch coordinates, X and Y.
+		 * The data for the second touch is in the same format and
+		 * immediately follows the data for the first.
+		 */
+		for (n = 0; n < 2; n++) {
+			u16 x, y;
+			bool active;
+
+			x = rd[offset+1] | ((rd[offset+2] & 0xF) << 8);
+			y = ((rd[offset+2] & 0xF0) >> 4) | (rd[offset+3] << 4);
+
+			active = !(rd[offset] >> 7);
+			input_mt_slot(input_dev, n);
+			input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, active);
 
-		offset += 4;
+			if (active) {
+				input_report_abs(input_dev, ABS_MT_POSITION_X, x);
+				input_report_abs(input_dev, ABS_MT_POSITION_Y, y);
+			}
+
+			offset += 4;
+		}
 	}
 }
 
-- 
2.7.4

Re: [PATCH 5/5] HID: sony: Handle multiple touch events input record

[Benjamin Tissoires]

On Oct 04 2016 or thereabouts, Roderick Colenbrander wrote:
> From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> 
> Read the touch history field in the HID descriptor and use this value
> to determine how many touch events to read from the report. As part
> of this patch, we did a first attempt of making the offset calculation
> code less magical.
> 
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> ---
>  drivers/hid/hid-sony.c | 76 +++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 50 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 24f7d19..2387aaf 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -1030,6 +1030,12 @@ struct motion_output_report_02 {
>  #define SIXAXIS_REPORT_0xF5_SIZE 8
>  #define MOTION_REPORT_0x02_SIZE 49
>  
> +/* Offsets relative to USB input report (0x1). Bluetooth (0x11) requires an
> + * additional +2.
> + */
> +#define DS4_INPUT_REPORT_BATTERY_OFFSET  30
> +#define DS4_INPUT_REPORT_TOUCHPAD_OFFSET 33
> +
>  static DEFINE_SPINLOCK(sony_dev_list_lock);
>  static LIST_HEAD(sony_device_list);
>  static DEFINE_IDA(sony_device_id_allocator);
> @@ -1226,19 +1232,17 @@ static void dualshock4_parse_report(struct sony_sc *sc, u8 *rd, int size)
>  						struct hid_input, list);
>  	struct input_dev *input_dev = hidinput->input;
>  	unsigned long flags;
> -	int n, offset;
> +	int n, m, offset, num_touch_data, max_touch_data;
>  	u8 cable_state, battery_capacity, battery_charging;
>  
> -	/*
> -	 * Battery and touchpad data starts at byte 30 in the USB report and
> -	 * 32 in Bluetooth report.
> -	 */
> -	offset = (sc->quirks & DUALSHOCK4_CONTROLLER_USB) ? 30 : 32;
> +	/* When using Bluetooth the header is 2 bytes longer, so skip these. */
> +	int data_offset = (sc->quirks & DUALSHOCK4_CONTROLLER_USB) ? 0 : 2;
>  
>  	/*
> -	 * The lower 4 bits of byte 30 contain the battery level
> +	 * The lower 4 bits of byte 30 (or 32 for BT) contain the battery level
>  	 * and the 5th bit contains the USB cable state.
>  	 */
> +	offset = data_offset + DS4_INPUT_REPORT_BATTERY_OFFSET;
>  	cable_state = (rd[offset] >> 4) & 0x01;
>  	battery_capacity = rd[offset] & 0x0F;
>  
> @@ -1265,30 +1269,50 @@ static void dualshock4_parse_report(struct sony_sc *sc, u8 *rd, int size)
>  	sc->battery_charging = battery_charging;
>  	spin_unlock_irqrestore(&sc->lock, flags);
>  
> -	offset += 5;
> -
>  	/*
> -	 * The Dualshock 4 multi-touch trackpad data starts at offset 35 on USB
> -	 * and 37 on Bluetooth.
> -	 * The first 7 bits of the first byte is a counter and bit 8 is a touch
> -	 * indicator that is 0 when pressed and 1 when not pressed.
> -	 * The next 3 bytes are two 12 bit touch coordinates, X and Y.
> -	 * The data for the second touch is in the same format and immediatly
> -	 * follows the data for the first.
> +	 * The Dualshock 4 multi-touch trackpad data starts at offset 33 on USB
> +	 * and 35 on Bluetooth.
> +	 * The first byte indicates the number of touch data in the report.
> +	 * Trackpad data starts 2 bytes later (e.g. 35 for USB).
>  	 */
> -	for (n = 0; n < 2; n++) {
> -		u16 x, y;
> +	offset = data_offset + DS4_INPUT_REPORT_TOUCHPAD_OFFSET;
> +	max_touch_data = (sc->quirks & DUALSHOCK4_CONTROLLER_USB) ? 3 : 4;
> +	if (rd[offset] > 0 && rd[offset] <= max_touch_data)
> +		num_touch_data = rd[offset];
> +	else
> +		num_touch_data = 1;
> +	offset += 1;
>  
> -		x = rd[offset+1] | ((rd[offset+2] & 0xF) << 8);
> -		y = ((rd[offset+2] & 0xF0) >> 4) | (rd[offset+3] << 4);
> +	for (m = 0; m < num_touch_data; m++) {
> +		/* Skip past timestamp */
> +		offset += 1;
>  
> -		input_mt_slot(input_dev, n);
> -		input_mt_report_slot_state(input_dev, MT_TOOL_FINGER,
> -					!(rd[offset] >> 7));
> -		input_report_abs(input_dev, ABS_MT_POSITION_X, x);
> -		input_report_abs(input_dev, ABS_MT_POSITION_Y, y);
> +		/*
> +		 * The first 7 bits of the first byte is a counter and bit 8 is
> +		 * a touch indicator that is 0 when pressed and 1 when not
> +		 * pressed.
> +		 * The next 3 bytes are two 12 bit touch coordinates, X and Y.
> +		 * The data for the second touch is in the same format and
> +		 * immediately follows the data for the first.
> +		 */
> +		for (n = 0; n < 2; n++) {
> +			u16 x, y;
> +			bool active;
> +
> +			x = rd[offset+1] | ((rd[offset+2] & 0xF) << 8);
> +			y = ((rd[offset+2] & 0xF0) >> 4) | (rd[offset+3] << 4);
> +
> +			active = !(rd[offset] >> 7);
> +			input_mt_slot(input_dev, n);

Just to be sure, the device reports 2 touches only, and the
"num_touch_data" chunks are just the history of these 2 touches, the
last chunk being the last known touches?

If so, then why aren't you simply jumping to the last valid value?
If not, then you probably need to report (n+m*2) slot, and change the
input_mt_init_slot parameter as well.

Cheers,
Benjamin

> +			input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, active);
>  
> -		offset += 4;
> +			if (active) {
> +				input_report_abs(input_dev, ABS_MT_POSITION_X, x);
> +				input_report_abs(input_dev, ABS_MT_POSITION_Y, y);
> +			}
> +
> +			offset += 4;
> +		}
>  	}
>  }
>  
> -- 
> 2.7.4
> 

Re: [PATCH 5/5] HID: sony: Handle multiple touch events input record

[Simon Wood]

On Wed, October 5, 2016 2:35 am, Benjamin Tissoires wrote:
> On Oct 04 2016 or thereabouts, Roderick Colenbrander wrote:

>> +		/*
>> +		 * The first 7 bits of the first byte is a counter and bit 8 is
>> +		 * a touch indicator that is 0 when pressed and 1 when not
>> +		 * pressed.
>> +		 * The next 3 bytes are two 12 bit touch coordinates, X and Y.
>> +		 * The data for the second touch is in the same format and
>> +		 * immediately follows the data for the first.
>> +		 */
>> +		for (n = 0; n < 2; n++) {
>> +			u16 x, y;
>> +			bool active;
>> +
>> +			x = rd[offset+1] | ((rd[offset+2] & 0xF) << 8);
>> +			y = ((rd[offset+2] & 0xF0) >> 4) | (rd[offset+3] << 4);
>> +
>> +			active = !(rd[offset] >> 7);
>> +			input_mt_slot(input_dev, n);
>>
>
> Just to be sure, the device reports 2 touches only, and the
> "num_touch_data" chunks are just the history of these 2 touches, the
> last chunk being the last known touches?

FYI - Community knowledge/understanding...
http://www.psdevwiki.com/ps4/DS4-BT#HID_INPUT_reports

Simon

Re: [PATCH 5/5] HID: sony: Handle multiple touch events input record

[Roderick Colenbrander]

On Wed, Oct 5, 2016 at 8:29 AM, Simon Wood <simon@mungewell.org> wrote:
> On Wed, October 5, 2016 2:35 am, Benjamin Tissoires wrote:
>> On Oct 04 2016 or thereabouts, Roderick Colenbrander wrote:
>
>>> +            /*
>>> +             * The first 7 bits of the first byte is a counter and bit 8 is
>>> +             * a touch indicator that is 0 when pressed and 1 when not
>>> +             * pressed.
>>> +             * The next 3 bytes are two 12 bit touch coordinates, X and Y.
>>> +             * The data for the second touch is in the same format and
>>> +             * immediately follows the data for the first.
>>> +             */
>>> +            for (n = 0; n < 2; n++) {
>>> +                    u16 x, y;
>>> +                    bool active;
>>> +
>>> +                    x = rd[offset+1] | ((rd[offset+2] & 0xF) << 8);
>>> +                    y = ((rd[offset+2] & 0xF0) >> 4) | (rd[offset+3] << 4);
>>> +
>>> +                    active = !(rd[offset] >> 7);
>>> +                    input_mt_slot(input_dev, n);
>>>
>>
>> Just to be sure, the device reports 2 touches only, and the
>> "num_touch_data" chunks are just the history of these 2 touches, the
>> last chunk being the last known touches?
>
> FYI - Community knowledge/understanding...
> http://www.psdevwiki.com/ps4/DS4-BT#HID_INPUT_reports
>
> Simon
>

Hi Ben and Simon,

Correct, the DS4 sends a touch history. The last element is the
latest. An input report can contain multiple touch samples, because
the device internally samples at a rate which is higher than at which
it generates HID reports. On USB you can easily see multiple touch
events and it is even easier on Bluetooth (especially if it is set to
a low frequency). The extra history is important for gestures.

Thanks,
Roderick

Re: [PATCH 5/5] HID: sony: Handle multiple touch events input record

[Benjamin Tissoires]

On Oct 05 2016 or thereabouts, Roderick Colenbrander wrote:
> On Wed, Oct 5, 2016 at 8:29 AM, Simon Wood <simon@mungewell.org> wrote:
> > On Wed, October 5, 2016 2:35 am, Benjamin Tissoires wrote:
> >> On Oct 04 2016 or thereabouts, Roderick Colenbrander wrote:
> >
> >>> +            /*
> >>> +             * The first 7 bits of the first byte is a counter and bit 8 is
> >>> +             * a touch indicator that is 0 when pressed and 1 when not
> >>> +             * pressed.
> >>> +             * The next 3 bytes are two 12 bit touch coordinates, X and Y.
> >>> +             * The data for the second touch is in the same format and
> >>> +             * immediately follows the data for the first.
> >>> +             */
> >>> +            for (n = 0; n < 2; n++) {
> >>> +                    u16 x, y;
> >>> +                    bool active;
> >>> +
> >>> +                    x = rd[offset+1] | ((rd[offset+2] & 0xF) << 8);
> >>> +                    y = ((rd[offset+2] & 0xF0) >> 4) | (rd[offset+3] << 4);
> >>> +
> >>> +                    active = !(rd[offset] >> 7);
> >>> +                    input_mt_slot(input_dev, n);
> >>>
> >>
> >> Just to be sure, the device reports 2 touches only, and the
> >> "num_touch_data" chunks are just the history of these 2 touches, the
> >> last chunk being the last known touches?
> >
> > FYI - Community knowledge/understanding...
> > http://www.psdevwiki.com/ps4/DS4-BT#HID_INPUT_reports
> >
> > Simon
> >
> 
> Hi Ben and Simon,
> 
> Correct, the DS4 sends a touch history. The last element is the
> latest. An input report can contain multiple touch samples, because
> the device internally samples at a rate which is higher than at which
> it generates HID reports. On USB you can easily see multiple touch
> events and it is even easier on Bluetooth (especially if it is set to
> a low frequency). The extra history is important for gestures.

OK, so if it's important, you need to actually send it by adding
input_mt_sync_frame() and input_sync() calls. Otherwise, the values will
just get mangled by the kernel in evdev and everything happens as if you
just sent the last pair in the history.

The first 4 patches are IMO mergeable, but this one will need a little
bit more polish to actually forward the events.

Cheers,
Benjamin

> 
> Thanks,
> Roderick

Re: [PATCH 5/5] HID: sony: Handle multiple touch events input record

[Roderick Colenbrander]

>
> > Correct, the DS4 sends a touch history. The last element is the
> > latest. An input report can contain multiple touch samples, because
> > the device internally samples at a rate which is higher than at which
> > it generates HID reports. On USB you can easily see multiple touch
> > events and it is even easier on Bluetooth (especially if it is set to
> > a low frequency). The extra history is important for gestures.
>
> OK, so if it's important, you need to actually send it by adding
> input_mt_sync_frame() and input_sync() calls. Otherwise, the values will
> just get mangled by the kernel in evdev and everything happens as if you
> just sent the last pair in the history.
>
> The first 4 patches are IMO mergeable, but this one will need a little
> bit more polish to actually forward the events.
>
> Cheers,
> Benjamin
>
> >
> > Thanks,
> > Roderick

Hi Ben,

A kind of follow-up to this patch feedback which made it into the
final patch. The recommendation was to use input_mt_sync_frame, which
was a good recommendation. There is one oversight, which I'm not sure
how we need to handle. Basically input_mt_sync_frame triggers pointer
emulation, which is a problem for the Dualshock 4 since it already
provides an ABS_X / ABS_Y, so the pointer emulation affects the analog
stick values.

The hid-sony driver registers the touchpad through input_mt_init_slots
with the flags parameter '0', which was intended not to request such
emulation I think. I'm not too familiar with the other input code, but
should input_mt_sync_frame gate pointer emulation based on
INPUT_MT_POINTER / INPUT_MT_DIRECT flags or something like that?

Thanks,
Roderick

Re: [PATCH 5/5] HID: sony: Handle multiple touch events input record

[Benjamin Tissoires]

Hi Roderick,

[sorry for the late reply, I was on PTO until today]

On Oct 18 2016 or thereabouts, Roderick Colenbrander wrote:
> >
> > > Correct, the DS4 sends a touch history. The last element is the
> > > latest. An input report can contain multiple touch samples, because
> > > the device internally samples at a rate which is higher than at which
> > > it generates HID reports. On USB you can easily see multiple touch
> > > events and it is even easier on Bluetooth (especially if it is set to
> > > a low frequency). The extra history is important for gestures.
> >
> > OK, so if it's important, you need to actually send it by adding
> > input_mt_sync_frame() and input_sync() calls. Otherwise, the values will
> > just get mangled by the kernel in evdev and everything happens as if you
> > just sent the last pair in the history.
> >
> > The first 4 patches are IMO mergeable, but this one will need a little
> > bit more polish to actually forward the events.
> >
> > Cheers,
> > Benjamin
> >
> > >
> > > Thanks,
> > > Roderick
> 
> Hi Ben,
> 
> A kind of follow-up to this patch feedback which made it into the
> final patch. The recommendation was to use input_mt_sync_frame, which
> was a good recommendation. There is one oversight, which I'm not sure
> how we need to handle. Basically input_mt_sync_frame triggers pointer
> emulation, which is a problem for the Dualshock 4 since it already
> provides an ABS_X / ABS_Y, so the pointer emulation affects the analog
> stick values.

Ouch, sorry, I did not realize this :(

> 
> The hid-sony driver registers the touchpad through input_mt_init_slots
> with the flags parameter '0', which was intended not to request such
> emulation I think. I'm not too familiar with the other input code, but
> should input_mt_sync_frame gate pointer emulation based on
> INPUT_MT_POINTER / INPUT_MT_DIRECT flags or something like that?

I'd say so, though I need to check the other thread first.

Cheers,
Benjamin

> 
> Thanks,
> Roderick