[PATCH 1/2] HID: wacom: Add WACOM_DEVICETYPE_DIRECT for Cintiqs and similar

[PATCH 1/2] HID: wacom: Add WACOM_DEVICETYPE_DIRECT for Cintiqs and similar

[Jason Gerecke]

"Direct" input deviecs like Cintiqs and Tablet PCs set the INPUT_PROP_DIRECT
property to notify userspace that the sensor and screen are overlayed. This
information can also be useful elsewhere within the kernel driver, however,
so we introduce a new WACOM_DEVICETYPE_DIRECT that signals this to other
kernel code.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 drivers/hid/wacom_wac.c | 40 ++++++++++++++++++++++++----------------
 drivers/hid/wacom_wac.h |  1 +
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index e499cdb..2523a29 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1757,8 +1757,10 @@ void wacom_wac_usage_mapping(struct hid_device *hdev,
 {
 	struct wacom *wacom = hid_get_drvdata(hdev);
 	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+	struct wacom_features *features = &wacom_wac->features;
 
 	/* currently, only direct devices have proper hid report descriptors */
+	features->device_type |= WACOM_DEVICETYPE_DIRECT;
 	__set_bit(INPUT_PROP_DIRECT, wacom_wac->pen_input->propbit);
 	__set_bit(INPUT_PROP_DIRECT, wacom_wac->touch_input->propbit);
 
@@ -2465,6 +2467,19 @@ void wacom_setup_device_quirks(struct wacom *wacom)
 	if (features->type == REMOTE)
 		features->device_type = WACOM_DEVICETYPE_PAD;
 
+	if (features->type == PL          || features->type == DTU           ||
+	    features->type == DTUS        || features->type == DTUSX         ||
+	    features->type == WACOM_21UX2 || features->type == WACOM_22HD    ||
+	    features->type == DTK         || features->type == WACOM_24HD    ||
+	    features->type == WACOM_27QHD || features->type == CINTIQ_HYBRID ||
+	    features->type == CINTIQ_COMPANION_2 || features->type == CINTIQ ||
+	    features->type == WACOM_BEE   || features->type == WACOM_13HD    ||
+	    features->type == WACOM_24HDT || features->type == WACOM_27QHDT  ||
+	    features->type == TABLETPC    || features->type == TABLETPCE     ||
+	    features->type == TABLETPC2FG || features->type == MTSCREEN      ||
+	    features->type == MTTPC       || features->type == MTTPC_B)
+	    features->device_type |= WACOM_DEVICETYPE_DIRECT;
+
 	if (wacom->hdev->bus == BUS_BLUETOOTH)
 		features->quirks |= WACOM_QUIRK_BATTERY;
 
@@ -2516,6 +2531,10 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 	input_abs_set_res(input_dev, ABS_X, features->x_resolution);
 	input_abs_set_res(input_dev, ABS_Y, features->y_resolution);
 
+	if (features->device_type & WACOM_DEVICETYPE_DIRECT)
+		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
+	else
+		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
 
 	switch (features->type) {
 	case GRAPHIRE_BT:
@@ -2540,8 +2559,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 		__set_bit(BTN_TOOL_MOUSE, input_dev->keybit);
 		__set_bit(BTN_STYLUS, input_dev->keybit);
 		__set_bit(BTN_STYLUS2, input_dev->keybit);
-
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
 		break;
 
 	case WACOM_27QHD:
@@ -2556,7 +2573,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 	case CINTIQ_COMPANION_2:
 		input_set_abs_params(input_dev, ABS_Z, -900, 899, 0, 0);
 		input_abs_set_res(input_dev, ABS_Z, 287);
-		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
 		wacom_setup_cintiq(wacom_wac);
 		break;
 
@@ -2572,8 +2588,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 		/* fall through */
 
 	case INTUOS:
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
-
 		wacom_setup_intuos(wacom_wac);
 		break;
 
@@ -2583,8 +2597,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 	case INTUOSPL:
 	case INTUOS5S:
 	case INTUOSPS:
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
-
 		input_set_abs_params(input_dev, ABS_DISTANCE, 0,
 				      features->distance_max,
 				      features->distance_fuzz, 0);
@@ -2614,8 +2626,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 		__set_bit(BTN_TOOL_RUBBER, input_dev->keybit);
 		__set_bit(BTN_STYLUS, input_dev->keybit);
 		__set_bit(BTN_STYLUS2, input_dev->keybit);
-
-		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
 		break;
 
 	case PTU:
@@ -2626,16 +2636,12 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 		__set_bit(BTN_TOOL_PEN, input_dev->keybit);
 		__set_bit(BTN_TOOL_RUBBER, input_dev->keybit);
 		__set_bit(BTN_STYLUS, input_dev->keybit);
-
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
 		break;
 
 	case INTUOSHT:
 	case BAMBOO_PT:
 	case BAMBOO_PEN:
 	case INTUOSHT2:
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
-
 		if (features->type == INTUOSHT2) {
 			wacom_setup_basic_pro_pen(wacom_wac);
 		} else {
@@ -2693,6 +2699,11 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 				  features->y_resolution);
 	}
 
+	if (features->device_type & WACOM_DEVICETYPE_DIRECT)
+		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
+	else
+		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
+
 	switch (features->type) {
 	case INTUOS5:
 	case INTUOS5L:
@@ -2700,8 +2711,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 	case INTUOSPL:
 	case INTUOS5S:
 	case INTUOSPS:
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
-
 		input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, features->x_max, 0, 0);
 		input_set_abs_params(input_dev, ABS_MT_TOUCH_MINOR, 0, features->y_max, 0, 0);
 		input_mt_init_slots(input_dev, features->touch_max, INPUT_MT_POINTER);
@@ -2724,7 +2733,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 
 	case TABLETPC:
 	case TABLETPCE:
-		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
 		break;
 
 	case INTUOSHT:
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index 8a8974c..7ad6273 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -82,6 +82,7 @@
 #define WACOM_DEVICETYPE_TOUCH          0x0002
 #define WACOM_DEVICETYPE_PAD            0x0004
 #define WACOM_DEVICETYPE_WL_MONITOR     0x0008
+#define WACOM_DEVICETYPE_DIRECT         0x0010
 
 #define WACOM_VENDORDEFINED_PEN		0xff0d0001
 #define WACOM_G9_PAGE			0xff090000
-- 
2.9.0

[PATCH 2/2] HID: wacom: Replace 'oVid' and 'oPid' with heuristics

[Jason Gerecke]

The 'oVid' and 'oPid' variables used by wacom_are_sibling are a hacky
solution to the problem of the driver having few good heuristics to use
to determine if two devices should be considered siblings or not. This
commit replaces them with heuristics that leverage the information we
have available to determine if two devices are likely siblings or not.

Written out, the new heuristics are basically:

  * If a device with the same VID/PID as that being probed already exists,
    it should be preferentially checked for siblingship.

  * Two HID devices which have the same VID/PID are not siblings if they
    are not part of the same logical hardware device.

  * Two HID devices which have different VID/PIDs are not siblings if
    they have different parent (e.g. USB hub) devices.

  * Devices without the WACOM_DEVICETYPE_DIRECT flag set may only be
    siblings of devies with the same VID/PID (Wacom often splits their
    direct input tablets into two devices to make it easier to meet
    Microsoft's touchscreen requirements).

  * Two devices which have different "directness" are not siblings.

  * Two devices which do not serve complementary roles (i.e. pen/touch)
    are not siblings.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 drivers/hid/wacom_sys.c | 58 +++++++++++++++++++++++++++++++++++++++----------
 drivers/hid/wacom_wac.c | 41 ++++++++++++++--------------------
 drivers/hid/wacom_wac.h |  2 --
 3 files changed, 62 insertions(+), 39 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 4a0bb6f..a5bc038 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -532,31 +532,65 @@ static bool wacom_are_sibling(struct hid_device *hdev,
 {
 	struct wacom *wacom = hid_get_drvdata(hdev);
 	struct wacom_features *features = &wacom->wacom_wac.features;
-	int vid = features->oVid;
-	int pid = features->oPid;
-	int n1,n2;
+	struct wacom *sibling_wacom = hid_get_drvdata(sibling);
+	struct wacom_features *sibling_features = &sibling_wacom->wacom_wac.features;
+	int n1, n2;
 
-	if (vid == 0 && pid == 0) {
-		vid = hdev->vendor;
-		pid = hdev->product;
+	/* Compare the physical path. Require devices with the same
+	 * PID to share the same device, and devices with different
+	 * PIDs to share parent devices.
+	 */
+	if (hdev->vendor == sibling->vendor && hdev->product == sibling->product) {
+		n1 = strrchr(hdev->phys, '/') - hdev->phys;
+		n2 = strrchr(sibling->phys, '/') - sibling->phys;
+	}
+	else {
+		n1 = strrchr(hdev->phys, '.') - hdev->phys;
+		n2 = strrchr(sibling->phys, '.') - sibling->phys;
 	}
 
-	if (vid != sibling->vendor || pid != sibling->product)
+	if (n1 != n2 || n1 <= 0 || n2 <= 0)
 		return false;
 
-	/* Compare the physical path. */
-	n1 = strrchr(hdev->phys, '.') - hdev->phys;
-	n2 = strrchr(sibling->phys, '.') - sibling->phys;
-	if (n1 != n2 || n1 <= 0 || n2 <= 0)
+	if (strncmp(hdev->phys, sibling->phys, n1))
+		return false;
+
+	if (hdev->vendor != sibling->vendor || hdev->product != sibling->product) {
+		if(!(features->device_type & WACOM_DEVICETYPE_DIRECT))
+			return false;
+	}
+
+	if ((features->device_type & WACOM_DEVICETYPE_DIRECT) !=
+	    (sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
 		return false;
 
-	return !strncmp(hdev->phys, sibling->phys, n1);
+	if ((features->device_type & WACOM_DEVICETYPE_PEN) &&
+	    !(sibling_features->device_type & WACOM_DEVICETYPE_TOUCH))
+		return false;
+
+	if ((features->device_type & WACOM_DEVICETYPE_TOUCH) &&
+	    !(sibling_features->device_type & WACOM_DEVICETYPE_PEN))
+		return false;
+
+	return true;
 }
 
 static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev)
 {
 	struct wacom_hdev_data *data;
 
+	/* Try to find an already-probed interface from the same device */
+	list_for_each_entry(data, &wacom_udev_list, list) {
+		int n1, n2;
+		n1 = strrchr(hdev->phys, '/') - hdev->phys;
+		n2 = strrchr(data->dev->phys, '/') - data->dev->phys;
+		if (n1 != n2 || n1 <= 0 || n2 <= 0)
+			continue;
+		if (!strncmp(hdev->phys, data->dev->phys, n1))
+			return data;
+	}
+
+	/* Fallback to finding devices that appear to be "siblings" */
 	list_for_each_entry(data, &wacom_udev_list, list) {
 		if (wacom_are_sibling(hdev, data->dev)) {
 			kref_get(&data->kref);
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 2523a29..cb6fc63 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -3229,11 +3229,10 @@ static const struct wacom_features wacom_features_0xF4 =
 static const struct wacom_features wacom_features_0xF8 =
 	{ "Wacom Cintiq 24HD touch", 104080, 65200, 2047, 63, /* Pen */
 	  WACOM_24HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 16,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0xf6 };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0xF6 =
 	{ "Wacom Cintiq 24HD touch", .type = WACOM_24HDT, /* Touch */
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0xf8, .touch_max = 10,
+	  .touch_max = 10,
 	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
 static const struct wacom_features wacom_features_0x32A =
 	{ "Wacom Cintiq 27QHD", 119740, 67520, 2047, 63,
@@ -3242,11 +3241,10 @@ static const struct wacom_features wacom_features_0x32A =
 static const struct wacom_features wacom_features_0x32B =
 	{ "Wacom Cintiq 27QHD touch", 119740, 67520, 2047, 63,
 	  WACOM_27QHD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 0,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x32C };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x32C =
 	{ "Wacom Cintiq 27QHD touch", .type = WACOM_27QHDT,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x32B, .touch_max = 10 };
+	  .touch_max = 10 };
 static const struct wacom_features wacom_features_0x3F =
 	{ "Wacom Cintiq 21UX", 87200, 65600, 1023, 63,
 	  CINTIQ, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 8 };
@@ -3263,11 +3261,10 @@ static const struct wacom_features wacom_features_0x304 =
 static const struct wacom_features wacom_features_0x333 =
 	{ "Wacom Cintiq 13HD touch", 59152, 33448, 2047, 63,
 	  WACOM_13HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x335 };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x335 =
 	{ "Wacom Cintiq 13HD touch", .type = WACOM_24HDT, /* Touch */
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x333, .touch_max = 10,
+	  .touch_max = 10,
 	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
 static const struct wacom_features wacom_features_0xC7 =
 	{ "Wacom DTU1931", 37832, 30305, 511, 0,
@@ -3298,11 +3295,10 @@ static const struct wacom_features wacom_features_0x57 =
 static const struct wacom_features wacom_features_0x59 = /* Pen */
 	{ "Wacom DTH2242", 95640, 54060, 2047, 63,
 	  DTK, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 6,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5D };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x5D = /* Touch */
 	{ "Wacom DTH2242",       .type = WACOM_24HDT,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x59, .touch_max = 10,
+	  .touch_max = 10,
 	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
 static const struct wacom_features wacom_features_0xCC =
 	{ "Wacom Cintiq 21UX2", 86800, 65200, 2047, 63,
@@ -3315,11 +3311,10 @@ static const struct wacom_features wacom_features_0xFA =
 static const struct wacom_features wacom_features_0x5B =
 	{ "Wacom Cintiq 22HDT", 95440, 53860, 2047, 63,
 	  WACOM_22HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 18,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5e };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x5E =
 	{ "Wacom Cintiq 22HDT", .type = WACOM_24HDT,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5b, .touch_max = 10,
+	  .touch_max = 10,
 	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
 static const struct wacom_features wacom_features_0x90 =
 	{ "Wacom ISDv4 90", 26202, 16325, 255, 0,
@@ -3461,20 +3456,18 @@ static const struct wacom_features wacom_features_0x6004 =
 static const struct wacom_features wacom_features_0x307 =
 	{ "Wacom ISDv5 307", 59152, 33448, 2047, 63,
 	  CINTIQ_HYBRID, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x309 };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x309 =
 	{ "Wacom ISDv5 309", .type = WACOM_24HDT, /* Touch */
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x0307, .touch_max = 10,
+	  .touch_max = 10,
 	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
 static const struct wacom_features wacom_features_0x30A =
 	{ "Wacom ISDv5 30A", 59152, 33448, 2047, 63,
 	  CINTIQ_HYBRID, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x30C };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x30C =
 	{ "Wacom ISDv5 30C", .type = WACOM_24HDT, /* Touch */
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x30A, .touch_max = 10,
+	  .touch_max = 10,
 	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
 static const struct wacom_features wacom_features_0x318 =
 	{ "Wacom USB Bamboo PAD", 4095, 4095, /* Touch */
@@ -3485,11 +3478,9 @@ static const struct wacom_features wacom_features_0x319 =
 static const struct wacom_features wacom_features_0x325 =
 	{ "Wacom ISDv5 325", 59552, 33848, 2047, 63,
 	  CINTIQ_COMPANION_2, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 11,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x326 };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x326 = /* Touch */
-	{ "Wacom ISDv5 326", .type = HID_GENERIC, .oVid = USB_VENDOR_ID_WACOM,
-	  .oPid = 0x325 };
+	{ "Wacom ISDv5 326", .type = HID_GENERIC };
 static const struct wacom_features wacom_features_0x323 =
 	{ "Wacom Intuos P M", 21600, 13500, 1023, 31,
 	  INTUOSHT, WACOM_INTUOS_RES, WACOM_INTUOS_RES,
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index 7ad6273..a5bd05a 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -181,8 +181,6 @@ struct wacom_features {
 	int tilt_fuzz;
 	unsigned quirks;
 	unsigned touch_max;
-	int oVid;
-	int oPid;
 	int pktlen;
 	bool check_for_hid_type;
 	int hid_type;
-- 
2.9.0

Re: [PATCH 2/2] HID: wacom: Replace 'oVid' and 'oPid' with heuristics

[Benjamin Tissoires]

On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
> The 'oVid' and 'oPid' variables used by wacom_are_sibling are a hacky
> solution to the problem of the driver having few good heuristics to use
> to determine if two devices should be considered siblings or not. This
> commit replaces them with heuristics that leverage the information we
> have available to determine if two devices are likely siblings or not.

I agree it's nicer to have a better heuristic for sibling matching,
but I wonder if this heuristic is reliable enough when talking about
future devices. It might be, but I know from experience that the
firmware team can be very original and find a way that screws up us all
:/

Keeping the safety net of reducing the checks with ovid/opid might come
handy in the future.

> 
> Written out, the new heuristics are basically:
> 
>   * If a device with the same VID/PID as that being probed already exists,
>     it should be preferentially checked for siblingship.

That's assuming you don't have 2 27QHD connected as 2 monitors (if you
really have a lot of money to spend) :)

I think the code is OK, just not the comment above (mostly the
"preferentially" word itches me)

> 
>   * Two HID devices which have the same VID/PID are not siblings if they
>     are not part of the same logical hardware device.
> 
>   * Two HID devices which have different VID/PIDs are not siblings if
>     they have different parent (e.g. USB hub) devices.
> 
>   * Devices without the WACOM_DEVICETYPE_DIRECT flag set may only be
>     siblings of devies with the same VID/PID (Wacom often splits their
>     direct input tablets into two devices to make it easier to meet
>     Microsoft's touchscreen requirements).

That's a strong assumption on the future. If you are forced to use the
Precision Touchpad protocol for Intuos, this will just blow up.

> 
>   * Two devices which have different "directness" are not siblings.
> 
>   * Two devices which do not serve complementary roles (i.e. pen/touch)
>     are not siblings.

I think it would be useful to have these write outs as comments in the
code. It's quite tricky to understand the actual code without these
explanations.

> 
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> ---
>  drivers/hid/wacom_sys.c | 58 +++++++++++++++++++++++++++++++++++++++----------
>  drivers/hid/wacom_wac.c | 41 ++++++++++++++--------------------
>  drivers/hid/wacom_wac.h |  2 --
>  3 files changed, 62 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 4a0bb6f..a5bc038 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -532,31 +532,65 @@ static bool wacom_are_sibling(struct hid_device *hdev,
>  {
>  	struct wacom *wacom = hid_get_drvdata(hdev);
>  	struct wacom_features *features = &wacom->wacom_wac.features;
> -	int vid = features->oVid;
> -	int pid = features->oPid;
> -	int n1,n2;
> +	struct wacom *sibling_wacom = hid_get_drvdata(sibling);
> +	struct wacom_features *sibling_features = &sibling_wacom->wacom_wac.features;
> +	int n1, n2;
>  
> -	if (vid == 0 && pid == 0) {
> -		vid = hdev->vendor;
> -		pid = hdev->product;
> +	/* Compare the physical path. Require devices with the same
> +	 * PID to share the same device, and devices with different
> +	 * PIDs to share parent devices.
> +	 */

I stumbled this morning upon:
http://www.theregister.co.uk/2016/07/11/linus_torvalds_in_sweary_rant_about_punctuation_in_kernel_comments/

Please make sure to follow the comments style guidelines :)

> +	if (hdev->vendor == sibling->vendor && hdev->product == sibling->product) {
> +		n1 = strrchr(hdev->phys, '/') - hdev->phys;
> +		n2 = strrchr(sibling->phys, '/') - sibling->phys;
> +	}
> +	else {
> +		n1 = strrchr(hdev->phys, '.') - hdev->phys;
> +		n2 = strrchr(sibling->phys, '.') - sibling->phys;
>  	}
>  
> -	if (vid != sibling->vendor || pid != sibling->product)
> +	if (n1 != n2 || n1 <= 0 || n2 <= 0)
>  		return false;
>  
> -	/* Compare the physical path. */
> -	n1 = strrchr(hdev->phys, '.') - hdev->phys;
> -	n2 = strrchr(sibling->phys, '.') - sibling->phys;
> -	if (n1 != n2 || n1 <= 0 || n2 <= 0)
> +	if (strncmp(hdev->phys, sibling->phys, n1))
> +		return false;
> +
> +	if (hdev->vendor != sibling->vendor || hdev->product != sibling->product) {
> +		if(!(features->device_type & WACOM_DEVICETYPE_DIRECT))
> +			return false;
> +	}

As mentioned in the commit log, I am not sure it's such a good idea to
have this check. Does it really remove a false positive or can this just
be considered as an extra check that can be safely removed?

> +
> +	if ((features->device_type & WACOM_DEVICETYPE_DIRECT) !=
> +	    (sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
>  		return false;

unless I am mistaken, you might as well need {if indirect and sibling
is not indirect, than false }.

>  
> -	return !strncmp(hdev->phys, sibling->phys, n1);
> +	if ((features->device_type & WACOM_DEVICETYPE_PEN) &&
> +	    !(sibling_features->device_type & WACOM_DEVICETYPE_TOUCH))
> +		return false;
> +
> +	if ((features->device_type & WACOM_DEVICETYPE_TOUCH) &&
> +	    !(sibling_features->device_type & WACOM_DEVICETYPE_PEN))
> +		return false;

I think it would be better to have those last 3 (plus mine) tests at the
beginning, before doing string lookouts. They seem to be the most
reliable ones and able to exclude a lot of false positive.

> +
> +	return true;
>  }
>  
>  static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev)
>  {
>  	struct wacom_hdev_data *data;
>  
> +	/* Try to find an already-probed interface from the same device */
> +	list_for_each_entry(data, &wacom_udev_list, list) {
> +		int n1, n2;
> +		n1 = strrchr(hdev->phys, '/') - hdev->phys;
> +		n2 = strrchr(data->dev->phys, '/') - data->dev->phys;
> +		if (n1 != n2 || n1 <= 0 || n2 <= 0)
> +			continue;
> +		if (!strncmp(hdev->phys, data->dev->phys, n1))
> +			return data;
> +	}

I can't see the benefit of having this here, while it seems to be
already tested in wacom_are_sibling().

Also, if there is a need for it, there is a common pattern used 3 times
here:
        int n1, n2;
        n1 = strrchr(hdev->phys, separator) - hdev->phys;
        n2 = strrchr(other->dev->phys, separator) - other->dev->phys;
        if (n1 != n2 || n1 <= 0 || n2 <= 0)
                 continue;
        return !strncmp(hdev->phys, other->dev->phys, n1);

It would make sense to put a name on it and have a separate function.

Cheers,
Benjamin

> +
> +	/* Fallback to finding devices that appear to be "siblings" */
>  	list_for_each_entry(data, &wacom_udev_list, list) {
>  		if (wacom_are_sibling(hdev, data->dev)) {
>  			kref_get(&data->kref);
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index 2523a29..cb6fc63 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -3229,11 +3229,10 @@ static const struct wacom_features wacom_features_0xF4 =
>  static const struct wacom_features wacom_features_0xF8 =
>  	{ "Wacom Cintiq 24HD touch", 104080, 65200, 2047, 63, /* Pen */
>  	  WACOM_24HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 16,
> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0xf6 };
> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>  static const struct wacom_features wacom_features_0xF6 =
>  	{ "Wacom Cintiq 24HD touch", .type = WACOM_24HDT, /* Touch */
> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0xf8, .touch_max = 10,
> +	  .touch_max = 10,
>  	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
>  static const struct wacom_features wacom_features_0x32A =
>  	{ "Wacom Cintiq 27QHD", 119740, 67520, 2047, 63,
> @@ -3242,11 +3241,10 @@ static const struct wacom_features wacom_features_0x32A =
>  static const struct wacom_features wacom_features_0x32B =
>  	{ "Wacom Cintiq 27QHD touch", 119740, 67520, 2047, 63,
>  	  WACOM_27QHD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 0,
> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x32C };
> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>  static const struct wacom_features wacom_features_0x32C =
>  	{ "Wacom Cintiq 27QHD touch", .type = WACOM_27QHDT,
> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x32B, .touch_max = 10 };
> +	  .touch_max = 10 };
>  static const struct wacom_features wacom_features_0x3F =
>  	{ "Wacom Cintiq 21UX", 87200, 65600, 1023, 63,
>  	  CINTIQ, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 8 };
> @@ -3263,11 +3261,10 @@ static const struct wacom_features wacom_features_0x304 =
>  static const struct wacom_features wacom_features_0x333 =
>  	{ "Wacom Cintiq 13HD touch", 59152, 33448, 2047, 63,
>  	  WACOM_13HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x335 };
> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>  static const struct wacom_features wacom_features_0x335 =
>  	{ "Wacom Cintiq 13HD touch", .type = WACOM_24HDT, /* Touch */
> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x333, .touch_max = 10,
> +	  .touch_max = 10,
>  	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
>  static const struct wacom_features wacom_features_0xC7 =
>  	{ "Wacom DTU1931", 37832, 30305, 511, 0,
> @@ -3298,11 +3295,10 @@ static const struct wacom_features wacom_features_0x57 =
>  static const struct wacom_features wacom_features_0x59 = /* Pen */
>  	{ "Wacom DTH2242", 95640, 54060, 2047, 63,
>  	  DTK, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 6,
> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5D };
> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>  static const struct wacom_features wacom_features_0x5D = /* Touch */
>  	{ "Wacom DTH2242",       .type = WACOM_24HDT,
> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x59, .touch_max = 10,
> +	  .touch_max = 10,
>  	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
>  static const struct wacom_features wacom_features_0xCC =
>  	{ "Wacom Cintiq 21UX2", 86800, 65200, 2047, 63,
> @@ -3315,11 +3311,10 @@ static const struct wacom_features wacom_features_0xFA =
>  static const struct wacom_features wacom_features_0x5B =
>  	{ "Wacom Cintiq 22HDT", 95440, 53860, 2047, 63,
>  	  WACOM_22HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 18,
> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5e };
> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>  static const struct wacom_features wacom_features_0x5E =
>  	{ "Wacom Cintiq 22HDT", .type = WACOM_24HDT,
> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5b, .touch_max = 10,
> +	  .touch_max = 10,
>  	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
>  static const struct wacom_features wacom_features_0x90 =
>  	{ "Wacom ISDv4 90", 26202, 16325, 255, 0,
> @@ -3461,20 +3456,18 @@ static const struct wacom_features wacom_features_0x6004 =
>  static const struct wacom_features wacom_features_0x307 =
>  	{ "Wacom ISDv5 307", 59152, 33448, 2047, 63,
>  	  CINTIQ_HYBRID, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x309 };
> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>  static const struct wacom_features wacom_features_0x309 =
>  	{ "Wacom ISDv5 309", .type = WACOM_24HDT, /* Touch */
> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x0307, .touch_max = 10,
> +	  .touch_max = 10,
>  	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
>  static const struct wacom_features wacom_features_0x30A =
>  	{ "Wacom ISDv5 30A", 59152, 33448, 2047, 63,
>  	  CINTIQ_HYBRID, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x30C };
> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>  static const struct wacom_features wacom_features_0x30C =
>  	{ "Wacom ISDv5 30C", .type = WACOM_24HDT, /* Touch */
> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x30A, .touch_max = 10,
> +	  .touch_max = 10,
>  	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
>  static const struct wacom_features wacom_features_0x318 =
>  	{ "Wacom USB Bamboo PAD", 4095, 4095, /* Touch */
> @@ -3485,11 +3478,9 @@ static const struct wacom_features wacom_features_0x319 =
>  static const struct wacom_features wacom_features_0x325 =
>  	{ "Wacom ISDv5 325", 59552, 33848, 2047, 63,
>  	  CINTIQ_COMPANION_2, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 11,
> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x326 };
> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>  static const struct wacom_features wacom_features_0x326 = /* Touch */
> -	{ "Wacom ISDv5 326", .type = HID_GENERIC, .oVid = USB_VENDOR_ID_WACOM,
> -	  .oPid = 0x325 };
> +	{ "Wacom ISDv5 326", .type = HID_GENERIC };
>  static const struct wacom_features wacom_features_0x323 =
>  	{ "Wacom Intuos P M", 21600, 13500, 1023, 31,
>  	  INTUOSHT, WACOM_INTUOS_RES, WACOM_INTUOS_RES,
> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
> index 7ad6273..a5bd05a 100644
> --- a/drivers/hid/wacom_wac.h
> +++ b/drivers/hid/wacom_wac.h
> @@ -181,8 +181,6 @@ struct wacom_features {
>  	int tilt_fuzz;
>  	unsigned quirks;
>  	unsigned touch_max;
> -	int oVid;
> -	int oPid;
>  	int pktlen;
>  	bool check_for_hid_type;
>  	int hid_type;
> -- 
> 2.9.0
> 

Re: [PATCH 2/2] HID: wacom: Replace 'oVid' and 'oPid' with heuristics

[Jason Gerecke]

On 07/12/2016 02:05 AM, Benjamin Tissoires wrote:
> On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
>> The 'oVid' and 'oPid' variables used by wacom_are_sibling are a hacky
>> solution to the problem of the driver having few good heuristics to use
>> to determine if two devices should be considered siblings or not. This
>> commit replaces them with heuristics that leverage the information we
>> have available to determine if two devices are likely siblings or not.
> 
> I agree it's nicer to have a better heuristic for sibling matching,
> but I wonder if this heuristic is reliable enough when talking about
> future devices. It might be, but I know from experience that the
> firmware team can be very original and find a way that screws up us all
> :/
> 
> Keeping the safety net of reducing the checks with ovid/opid might come
> handy in the future.
> 

The biggest problem with oVid/oPid is that they're kinda antithetical to
the HID_GENERIC codepath. If a device is split between two PIDs then
arbitration is broken for any kernel that doesn't include specific
support for the device.

I agree that heuristics aren't as confidence-inspiring as explicit
notation, but if they can be made to work well enough then I'd prefer
using them. Either way, I suppose as userspace starts taking over
arbitration duties it will become a more and more moot issue.

>>
>> Written out, the new heuristics are basically:
>>
>>   * If a device with the same VID/PID as that being probed already exists,
>>     it should be preferentially checked for siblingship.
> 
> That's assuming you don't have 2 27QHD connected as 2 monitors (if you
> really have a lot of money to spend) :)
> 
> I think the code is OK, just not the comment above (mostly the
> "preferentially" word itches me)
> 

I'll try to come up with better / more clear wording (see my later
comments on the "Try to find an already-probed interface from the same
device" hunk for more detail).

>>
>>   * Two HID devices which have the same VID/PID are not siblings if they
>>     are not part of the same logical hardware device.
>>
>>   * Two HID devices which have different VID/PIDs are not siblings if
>>     they have different parent (e.g. USB hub) devices.
>>
>>   * Devices without the WACOM_DEVICETYPE_DIRECT flag set may only be
>>     siblings of devies with the same VID/PID (Wacom often splits their
>>     direct input tablets into two devices to make it easier to meet
>>     Microsoft's touchscreen requirements).
> 
> That's a strong assumption on the future. If you are forced to use the
> Precision Touchpad protocol for Intuos, this will just blow up.
> 

I originally didn't include this condition in my checks since I was
similarly wary. Leaving it out does open two small corner cases though.

1) A pen-only tablet connected to the same hub as a touch-only tablet.
Touch-only Wacom tablets aren't particularly common and it would
be a little strange to have one paired with a pen-only tablet, but it
could conceivably happen. Although pairing the devices shouldn't
normally be an issue since a user isn't likely to use both
simultaneously, it might cause problems for multi-seat setups.

2) A pen-only tablet connected to the same hub as a pen-and-touch tablet
which has the *touch* interface probed first. As far as I'm aware, there
aren't any Wacom devices that have the USB interfaces ordered
touch-then-pen, but as you point out, who knows what those tricksy
firmware engineers will do in the future to ruin your day.

I'm open to leaving the check in or out depending on your feelings. If
you've got thoughts on how to close these corner cases as well, I'm all
ears :)

>>
>>   * Two devices which have different "directness" are not siblings.
>>
>>   * Two devices which do not serve complementary roles (i.e. pen/touch)
>>     are not siblings.
> 
> I think it would be useful to have these write outs as comments in the
> code. It's quite tricky to understand the actual code without these
> explanations.
> 

Ack.

>>
>> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
>> ---
>>  drivers/hid/wacom_sys.c | 58 +++++++++++++++++++++++++++++++++++++++----------
>>  drivers/hid/wacom_wac.c | 41 ++++++++++++++--------------------
>>  drivers/hid/wacom_wac.h |  2 --
>>  3 files changed, 62 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
>> index 4a0bb6f..a5bc038 100644
>> --- a/drivers/hid/wacom_sys.c
>> +++ b/drivers/hid/wacom_sys.c
>> @@ -532,31 +532,65 @@ static bool wacom_are_sibling(struct hid_device *hdev,
>>  {
>>  	struct wacom *wacom = hid_get_drvdata(hdev);
>>  	struct wacom_features *features = &wacom->wacom_wac.features;
>> -	int vid = features->oVid;
>> -	int pid = features->oPid;
>> -	int n1,n2;
>> +	struct wacom *sibling_wacom = hid_get_drvdata(sibling);
>> +	struct wacom_features *sibling_features = &sibling_wacom->wacom_wac.features;
>> +	int n1, n2;
>>  
>> -	if (vid == 0 && pid == 0) {
>> -		vid = hdev->vendor;
>> -		pid = hdev->product;
>> +	/* Compare the physical path. Require devices with the same
>> +	 * PID to share the same device, and devices with different
>> +	 * PIDs to share parent devices.
>> +	 */
> 
> I stumbled this morning upon:
> http://www.theregister.co.uk/2016/07/11/linus_torvalds_in_sweary_rant_about_punctuation_in_kernel_comments/
> 
> Please make sure to follow the comments style guidelines :)
> 

Whatever makes the style gods happy :)

>> +	if (hdev->vendor == sibling->vendor && hdev->product == sibling->product) {
>> +		n1 = strrchr(hdev->phys, '/') - hdev->phys;
>> +		n2 = strrchr(sibling->phys, '/') - sibling->phys;
>> +	}
>> +	else {
>> +		n1 = strrchr(hdev->phys, '.') - hdev->phys;
>> +		n2 = strrchr(sibling->phys, '.') - sibling->phys;
>>  	}
>>  
>> -	if (vid != sibling->vendor || pid != sibling->product)
>> +	if (n1 != n2 || n1 <= 0 || n2 <= 0)
>>  		return false;
>>  
>> -	/* Compare the physical path. */
>> -	n1 = strrchr(hdev->phys, '.') - hdev->phys;
>> -	n2 = strrchr(sibling->phys, '.') - sibling->phys;
>> -	if (n1 != n2 || n1 <= 0 || n2 <= 0)
>> +	if (strncmp(hdev->phys, sibling->phys, n1))
>> +		return false;
>> +
>> +	if (hdev->vendor != sibling->vendor || hdev->product != sibling->product) {
>> +		if(!(features->device_type & WACOM_DEVICETYPE_DIRECT))
>> +			return false;
>> +	}
> 
> As mentioned in the commit log, I am not sure it's such a good idea to
> have this check. Does it really remove a false positive or can this just
> be considered as an extra check that can be safely removed?
> 

See comment above.

>> +
>> +	if ((features->device_type & WACOM_DEVICETYPE_DIRECT) !=
>> +	    (sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
>>  		return false;
> 
> unless I am mistaken, you might as well need {if indirect and sibling
> is not indirect, than false }.
> 

That case should be covered since we're comparing the actual values of
each device's "direct" flag for equality (direct/indirect and
indirect/direct will return false).

If its not immediately clear though, I could decompose it into
independent checks for each case. E.g.:

  if ((features->device_type & WACOM_DEVICETYPE_DIRECT) &&
      !(sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
          return false;

  if (!(features->device_type & WACOM_DEVICETYPE_DIRECT) &&
       (sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
          return false;

>>  
>> -	return !strncmp(hdev->phys, sibling->phys, n1);
>> +	if ((features->device_type & WACOM_DEVICETYPE_PEN) &&
>> +	    !(sibling_features->device_type & WACOM_DEVICETYPE_TOUCH))
>> +		return false;
>> +
>> +	if ((features->device_type & WACOM_DEVICETYPE_TOUCH) &&
>> +	    !(sibling_features->device_type & WACOM_DEVICETYPE_PEN))
>> +		return false;
> 
> I think it would be better to have those last 3 (plus mine) tests at the
> beginning, before doing string lookouts. They seem to be the most
> reliable ones and able to exclude a lot of false positive.
> 

Reasonable enough.

>> +
>> +	return true;
>>  }
>>  
>>  static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev)
>>  {
>>  	struct wacom_hdev_data *data;
>>  
>> +	/* Try to find an already-probed interface from the same device */
>> +	list_for_each_entry(data, &wacom_udev_list, list) {
>> +		int n1, n2;
>> +		n1 = strrchr(hdev->phys, '/') - hdev->phys;
>> +		n2 = strrchr(data->dev->phys, '/') - data->dev->phys;
>> +		if (n1 != n2 || n1 <= 0 || n2 <= 0)
>> +			continue;
>> +		if (!strncmp(hdev->phys, data->dev->phys, n1))
>> +			return data;
>> +	}
> 
> I can't see the benefit of having this here, while it seems to be
> already tested in wacom_are_sibling().
> 

There's a subtle issue with not performing this search before proceeding
to trying to pair arbitrary devices in wacom_are_sibling. Imagine that
you've already got a pen-only device connected to your system. Now,
connect a (non-split) pen+touch device to the same hub. When the touch
interface is probed, the first device checked its checked against in
wacom_are_sibling is that pen-only device, and provided it passes all
the checks then it will be incorrectly paired with it.

This is actually probably a leftover from before I added the requirement
that only direct devices can have different PIDs and might not be
strictly necessary in the patch's current state, but I'd feel better
keeping it in either way.

> Also, if there is a need for it, there is a common pattern used 3 times
> here:
>         int n1, n2;
>         n1 = strrchr(hdev->phys, separator) - hdev->phys;
>         n2 = strrchr(other->dev->phys, separator) - other->dev->phys;
>         if (n1 != n2 || n1 <= 0 || n2 <= 0)
>                  continue;
>         return !strncmp(hdev->phys, other->dev->phys, n1);
> 
> It would make sense to put a name on it and have a separate function.
> 
> Cheers,
> Benjamin
> 

Good call, if the block remains necessary.

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....

>> +
>> +	/* Fallback to finding devices that appear to be "siblings" */
>>  	list_for_each_entry(data, &wacom_udev_list, list) {
>>  		if (wacom_are_sibling(hdev, data->dev)) {
>>  			kref_get(&data->kref);
>> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
>> index 2523a29..cb6fc63 100644
>> --- a/drivers/hid/wacom_wac.c
>> +++ b/drivers/hid/wacom_wac.c
>> @@ -3229,11 +3229,10 @@ static const struct wacom_features wacom_features_0xF4 =
>>  static const struct wacom_features wacom_features_0xF8 =
>>  	{ "Wacom Cintiq 24HD touch", 104080, 65200, 2047, 63, /* Pen */
>>  	  WACOM_24HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 16,
>> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
>> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0xf6 };
>> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>>  static const struct wacom_features wacom_features_0xF6 =
>>  	{ "Wacom Cintiq 24HD touch", .type = WACOM_24HDT, /* Touch */
>> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0xf8, .touch_max = 10,
>> +	  .touch_max = 10,
>>  	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
>>  static const struct wacom_features wacom_features_0x32A =
>>  	{ "Wacom Cintiq 27QHD", 119740, 67520, 2047, 63,
>> @@ -3242,11 +3241,10 @@ static const struct wacom_features wacom_features_0x32A =
>>  static const struct wacom_features wacom_features_0x32B =
>>  	{ "Wacom Cintiq 27QHD touch", 119740, 67520, 2047, 63,
>>  	  WACOM_27QHD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 0,
>> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
>> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x32C };
>> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>>  static const struct wacom_features wacom_features_0x32C =
>>  	{ "Wacom Cintiq 27QHD touch", .type = WACOM_27QHDT,
>> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x32B, .touch_max = 10 };
>> +	  .touch_max = 10 };
>>  static const struct wacom_features wacom_features_0x3F =
>>  	{ "Wacom Cintiq 21UX", 87200, 65600, 1023, 63,
>>  	  CINTIQ, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 8 };
>> @@ -3263,11 +3261,10 @@ static const struct wacom_features wacom_features_0x304 =
>>  static const struct wacom_features wacom_features_0x333 =
>>  	{ "Wacom Cintiq 13HD touch", 59152, 33448, 2047, 63,
>>  	  WACOM_13HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
>> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
>> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x335 };
>> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>>  static const struct wacom_features wacom_features_0x335 =
>>  	{ "Wacom Cintiq 13HD touch", .type = WACOM_24HDT, /* Touch */
>> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x333, .touch_max = 10,
>> +	  .touch_max = 10,
>>  	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
>>  static const struct wacom_features wacom_features_0xC7 =
>>  	{ "Wacom DTU1931", 37832, 30305, 511, 0,
>> @@ -3298,11 +3295,10 @@ static const struct wacom_features wacom_features_0x57 =
>>  static const struct wacom_features wacom_features_0x59 = /* Pen */
>>  	{ "Wacom DTH2242", 95640, 54060, 2047, 63,
>>  	  DTK, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 6,
>> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
>> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5D };
>> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>>  static const struct wacom_features wacom_features_0x5D = /* Touch */
>>  	{ "Wacom DTH2242",       .type = WACOM_24HDT,
>> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x59, .touch_max = 10,
>> +	  .touch_max = 10,
>>  	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
>>  static const struct wacom_features wacom_features_0xCC =
>>  	{ "Wacom Cintiq 21UX2", 86800, 65200, 2047, 63,
>> @@ -3315,11 +3311,10 @@ static const struct wacom_features wacom_features_0xFA =
>>  static const struct wacom_features wacom_features_0x5B =
>>  	{ "Wacom Cintiq 22HDT", 95440, 53860, 2047, 63,
>>  	  WACOM_22HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 18,
>> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
>> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5e };
>> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>>  static const struct wacom_features wacom_features_0x5E =
>>  	{ "Wacom Cintiq 22HDT", .type = WACOM_24HDT,
>> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5b, .touch_max = 10,
>> +	  .touch_max = 10,
>>  	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
>>  static const struct wacom_features wacom_features_0x90 =
>>  	{ "Wacom ISDv4 90", 26202, 16325, 255, 0,
>> @@ -3461,20 +3456,18 @@ static const struct wacom_features wacom_features_0x6004 =
>>  static const struct wacom_features wacom_features_0x307 =
>>  	{ "Wacom ISDv5 307", 59152, 33448, 2047, 63,
>>  	  CINTIQ_HYBRID, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
>> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
>> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x309 };
>> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>>  static const struct wacom_features wacom_features_0x309 =
>>  	{ "Wacom ISDv5 309", .type = WACOM_24HDT, /* Touch */
>> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x0307, .touch_max = 10,
>> +	  .touch_max = 10,
>>  	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
>>  static const struct wacom_features wacom_features_0x30A =
>>  	{ "Wacom ISDv5 30A", 59152, 33448, 2047, 63,
>>  	  CINTIQ_HYBRID, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
>> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
>> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x30C };
>> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>>  static const struct wacom_features wacom_features_0x30C =
>>  	{ "Wacom ISDv5 30C", .type = WACOM_24HDT, /* Touch */
>> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x30A, .touch_max = 10,
>> +	  .touch_max = 10,
>>  	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
>>  static const struct wacom_features wacom_features_0x318 =
>>  	{ "Wacom USB Bamboo PAD", 4095, 4095, /* Touch */
>> @@ -3485,11 +3478,9 @@ static const struct wacom_features wacom_features_0x319 =
>>  static const struct wacom_features wacom_features_0x325 =
>>  	{ "Wacom ISDv5 325", 59552, 33848, 2047, 63,
>>  	  CINTIQ_COMPANION_2, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 11,
>> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
>> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x326 };
>> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>>  static const struct wacom_features wacom_features_0x326 = /* Touch */
>> -	{ "Wacom ISDv5 326", .type = HID_GENERIC, .oVid = USB_VENDOR_ID_WACOM,
>> -	  .oPid = 0x325 };
>> +	{ "Wacom ISDv5 326", .type = HID_GENERIC };
>>  static const struct wacom_features wacom_features_0x323 =
>>  	{ "Wacom Intuos P M", 21600, 13500, 1023, 31,
>>  	  INTUOSHT, WACOM_INTUOS_RES, WACOM_INTUOS_RES,
>> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
>> index 7ad6273..a5bd05a 100644
>> --- a/drivers/hid/wacom_wac.h
>> +++ b/drivers/hid/wacom_wac.h
>> @@ -181,8 +181,6 @@ struct wacom_features {
>>  	int tilt_fuzz;
>>  	unsigned quirks;
>>  	unsigned touch_max;
>> -	int oVid;
>> -	int oPid;
>>  	int pktlen;
>>  	bool check_for_hid_type;
>>  	int hid_type;
>> -- 
>> 2.9.0
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] HID: wacom: Replace 'oVid' and 'oPid' with heuristics

[Benjamin Tissoires]

Hi Jason,

[I've seen v2 and v3 but the discussion is mainly going there, so
pulling this one out of the archives]

On Jul 20 2016 or thereabouts, Jason Gerecke wrote:
> On 07/12/2016 02:05 AM, Benjamin Tissoires wrote:
> > On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
> >> The 'oVid' and 'oPid' variables used by wacom_are_sibling are a hacky
> >> solution to the problem of the driver having few good heuristics to use
> >> to determine if two devices should be considered siblings or not. This
> >> commit replaces them with heuristics that leverage the information we
> >> have available to determine if two devices are likely siblings or not.
> > 
> > I agree it's nicer to have a better heuristic for sibling matching,
> > but I wonder if this heuristic is reliable enough when talking about
> > future devices. It might be, but I know from experience that the
> > firmware team can be very original and find a way that screws up us all
> > :/
> > 
> > Keeping the safety net of reducing the checks with ovid/opid might come
> > handy in the future.
> > 
> 
> The biggest problem with oVid/oPid is that they're kinda antithetical to
> the HID_GENERIC codepath. If a device is split between two PIDs then
> arbitration is broken for any kernel that doesn't include specific
> support for the device.

Well, we currently already have different paths for HID_GENERIC and 
other various protocols. Why is that a problem to have heuristics for
HID_GENERIC and ovid/opid for those we need to have special handling?

> 
> I agree that heuristics aren't as confidence-inspiring as explicit
> notation, but if they can be made to work well enough then I'd prefer
> using them. Either way, I suppose as userspace starts taking over
> arbitration duties it will become a more and more moot issue.

Yep, but currently the patch might link 2 devices that were not bound
together before, so I still think ovid/opid is the best way for the non
generic devices. For generic devices (future I think) we can always ask
people to use userspace touch arbitration.

> 
> >>
> >> Written out, the new heuristics are basically:
> >>
> >>   * If a device with the same VID/PID as that being probed already exists,
> >>     it should be preferentially checked for siblingship.
> > 
> > That's assuming you don't have 2 27QHD connected as 2 monitors (if you
> > really have a lot of money to spend) :)
> > 
> > I think the code is OK, just not the comment above (mostly the
> > "preferentially" word itches me)
> > 
> 
> I'll try to come up with better / more clear wording (see my later
> comments on the "Try to find an already-probed interface from the same
> device" hunk for more detail).

Thanks for the changes in v2/3

> 
> >>
> >>   * Two HID devices which have the same VID/PID are not siblings if they
> >>     are not part of the same logical hardware device.
> >>
> >>   * Two HID devices which have different VID/PIDs are not siblings if
> >>     they have different parent (e.g. USB hub) devices.
> >>
> >>   * Devices without the WACOM_DEVICETYPE_DIRECT flag set may only be
> >>     siblings of devies with the same VID/PID (Wacom often splits their
> >>     direct input tablets into two devices to make it easier to meet
> >>     Microsoft's touchscreen requirements).
> > 
> > That's a strong assumption on the future. If you are forced to use the
> > Precision Touchpad protocol for Intuos, this will just blow up.
> > 
> 
> I originally didn't include this condition in my checks since I was
> similarly wary. Leaving it out does open two small corner cases though.
> 
> 1) A pen-only tablet connected to the same hub as a touch-only tablet.
> Touch-only Wacom tablets aren't particularly common and it would
> be a little strange to have one paired with a pen-only tablet, but it
> could conceivably happen. Although pairing the devices shouldn't
> normally be an issue since a user isn't likely to use both
> simultaneously, it might cause problems for multi-seat setups.

Yes, multi-seats is one big issue here.

> 
> 2) A pen-only tablet connected to the same hub as a pen-and-touch tablet
> which has the *touch* interface probed first. As far as I'm aware, there
> aren't any Wacom devices that have the USB interfaces ordered
> touch-then-pen, but as you point out, who knows what those tricksy
> firmware engineers will do in the future to ruin your day.

And the winner is... the Cintiq 13HDT -> touch interface and then Pen :)

Which means with the new patch, connecting a Cintiq 21UX (1 not 2) which
is a direct device which I assume doesn't have an internal hub, you end
up binding the 21UX pen with the 13HD touch, and things gets nasty for
the 13HD pen interface.

> 
> I'm open to leaving the check in or out depending on your feelings. If
> you've got thoughts on how to close these corner cases as well, I'm all
> ears :)

I really think the ovid/opid approach solves most of the issues with the
current hardware. You are concerned about future hardware that will be
handled by HID_GENERIC. Why not just adding a special case for
HID_GENERIC that uses your heuristics?
In userspace I think we will still have the ovid/opid approach in
libwacom because it restricts the amount of combinations by a very good
factor.

If the kernel with the new heuristics (HID_GENERIC only) fails, we will
be able to tell users to switch to userspace touch arbitration.

/me turns in circle a little, but how does that sounds?

Cheers,
Benjamin

> 
> >>
> >>   * Two devices which have different "directness" are not siblings.
> >>
> >>   * Two devices which do not serve complementary roles (i.e. pen/touch)
> >>     are not siblings.
> > 
> > I think it would be useful to have these write outs as comments in the
> > code. It's quite tricky to understand the actual code without these
> > explanations.
> > 
> 
> Ack.
> 
> >>
> >> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> >> ---
> >>  drivers/hid/wacom_sys.c | 58 +++++++++++++++++++++++++++++++++++++++----------
> >>  drivers/hid/wacom_wac.c | 41 ++++++++++++++--------------------
> >>  drivers/hid/wacom_wac.h |  2 --
> >>  3 files changed, 62 insertions(+), 39 deletions(-)
> >>
> >> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> >> index 4a0bb6f..a5bc038 100644
> >> --- a/drivers/hid/wacom_sys.c
> >> +++ b/drivers/hid/wacom_sys.c
> >> @@ -532,31 +532,65 @@ static bool wacom_are_sibling(struct hid_device *hdev,
> >>  {
> >>  	struct wacom *wacom = hid_get_drvdata(hdev);
> >>  	struct wacom_features *features = &wacom->wacom_wac.features;
> >> -	int vid = features->oVid;
> >> -	int pid = features->oPid;
> >> -	int n1,n2;
> >> +	struct wacom *sibling_wacom = hid_get_drvdata(sibling);
> >> +	struct wacom_features *sibling_features = &sibling_wacom->wacom_wac.features;
> >> +	int n1, n2;
> >>  
> >> -	if (vid == 0 && pid == 0) {
> >> -		vid = hdev->vendor;
> >> -		pid = hdev->product;
> >> +	/* Compare the physical path. Require devices with the same
> >> +	 * PID to share the same device, and devices with different
> >> +	 * PIDs to share parent devices.
> >> +	 */
> > 
> > I stumbled this morning upon:
> > http://www.theregister.co.uk/2016/07/11/linus_torvalds_in_sweary_rant_about_punctuation_in_kernel_comments/
> > 
> > Please make sure to follow the comments style guidelines :)
> > 
> 
> Whatever makes the style gods happy :)
> 
> >> +	if (hdev->vendor == sibling->vendor && hdev->product == sibling->product) {
> >> +		n1 = strrchr(hdev->phys, '/') - hdev->phys;
> >> +		n2 = strrchr(sibling->phys, '/') - sibling->phys;
> >> +	}
> >> +	else {
> >> +		n1 = strrchr(hdev->phys, '.') - hdev->phys;
> >> +		n2 = strrchr(sibling->phys, '.') - sibling->phys;
> >>  	}
> >>  
> >> -	if (vid != sibling->vendor || pid != sibling->product)
> >> +	if (n1 != n2 || n1 <= 0 || n2 <= 0)
> >>  		return false;
> >>  
> >> -	/* Compare the physical path. */
> >> -	n1 = strrchr(hdev->phys, '.') - hdev->phys;
> >> -	n2 = strrchr(sibling->phys, '.') - sibling->phys;
> >> -	if (n1 != n2 || n1 <= 0 || n2 <= 0)
> >> +	if (strncmp(hdev->phys, sibling->phys, n1))
> >> +		return false;
> >> +
> >> +	if (hdev->vendor != sibling->vendor || hdev->product != sibling->product) {
> >> +		if(!(features->device_type & WACOM_DEVICETYPE_DIRECT))
> >> +			return false;
> >> +	}
> > 
> > As mentioned in the commit log, I am not sure it's such a good idea to
> > have this check. Does it really remove a false positive or can this just
> > be considered as an extra check that can be safely removed?
> > 
> 
> See comment above.
> 
> >> +
> >> +	if ((features->device_type & WACOM_DEVICETYPE_DIRECT) !=
> >> +	    (sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
> >>  		return false;
> > 
> > unless I am mistaken, you might as well need {if indirect and sibling
> > is not indirect, than false }.
> > 
> 
> That case should be covered since we're comparing the actual values of
> each device's "direct" flag for equality (direct/indirect and
> indirect/direct will return false).
> 
> If its not immediately clear though, I could decompose it into
> independent checks for each case. E.g.:
> 
>   if ((features->device_type & WACOM_DEVICETYPE_DIRECT) &&
>       !(sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
>           return false;
> 
>   if (!(features->device_type & WACOM_DEVICETYPE_DIRECT) &&
>        (sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
>           return false;
> 
> >>  
> >> -	return !strncmp(hdev->phys, sibling->phys, n1);
> >> +	if ((features->device_type & WACOM_DEVICETYPE_PEN) &&
> >> +	    !(sibling_features->device_type & WACOM_DEVICETYPE_TOUCH))
> >> +		return false;
> >> +
> >> +	if ((features->device_type & WACOM_DEVICETYPE_TOUCH) &&
> >> +	    !(sibling_features->device_type & WACOM_DEVICETYPE_PEN))
> >> +		return false;
> > 
> > I think it would be better to have those last 3 (plus mine) tests at the
> > beginning, before doing string lookouts. They seem to be the most
> > reliable ones and able to exclude a lot of false positive.
> > 
> 
> Reasonable enough.
> 
> >> +
> >> +	return true;
> >>  }
> >>  
> >>  static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev)
> >>  {
> >>  	struct wacom_hdev_data *data;
> >>  
> >> +	/* Try to find an already-probed interface from the same device */
> >> +	list_for_each_entry(data, &wacom_udev_list, list) {
> >> +		int n1, n2;
> >> +		n1 = strrchr(hdev->phys, '/') - hdev->phys;
> >> +		n2 = strrchr(data->dev->phys, '/') - data->dev->phys;
> >> +		if (n1 != n2 || n1 <= 0 || n2 <= 0)
> >> +			continue;
> >> +		if (!strncmp(hdev->phys, data->dev->phys, n1))
> >> +			return data;
> >> +	}
> > 
> > I can't see the benefit of having this here, while it seems to be
> > already tested in wacom_are_sibling().
> > 
> 
> There's a subtle issue with not performing this search before proceeding
> to trying to pair arbitrary devices in wacom_are_sibling. Imagine that
> you've already got a pen-only device connected to your system. Now,
> connect a (non-split) pen+touch device to the same hub. When the touch
> interface is probed, the first device checked its checked against in
> wacom_are_sibling is that pen-only device, and provided it passes all
> the checks then it will be incorrectly paired with it.
> 
> This is actually probably a leftover from before I added the requirement
> that only direct devices can have different PIDs and might not be
> strictly necessary in the patch's current state, but I'd feel better
> keeping it in either way.
> 
> > Also, if there is a need for it, there is a common pattern used 3 times
> > here:
> >         int n1, n2;
> >         n1 = strrchr(hdev->phys, separator) - hdev->phys;
> >         n2 = strrchr(other->dev->phys, separator) - other->dev->phys;
> >         if (n1 != n2 || n1 <= 0 || n2 <= 0)
> >                  continue;
> >         return !strncmp(hdev->phys, other->dev->phys, n1);
> > 
> > It would make sense to put a name on it and have a separate function.
> > 
> > Cheers,
> > Benjamin
> > 
> 
> Good call, if the block remains necessary.
> 
> Jason
> ---
> Now instead of four in the eights place /
> you’ve got three, ‘Cause you added one /
> (That is to say, eight) to the two, /
> But you can’t take seven from three, /
> So you look at the sixty-fours....
> 
> >> +
> >> +	/* Fallback to finding devices that appear to be "siblings" */
> >>  	list_for_each_entry(data, &wacom_udev_list, list) {
> >>  		if (wacom_are_sibling(hdev, data->dev)) {
> >>  			kref_get(&data->kref);
> >> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> >> index 2523a29..cb6fc63 100644
> >> --- a/drivers/hid/wacom_wac.c
> >> +++ b/drivers/hid/wacom_wac.c
> >> @@ -3229,11 +3229,10 @@ static const struct wacom_features wacom_features_0xF4 =
> >>  static const struct wacom_features wacom_features_0xF8 =
> >>  	{ "Wacom Cintiq 24HD touch", 104080, 65200, 2047, 63, /* Pen */
> >>  	  WACOM_24HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 16,
> >> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> >> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0xf6 };
> >> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
> >>  static const struct wacom_features wacom_features_0xF6 =
> >>  	{ "Wacom Cintiq 24HD touch", .type = WACOM_24HDT, /* Touch */
> >> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0xf8, .touch_max = 10,
> >> +	  .touch_max = 10,
> >>  	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
> >>  static const struct wacom_features wacom_features_0x32A =
> >>  	{ "Wacom Cintiq 27QHD", 119740, 67520, 2047, 63,
> >> @@ -3242,11 +3241,10 @@ static const struct wacom_features wacom_features_0x32A =
> >>  static const struct wacom_features wacom_features_0x32B =
> >>  	{ "Wacom Cintiq 27QHD touch", 119740, 67520, 2047, 63,
> >>  	  WACOM_27QHD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 0,
> >> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> >> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x32C };
> >> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
> >>  static const struct wacom_features wacom_features_0x32C =
> >>  	{ "Wacom Cintiq 27QHD touch", .type = WACOM_27QHDT,
> >> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x32B, .touch_max = 10 };
> >> +	  .touch_max = 10 };
> >>  static const struct wacom_features wacom_features_0x3F =
> >>  	{ "Wacom Cintiq 21UX", 87200, 65600, 1023, 63,
> >>  	  CINTIQ, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 8 };
> >> @@ -3263,11 +3261,10 @@ static const struct wacom_features wacom_features_0x304 =
> >>  static const struct wacom_features wacom_features_0x333 =
> >>  	{ "Wacom Cintiq 13HD touch", 59152, 33448, 2047, 63,
> >>  	  WACOM_13HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
> >> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> >> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x335 };
> >> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
> >>  static const struct wacom_features wacom_features_0x335 =
> >>  	{ "Wacom Cintiq 13HD touch", .type = WACOM_24HDT, /* Touch */
> >> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x333, .touch_max = 10,
> >> +	  .touch_max = 10,
> >>  	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
> >>  static const struct wacom_features wacom_features_0xC7 =
> >>  	{ "Wacom DTU1931", 37832, 30305, 511, 0,
> >> @@ -3298,11 +3295,10 @@ static const struct wacom_features wacom_features_0x57 =
> >>  static const struct wacom_features wacom_features_0x59 = /* Pen */
> >>  	{ "Wacom DTH2242", 95640, 54060, 2047, 63,
> >>  	  DTK, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 6,
> >> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> >> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5D };
> >> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
> >>  static const struct wacom_features wacom_features_0x5D = /* Touch */
> >>  	{ "Wacom DTH2242",       .type = WACOM_24HDT,
> >> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x59, .touch_max = 10,
> >> +	  .touch_max = 10,
> >>  	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
> >>  static const struct wacom_features wacom_features_0xCC =
> >>  	{ "Wacom Cintiq 21UX2", 86800, 65200, 2047, 63,
> >> @@ -3315,11 +3311,10 @@ static const struct wacom_features wacom_features_0xFA =
> >>  static const struct wacom_features wacom_features_0x5B =
> >>  	{ "Wacom Cintiq 22HDT", 95440, 53860, 2047, 63,
> >>  	  WACOM_22HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 18,
> >> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> >> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5e };
> >> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
> >>  static const struct wacom_features wacom_features_0x5E =
> >>  	{ "Wacom Cintiq 22HDT", .type = WACOM_24HDT,
> >> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5b, .touch_max = 10,
> >> +	  .touch_max = 10,
> >>  	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
> >>  static const struct wacom_features wacom_features_0x90 =
> >>  	{ "Wacom ISDv4 90", 26202, 16325, 255, 0,
> >> @@ -3461,20 +3456,18 @@ static const struct wacom_features wacom_features_0x6004 =
> >>  static const struct wacom_features wacom_features_0x307 =
> >>  	{ "Wacom ISDv5 307", 59152, 33448, 2047, 63,
> >>  	  CINTIQ_HYBRID, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
> >> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> >> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x309 };
> >> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
> >>  static const struct wacom_features wacom_features_0x309 =
> >>  	{ "Wacom ISDv5 309", .type = WACOM_24HDT, /* Touch */
> >> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x0307, .touch_max = 10,
> >> +	  .touch_max = 10,
> >>  	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
> >>  static const struct wacom_features wacom_features_0x30A =
> >>  	{ "Wacom ISDv5 30A", 59152, 33448, 2047, 63,
> >>  	  CINTIQ_HYBRID, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
> >> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> >> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x30C };
> >> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
> >>  static const struct wacom_features wacom_features_0x30C =
> >>  	{ "Wacom ISDv5 30C", .type = WACOM_24HDT, /* Touch */
> >> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x30A, .touch_max = 10,
> >> +	  .touch_max = 10,
> >>  	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
> >>  static const struct wacom_features wacom_features_0x318 =
> >>  	{ "Wacom USB Bamboo PAD", 4095, 4095, /* Touch */
> >> @@ -3485,11 +3478,9 @@ static const struct wacom_features wacom_features_0x319 =
> >>  static const struct wacom_features wacom_features_0x325 =
> >>  	{ "Wacom ISDv5 325", 59552, 33848, 2047, 63,
> >>  	  CINTIQ_COMPANION_2, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 11,
> >> -	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
> >> -	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x326 };
> >> +	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
> >>  static const struct wacom_features wacom_features_0x326 = /* Touch */
> >> -	{ "Wacom ISDv5 326", .type = HID_GENERIC, .oVid = USB_VENDOR_ID_WACOM,
> >> -	  .oPid = 0x325 };
> >> +	{ "Wacom ISDv5 326", .type = HID_GENERIC };
> >>  static const struct wacom_features wacom_features_0x323 =
> >>  	{ "Wacom Intuos P M", 21600, 13500, 1023, 31,
> >>  	  INTUOSHT, WACOM_INTUOS_RES, WACOM_INTUOS_RES,
> >> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
> >> index 7ad6273..a5bd05a 100644
> >> --- a/drivers/hid/wacom_wac.h
> >> +++ b/drivers/hid/wacom_wac.h
> >> @@ -181,8 +181,6 @@ struct wacom_features {
> >>  	int tilt_fuzz;
> >>  	unsigned quirks;
> >>  	unsigned touch_max;
> >> -	int oVid;
> >> -	int oPid;
> >>  	int pktlen;
> >>  	bool check_for_hid_type;
> >>  	int hid_type;
> >> -- 
> >> 2.9.0
> >>
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] HID: wacom: Replace 'oVid' and 'oPid' with heuristics

[Jason Gerecke]

On Mon, Jul 25, 2016 at 2:02 AM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> Hi Jason,
>
> [I've seen v2 and v3 but the discussion is mainly going there, so
> pulling this one out of the archives]
>
> On Jul 20 2016 or thereabouts, Jason Gerecke wrote:
>> On 07/12/2016 02:05 AM, Benjamin Tissoires wrote:
>> > On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
>> >> The 'oVid' and 'oPid' variables used by wacom_are_sibling are a hacky
>> >> solution to the problem of the driver having few good heuristics to use
>> >> to determine if two devices should be considered siblings or not. This
>> >> commit replaces them with heuristics that leverage the information we
>> >> have available to determine if two devices are likely siblings or not.
>> >
>> > I agree it's nicer to have a better heuristic for sibling matching,
>> > but I wonder if this heuristic is reliable enough when talking about
>> > future devices. It might be, but I know from experience that the
>> > firmware team can be very original and find a way that screws up us all
>> > :/
>> >
>> > Keeping the safety net of reducing the checks with ovid/opid might come
>> > handy in the future.
>> >
>>
>> The biggest problem with oVid/oPid is that they're kinda antithetical to
>> the HID_GENERIC codepath. If a device is split between two PIDs then
>> arbitration is broken for any kernel that doesn't include specific
>> support for the device.
>
> Well, we currently already have different paths for HID_GENERIC and
> other various protocols. Why is that a problem to have heuristics for
> HID_GENERIC and ovid/opid for those we need to have special handling?
>
>>
>> I agree that heuristics aren't as confidence-inspiring as explicit
>> notation, but if they can be made to work well enough then I'd prefer
>> using them. Either way, I suppose as userspace starts taking over
>> arbitration duties it will become a more and more moot issue.
>
> Yep, but currently the patch might link 2 devices that were not bound
> together before, so I still think ovid/opid is the best way for the non
> generic devices. For generic devices (future I think) we can always ask
> people to use userspace touch arbitration.
>
>>
>> >>
>> >> Written out, the new heuristics are basically:
>> >>
>> >>   * If a device with the same VID/PID as that being probed already exists,
>> >>     it should be preferentially checked for siblingship.
>> >
>> > That's assuming you don't have 2 27QHD connected as 2 monitors (if you
>> > really have a lot of money to spend) :)
>> >
>> > I think the code is OK, just not the comment above (mostly the
>> > "preferentially" word itches me)
>> >
>>
>> I'll try to come up with better / more clear wording (see my later
>> comments on the "Try to find an already-probed interface from the same
>> device" hunk for more detail).
>
> Thanks for the changes in v2/3
>
>>
>> >>
>> >>   * Two HID devices which have the same VID/PID are not siblings if they
>> >>     are not part of the same logical hardware device.
>> >>
>> >>   * Two HID devices which have different VID/PIDs are not siblings if
>> >>     they have different parent (e.g. USB hub) devices.
>> >>
>> >>   * Devices without the WACOM_DEVICETYPE_DIRECT flag set may only be
>> >>     siblings of devies with the same VID/PID (Wacom often splits their
>> >>     direct input tablets into two devices to make it easier to meet
>> >>     Microsoft's touchscreen requirements).
>> >
>> > That's a strong assumption on the future. If you are forced to use the
>> > Precision Touchpad protocol for Intuos, this will just blow up.
>> >
>>
>> I originally didn't include this condition in my checks since I was
>> similarly wary. Leaving it out does open two small corner cases though.
>>
>> 1) A pen-only tablet connected to the same hub as a touch-only tablet.
>> Touch-only Wacom tablets aren't particularly common and it would
>> be a little strange to have one paired with a pen-only tablet, but it
>> could conceivably happen. Although pairing the devices shouldn't
>> normally be an issue since a user isn't likely to use both
>> simultaneously, it might cause problems for multi-seat setups.
>
> Yes, multi-seats is one big issue here.
>
>>
>> 2) A pen-only tablet connected to the same hub as a pen-and-touch tablet
>> which has the *touch* interface probed first. As far as I'm aware, there
>> aren't any Wacom devices that have the USB interfaces ordered
>> touch-then-pen, but as you point out, who knows what those tricksy
>> firmware engineers will do in the future to ruin your day.
>
> And the winner is... the Cintiq 13HDT -> touch interface and then Pen :)
>
> Which means with the new patch, connecting a Cintiq 21UX (1 not 2) which
> is a direct device which I assume doesn't have an internal hub, you end
> up binding the 21UX pen with the 13HD touch, and things gets nasty for
> the 13HD pen interface.
>

Argh. Good to know.

>>
>> I'm open to leaving the check in or out depending on your feelings. If
>> you've got thoughts on how to close these corner cases as well, I'm all
>> ears :)
>
> I really think the ovid/opid approach solves most of the issues with the
> current hardware. You are concerned about future hardware that will be
> handled by HID_GENERIC. Why not just adding a special case for
> HID_GENERIC that uses your heuristics?
> In userspace I think we will still have the ovid/opid approach in
> libwacom because it restricts the amount of combinations by a very good
> factor.
>
> If the kernel with the new heuristics (HID_GENERIC only) fails, we will
> be able to tell users to switch to userspace touch arbitration.
>
> /me turns in circle a little, but how does that sounds?
>
> Cheers,
> Benjamin
>

That sounds reasonable. I'll return the old oVid/oPid checks, and
integrate them with heuristics for HID_GENERIC devices. Patch to come
soon (TM).

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one  /
(That is to say, eight) to the two,     /
But you can’t take seven from three,    /
So you look at the sixty-fours....

>>
>> >>
>> >>   * Two devices which have different "directness" are not siblings.
>> >>
>> >>   * Two devices which do not serve complementary roles (i.e. pen/touch)
>> >>     are not siblings.
>> >
>> > I think it would be useful to have these write outs as comments in the
>> > code. It's quite tricky to understand the actual code without these
>> > explanations.
>> >
>>
>> Ack.
>>
>> >>
>> >> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
>> >> ---
>> >>  drivers/hid/wacom_sys.c | 58 +++++++++++++++++++++++++++++++++++++++----------
>> >>  drivers/hid/wacom_wac.c | 41 ++++++++++++++--------------------
>> >>  drivers/hid/wacom_wac.h |  2 --
>> >>  3 files changed, 62 insertions(+), 39 deletions(-)
>> >>
>> >> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
>> >> index 4a0bb6f..a5bc038 100644
>> >> --- a/drivers/hid/wacom_sys.c
>> >> +++ b/drivers/hid/wacom_sys.c
>> >> @@ -532,31 +532,65 @@ static bool wacom_are_sibling(struct hid_device *hdev,
>> >>  {
>> >>    struct wacom *wacom = hid_get_drvdata(hdev);
>> >>    struct wacom_features *features = &wacom->wacom_wac.features;
>> >> -  int vid = features->oVid;
>> >> -  int pid = features->oPid;
>> >> -  int n1,n2;
>> >> +  struct wacom *sibling_wacom = hid_get_drvdata(sibling);
>> >> +  struct wacom_features *sibling_features = &sibling_wacom->wacom_wac.features;
>> >> +  int n1, n2;
>> >>
>> >> -  if (vid == 0 && pid == 0) {
>> >> -          vid = hdev->vendor;
>> >> -          pid = hdev->product;
>> >> +  /* Compare the physical path. Require devices with the same
>> >> +   * PID to share the same device, and devices with different
>> >> +   * PIDs to share parent devices.
>> >> +   */
>> >
>> > I stumbled this morning upon:
>> > http://www.theregister.co.uk/2016/07/11/linus_torvalds_in_sweary_rant_about_punctuation_in_kernel_comments/
>> >
>> > Please make sure to follow the comments style guidelines :)
>> >
>>
>> Whatever makes the style gods happy :)
>>
>> >> +  if (hdev->vendor == sibling->vendor && hdev->product == sibling->product) {
>> >> +          n1 = strrchr(hdev->phys, '/') - hdev->phys;
>> >> +          n2 = strrchr(sibling->phys, '/') - sibling->phys;
>> >> +  }
>> >> +  else {
>> >> +          n1 = strrchr(hdev->phys, '.') - hdev->phys;
>> >> +          n2 = strrchr(sibling->phys, '.') - sibling->phys;
>> >>    }
>> >>
>> >> -  if (vid != sibling->vendor || pid != sibling->product)
>> >> +  if (n1 != n2 || n1 <= 0 || n2 <= 0)
>> >>            return false;
>> >>
>> >> -  /* Compare the physical path. */
>> >> -  n1 = strrchr(hdev->phys, '.') - hdev->phys;
>> >> -  n2 = strrchr(sibling->phys, '.') - sibling->phys;
>> >> -  if (n1 != n2 || n1 <= 0 || n2 <= 0)
>> >> +  if (strncmp(hdev->phys, sibling->phys, n1))
>> >> +          return false;
>> >> +
>> >> +  if (hdev->vendor != sibling->vendor || hdev->product != sibling->product) {
>> >> +          if(!(features->device_type & WACOM_DEVICETYPE_DIRECT))
>> >> +                  return false;
>> >> +  }
>> >
>> > As mentioned in the commit log, I am not sure it's such a good idea to
>> > have this check. Does it really remove a false positive or can this just
>> > be considered as an extra check that can be safely removed?
>> >
>>
>> See comment above.
>>
>> >> +
>> >> +  if ((features->device_type & WACOM_DEVICETYPE_DIRECT) !=
>> >> +      (sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
>> >>            return false;
>> >
>> > unless I am mistaken, you might as well need {if indirect and sibling
>> > is not indirect, than false }.
>> >
>>
>> That case should be covered since we're comparing the actual values of
>> each device's "direct" flag for equality (direct/indirect and
>> indirect/direct will return false).
>>
>> If its not immediately clear though, I could decompose it into
>> independent checks for each case. E.g.:
>>
>>   if ((features->device_type & WACOM_DEVICETYPE_DIRECT) &&
>>       !(sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
>>           return false;
>>
>>   if (!(features->device_type & WACOM_DEVICETYPE_DIRECT) &&
>>        (sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
>>           return false;
>>
>> >>
>> >> -  return !strncmp(hdev->phys, sibling->phys, n1);
>> >> +  if ((features->device_type & WACOM_DEVICETYPE_PEN) &&
>> >> +      !(sibling_features->device_type & WACOM_DEVICETYPE_TOUCH))
>> >> +          return false;
>> >> +
>> >> +  if ((features->device_type & WACOM_DEVICETYPE_TOUCH) &&
>> >> +      !(sibling_features->device_type & WACOM_DEVICETYPE_PEN))
>> >> +          return false;
>> >
>> > I think it would be better to have those last 3 (plus mine) tests at the
>> > beginning, before doing string lookouts. They seem to be the most
>> > reliable ones and able to exclude a lot of false positive.
>> >
>>
>> Reasonable enough.
>>
>> >> +
>> >> +  return true;
>> >>  }
>> >>
>> >>  static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev)
>> >>  {
>> >>    struct wacom_hdev_data *data;
>> >>
>> >> +  /* Try to find an already-probed interface from the same device */
>> >> +  list_for_each_entry(data, &wacom_udev_list, list) {
>> >> +          int n1, n2;
>> >> +          n1 = strrchr(hdev->phys, '/') - hdev->phys;
>> >> +          n2 = strrchr(data->dev->phys, '/') - data->dev->phys;
>> >> +          if (n1 != n2 || n1 <= 0 || n2 <= 0)
>> >> +                  continue;
>> >> +          if (!strncmp(hdev->phys, data->dev->phys, n1))
>> >> +                  return data;
>> >> +  }
>> >
>> > I can't see the benefit of having this here, while it seems to be
>> > already tested in wacom_are_sibling().
>> >
>>
>> There's a subtle issue with not performing this search before proceeding
>> to trying to pair arbitrary devices in wacom_are_sibling. Imagine that
>> you've already got a pen-only device connected to your system. Now,
>> connect a (non-split) pen+touch device to the same hub. When the touch
>> interface is probed, the first device checked its checked against in
>> wacom_are_sibling is that pen-only device, and provided it passes all
>> the checks then it will be incorrectly paired with it.
>>
>> This is actually probably a leftover from before I added the requirement
>> that only direct devices can have different PIDs and might not be
>> strictly necessary in the patch's current state, but I'd feel better
>> keeping it in either way.
>>
>> > Also, if there is a need for it, there is a common pattern used 3 times
>> > here:
>> >         int n1, n2;
>> >         n1 = strrchr(hdev->phys, separator) - hdev->phys;
>> >         n2 = strrchr(other->dev->phys, separator) - other->dev->phys;
>> >         if (n1 != n2 || n1 <= 0 || n2 <= 0)
>> >                  continue;
>> >         return !strncmp(hdev->phys, other->dev->phys, n1);
>> >
>> > It would make sense to put a name on it and have a separate function.
>> >
>> > Cheers,
>> > Benjamin
>> >
>>
>> Good call, if the block remains necessary.
>>
>> Jason
>> ---
>> Now instead of four in the eights place /
>> you’ve got three, ‘Cause you added one /
>> (That is to say, eight) to the two, /
>> But you can’t take seven from three, /
>> So you look at the sixty-fours....
>>
>> >> +
>> >> +  /* Fallback to finding devices that appear to be "siblings" */
>> >>    list_for_each_entry(data, &wacom_udev_list, list) {
>> >>            if (wacom_are_sibling(hdev, data->dev)) {
>> >>                    kref_get(&data->kref);
>> >> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
>> >> index 2523a29..cb6fc63 100644
>> >> --- a/drivers/hid/wacom_wac.c
>> >> +++ b/drivers/hid/wacom_wac.c
>> >> @@ -3229,11 +3229,10 @@ static const struct wacom_features wacom_features_0xF4 =
>> >>  static const struct wacom_features wacom_features_0xF8 =
>> >>    { "Wacom Cintiq 24HD touch", 104080, 65200, 2047, 63, /* Pen */
>> >>      WACOM_24HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 16,
>> >> -    WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
>> >> -    .oVid = USB_VENDOR_ID_WACOM, .oPid = 0xf6 };
>> >> +    WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>> >>  static const struct wacom_features wacom_features_0xF6 =
>> >>    { "Wacom Cintiq 24HD touch", .type = WACOM_24HDT, /* Touch */
>> >> -    .oVid = USB_VENDOR_ID_WACOM, .oPid = 0xf8, .touch_max = 10,
>> >> +    .touch_max = 10,
>> >>      .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
>> >>  static const struct wacom_features wacom_features_0x32A =
>> >>    { "Wacom Cintiq 27QHD", 119740, 67520, 2047, 63,
>> >> @@ -3242,11 +3241,10 @@ static const struct wacom_features wacom_features_0x32A =
>> >>  static const struct wacom_features wacom_features_0x32B =
>> >>    { "Wacom Cintiq 27QHD touch", 119740, 67520, 2047, 63,
>> >>      WACOM_27QHD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 0,
>> >> -    WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
>> >> -    .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x32C };
>> >> +    WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>> >>  static const struct wacom_features wacom_features_0x32C =
>> >>    { "Wacom Cintiq 27QHD touch", .type = WACOM_27QHDT,
>> >> -    .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x32B, .touch_max = 10 };
>> >> +    .touch_max = 10 };
>> >>  static const struct wacom_features wacom_features_0x3F =
>> >>    { "Wacom Cintiq 21UX", 87200, 65600, 1023, 63,
>> >>      CINTIQ, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 8 };
>> >> @@ -3263,11 +3261,10 @@ static const struct wacom_features wacom_features_0x304 =
>> >>  static const struct wacom_features wacom_features_0x333 =
>> >>    { "Wacom Cintiq 13HD touch", 59152, 33448, 2047, 63,
>> >>      WACOM_13HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
>> >> -    WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
>> >> -    .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x335 };
>> >> +    WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>> >>  static const struct wacom_features wacom_features_0x335 =
>> >>    { "Wacom Cintiq 13HD touch", .type = WACOM_24HDT, /* Touch */
>> >> -    .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x333, .touch_max = 10,
>> >> +    .touch_max = 10,
>> >>      .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
>> >>  static const struct wacom_features wacom_features_0xC7 =
>> >>    { "Wacom DTU1931", 37832, 30305, 511, 0,
>> >> @@ -3298,11 +3295,10 @@ static const struct wacom_features wacom_features_0x57 =
>> >>  static const struct wacom_features wacom_features_0x59 = /* Pen */
>> >>    { "Wacom DTH2242", 95640, 54060, 2047, 63,
>> >>      DTK, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 6,
>> >> -    WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
>> >> -    .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5D };
>> >> +    WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>> >>  static const struct wacom_features wacom_features_0x5D = /* Touch */
>> >>    { "Wacom DTH2242",       .type = WACOM_24HDT,
>> >> -    .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x59, .touch_max = 10,
>> >> +    .touch_max = 10,
>> >>      .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
>> >>  static const struct wacom_features wacom_features_0xCC =
>> >>    { "Wacom Cintiq 21UX2", 86800, 65200, 2047, 63,
>> >> @@ -3315,11 +3311,10 @@ static const struct wacom_features wacom_features_0xFA =
>> >>  static const struct wacom_features wacom_features_0x5B =
>> >>    { "Wacom Cintiq 22HDT", 95440, 53860, 2047, 63,
>> >>      WACOM_22HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 18,
>> >> -    WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
>> >> -    .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5e };
>> >> +    WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>> >>  static const struct wacom_features wacom_features_0x5E =
>> >>    { "Wacom Cintiq 22HDT", .type = WACOM_24HDT,
>> >> -    .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5b, .touch_max = 10,
>> >> +    .touch_max = 10,
>> >>      .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
>> >>  static const struct wacom_features wacom_features_0x90 =
>> >>    { "Wacom ISDv4 90", 26202, 16325, 255, 0,
>> >> @@ -3461,20 +3456,18 @@ static const struct wacom_features wacom_features_0x6004 =
>> >>  static const struct wacom_features wacom_features_0x307 =
>> >>    { "Wacom ISDv5 307", 59152, 33448, 2047, 63,
>> >>      CINTIQ_HYBRID, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
>> >> -    WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
>> >> -    .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x309 };
>> >> +    WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>> >>  static const struct wacom_features wacom_features_0x309 =
>> >>    { "Wacom ISDv5 309", .type = WACOM_24HDT, /* Touch */
>> >> -    .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x0307, .touch_max = 10,
>> >> +    .touch_max = 10,
>> >>      .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
>> >>  static const struct wacom_features wacom_features_0x30A =
>> >>    { "Wacom ISDv5 30A", 59152, 33448, 2047, 63,
>> >>      CINTIQ_HYBRID, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
>> >> -    WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
>> >> -    .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x30C };
>> >> +    WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>> >>  static const struct wacom_features wacom_features_0x30C =
>> >>    { "Wacom ISDv5 30C", .type = WACOM_24HDT, /* Touch */
>> >> -    .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x30A, .touch_max = 10,
>> >> +    .touch_max = 10,
>> >>      .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
>> >>  static const struct wacom_features wacom_features_0x318 =
>> >>    { "Wacom USB Bamboo PAD", 4095, 4095, /* Touch */
>> >> @@ -3485,11 +3478,9 @@ static const struct wacom_features wacom_features_0x319 =
>> >>  static const struct wacom_features wacom_features_0x325 =
>> >>    { "Wacom ISDv5 325", 59552, 33848, 2047, 63,
>> >>      CINTIQ_COMPANION_2, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 11,
>> >> -    WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
>> >> -    .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x326 };
>> >> +    WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
>> >>  static const struct wacom_features wacom_features_0x326 = /* Touch */
>> >> -  { "Wacom ISDv5 326", .type = HID_GENERIC, .oVid = USB_VENDOR_ID_WACOM,
>> >> -    .oPid = 0x325 };
>> >> +  { "Wacom ISDv5 326", .type = HID_GENERIC };
>> >>  static const struct wacom_features wacom_features_0x323 =
>> >>    { "Wacom Intuos P M", 21600, 13500, 1023, 31,
>> >>      INTUOSHT, WACOM_INTUOS_RES, WACOM_INTUOS_RES,
>> >> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
>> >> index 7ad6273..a5bd05a 100644
>> >> --- a/drivers/hid/wacom_wac.h
>> >> +++ b/drivers/hid/wacom_wac.h
>> >> @@ -181,8 +181,6 @@ struct wacom_features {
>> >>    int tilt_fuzz;
>> >>    unsigned quirks;
>> >>    unsigned touch_max;
>> >> -  int oVid;
>> >> -  int oPid;
>> >>    int pktlen;
>> >>    bool check_for_hid_type;
>> >>    int hid_type;
>> >> --
>> >> 2.9.0
>> >>
>>

Re: [PATCH 2/2] HID: wacom: Replace 'oVid' and 'oPid' with heuristics

[Jason Gerecke]

On 08/03/2016 10:13 AM, Jason Gerecke wrote:
> On Mon, Jul 25, 2016 at 2:02 AM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> Hi Jason,
>>
>> [I've seen v2 and v3 but the discussion is mainly going there, so
>> pulling this one out of the archives]
>>
>> On Jul 20 2016 or thereabouts, Jason Gerecke wrote:
>>> On 07/12/2016 02:05 AM, Benjamin Tissoires wrote:
>>>> On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
>>>>> The 'oVid' and 'oPid' variables used by wacom_are_sibling are a hacky
>>>>> solution to the problem of the driver having few good heuristics to use
>>>>> to determine if two devices should be considered siblings or not. This
>>>>> commit replaces them with heuristics that leverage the information we
>>>>> have available to determine if two devices are likely siblings or not.
>>>>
>>>> I agree it's nicer to have a better heuristic for sibling matching,
>>>> but I wonder if this heuristic is reliable enough when talking about
>>>> future devices. It might be, but I know from experience that the
>>>> firmware team can be very original and find a way that screws up us all
>>>> :/
>>>>
>>>> Keeping the safety net of reducing the checks with ovid/opid might come
>>>> handy in the future.
>>>>
>>>
>>> The biggest problem with oVid/oPid is that they're kinda antithetical to
>>> the HID_GENERIC codepath. If a device is split between two PIDs then
>>> arbitration is broken for any kernel that doesn't include specific
>>> support for the device.
>>
>> Well, we currently already have different paths for HID_GENERIC and
>> other various protocols. Why is that a problem to have heuristics for
>> HID_GENERIC and ovid/opid for those we need to have special handling?
>>
>>>
>>> I agree that heuristics aren't as confidence-inspiring as explicit
>>> notation, but if they can be made to work well enough then I'd prefer
>>> using them. Either way, I suppose as userspace starts taking over
>>> arbitration duties it will become a more and more moot issue.
>>
>> Yep, but currently the patch might link 2 devices that were not bound
>> together before, so I still think ovid/opid is the best way for the non
>> generic devices. For generic devices (future I think) we can always ask
>> people to use userspace touch arbitration.
>>
>>>
>>>>>
>>>>> Written out, the new heuristics are basically:
>>>>>
>>>>>   * If a device with the same VID/PID as that being probed already exists,
>>>>>     it should be preferentially checked for siblingship.
>>>>
>>>> That's assuming you don't have 2 27QHD connected as 2 monitors (if you
>>>> really have a lot of money to spend) :)
>>>>
>>>> I think the code is OK, just not the comment above (mostly the
>>>> "preferentially" word itches me)
>>>>
>>>
>>> I'll try to come up with better / more clear wording (see my later
>>> comments on the "Try to find an already-probed interface from the same
>>> device" hunk for more detail).
>>
>> Thanks for the changes in v2/3
>>
>>>
>>>>>
>>>>>   * Two HID devices which have the same VID/PID are not siblings if they
>>>>>     are not part of the same logical hardware device.
>>>>>
>>>>>   * Two HID devices which have different VID/PIDs are not siblings if
>>>>>     they have different parent (e.g. USB hub) devices.
>>>>>
>>>>>   * Devices without the WACOM_DEVICETYPE_DIRECT flag set may only be
>>>>>     siblings of devies with the same VID/PID (Wacom often splits their
>>>>>     direct input tablets into two devices to make it easier to meet
>>>>>     Microsoft's touchscreen requirements).
>>>>
>>>> That's a strong assumption on the future. If you are forced to use the
>>>> Precision Touchpad protocol for Intuos, this will just blow up.
>>>>
>>>
>>> I originally didn't include this condition in my checks since I was
>>> similarly wary. Leaving it out does open two small corner cases though.
>>>
>>> 1) A pen-only tablet connected to the same hub as a touch-only tablet.
>>> Touch-only Wacom tablets aren't particularly common and it would
>>> be a little strange to have one paired with a pen-only tablet, but it
>>> could conceivably happen. Although pairing the devices shouldn't
>>> normally be an issue since a user isn't likely to use both
>>> simultaneously, it might cause problems for multi-seat setups.
>>
>> Yes, multi-seats is one big issue here.
>>
>>>
>>> 2) A pen-only tablet connected to the same hub as a pen-and-touch tablet
>>> which has the *touch* interface probed first. As far as I'm aware, there
>>> aren't any Wacom devices that have the USB interfaces ordered
>>> touch-then-pen, but as you point out, who knows what those tricksy
>>> firmware engineers will do in the future to ruin your day.
>>
>> And the winner is... the Cintiq 13HDT -> touch interface and then Pen :)
>>
>> Which means with the new patch, connecting a Cintiq 21UX (1 not 2) which
>> is a direct device which I assume doesn't have an internal hub, you end
>> up binding the 21UX pen with the 13HD touch, and things gets nasty for
>> the 13HD pen interface.
>>
> 
> Argh. Good to know.
> 
>>>
>>> I'm open to leaving the check in or out depending on your feelings. If
>>> you've got thoughts on how to close these corner cases as well, I'm all
>>> ears :)
>>
>> I really think the ovid/opid approach solves most of the issues with the
>> current hardware. You are concerned about future hardware that will be
>> handled by HID_GENERIC. Why not just adding a special case for
>> HID_GENERIC that uses your heuristics?
>> In userspace I think we will still have the ovid/opid approach in
>> libwacom because it restricts the amount of combinations by a very good
>> factor.
>>
>> If the kernel with the new heuristics (HID_GENERIC only) fails, we will
>> be able to tell users to switch to userspace touch arbitration.
>>
>> /me turns in circle a little, but how does that sounds?
>>
>> Cheers,
>> Benjamin
>>
> 
> That sounds reasonable. I'll return the old oVid/oPid checks, and
> integrate them with heuristics for HID_GENERIC devices. Patch to come
> soon (TM).
> 
> Jason
> ---
> Now instead of four in the eights place /
> you’ve got three, ‘Cause you added one  /
> (That is to say, eight) to the two,     /
> But you can’t take seven from three,    /
> So you look at the sixty-fours....
> 

One question before I post the updated v4 patch: did you want me to
remove the "direct-input devices may not be siblings of indirect-input
devices" check? It opens up the holes mentioned above, but would
properly arbitrate hypothetical future split-indirect devices.

Since the new arbitration rules only apply to HID_GENERIC devices and
userspace will eventually take over the task anyway, I'm okay with
either option personally.

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....

Re: [PATCH 2/2] HID: wacom: Replace 'oVid' and 'oPid' with heuristics

[Benjamin Tissoires]

On Aug 05 2016 or thereabouts, Jason Gerecke wrote:
> On 08/03/2016 10:13 AM, Jason Gerecke wrote:
> > On Mon, Jul 25, 2016 at 2:02 AM, Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> >> Hi Jason,
> >>
> >> [I've seen v2 and v3 but the discussion is mainly going there, so
> >> pulling this one out of the archives]
> >>
> >> On Jul 20 2016 or thereabouts, Jason Gerecke wrote:
> >>> On 07/12/2016 02:05 AM, Benjamin Tissoires wrote:
> >>>> On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
> >>>>> The 'oVid' and 'oPid' variables used by wacom_are_sibling are a hacky
> >>>>> solution to the problem of the driver having few good heuristics to use
> >>>>> to determine if two devices should be considered siblings or not. This
> >>>>> commit replaces them with heuristics that leverage the information we
> >>>>> have available to determine if two devices are likely siblings or not.
> >>>>
> >>>> I agree it's nicer to have a better heuristic for sibling matching,
> >>>> but I wonder if this heuristic is reliable enough when talking about
> >>>> future devices. It might be, but I know from experience that the
> >>>> firmware team can be very original and find a way that screws up us all
> >>>> :/
> >>>>
> >>>> Keeping the safety net of reducing the checks with ovid/opid might come
> >>>> handy in the future.
> >>>>
> >>>
> >>> The biggest problem with oVid/oPid is that they're kinda antithetical to
> >>> the HID_GENERIC codepath. If a device is split between two PIDs then
> >>> arbitration is broken for any kernel that doesn't include specific
> >>> support for the device.
> >>
> >> Well, we currently already have different paths for HID_GENERIC and
> >> other various protocols. Why is that a problem to have heuristics for
> >> HID_GENERIC and ovid/opid for those we need to have special handling?
> >>
> >>>
> >>> I agree that heuristics aren't as confidence-inspiring as explicit
> >>> notation, but if they can be made to work well enough then I'd prefer
> >>> using them. Either way, I suppose as userspace starts taking over
> >>> arbitration duties it will become a more and more moot issue.
> >>
> >> Yep, but currently the patch might link 2 devices that were not bound
> >> together before, so I still think ovid/opid is the best way for the non
> >> generic devices. For generic devices (future I think) we can always ask
> >> people to use userspace touch arbitration.
> >>
> >>>
> >>>>>
> >>>>> Written out, the new heuristics are basically:
> >>>>>
> >>>>>   * If a device with the same VID/PID as that being probed already exists,
> >>>>>     it should be preferentially checked for siblingship.
> >>>>
> >>>> That's assuming you don't have 2 27QHD connected as 2 monitors (if you
> >>>> really have a lot of money to spend) :)
> >>>>
> >>>> I think the code is OK, just not the comment above (mostly the
> >>>> "preferentially" word itches me)
> >>>>
> >>>
> >>> I'll try to come up with better / more clear wording (see my later
> >>> comments on the "Try to find an already-probed interface from the same
> >>> device" hunk for more detail).
> >>
> >> Thanks for the changes in v2/3
> >>
> >>>
> >>>>>
> >>>>>   * Two HID devices which have the same VID/PID are not siblings if they
> >>>>>     are not part of the same logical hardware device.
> >>>>>
> >>>>>   * Two HID devices which have different VID/PIDs are not siblings if
> >>>>>     they have different parent (e.g. USB hub) devices.
> >>>>>
> >>>>>   * Devices without the WACOM_DEVICETYPE_DIRECT flag set may only be
> >>>>>     siblings of devies with the same VID/PID (Wacom often splits their
> >>>>>     direct input tablets into two devices to make it easier to meet
> >>>>>     Microsoft's touchscreen requirements).
> >>>>
> >>>> That's a strong assumption on the future. If you are forced to use the
> >>>> Precision Touchpad protocol for Intuos, this will just blow up.
> >>>>
> >>>
> >>> I originally didn't include this condition in my checks since I was
> >>> similarly wary. Leaving it out does open two small corner cases though.
> >>>
> >>> 1) A pen-only tablet connected to the same hub as a touch-only tablet.
> >>> Touch-only Wacom tablets aren't particularly common and it would
> >>> be a little strange to have one paired with a pen-only tablet, but it
> >>> could conceivably happen. Although pairing the devices shouldn't
> >>> normally be an issue since a user isn't likely to use both
> >>> simultaneously, it might cause problems for multi-seat setups.
> >>
> >> Yes, multi-seats is one big issue here.
> >>
> >>>
> >>> 2) A pen-only tablet connected to the same hub as a pen-and-touch tablet
> >>> which has the *touch* interface probed first. As far as I'm aware, there
> >>> aren't any Wacom devices that have the USB interfaces ordered
> >>> touch-then-pen, but as you point out, who knows what those tricksy
> >>> firmware engineers will do in the future to ruin your day.
> >>
> >> And the winner is... the Cintiq 13HDT -> touch interface and then Pen :)
> >>
> >> Which means with the new patch, connecting a Cintiq 21UX (1 not 2) which
> >> is a direct device which I assume doesn't have an internal hub, you end
> >> up binding the 21UX pen with the 13HD touch, and things gets nasty for
> >> the 13HD pen interface.
> >>
> > 
> > Argh. Good to know.
> > 
> >>>
> >>> I'm open to leaving the check in or out depending on your feelings. If
> >>> you've got thoughts on how to close these corner cases as well, I'm all
> >>> ears :)
> >>
> >> I really think the ovid/opid approach solves most of the issues with the
> >> current hardware. You are concerned about future hardware that will be
> >> handled by HID_GENERIC. Why not just adding a special case for
> >> HID_GENERIC that uses your heuristics?
> >> In userspace I think we will still have the ovid/opid approach in
> >> libwacom because it restricts the amount of combinations by a very good
> >> factor.
> >>
> >> If the kernel with the new heuristics (HID_GENERIC only) fails, we will
> >> be able to tell users to switch to userspace touch arbitration.
> >>
> >> /me turns in circle a little, but how does that sounds?
> >>
> >> Cheers,
> >> Benjamin
> >>
> > 
> > That sounds reasonable. I'll return the old oVid/oPid checks, and
> > integrate them with heuristics for HID_GENERIC devices. Patch to come
> > soon (TM).
> > 
> > Jason
> > ---
> > Now instead of four in the eights place /
> > you’ve got three, ‘Cause you added one  /
> > (That is to say, eight) to the two,     /
> > But you can’t take seven from three,    /
> > So you look at the sixty-fours....
> > 
> 
> One question before I post the updated v4 patch: did you want me to
> remove the "direct-input devices may not be siblings of indirect-input
> devices" check? It opens up the holes mentioned above, but would
> properly arbitrate hypothetical future split-indirect devices.

I thought this check would be without ambiguity (if the directness is
not the same, that means they are on distinct physical devices).
So I'd say this check is necessary.

> 
> Since the new arbitration rules only apply to HID_GENERIC devices and
> userspace will eventually take over the task anyway, I'm okay with
> either option personally.

Also, there is one thing that might have sense since you are now having
the heuristic only for hid-generic.
We might want to be sure to have the proper sibling matching on some
rare cases. So I think it should be interesting to have:
- .ovid/.opid == 0/0 meaning "match against the current device vid/pid"
  (like what the current code does)
- .ovid/.opid == 0xffff/0xffff (HID_ANY_ID) meaning "use the heuristic,
  you can have any other vid/pid"
- .ovid/.opid != 0/0 and not 0xffff/0xffff meaning "match only the
  specified vid/pid"

This would allow to register a new device using HID_GENERIC but with a
specific ovid/opid.

One extra check could also be that we are sure that the sibling device
also is registered as HID_GENERIC + .ovid/.opid == 0xffff/0xffff to
avoid matching against something we already fixed the ovid/opid...

This might be a little over-processed however :)

Cheers,
Benjamin

> 
> Jason
> ---
> Now instead of four in the eights place /
> you’ve got three, ‘Cause you added one /
> (That is to say, eight) to the two, /
> But you can’t take seven from three, /
> So you look at the sixty-fours....

Re: [PATCH 2/2] HID: wacom: Replace 'oVid' and 'oPid' with heuristics

[Jason Gerecke]

On 08/08/2016 09:36 AM, Benjamin Tissoires wrote:
> On Aug 05 2016 or thereabouts, Jason Gerecke wrote:
>> On 08/03/2016 10:13 AM, Jason Gerecke wrote:
>>> On Mon, Jul 25, 2016 at 2:02 AM, Benjamin Tissoires
>>> <benjamin.tissoires@redhat.com> wrote:
>>>> Hi Jason,
>>>>
>>>> [I've seen v2 and v3 but the discussion is mainly going there, so
>>>> pulling this one out of the archives]
>>>>
>>>> On Jul 20 2016 or thereabouts, Jason Gerecke wrote:
>>>>> On 07/12/2016 02:05 AM, Benjamin Tissoires wrote:
>>>>>> On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
>>>>>>> The 'oVid' and 'oPid' variables used by wacom_are_sibling are a hacky
>>>>>>> solution to the problem of the driver having few good heuristics to use
>>>>>>> to determine if two devices should be considered siblings or not. This
>>>>>>> commit replaces them with heuristics that leverage the information we
>>>>>>> have available to determine if two devices are likely siblings or not.
>>>>>>
>>>>>> I agree it's nicer to have a better heuristic for sibling matching,
>>>>>> but I wonder if this heuristic is reliable enough when talking about
>>>>>> future devices. It might be, but I know from experience that the
>>>>>> firmware team can be very original and find a way that screws up us all
>>>>>> :/
>>>>>>
>>>>>> Keeping the safety net of reducing the checks with ovid/opid might come
>>>>>> handy in the future.
>>>>>>
>>>>>
>>>>> The biggest problem with oVid/oPid is that they're kinda antithetical to
>>>>> the HID_GENERIC codepath. If a device is split between two PIDs then
>>>>> arbitration is broken for any kernel that doesn't include specific
>>>>> support for the device.
>>>>
>>>> Well, we currently already have different paths for HID_GENERIC and
>>>> other various protocols. Why is that a problem to have heuristics for
>>>> HID_GENERIC and ovid/opid for those we need to have special handling?
>>>>
>>>>>
>>>>> I agree that heuristics aren't as confidence-inspiring as explicit
>>>>> notation, but if they can be made to work well enough then I'd prefer
>>>>> using them. Either way, I suppose as userspace starts taking over
>>>>> arbitration duties it will become a more and more moot issue.
>>>>
>>>> Yep, but currently the patch might link 2 devices that were not bound
>>>> together before, so I still think ovid/opid is the best way for the non
>>>> generic devices. For generic devices (future I think) we can always ask
>>>> people to use userspace touch arbitration.
>>>>
>>>>>
>>>>>>>
>>>>>>> Written out, the new heuristics are basically:
>>>>>>>
>>>>>>>   * If a device with the same VID/PID as that being probed already exists,
>>>>>>>     it should be preferentially checked for siblingship.
>>>>>>
>>>>>> That's assuming you don't have 2 27QHD connected as 2 monitors (if you
>>>>>> really have a lot of money to spend) :)
>>>>>>
>>>>>> I think the code is OK, just not the comment above (mostly the
>>>>>> "preferentially" word itches me)
>>>>>>
>>>>>
>>>>> I'll try to come up with better / more clear wording (see my later
>>>>> comments on the "Try to find an already-probed interface from the same
>>>>> device" hunk for more detail).
>>>>
>>>> Thanks for the changes in v2/3
>>>>
>>>>>
>>>>>>>
>>>>>>>   * Two HID devices which have the same VID/PID are not siblings if they
>>>>>>>     are not part of the same logical hardware device.
>>>>>>>
>>>>>>>   * Two HID devices which have different VID/PIDs are not siblings if
>>>>>>>     they have different parent (e.g. USB hub) devices.
>>>>>>>
>>>>>>>   * Devices without the WACOM_DEVICETYPE_DIRECT flag set may only be
>>>>>>>     siblings of devies with the same VID/PID (Wacom often splits their
>>>>>>>     direct input tablets into two devices to make it easier to meet
>>>>>>>     Microsoft's touchscreen requirements).
>>>>>>
>>>>>> That's a strong assumption on the future. If you are forced to use the
>>>>>> Precision Touchpad protocol for Intuos, this will just blow up.
>>>>>>
>>>>>
>>>>> I originally didn't include this condition in my checks since I was
>>>>> similarly wary. Leaving it out does open two small corner cases though.
>>>>>
>>>>> 1) A pen-only tablet connected to the same hub as a touch-only tablet.
>>>>> Touch-only Wacom tablets aren't particularly common and it would
>>>>> be a little strange to have one paired with a pen-only tablet, but it
>>>>> could conceivably happen. Although pairing the devices shouldn't
>>>>> normally be an issue since a user isn't likely to use both
>>>>> simultaneously, it might cause problems for multi-seat setups.
>>>>
>>>> Yes, multi-seats is one big issue here.
>>>>
>>>>>
>>>>> 2) A pen-only tablet connected to the same hub as a pen-and-touch tablet
>>>>> which has the *touch* interface probed first. As far as I'm aware, there
>>>>> aren't any Wacom devices that have the USB interfaces ordered
>>>>> touch-then-pen, but as you point out, who knows what those tricksy
>>>>> firmware engineers will do in the future to ruin your day.
>>>>
>>>> And the winner is... the Cintiq 13HDT -> touch interface and then Pen :)
>>>>
>>>> Which means with the new patch, connecting a Cintiq 21UX (1 not 2) which
>>>> is a direct device which I assume doesn't have an internal hub, you end
>>>> up binding the 21UX pen with the 13HD touch, and things gets nasty for
>>>> the 13HD pen interface.
>>>>
>>>
>>> Argh. Good to know.
>>>
>>>>>
>>>>> I'm open to leaving the check in or out depending on your feelings. If
>>>>> you've got thoughts on how to close these corner cases as well, I'm all
>>>>> ears :)
>>>>
>>>> I really think the ovid/opid approach solves most of the issues with the
>>>> current hardware. You are concerned about future hardware that will be
>>>> handled by HID_GENERIC. Why not just adding a special case for
>>>> HID_GENERIC that uses your heuristics?
>>>> In userspace I think we will still have the ovid/opid approach in
>>>> libwacom because it restricts the amount of combinations by a very good
>>>> factor.
>>>>
>>>> If the kernel with the new heuristics (HID_GENERIC only) fails, we will
>>>> be able to tell users to switch to userspace touch arbitration.
>>>>
>>>> /me turns in circle a little, but how does that sounds?
>>>>
>>>> Cheers,
>>>> Benjamin
>>>>
>>>
>>> That sounds reasonable. I'll return the old oVid/oPid checks, and
>>> integrate them with heuristics for HID_GENERIC devices. Patch to come
>>> soon (TM).
>>>
>>> Jason
>>> ---
>>> Now instead of four in the eights place /
>>> you’ve got three, ‘Cause you added one  /
>>> (That is to say, eight) to the two,     /
>>> But you can’t take seven from three,    /
>>> So you look at the sixty-fours....
>>>
>>
>> One question before I post the updated v4 patch: did you want me to
>> remove the "direct-input devices may not be siblings of indirect-input
>> devices" check? It opens up the holes mentioned above, but would
>> properly arbitrate hypothetical future split-indirect devices.
> 
> I thought this check would be without ambiguity (if the directness is
> not the same, that means they are on distinct physical devices).
> So I'd say this check is necessary.
> 

Oops -- my brain must have shut off early for the weekend. Copy/pasted
the wrong heuristic description. I actually meant to ask if you wanted
me to keep/nuke the "Devices with different VID/PIDs may not be siblings
unless they are direct input devices" check since its presence may cause
problems for future precision touchpads but its absence will cause
problems for pen-only/touch-only tablets behind the same hub.

>>
>> Since the new arbitration rules only apply to HID_GENERIC devices and
>> userspace will eventually take over the task anyway, I'm okay with
>> either option personally.
> 
> Also, there is one thing that might have sense since you are now having
> the heuristic only for hid-generic.
> We might want to be sure to have the proper sibling matching on some
> rare cases. So I think it should be interesting to have:
> - .ovid/.opid == 0/0 meaning "match against the current device vid/pid"
>   (like what the current code does)
> - .ovid/.opid == 0xffff/0xffff (HID_ANY_ID) meaning "use the heuristic,
>   you can have any other vid/pid"
> - .ovid/.opid != 0/0 and not 0xffff/0xffff meaning "match only the
>   specified vid/pid"
> 
> This would allow to register a new device using HID_GENERIC but with a
> specific ovid/opid.
> 
> One extra check could also be that we are sure that the sibling device
> also is registered as HID_GENERIC + .ovid/.opid == 0xffff/0xffff to
> avoid matching against something we already fixed the ovid/opid...
> 
> This might be a little over-processed however :)
> 
> Cheers,
> Benjamin
> 

I kinda like the sound of it since it would make the logic a little more
straightforward. The special ovid/opid meanings would apply equally to
all devices, meaning I wouldn't have to anymore ignore the 0/0 case for
HID_GENERIC.

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....

Re: [PATCH 2/2] HID: wacom: Replace 'oVid' and 'oPid' with heuristics

[Benjamin Tissoires]

On Aug 08 2016 or thereabouts, Jason Gerecke wrote:
> On 08/08/2016 09:36 AM, Benjamin Tissoires wrote:
> > On Aug 05 2016 or thereabouts, Jason Gerecke wrote:
> >> On 08/03/2016 10:13 AM, Jason Gerecke wrote:
> >>> On Mon, Jul 25, 2016 at 2:02 AM, Benjamin Tissoires
> >>> <benjamin.tissoires@redhat.com> wrote:
> >>>> Hi Jason,
> >>>>
> >>>> [I've seen v2 and v3 but the discussion is mainly going there, so
> >>>> pulling this one out of the archives]
> >>>>
> >>>> On Jul 20 2016 or thereabouts, Jason Gerecke wrote:
> >>>>> On 07/12/2016 02:05 AM, Benjamin Tissoires wrote:
> >>>>>> On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
> >>>>>>> The 'oVid' and 'oPid' variables used by wacom_are_sibling are a hacky
> >>>>>>> solution to the problem of the driver having few good heuristics to use
> >>>>>>> to determine if two devices should be considered siblings or not. This
> >>>>>>> commit replaces them with heuristics that leverage the information we
> >>>>>>> have available to determine if two devices are likely siblings or not.
> >>>>>>
> >>>>>> I agree it's nicer to have a better heuristic for sibling matching,
> >>>>>> but I wonder if this heuristic is reliable enough when talking about
> >>>>>> future devices. It might be, but I know from experience that the
> >>>>>> firmware team can be very original and find a way that screws up us all
> >>>>>> :/
> >>>>>>
> >>>>>> Keeping the safety net of reducing the checks with ovid/opid might come
> >>>>>> handy in the future.
> >>>>>>
> >>>>>
> >>>>> The biggest problem with oVid/oPid is that they're kinda antithetical to
> >>>>> the HID_GENERIC codepath. If a device is split between two PIDs then
> >>>>> arbitration is broken for any kernel that doesn't include specific
> >>>>> support for the device.
> >>>>
> >>>> Well, we currently already have different paths for HID_GENERIC and
> >>>> other various protocols. Why is that a problem to have heuristics for
> >>>> HID_GENERIC and ovid/opid for those we need to have special handling?
> >>>>
> >>>>>
> >>>>> I agree that heuristics aren't as confidence-inspiring as explicit
> >>>>> notation, but if they can be made to work well enough then I'd prefer
> >>>>> using them. Either way, I suppose as userspace starts taking over
> >>>>> arbitration duties it will become a more and more moot issue.
> >>>>
> >>>> Yep, but currently the patch might link 2 devices that were not bound
> >>>> together before, so I still think ovid/opid is the best way for the non
> >>>> generic devices. For generic devices (future I think) we can always ask
> >>>> people to use userspace touch arbitration.
> >>>>
> >>>>>
> >>>>>>>
> >>>>>>> Written out, the new heuristics are basically:
> >>>>>>>
> >>>>>>>   * If a device with the same VID/PID as that being probed already exists,
> >>>>>>>     it should be preferentially checked for siblingship.
> >>>>>>
> >>>>>> That's assuming you don't have 2 27QHD connected as 2 monitors (if you
> >>>>>> really have a lot of money to spend) :)
> >>>>>>
> >>>>>> I think the code is OK, just not the comment above (mostly the
> >>>>>> "preferentially" word itches me)
> >>>>>>
> >>>>>
> >>>>> I'll try to come up with better / more clear wording (see my later
> >>>>> comments on the "Try to find an already-probed interface from the same
> >>>>> device" hunk for more detail).
> >>>>
> >>>> Thanks for the changes in v2/3
> >>>>
> >>>>>
> >>>>>>>
> >>>>>>>   * Two HID devices which have the same VID/PID are not siblings if they
> >>>>>>>     are not part of the same logical hardware device.
> >>>>>>>
> >>>>>>>   * Two HID devices which have different VID/PIDs are not siblings if
> >>>>>>>     they have different parent (e.g. USB hub) devices.
> >>>>>>>
> >>>>>>>   * Devices without the WACOM_DEVICETYPE_DIRECT flag set may only be
> >>>>>>>     siblings of devies with the same VID/PID (Wacom often splits their
> >>>>>>>     direct input tablets into two devices to make it easier to meet
> >>>>>>>     Microsoft's touchscreen requirements).
> >>>>>>
> >>>>>> That's a strong assumption on the future. If you are forced to use the
> >>>>>> Precision Touchpad protocol for Intuos, this will just blow up.
> >>>>>>
> >>>>>
> >>>>> I originally didn't include this condition in my checks since I was
> >>>>> similarly wary. Leaving it out does open two small corner cases though.
> >>>>>
> >>>>> 1) A pen-only tablet connected to the same hub as a touch-only tablet.
> >>>>> Touch-only Wacom tablets aren't particularly common and it would
> >>>>> be a little strange to have one paired with a pen-only tablet, but it
> >>>>> could conceivably happen. Although pairing the devices shouldn't
> >>>>> normally be an issue since a user isn't likely to use both
> >>>>> simultaneously, it might cause problems for multi-seat setups.
> >>>>
> >>>> Yes, multi-seats is one big issue here.
> >>>>
> >>>>>
> >>>>> 2) A pen-only tablet connected to the same hub as a pen-and-touch tablet
> >>>>> which has the *touch* interface probed first. As far as I'm aware, there
> >>>>> aren't any Wacom devices that have the USB interfaces ordered
> >>>>> touch-then-pen, but as you point out, who knows what those tricksy
> >>>>> firmware engineers will do in the future to ruin your day.
> >>>>
> >>>> And the winner is... the Cintiq 13HDT -> touch interface and then Pen :)
> >>>>
> >>>> Which means with the new patch, connecting a Cintiq 21UX (1 not 2) which
> >>>> is a direct device which I assume doesn't have an internal hub, you end
> >>>> up binding the 21UX pen with the 13HD touch, and things gets nasty for
> >>>> the 13HD pen interface.
> >>>>
> >>>
> >>> Argh. Good to know.
> >>>
> >>>>>
> >>>>> I'm open to leaving the check in or out depending on your feelings. If
> >>>>> you've got thoughts on how to close these corner cases as well, I'm all
> >>>>> ears :)
> >>>>
> >>>> I really think the ovid/opid approach solves most of the issues with the
> >>>> current hardware. You are concerned about future hardware that will be
> >>>> handled by HID_GENERIC. Why not just adding a special case for
> >>>> HID_GENERIC that uses your heuristics?
> >>>> In userspace I think we will still have the ovid/opid approach in
> >>>> libwacom because it restricts the amount of combinations by a very good
> >>>> factor.
> >>>>
> >>>> If the kernel with the new heuristics (HID_GENERIC only) fails, we will
> >>>> be able to tell users to switch to userspace touch arbitration.
> >>>>
> >>>> /me turns in circle a little, but how does that sounds?
> >>>>
> >>>> Cheers,
> >>>> Benjamin
> >>>>
> >>>
> >>> That sounds reasonable. I'll return the old oVid/oPid checks, and
> >>> integrate them with heuristics for HID_GENERIC devices. Patch to come
> >>> soon (TM).
> >>>
> >>> Jason
> >>> ---
> >>> Now instead of four in the eights place /
> >>> you’ve got three, ‘Cause you added one  /
> >>> (That is to say, eight) to the two,     /
> >>> But you can’t take seven from three,    /
> >>> So you look at the sixty-fours....
> >>>
> >>
> >> One question before I post the updated v4 patch: did you want me to
> >> remove the "direct-input devices may not be siblings of indirect-input
> >> devices" check? It opens up the holes mentioned above, but would
> >> properly arbitrate hypothetical future split-indirect devices.
> > 
> > I thought this check would be without ambiguity (if the directness is
> > not the same, that means they are on distinct physical devices).
> > So I'd say this check is necessary.
> > 
> 
> Oops -- my brain must have shut off early for the weekend. Copy/pasted
> the wrong heuristic description. I actually meant to ask if you wanted
> me to keep/nuke the "Devices with different VID/PIDs may not be siblings
> unless they are direct input devices" check since its presence may cause
> problems for future precision touchpads but its absence will cause
> problems for pen-only/touch-only tablets behind the same hub.

In that case, I'd prefer this heuristic to be out. Besides the
multi-seats use case, I am not sure a user would bother having both a
pen-only tablet and a touch-only tablet, using both at the same time.

These are the cases where we can teach users to switch to no touch
arbitration or do a per-case entry in the list of devices saying that
they don't have any sibling (or can be matched with themself only).

> 
> >>
> >> Since the new arbitration rules only apply to HID_GENERIC devices and
> >> userspace will eventually take over the task anyway, I'm okay with
> >> either option personally.
> > 
> > Also, there is one thing that might have sense since you are now having
> > the heuristic only for hid-generic.
> > We might want to be sure to have the proper sibling matching on some
> > rare cases. So I think it should be interesting to have:
> > - .ovid/.opid == 0/0 meaning "match against the current device vid/pid"
> >   (like what the current code does)
> > - .ovid/.opid == 0xffff/0xffff (HID_ANY_ID) meaning "use the heuristic,
> >   you can have any other vid/pid"
> > - .ovid/.opid != 0/0 and not 0xffff/0xffff meaning "match only the
> >   specified vid/pid"
> > 
> > This would allow to register a new device using HID_GENERIC but with a
> > specific ovid/opid.
> > 
> > One extra check could also be that we are sure that the sibling device
> > also is registered as HID_GENERIC + .ovid/.opid == 0xffff/0xffff to
> > avoid matching against something we already fixed the ovid/opid...
> > 
> > This might be a little over-processed however :)
> > 
> > Cheers,
> > Benjamin
> > 
> 
> I kinda like the sound of it since it would make the logic a little more
> straightforward. The special ovid/opid meanings would apply equally to
> all devices, meaning I wouldn't have to anymore ignore the 0/0 case for
> HID_GENERIC.

Oh, then if it makes the logic simpler, go for it :)

Cheers,
Benjamin

> 
> Jason
> ---
> Now instead of four in the eights place /
> you’ve got three, ‘Cause you added one /
> (That is to say, eight) to the two, /
> But you can’t take seven from three, /
> So you look at the sixty-fours....

Re: [PATCH 1/2] HID: wacom: Add WACOM_DEVICETYPE_DIRECT for Cintiqs and similar

[Bastien Nocera]

On Mon, 2016-07-11 at 11:07 -0700, Jason Gerecke wrote:
> "Direct" input deviecs like Cintiqs and Tablet PCs set the
> INPUT_PROP_DIRECT

"devices"

> property to notify userspace that the sensor and screen are
> overlayed. This

"overlaid"

The huge "if" is quite horrible, FWIW.

Re: [PATCH 1/2] HID: wacom: Add WACOM_DEVICETYPE_DIRECT for Cintiqs and similar

[Benjamin Tissoires]

On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
> "Direct" input deviecs like Cintiqs and Tablet PCs set the INPUT_PROP_DIRECT
> property to notify userspace that the sensor and screen are overlayed. This
> information can also be useful elsewhere within the kernel driver, however,
> so we introduce a new WACOM_DEVICETYPE_DIRECT that signals this to other
> kernel code.
> 
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> ---
>  drivers/hid/wacom_wac.c | 40 ++++++++++++++++++++++++----------------
>  drivers/hid/wacom_wac.h |  1 +
>  2 files changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index e499cdb..2523a29 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1757,8 +1757,10 @@ void wacom_wac_usage_mapping(struct hid_device *hdev,
>  {
>  	struct wacom *wacom = hid_get_drvdata(hdev);
>  	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
> +	struct wacom_features *features = &wacom_wac->features;
>  
>  	/* currently, only direct devices have proper hid report descriptors */
> +	features->device_type |= WACOM_DEVICETYPE_DIRECT;
>  	__set_bit(INPUT_PROP_DIRECT, wacom_wac->pen_input->propbit);
>  	__set_bit(INPUT_PROP_DIRECT, wacom_wac->touch_input->propbit);

nitpick: you might as well remove these additions of INPUT_PROP_DIRECT,
as you are handling the test later and adding them no matter what.

Rest looks good to me.

Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

>  
> @@ -2465,6 +2467,19 @@ void wacom_setup_device_quirks(struct wacom *wacom)
>  	if (features->type == REMOTE)
>  		features->device_type = WACOM_DEVICETYPE_PAD;
>  
> +	if (features->type == PL          || features->type == DTU           ||
> +	    features->type == DTUS        || features->type == DTUSX         ||
> +	    features->type == WACOM_21UX2 || features->type == WACOM_22HD    ||
> +	    features->type == DTK         || features->type == WACOM_24HD    ||
> +	    features->type == WACOM_27QHD || features->type == CINTIQ_HYBRID ||
> +	    features->type == CINTIQ_COMPANION_2 || features->type == CINTIQ ||
> +	    features->type == WACOM_BEE   || features->type == WACOM_13HD    ||
> +	    features->type == WACOM_24HDT || features->type == WACOM_27QHDT  ||
> +	    features->type == TABLETPC    || features->type == TABLETPCE     ||
> +	    features->type == TABLETPC2FG || features->type == MTSCREEN      ||
> +	    features->type == MTTPC       || features->type == MTTPC_B)
> +	    features->device_type |= WACOM_DEVICETYPE_DIRECT;
> +
>  	if (wacom->hdev->bus == BUS_BLUETOOTH)
>  		features->quirks |= WACOM_QUIRK_BATTERY;
>  
> @@ -2516,6 +2531,10 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>  	input_abs_set_res(input_dev, ABS_X, features->x_resolution);
>  	input_abs_set_res(input_dev, ABS_Y, features->y_resolution);
>  
> +	if (features->device_type & WACOM_DEVICETYPE_DIRECT)
> +		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
> +	else
> +		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
>  
>  	switch (features->type) {
>  	case GRAPHIRE_BT:
> @@ -2540,8 +2559,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>  		__set_bit(BTN_TOOL_MOUSE, input_dev->keybit);
>  		__set_bit(BTN_STYLUS, input_dev->keybit);
>  		__set_bit(BTN_STYLUS2, input_dev->keybit);
> -
> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
>  		break;
>  
>  	case WACOM_27QHD:
> @@ -2556,7 +2573,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>  	case CINTIQ_COMPANION_2:
>  		input_set_abs_params(input_dev, ABS_Z, -900, 899, 0, 0);
>  		input_abs_set_res(input_dev, ABS_Z, 287);
> -		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
>  		wacom_setup_cintiq(wacom_wac);
>  		break;
>  
> @@ -2572,8 +2588,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>  		/* fall through */
>  
>  	case INTUOS:
> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> -
>  		wacom_setup_intuos(wacom_wac);
>  		break;
>  
> @@ -2583,8 +2597,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>  	case INTUOSPL:
>  	case INTUOS5S:
>  	case INTUOSPS:
> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> -
>  		input_set_abs_params(input_dev, ABS_DISTANCE, 0,
>  				      features->distance_max,
>  				      features->distance_fuzz, 0);
> @@ -2614,8 +2626,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>  		__set_bit(BTN_TOOL_RUBBER, input_dev->keybit);
>  		__set_bit(BTN_STYLUS, input_dev->keybit);
>  		__set_bit(BTN_STYLUS2, input_dev->keybit);
> -
> -		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
>  		break;
>  
>  	case PTU:
> @@ -2626,16 +2636,12 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>  		__set_bit(BTN_TOOL_PEN, input_dev->keybit);
>  		__set_bit(BTN_TOOL_RUBBER, input_dev->keybit);
>  		__set_bit(BTN_STYLUS, input_dev->keybit);
> -
> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
>  		break;
>  
>  	case INTUOSHT:
>  	case BAMBOO_PT:
>  	case BAMBOO_PEN:
>  	case INTUOSHT2:
> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> -
>  		if (features->type == INTUOSHT2) {
>  			wacom_setup_basic_pro_pen(wacom_wac);
>  		} else {
> @@ -2693,6 +2699,11 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
>  				  features->y_resolution);
>  	}
>  
> +	if (features->device_type & WACOM_DEVICETYPE_DIRECT)
> +		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
> +	else
> +		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> +
>  	switch (features->type) {
>  	case INTUOS5:
>  	case INTUOS5L:
> @@ -2700,8 +2711,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
>  	case INTUOSPL:
>  	case INTUOS5S:
>  	case INTUOSPS:
> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> -
>  		input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, features->x_max, 0, 0);
>  		input_set_abs_params(input_dev, ABS_MT_TOUCH_MINOR, 0, features->y_max, 0, 0);
>  		input_mt_init_slots(input_dev, features->touch_max, INPUT_MT_POINTER);
> @@ -2724,7 +2733,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
>  
>  	case TABLETPC:
>  	case TABLETPCE:
> -		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
>  		break;
>  
>  	case INTUOSHT:
> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
> index 8a8974c..7ad6273 100644
> --- a/drivers/hid/wacom_wac.h
> +++ b/drivers/hid/wacom_wac.h
> @@ -82,6 +82,7 @@
>  #define WACOM_DEVICETYPE_TOUCH          0x0002
>  #define WACOM_DEVICETYPE_PAD            0x0004
>  #define WACOM_DEVICETYPE_WL_MONITOR     0x0008
> +#define WACOM_DEVICETYPE_DIRECT         0x0010
>  
>  #define WACOM_VENDORDEFINED_PEN		0xff0d0001
>  #define WACOM_G9_PAGE			0xff090000
> -- 
> 2.9.0
> 

Re: [PATCH 1/2] HID: wacom: Add WACOM_DEVICETYPE_DIRECT for Cintiqs and similar

[Jason Gerecke]

On 07/12/2016 12:36 AM, Benjamin Tissoires wrote:
> On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
>> "Direct" input deviecs like Cintiqs and Tablet PCs set the INPUT_PROP_DIRECT
>> property to notify userspace that the sensor and screen are overlayed. This
>> information can also be useful elsewhere within the kernel driver, however,
>> so we introduce a new WACOM_DEVICETYPE_DIRECT that signals this to other
>> kernel code.
>>
>> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
>> ---
>>  drivers/hid/wacom_wac.c | 40 ++++++++++++++++++++++++----------------
>>  drivers/hid/wacom_wac.h |  1 +
>>  2 files changed, 25 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
>> index e499cdb..2523a29 100644
>> --- a/drivers/hid/wacom_wac.c
>> +++ b/drivers/hid/wacom_wac.c
>> @@ -1757,8 +1757,10 @@ void wacom_wac_usage_mapping(struct hid_device *hdev,
>>  {
>>  	struct wacom *wacom = hid_get_drvdata(hdev);
>>  	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
>> +	struct wacom_features *features = &wacom_wac->features;
>>  
>>  	/* currently, only direct devices have proper hid report descriptors */
>> +	features->device_type |= WACOM_DEVICETYPE_DIRECT;
>>  	__set_bit(INPUT_PROP_DIRECT, wacom_wac->pen_input->propbit);
>>  	__set_bit(INPUT_PROP_DIRECT, wacom_wac->touch_input->propbit);
> 
> nitpick: you might as well remove these additions of INPUT_PROP_DIRECT,
> as you are handling the test later and adding them no matter what.
> 

Are you seeing something I'm missing? The new calls in
wacom_setup_{pen,touch}_input_capabilities that setup INPUT_PROP_DIRECT
won't be called for HID_GENERIC devices since we bail out at the top of
the functions. Perhaps you'd prefer me to move their setting of
INPUT_PROP_DIRECT to before the point we bail, letting me remove them
from here?

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....

> Rest looks good to me.
> 
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> Cheers,
> Benjamin
> 
>>  
>> @@ -2465,6 +2467,19 @@ void wacom_setup_device_quirks(struct wacom *wacom)
>>  	if (features->type == REMOTE)
>>  		features->device_type = WACOM_DEVICETYPE_PAD;
>>  
>> +	if (features->type == PL          || features->type == DTU           ||
>> +	    features->type == DTUS        || features->type == DTUSX         ||
>> +	    features->type == WACOM_21UX2 || features->type == WACOM_22HD    ||
>> +	    features->type == DTK         || features->type == WACOM_24HD    ||
>> +	    features->type == WACOM_27QHD || features->type == CINTIQ_HYBRID ||
>> +	    features->type == CINTIQ_COMPANION_2 || features->type == CINTIQ ||
>> +	    features->type == WACOM_BEE   || features->type == WACOM_13HD    ||
>> +	    features->type == WACOM_24HDT || features->type == WACOM_27QHDT  ||
>> +	    features->type == TABLETPC    || features->type == TABLETPCE     ||
>> +	    features->type == TABLETPC2FG || features->type == MTSCREEN      ||
>> +	    features->type == MTTPC       || features->type == MTTPC_B)
>> +	    features->device_type |= WACOM_DEVICETYPE_DIRECT;
>> +
>>  	if (wacom->hdev->bus == BUS_BLUETOOTH)
>>  		features->quirks |= WACOM_QUIRK_BATTERY;
>>  
>> @@ -2516,6 +2531,10 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>>  	input_abs_set_res(input_dev, ABS_X, features->x_resolution);
>>  	input_abs_set_res(input_dev, ABS_Y, features->y_resolution);
>>  
>> +	if (features->device_type & WACOM_DEVICETYPE_DIRECT)
>> +		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
>> +	else
>> +		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
>>  
>>  	switch (features->type) {
>>  	case GRAPHIRE_BT:
>> @@ -2540,8 +2559,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>>  		__set_bit(BTN_TOOL_MOUSE, input_dev->keybit);
>>  		__set_bit(BTN_STYLUS, input_dev->keybit);
>>  		__set_bit(BTN_STYLUS2, input_dev->keybit);
>> -
>> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
>>  		break;
>>  
>>  	case WACOM_27QHD:
>> @@ -2556,7 +2573,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>>  	case CINTIQ_COMPANION_2:
>>  		input_set_abs_params(input_dev, ABS_Z, -900, 899, 0, 0);
>>  		input_abs_set_res(input_dev, ABS_Z, 287);
>> -		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
>>  		wacom_setup_cintiq(wacom_wac);
>>  		break;
>>  
>> @@ -2572,8 +2588,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>>  		/* fall through */
>>  
>>  	case INTUOS:
>> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
>> -
>>  		wacom_setup_intuos(wacom_wac);
>>  		break;
>>  
>> @@ -2583,8 +2597,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>>  	case INTUOSPL:
>>  	case INTUOS5S:
>>  	case INTUOSPS:
>> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
>> -
>>  		input_set_abs_params(input_dev, ABS_DISTANCE, 0,
>>  				      features->distance_max,
>>  				      features->distance_fuzz, 0);
>> @@ -2614,8 +2626,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>>  		__set_bit(BTN_TOOL_RUBBER, input_dev->keybit);
>>  		__set_bit(BTN_STYLUS, input_dev->keybit);
>>  		__set_bit(BTN_STYLUS2, input_dev->keybit);
>> -
>> -		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
>>  		break;
>>  
>>  	case PTU:
>> @@ -2626,16 +2636,12 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>>  		__set_bit(BTN_TOOL_PEN, input_dev->keybit);
>>  		__set_bit(BTN_TOOL_RUBBER, input_dev->keybit);
>>  		__set_bit(BTN_STYLUS, input_dev->keybit);
>> -
>> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
>>  		break;
>>  
>>  	case INTUOSHT:
>>  	case BAMBOO_PT:
>>  	case BAMBOO_PEN:
>>  	case INTUOSHT2:
>> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
>> -
>>  		if (features->type == INTUOSHT2) {
>>  			wacom_setup_basic_pro_pen(wacom_wac);
>>  		} else {
>> @@ -2693,6 +2699,11 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
>>  				  features->y_resolution);
>>  	}
>>  
>> +	if (features->device_type & WACOM_DEVICETYPE_DIRECT)
>> +		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
>> +	else
>> +		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
>> +
>>  	switch (features->type) {
>>  	case INTUOS5:
>>  	case INTUOS5L:
>> @@ -2700,8 +2711,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
>>  	case INTUOSPL:
>>  	case INTUOS5S:
>>  	case INTUOSPS:
>> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
>> -
>>  		input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, features->x_max, 0, 0);
>>  		input_set_abs_params(input_dev, ABS_MT_TOUCH_MINOR, 0, features->y_max, 0, 0);
>>  		input_mt_init_slots(input_dev, features->touch_max, INPUT_MT_POINTER);
>> @@ -2724,7 +2733,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
>>  
>>  	case TABLETPC:
>>  	case TABLETPCE:
>> -		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
>>  		break;
>>  
>>  	case INTUOSHT:
>> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
>> index 8a8974c..7ad6273 100644
>> --- a/drivers/hid/wacom_wac.h
>> +++ b/drivers/hid/wacom_wac.h
>> @@ -82,6 +82,7 @@
>>  #define WACOM_DEVICETYPE_TOUCH          0x0002
>>  #define WACOM_DEVICETYPE_PAD            0x0004
>>  #define WACOM_DEVICETYPE_WL_MONITOR     0x0008
>> +#define WACOM_DEVICETYPE_DIRECT         0x0010
>>  
>>  #define WACOM_VENDORDEFINED_PEN		0xff0d0001
>>  #define WACOM_G9_PAGE			0xff090000
>> -- 
>> 2.9.0
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] HID: wacom: Add WACOM_DEVICETYPE_DIRECT for Cintiqs and similar

[Benjamin Tissoires]

On Jul 20 2016 or thereabouts, Jason Gerecke wrote:
> On 07/12/2016 12:36 AM, Benjamin Tissoires wrote:
> > On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
> >> "Direct" input deviecs like Cintiqs and Tablet PCs set the INPUT_PROP_DIRECT
> >> property to notify userspace that the sensor and screen are overlayed. This
> >> information can also be useful elsewhere within the kernel driver, however,
> >> so we introduce a new WACOM_DEVICETYPE_DIRECT that signals this to other
> >> kernel code.
> >>
> >> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> >> ---
> >>  drivers/hid/wacom_wac.c | 40 ++++++++++++++++++++++++----------------
> >>  drivers/hid/wacom_wac.h |  1 +
> >>  2 files changed, 25 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> >> index e499cdb..2523a29 100644
> >> --- a/drivers/hid/wacom_wac.c
> >> +++ b/drivers/hid/wacom_wac.c
> >> @@ -1757,8 +1757,10 @@ void wacom_wac_usage_mapping(struct hid_device *hdev,
> >>  {
> >>  	struct wacom *wacom = hid_get_drvdata(hdev);
> >>  	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
> >> +	struct wacom_features *features = &wacom_wac->features;
> >>  
> >>  	/* currently, only direct devices have proper hid report descriptors */
> >> +	features->device_type |= WACOM_DEVICETYPE_DIRECT;
> >>  	__set_bit(INPUT_PROP_DIRECT, wacom_wac->pen_input->propbit);
> >>  	__set_bit(INPUT_PROP_DIRECT, wacom_wac->touch_input->propbit);
> > 
> > nitpick: you might as well remove these additions of INPUT_PROP_DIRECT,
> > as you are handling the test later and adding them no matter what.
> > 
> 
> Are you seeing something I'm missing? The new calls in
> wacom_setup_{pen,touch}_input_capabilities that setup INPUT_PROP_DIRECT
> won't be called for HID_GENERIC devices since we bail out at the top of

Oh, yes, you are right.

> the functions. Perhaps you'd prefer me to move their setting of
> INPUT_PROP_DIRECT to before the point we bail, letting me remove them
> from here?

I think it would be better to set the flag in wacom_wac_usage_mapping()
and rely on this flag to set INPUT_PROP_DIRECT (by moving up the
setting of INPUT_PROP_DIRECT). It feels weird to set the flag + the
consequences of the flag one after the other. This way, we could also
imagine that if indirect devices are around and use generic, we could
just remove the flag, and the INPUT_PROP will be removed without any
extra effort.

> 
> Jason
> ---
> Now instead of four in the eights place /
> you’ve got three, ‘Cause you added one /
> (That is to say, eight) to the two, /
> But you can’t take seven from three, /
> So you look at the sixty-fours....
> 
> > Rest looks good to me.
> > 
> > Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > 
> > Cheers,
> > Benjamin
> > 
> >>  
> >> @@ -2465,6 +2467,19 @@ void wacom_setup_device_quirks(struct wacom *wacom)
> >>  	if (features->type == REMOTE)
> >>  		features->device_type = WACOM_DEVICETYPE_PAD;
> >>  
> >> +	if (features->type == PL          || features->type == DTU           ||
> >> +	    features->type == DTUS        || features->type == DTUSX         ||
> >> +	    features->type == WACOM_21UX2 || features->type == WACOM_22HD    ||
> >> +	    features->type == DTK         || features->type == WACOM_24HD    ||
> >> +	    features->type == WACOM_27QHD || features->type == CINTIQ_HYBRID ||
> >> +	    features->type == CINTIQ_COMPANION_2 || features->type == CINTIQ ||
> >> +	    features->type == WACOM_BEE   || features->type == WACOM_13HD    ||
> >> +	    features->type == WACOM_24HDT || features->type == WACOM_27QHDT  ||
> >> +	    features->type == TABLETPC    || features->type == TABLETPCE     ||
> >> +	    features->type == TABLETPC2FG || features->type == MTSCREEN      ||
> >> +	    features->type == MTTPC       || features->type == MTTPC_B)
> >> +	    features->device_type |= WACOM_DEVICETYPE_DIRECT;

As Bastien said, this is ugly, and a switch/case fits better.

Cheers,
Benjamin

> >> +
> >>  	if (wacom->hdev->bus == BUS_BLUETOOTH)
> >>  		features->quirks |= WACOM_QUIRK_BATTERY;
> >>  
> >> @@ -2516,6 +2531,10 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
> >>  	input_abs_set_res(input_dev, ABS_X, features->x_resolution);
> >>  	input_abs_set_res(input_dev, ABS_Y, features->y_resolution);
> >>  
> >> +	if (features->device_type & WACOM_DEVICETYPE_DIRECT)
> >> +		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
> >> +	else
> >> +		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> >>  
> >>  	switch (features->type) {
> >>  	case GRAPHIRE_BT:
> >> @@ -2540,8 +2559,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
> >>  		__set_bit(BTN_TOOL_MOUSE, input_dev->keybit);
> >>  		__set_bit(BTN_STYLUS, input_dev->keybit);
> >>  		__set_bit(BTN_STYLUS2, input_dev->keybit);
> >> -
> >> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> >>  		break;
> >>  
> >>  	case WACOM_27QHD:
> >> @@ -2556,7 +2573,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
> >>  	case CINTIQ_COMPANION_2:
> >>  		input_set_abs_params(input_dev, ABS_Z, -900, 899, 0, 0);
> >>  		input_abs_set_res(input_dev, ABS_Z, 287);
> >> -		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
> >>  		wacom_setup_cintiq(wacom_wac);
> >>  		break;
> >>  
> >> @@ -2572,8 +2588,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
> >>  		/* fall through */
> >>  
> >>  	case INTUOS:
> >> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> >> -
> >>  		wacom_setup_intuos(wacom_wac);
> >>  		break;
> >>  
> >> @@ -2583,8 +2597,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
> >>  	case INTUOSPL:
> >>  	case INTUOS5S:
> >>  	case INTUOSPS:
> >> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> >> -
> >>  		input_set_abs_params(input_dev, ABS_DISTANCE, 0,
> >>  				      features->distance_max,
> >>  				      features->distance_fuzz, 0);
> >> @@ -2614,8 +2626,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
> >>  		__set_bit(BTN_TOOL_RUBBER, input_dev->keybit);
> >>  		__set_bit(BTN_STYLUS, input_dev->keybit);
> >>  		__set_bit(BTN_STYLUS2, input_dev->keybit);
> >> -
> >> -		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
> >>  		break;
> >>  
> >>  	case PTU:
> >> @@ -2626,16 +2636,12 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
> >>  		__set_bit(BTN_TOOL_PEN, input_dev->keybit);
> >>  		__set_bit(BTN_TOOL_RUBBER, input_dev->keybit);
> >>  		__set_bit(BTN_STYLUS, input_dev->keybit);
> >> -
> >> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> >>  		break;
> >>  
> >>  	case INTUOSHT:
> >>  	case BAMBOO_PT:
> >>  	case BAMBOO_PEN:
> >>  	case INTUOSHT2:
> >> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> >> -
> >>  		if (features->type == INTUOSHT2) {
> >>  			wacom_setup_basic_pro_pen(wacom_wac);
> >>  		} else {
> >> @@ -2693,6 +2699,11 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
> >>  				  features->y_resolution);
> >>  	}
> >>  
> >> +	if (features->device_type & WACOM_DEVICETYPE_DIRECT)
> >> +		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
> >> +	else
> >> +		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> >> +
> >>  	switch (features->type) {
> >>  	case INTUOS5:
> >>  	case INTUOS5L:
> >> @@ -2700,8 +2711,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
> >>  	case INTUOSPL:
> >>  	case INTUOS5S:
> >>  	case INTUOSPS:
> >> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> >> -
> >>  		input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, features->x_max, 0, 0);
> >>  		input_set_abs_params(input_dev, ABS_MT_TOUCH_MINOR, 0, features->y_max, 0, 0);
> >>  		input_mt_init_slots(input_dev, features->touch_max, INPUT_MT_POINTER);
> >> @@ -2724,7 +2733,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
> >>  
> >>  	case TABLETPC:
> >>  	case TABLETPCE:
> >> -		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
> >>  		break;
> >>  
> >>  	case INTUOSHT:
> >> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
> >> index 8a8974c..7ad6273 100644
> >> --- a/drivers/hid/wacom_wac.h
> >> +++ b/drivers/hid/wacom_wac.h
> >> @@ -82,6 +82,7 @@
> >>  #define WACOM_DEVICETYPE_TOUCH          0x0002
> >>  #define WACOM_DEVICETYPE_PAD            0x0004
> >>  #define WACOM_DEVICETYPE_WL_MONITOR     0x0008
> >> +#define WACOM_DEVICETYPE_DIRECT         0x0010
> >>  
> >>  #define WACOM_VENDORDEFINED_PEN		0xff0d0001
> >>  #define WACOM_G9_PAGE			0xff090000
> >> -- 
> >> 2.9.0
> >>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] HID: wacom: Add WACOM_DEVICETYPE_DIRECT for Cintiqs and similar

[Dmitry Torokhov]

On Fri, Jul 22, 2016 at 2:09 AM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> On Jul 20 2016 or thereabouts, Jason Gerecke wrote:
>> On 07/12/2016 12:36 AM, Benjamin Tissoires wrote:
>> > On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
>> >> "Direct" input deviecs like Cintiqs and Tablet PCs set the INPUT_PROP_DIRECT
>> >> property to notify userspace that the sensor and screen are overlayed. This
>> >> information can also be useful elsewhere within the kernel driver, however,
>> >> so we introduce a new WACOM_DEVICETYPE_DIRECT that signals this to other
>> >> kernel code.
>> >>
>> >> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
>> >> ---
>> >>  drivers/hid/wacom_wac.c | 40 ++++++++++++++++++++++++----------------
>> >>  drivers/hid/wacom_wac.h |  1 +
>> >>  2 files changed, 25 insertions(+), 16 deletions(-)
>> >>
>> >> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
>> >> index e499cdb..2523a29 100644
>> >> --- a/drivers/hid/wacom_wac.c
>> >> +++ b/drivers/hid/wacom_wac.c
>> >> @@ -1757,8 +1757,10 @@ void wacom_wac_usage_mapping(struct hid_device *hdev,
>> >>  {
>> >>    struct wacom *wacom = hid_get_drvdata(hdev);
>> >>    struct wacom_wac *wacom_wac = &wacom->wacom_wac;
>> >> +  struct wacom_features *features = &wacom_wac->features;
>> >>
>> >>    /* currently, only direct devices have proper hid report descriptors */
>> >> +  features->device_type |= WACOM_DEVICETYPE_DIRECT;
>> >>    __set_bit(INPUT_PROP_DIRECT, wacom_wac->pen_input->propbit);
>> >>    __set_bit(INPUT_PROP_DIRECT, wacom_wac->touch_input->propbit);
>> >
>> > nitpick: you might as well remove these additions of INPUT_PROP_DIRECT,
>> > as you are handling the test later and adding them no matter what.
>> >
>>
>> Are you seeing something I'm missing? The new calls in
>> wacom_setup_{pen,touch}_input_capabilities that setup INPUT_PROP_DIRECT
>> won't be called for HID_GENERIC devices since we bail out at the top of
>
> Oh, yes, you are right.
>
>> the functions. Perhaps you'd prefer me to move their setting of
>> INPUT_PROP_DIRECT to before the point we bail, letting me remove them
>> from here?
>
> I think it would be better to set the flag in wacom_wac_usage_mapping()
> and rely on this flag to set INPUT_PROP_DIRECT (by moving up the
> setting of INPUT_PROP_DIRECT). It feels weird to set the flag + the
> consequences of the flag one after the other. This way, we could also
> imagine that if indirect devices are around and use generic, we could
> just remove the flag, and the INPUT_PROP will be removed without any
> extra effort.
>
>>
>> Jason
>> ---
>> Now instead of four in the eights place /
>> you’ve got three, ‘Cause you added one /
>> (That is to say, eight) to the two, /
>> But you can’t take seven from three, /
>> So you look at the sixty-fours....
>>
>> > Rest looks good to me.
>> >
>> > Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> >
>> > Cheers,
>> > Benjamin
>> >
>> >>
>> >> @@ -2465,6 +2467,19 @@ void wacom_setup_device_quirks(struct wacom *wacom)
>> >>    if (features->type == REMOTE)
>> >>            features->device_type = WACOM_DEVICETYPE_PAD;
>> >>
>> >> +  if (features->type == PL          || features->type == DTU           ||
>> >> +      features->type == DTUS        || features->type == DTUSX         ||
>> >> +      features->type == WACOM_21UX2 || features->type == WACOM_22HD    ||
>> >> +      features->type == DTK         || features->type == WACOM_24HD    ||
>> >> +      features->type == WACOM_27QHD || features->type == CINTIQ_HYBRID ||
>> >> +      features->type == CINTIQ_COMPANION_2 || features->type == CINTIQ ||
>> >> +      features->type == WACOM_BEE   || features->type == WACOM_13HD    ||
>> >> +      features->type == WACOM_24HDT || features->type == WACOM_27QHDT  ||
>> >> +      features->type == TABLETPC    || features->type == TABLETPCE     ||
>> >> +      features->type == TABLETPC2FG || features->type == MTSCREEN      ||
>> >> +      features->type == MTTPC       || features->type == MTTPC_B)
>> >> +      features->device_type |= WACOM_DEVICETYPE_DIRECT;
>
> As Bastien said, this is ugly, and a switch/case fits better.

Or have an array and iterate through it...

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] HID: wacom: Add WACOM_DEVICETYPE_DIRECT for Cintiqs and similar

[Jason Gerecke]

"Direct" input devices like Cintiqs and Tablet PCs set the INPUT_PROP_DIRECT
property to notify userspace that the sensor and screen are overlaid. This
information can also be useful elsewhere within the kernel driver, however,
so we introduce a new WACOM_DEVICETYPE_DIRECT that signals this to other
kernel code.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/wacom_wac.c | 40 ++++++++++++++++++++++++----------------
 drivers/hid/wacom_wac.h |  1 +
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index d2611f3..9a13d09 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1742,8 +1742,10 @@ void wacom_wac_usage_mapping(struct hid_device *hdev,
 {
 	struct wacom *wacom = hid_get_drvdata(hdev);
 	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+	struct wacom_features *features = &wacom_wac->features;
 
 	/* currently, only direct devices have proper hid report descriptors */
+	features->device_type |= WACOM_DEVICETYPE_DIRECT;
 	__set_bit(INPUT_PROP_DIRECT, wacom_wac->pen_input->propbit);
 	__set_bit(INPUT_PROP_DIRECT, wacom_wac->touch_input->propbit);
 
@@ -2450,6 +2452,19 @@ void wacom_setup_device_quirks(struct wacom *wacom)
 	if (features->type == REMOTE)
 		features->device_type = WACOM_DEVICETYPE_PAD;
 
+	if (features->type == PL          || features->type == DTU           ||
+	    features->type == DTUS        || features->type == DTUSX         ||
+	    features->type == WACOM_21UX2 || features->type == WACOM_22HD    ||
+	    features->type == DTK         || features->type == WACOM_24HD    ||
+	    features->type == WACOM_27QHD || features->type == CINTIQ_HYBRID ||
+	    features->type == CINTIQ_COMPANION_2 || features->type == CINTIQ ||
+	    features->type == WACOM_BEE   || features->type == WACOM_13HD    ||
+	    features->type == WACOM_24HDT || features->type == WACOM_27QHDT  ||
+	    features->type == TABLETPC    || features->type == TABLETPCE     ||
+	    features->type == TABLETPC2FG || features->type == MTSCREEN      ||
+	    features->type == MTTPC       || features->type == MTTPC_B)
+	    features->device_type |= WACOM_DEVICETYPE_DIRECT;
+
 	if (wacom->hdev->bus == BUS_BLUETOOTH)
 		features->quirks |= WACOM_QUIRK_BATTERY;
 
@@ -2501,6 +2516,10 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 	input_abs_set_res(input_dev, ABS_X, features->x_resolution);
 	input_abs_set_res(input_dev, ABS_Y, features->y_resolution);
 
+	if (features->device_type & WACOM_DEVICETYPE_DIRECT)
+		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
+	else
+		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
 
 	switch (features->type) {
 	case GRAPHIRE_BT:
@@ -2525,8 +2544,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 		__set_bit(BTN_TOOL_MOUSE, input_dev->keybit);
 		__set_bit(BTN_STYLUS, input_dev->keybit);
 		__set_bit(BTN_STYLUS2, input_dev->keybit);
-
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
 		break;
 
 	case WACOM_27QHD:
@@ -2541,7 +2558,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 	case CINTIQ_COMPANION_2:
 		input_set_abs_params(input_dev, ABS_Z, -900, 899, 0, 0);
 		input_abs_set_res(input_dev, ABS_Z, 287);
-		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
 		wacom_setup_cintiq(wacom_wac);
 		break;
 
@@ -2557,8 +2573,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 		/* fall through */
 
 	case INTUOS:
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
-
 		wacom_setup_intuos(wacom_wac);
 		break;
 
@@ -2568,8 +2582,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 	case INTUOSPL:
 	case INTUOS5S:
 	case INTUOSPS:
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
-
 		input_set_abs_params(input_dev, ABS_DISTANCE, 0,
 				      features->distance_max,
 				      features->distance_fuzz, 0);
@@ -2599,8 +2611,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 		__set_bit(BTN_TOOL_RUBBER, input_dev->keybit);
 		__set_bit(BTN_STYLUS, input_dev->keybit);
 		__set_bit(BTN_STYLUS2, input_dev->keybit);
-
-		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
 		break;
 
 	case PTU:
@@ -2611,16 +2621,12 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 		__set_bit(BTN_TOOL_PEN, input_dev->keybit);
 		__set_bit(BTN_TOOL_RUBBER, input_dev->keybit);
 		__set_bit(BTN_STYLUS, input_dev->keybit);
-
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
 		break;
 
 	case INTUOSHT:
 	case BAMBOO_PT:
 	case BAMBOO_PEN:
 	case INTUOSHT2:
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
-
 		if (features->type == INTUOSHT2) {
 			wacom_setup_basic_pro_pen(wacom_wac);
 		} else {
@@ -2678,6 +2684,11 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 				  features->y_resolution);
 	}
 
+	if (features->device_type & WACOM_DEVICETYPE_DIRECT)
+		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
+	else
+		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
+
 	switch (features->type) {
 	case INTUOS5:
 	case INTUOS5L:
@@ -2685,8 +2696,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 	case INTUOSPL:
 	case INTUOS5S:
 	case INTUOSPS:
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
-
 		input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, features->x_max, 0, 0);
 		input_set_abs_params(input_dev, ABS_MT_TOUCH_MINOR, 0, features->y_max, 0, 0);
 		input_mt_init_slots(input_dev, features->touch_max, INPUT_MT_POINTER);
@@ -2709,7 +2718,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 
 	case TABLETPC:
 	case TABLETPCE:
-		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
 		break;
 
 	case INTUOSHT:
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index 267025c..5624268 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -82,6 +82,7 @@
 #define WACOM_DEVICETYPE_TOUCH          0x0002
 #define WACOM_DEVICETYPE_PAD            0x0004
 #define WACOM_DEVICETYPE_WL_MONITOR     0x0008
+#define WACOM_DEVICETYPE_DIRECT         0x0010
 
 #define WACOM_VENDORDEFINED_PEN		0xff0d0001
 #define WACOM_G9_PAGE			0xff090000
-- 
2.9.0

[PATCH v2 2/2] HID: wacom: Replace 'oVid' and 'oPid' with heuristics

[Jason Gerecke]

The 'oVid' and 'oPid' variables used by wacom_are_sibling are a hacky
solution to the problem of the driver historically having few good
heuristics to use in determining if two devices should be considered
siblings or not. While it works well enough for explicitly supported
devices, it offers no help for HID_GENERIC devices. Now that we have
a bit more information (e.g. direct/indirect) available to us though,
we should be able to safely replace oVid/oPid with heuristics that
work in most circumstances.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
Changes from v1:
 * Slightly modified commit message to make motivation clearer

 * Introduce 'compare_device_paths' function to reduce code duplication

 * Move descriptions of each huristic from commit message to in-line
   comments.

 * Comment style updated to appease our benevolent dictator for life

 * Change order of checks, placing path checks at the end of
   wacom_are_sibling

 * Break check of equal "directness" into two seperate checks, one for
   direct/indirect and one for indirect/direct.

!! NOTE !! The "Devices with different VID/PIDs may not be siblings unless
they are direct input devices" check from v1 remains since I have not yet
heard back from Benjamin regarding if he thinks the potential problems its
presence may cuase outweigh those its lack of presence can cause.


 drivers/hid/wacom_sys.c | 80 +++++++++++++++++++++++++++++++++++++++++--------
 drivers/hid/wacom_wac.c | 41 ++++++++++---------------
 drivers/hid/wacom_wac.h |  2 --
 3 files changed, 84 insertions(+), 39 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 4a0bb6f..6a9208c 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -527,36 +527,92 @@ struct wacom_hdev_data {
 static LIST_HEAD(wacom_udev_list);
 static DEFINE_MUTEX(wacom_udev_list_lock);
 
+static bool compare_device_paths(struct hid_device *hdev_a,
+		struct hid_device *hdev_b, char separator)
+{
+	int n1 = strrchr(hdev_a->phys, separator) - hdev_a->phys;
+	int n2 = strrchr(hdev_b->phys, separator) - hdev_b->phys;
+
+	if (n1 != n2 || n1 <= 0 || n2 <= 0)
+		return false;
+
+	return !strncmp(hdev_a->phys, hdev_b->phys, n1);
+}
+
 static bool wacom_are_sibling(struct hid_device *hdev,
 		struct hid_device *sibling)
 {
 	struct wacom *wacom = hid_get_drvdata(hdev);
 	struct wacom_features *features = &wacom->wacom_wac.features;
-	int vid = features->oVid;
-	int pid = features->oPid;
-	int n1,n2;
+	struct wacom *sibling_wacom = hid_get_drvdata(sibling);
+	struct wacom_features *sibling_features = &sibling_wacom->wacom_wac.features;
 
-	if (vid == 0 && pid == 0) {
-		vid = hdev->vendor;
-		pid = hdev->product;
+	/*
+	 * Devices with different VID/PIDs may not be siblings unless
+	 * they are direct input devices.
+	 */
+	if (hdev->vendor != sibling->vendor || hdev->product != sibling->product) {
+		if (!(features->device_type & WACOM_DEVICETYPE_DIRECT))
+			return false;
 	}
 
-	if (vid != sibling->vendor || pid != sibling->product)
+	/*
+	 * Direct-input devices may not be siblings of indirect-input
+	 * devices.
+	 */
+	if ((features->device_type & WACOM_DEVICETYPE_DIRECT) &&
+	    !(sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
 		return false;
 
-	/* Compare the physical path. */
-	n1 = strrchr(hdev->phys, '.') - hdev->phys;
-	n2 = strrchr(sibling->phys, '.') - sibling->phys;
-	if (n1 != n2 || n1 <= 0 || n2 <= 0)
+	/*
+	 * Indirect-input devices may not be siblings of direct-input
+	 * devices.
+	 */
+	if (!(features->device_type & WACOM_DEVICETYPE_DIRECT) &&
+	    (sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
 		return false;
 
-	return !strncmp(hdev->phys, sibling->phys, n1);
+	/* Pen devices may only be siblings of touch devices */
+	if ((features->device_type & WACOM_DEVICETYPE_PEN) &&
+	    !(sibling_features->device_type & WACOM_DEVICETYPE_TOUCH))
+		return false;
+
+	/* Touch devices may only be siblings of pen devices */
+	if ((features->device_type & WACOM_DEVICETYPE_TOUCH) &&
+	    !(sibling_features->device_type & WACOM_DEVICETYPE_PEN))
+		return false;
+
+	/*
+	 * Devices with the same VID/PID must share the same physical
+	 * device path, while those with different VID/PID must share
+	 * the same physical parent device path.
+	 */
+	if (hdev->vendor == sibling->vendor && hdev->product == sibling->product) {
+		if (!compare_device_paths(hdev, sibling, '/'))
+			return false;
+	} else {
+		if (!compare_device_paths(hdev, sibling, '.'))
+			return false;
+	}
+
+	/*
+	 * No reason could be found for these two devices to NOT be
+	 * siblings, so there's a good chance they ARE siblings
+	 */
+	return true;
 }
 
 static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev)
 {
 	struct wacom_hdev_data *data;
 
+	/* Try to find an already-probed interface from the same device */
+	list_for_each_entry(data, &wacom_udev_list, list) {
+		if (compare_device_paths(hdev, data->dev, '/'))
+			return data;
+	}
+
+	/* Fallback to finding devices that appear to be "siblings" */
 	list_for_each_entry(data, &wacom_udev_list, list) {
 		if (wacom_are_sibling(hdev, data->dev)) {
 			kref_get(&data->kref);
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 9a13d09..093c5a5 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -3214,11 +3214,10 @@ static const struct wacom_features wacom_features_0xF4 =
 static const struct wacom_features wacom_features_0xF8 =
 	{ "Wacom Cintiq 24HD touch", 104080, 65200, 2047, 63, /* Pen */
 	  WACOM_24HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 16,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0xf6 };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0xF6 =
 	{ "Wacom Cintiq 24HD touch", .type = WACOM_24HDT, /* Touch */
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0xf8, .touch_max = 10,
+	  .touch_max = 10,
 	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
 static const struct wacom_features wacom_features_0x32A =
 	{ "Wacom Cintiq 27QHD", 119740, 67520, 2047, 63,
@@ -3227,11 +3226,10 @@ static const struct wacom_features wacom_features_0x32A =
 static const struct wacom_features wacom_features_0x32B =
 	{ "Wacom Cintiq 27QHD touch", 119740, 67520, 2047, 63,
 	  WACOM_27QHD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 0,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x32C };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x32C =
 	{ "Wacom Cintiq 27QHD touch", .type = WACOM_27QHDT,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x32B, .touch_max = 10 };
+	  .touch_max = 10 };
 static const struct wacom_features wacom_features_0x3F =
 	{ "Wacom Cintiq 21UX", 87200, 65600, 1023, 63,
 	  CINTIQ, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 8 };
@@ -3248,11 +3246,10 @@ static const struct wacom_features wacom_features_0x304 =
 static const struct wacom_features wacom_features_0x333 =
 	{ "Wacom Cintiq 13HD touch", 59152, 33448, 2047, 63,
 	  WACOM_13HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x335 };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x335 =
 	{ "Wacom Cintiq 13HD touch", .type = WACOM_24HDT, /* Touch */
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x333, .touch_max = 10,
+	  .touch_max = 10,
 	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
 static const struct wacom_features wacom_features_0xC7 =
 	{ "Wacom DTU1931", 37832, 30305, 511, 0,
@@ -3283,11 +3280,10 @@ static const struct wacom_features wacom_features_0x57 =
 static const struct wacom_features wacom_features_0x59 = /* Pen */
 	{ "Wacom DTH2242", 95640, 54060, 2047, 63,
 	  DTK, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 6,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5D };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x5D = /* Touch */
 	{ "Wacom DTH2242",       .type = WACOM_24HDT,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x59, .touch_max = 10,
+	  .touch_max = 10,
 	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
 static const struct wacom_features wacom_features_0xCC =
 	{ "Wacom Cintiq 21UX2", 86800, 65200, 2047, 63,
@@ -3300,11 +3296,10 @@ static const struct wacom_features wacom_features_0xFA =
 static const struct wacom_features wacom_features_0x5B =
 	{ "Wacom Cintiq 22HDT", 95440, 53860, 2047, 63,
 	  WACOM_22HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 18,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5e };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x5E =
 	{ "Wacom Cintiq 22HDT", .type = WACOM_24HDT,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5b, .touch_max = 10,
+	  .touch_max = 10,
 	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
 static const struct wacom_features wacom_features_0x90 =
 	{ "Wacom ISDv4 90", 26202, 16325, 255, 0,
@@ -3446,20 +3441,18 @@ static const struct wacom_features wacom_features_0x6004 =
 static const struct wacom_features wacom_features_0x307 =
 	{ "Wacom ISDv5 307", 59152, 33448, 2047, 63,
 	  CINTIQ_HYBRID, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x309 };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x309 =
 	{ "Wacom ISDv5 309", .type = WACOM_24HDT, /* Touch */
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x0307, .touch_max = 10,
+	  .touch_max = 10,
 	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
 static const struct wacom_features wacom_features_0x30A =
 	{ "Wacom ISDv5 30A", 59152, 33448, 2047, 63,
 	  CINTIQ_HYBRID, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x30C };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x30C =
 	{ "Wacom ISDv5 30C", .type = WACOM_24HDT, /* Touch */
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x30A, .touch_max = 10,
+	  .touch_max = 10,
 	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
 static const struct wacom_features wacom_features_0x318 =
 	{ "Wacom USB Bamboo PAD", 4095, 4095, /* Touch */
@@ -3470,11 +3463,9 @@ static const struct wacom_features wacom_features_0x319 =
 static const struct wacom_features wacom_features_0x325 =
 	{ "Wacom ISDv5 325", 59552, 33848, 2047, 63,
 	  CINTIQ_COMPANION_2, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 11,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x326 };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x326 = /* Touch */
-	{ "Wacom ISDv5 326", .type = HID_GENERIC, .oVid = USB_VENDOR_ID_WACOM,
-	  .oPid = 0x325 };
+	{ "Wacom ISDv5 326", .type = HID_GENERIC };
 static const struct wacom_features wacom_features_0x323 =
 	{ "Wacom Intuos P M", 21600, 13500, 1023, 31,
 	  INTUOSHT, WACOM_INTUOS_RES, WACOM_INTUOS_RES,
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index 5624268..77e7531 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -181,8 +181,6 @@ struct wacom_features {
 	int tilt_fuzz;
 	unsigned quirks;
 	unsigned touch_max;
-	int oVid;
-	int oPid;
 	int pktlen;
 	bool check_for_hid_type;
 	int hid_type;
-- 
2.9.0

[PATCH v3 1/2] HID: wacom: Add WACOM_DEVICETYPE_DIRECT for Cintiqs and similar

[Jason Gerecke]

"Direct" input devices like Cintiqs and Tablet PCs set the INPUT_PROP_DIRECT
property to notify userspace that the sensor and screen are overlaid. This
information can also be useful elsewhere within the kernel driver, however,
so we introduce a new WACOM_DEVICETYPE_DIRECT that signals this to other
kernel code.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
Changes from v2:
 * Moved setting of INPUT_PROP_DIRECT in wacom_setup_pen_input_capabilities
   and wacom_setup_touch_input_capabilities to occur before the functions
   exit if a HID_GENERIC device is being probed.

 * Changed large 'if' to 'switch' statement

 drivers/hid/wacom_wac.c | 58 +++++++++++++++++++++++++++++++++----------------
 drivers/hid/wacom_wac.h |  1 +
 2 files changed, 40 insertions(+), 19 deletions(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index d2611f3..8621d49 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1742,10 +1742,10 @@ void wacom_wac_usage_mapping(struct hid_device *hdev,
 {
 	struct wacom *wacom = hid_get_drvdata(hdev);
 	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+	struct wacom_features *features = &wacom_wac->features;
 
 	/* currently, only direct devices have proper hid report descriptors */
-	__set_bit(INPUT_PROP_DIRECT, wacom_wac->pen_input->propbit);
-	__set_bit(INPUT_PROP_DIRECT, wacom_wac->touch_input->propbit);
+	features->device_type |= WACOM_DEVICETYPE_DIRECT;
 
 	if (WACOM_PEN_FIELD(field))
 		return wacom_wac_pen_usage_mapping(hdev, field, usage);
@@ -2450,6 +2450,33 @@ void wacom_setup_device_quirks(struct wacom *wacom)
 	if (features->type == REMOTE)
 		features->device_type = WACOM_DEVICETYPE_PAD;
 
+	switch (features->type) {
+	case PL:
+	case DTU:
+	case DTUS:
+	case DTUSX:
+	case WACOM_21UX2:
+	case WACOM_22HD:
+	case DTK:
+	case WACOM24_HD:
+	case WACOM_27QHD:
+	case CINTIQ_HYBRID:
+	case CINTIQ_COMPANION_2:
+	case CINTIQ:
+	case WACOM_BEE:
+	case WACOM_13HD:
+	case WACOM_24HDT:
+	case WACOM_27QHDT:
+	case TABLETPC:
+	case TABLETPCE:
+	case TABLETPC2FG:
+	case MTSCREEN:
+	case MTTPC:
+	case MTTPC_B:
+		features->device_type |= WACOM_DEVICETYPE_DIRECT;
+		break;
+	}
+
 	if (wacom->hdev->bus == BUS_BLUETOOTH)
 		features->quirks |= WACOM_QUIRK_BATTERY;
 
@@ -2483,6 +2510,11 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 	if (!(features->device_type & WACOM_DEVICETYPE_PEN))
 		return -ENODEV;
 
+	if (features->device_type & WACOM_DEVICETYPE_DIRECT)
+		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
+	else
+		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
+
 	if (features->type == HID_GENERIC)
 		/* setup has already been done */
 		return 0;
@@ -2501,7 +2533,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 	input_abs_set_res(input_dev, ABS_X, features->x_resolution);
 	input_abs_set_res(input_dev, ABS_Y, features->y_resolution);
 
-
 	switch (features->type) {
 	case GRAPHIRE_BT:
 		__clear_bit(ABS_MISC, input_dev->absbit);
@@ -2525,8 +2556,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 		__set_bit(BTN_TOOL_MOUSE, input_dev->keybit);
 		__set_bit(BTN_STYLUS, input_dev->keybit);
 		__set_bit(BTN_STYLUS2, input_dev->keybit);
-
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
 		break;
 
 	case WACOM_27QHD:
@@ -2541,7 +2570,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 	case CINTIQ_COMPANION_2:
 		input_set_abs_params(input_dev, ABS_Z, -900, 899, 0, 0);
 		input_abs_set_res(input_dev, ABS_Z, 287);
-		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
 		wacom_setup_cintiq(wacom_wac);
 		break;
 
@@ -2557,8 +2585,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 		/* fall through */
 
 	case INTUOS:
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
-
 		wacom_setup_intuos(wacom_wac);
 		break;
 
@@ -2568,8 +2594,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 	case INTUOSPL:
 	case INTUOS5S:
 	case INTUOSPS:
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
-
 		input_set_abs_params(input_dev, ABS_DISTANCE, 0,
 				      features->distance_max,
 				      features->distance_fuzz, 0);
@@ -2599,8 +2623,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 		__set_bit(BTN_TOOL_RUBBER, input_dev->keybit);
 		__set_bit(BTN_STYLUS, input_dev->keybit);
 		__set_bit(BTN_STYLUS2, input_dev->keybit);
-
-		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
 		break;
 
 	case PTU:
@@ -2611,16 +2633,12 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 		__set_bit(BTN_TOOL_PEN, input_dev->keybit);
 		__set_bit(BTN_TOOL_RUBBER, input_dev->keybit);
 		__set_bit(BTN_STYLUS, input_dev->keybit);
-
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
 		break;
 
 	case INTUOSHT:
 	case BAMBOO_PT:
 	case BAMBOO_PEN:
 	case INTUOSHT2:
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
-
 		if (features->type == INTUOSHT2) {
 			wacom_setup_basic_pro_pen(wacom_wac);
 		} else {
@@ -2651,6 +2669,11 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 	if (!(features->device_type & WACOM_DEVICETYPE_TOUCH))
 		return -ENODEV;
 
+	if (features->device_type & WACOM_DEVICETYPE_DIRECT)
+		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
+	else
+		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
+
 	if (features->type == HID_GENERIC)
 		/* setup has already been done */
 		return 0;
@@ -2685,8 +2708,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 	case INTUOSPL:
 	case INTUOS5S:
 	case INTUOSPS:
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
-
 		input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, features->x_max, 0, 0);
 		input_set_abs_params(input_dev, ABS_MT_TOUCH_MINOR, 0, features->y_max, 0, 0);
 		input_mt_init_slots(input_dev, features->touch_max, INPUT_MT_POINTER);
@@ -2709,7 +2730,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 
 	case TABLETPC:
 	case TABLETPCE:
-		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
 		break;
 
 	case INTUOSHT:
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index 267025c..5624268 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -82,6 +82,7 @@
 #define WACOM_DEVICETYPE_TOUCH          0x0002
 #define WACOM_DEVICETYPE_PAD            0x0004
 #define WACOM_DEVICETYPE_WL_MONITOR     0x0008
+#define WACOM_DEVICETYPE_DIRECT         0x0010
 
 #define WACOM_VENDORDEFINED_PEN		0xff0d0001
 #define WACOM_G9_PAGE			0xff090000
-- 
2.9.0

[PATCH v3 2/2] HID: wacom: Replace 'oVid' and 'oPid' with heuristics

[Jason Gerecke]

The 'oVid' and 'oPid' variables used by wacom_are_sibling are a hacky
solution to the problem of the driver historically having few good
heuristics to use in determining if two devices should be considered
siblings or not. While it works well enough for explicitly supported
devices, it offers no help for HID_GENERIC devices. Now that we have
a bit more information (e.g. direct/indirect) available to us though,
we should be able to safely replace oVid/oPid with heuristics that
work in most circumstances.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
Changes from v2:
 * None

 drivers/hid/wacom_sys.c | 80 +++++++++++++++++++++++++++++++++++++++++--------
 drivers/hid/wacom_wac.c | 41 ++++++++++---------------
 drivers/hid/wacom_wac.h |  2 --
 3 files changed, 84 insertions(+), 39 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 4a0bb6f..6a9208c 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -527,36 +527,92 @@ struct wacom_hdev_data {
 static LIST_HEAD(wacom_udev_list);
 static DEFINE_MUTEX(wacom_udev_list_lock);
 
+static bool compare_device_paths(struct hid_device *hdev_a,
+		struct hid_device *hdev_b, char separator)
+{
+	int n1 = strrchr(hdev_a->phys, separator) - hdev_a->phys;
+	int n2 = strrchr(hdev_b->phys, separator) - hdev_b->phys;
+
+	if (n1 != n2 || n1 <= 0 || n2 <= 0)
+		return false;
+
+	return !strncmp(hdev_a->phys, hdev_b->phys, n1);
+}
+
 static bool wacom_are_sibling(struct hid_device *hdev,
 		struct hid_device *sibling)
 {
 	struct wacom *wacom = hid_get_drvdata(hdev);
 	struct wacom_features *features = &wacom->wacom_wac.features;
-	int vid = features->oVid;
-	int pid = features->oPid;
-	int n1,n2;
+	struct wacom *sibling_wacom = hid_get_drvdata(sibling);
+	struct wacom_features *sibling_features = &sibling_wacom->wacom_wac.features;
 
-	if (vid == 0 && pid == 0) {
-		vid = hdev->vendor;
-		pid = hdev->product;
+	/*
+	 * Devices with different VID/PIDs may not be siblings unless
+	 * they are direct input devices.
+	 */
+	if (hdev->vendor != sibling->vendor || hdev->product != sibling->product) {
+		if (!(features->device_type & WACOM_DEVICETYPE_DIRECT))
+			return false;
 	}
 
-	if (vid != sibling->vendor || pid != sibling->product)
+	/*
+	 * Direct-input devices may not be siblings of indirect-input
+	 * devices.
+	 */
+	if ((features->device_type & WACOM_DEVICETYPE_DIRECT) &&
+	    !(sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
 		return false;
 
-	/* Compare the physical path. */
-	n1 = strrchr(hdev->phys, '.') - hdev->phys;
-	n2 = strrchr(sibling->phys, '.') - sibling->phys;
-	if (n1 != n2 || n1 <= 0 || n2 <= 0)
+	/*
+	 * Indirect-input devices may not be siblings of direct-input
+	 * devices.
+	 */
+	if (!(features->device_type & WACOM_DEVICETYPE_DIRECT) &&
+	    (sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
 		return false;
 
-	return !strncmp(hdev->phys, sibling->phys, n1);
+	/* Pen devices may only be siblings of touch devices */
+	if ((features->device_type & WACOM_DEVICETYPE_PEN) &&
+	    !(sibling_features->device_type & WACOM_DEVICETYPE_TOUCH))
+		return false;
+
+	/* Touch devices may only be siblings of pen devices */
+	if ((features->device_type & WACOM_DEVICETYPE_TOUCH) &&
+	    !(sibling_features->device_type & WACOM_DEVICETYPE_PEN))
+		return false;
+
+	/*
+	 * Devices with the same VID/PID must share the same physical
+	 * device path, while those with different VID/PID must share
+	 * the same physical parent device path.
+	 */
+	if (hdev->vendor == sibling->vendor && hdev->product == sibling->product) {
+		if (!compare_device_paths(hdev, sibling, '/'))
+			return false;
+	} else {
+		if (!compare_device_paths(hdev, sibling, '.'))
+			return false;
+	}
+
+	/*
+	 * No reason could be found for these two devices to NOT be
+	 * siblings, so there's a good chance they ARE siblings
+	 */
+	return true;
 }
 
 static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev)
 {
 	struct wacom_hdev_data *data;
 
+	/* Try to find an already-probed interface from the same device */
+	list_for_each_entry(data, &wacom_udev_list, list) {
+		if (compare_device_paths(hdev, data->dev, '/'))
+			return data;
+	}
+
+	/* Fallback to finding devices that appear to be "siblings" */
 	list_for_each_entry(data, &wacom_udev_list, list) {
 		if (wacom_are_sibling(hdev, data->dev)) {
 			kref_get(&data->kref);
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 8621d49..40c1dae 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -3226,11 +3226,10 @@ static const struct wacom_features wacom_features_0xF4 =
 static const struct wacom_features wacom_features_0xF8 =
 	{ "Wacom Cintiq 24HD touch", 104080, 65200, 2047, 63, /* Pen */
 	  WACOM_24HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 16,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0xf6 };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0xF6 =
 	{ "Wacom Cintiq 24HD touch", .type = WACOM_24HDT, /* Touch */
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0xf8, .touch_max = 10,
+	  .touch_max = 10,
 	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
 static const struct wacom_features wacom_features_0x32A =
 	{ "Wacom Cintiq 27QHD", 119740, 67520, 2047, 63,
@@ -3239,11 +3238,10 @@ static const struct wacom_features wacom_features_0x32A =
 static const struct wacom_features wacom_features_0x32B =
 	{ "Wacom Cintiq 27QHD touch", 119740, 67520, 2047, 63,
 	  WACOM_27QHD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 0,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x32C };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x32C =
 	{ "Wacom Cintiq 27QHD touch", .type = WACOM_27QHDT,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x32B, .touch_max = 10 };
+	  .touch_max = 10 };
 static const struct wacom_features wacom_features_0x3F =
 	{ "Wacom Cintiq 21UX", 87200, 65600, 1023, 63,
 	  CINTIQ, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 8 };
@@ -3260,11 +3258,10 @@ static const struct wacom_features wacom_features_0x304 =
 static const struct wacom_features wacom_features_0x333 =
 	{ "Wacom Cintiq 13HD touch", 59152, 33448, 2047, 63,
 	  WACOM_13HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x335 };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x335 =
 	{ "Wacom Cintiq 13HD touch", .type = WACOM_24HDT, /* Touch */
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x333, .touch_max = 10,
+	  .touch_max = 10,
 	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
 static const struct wacom_features wacom_features_0xC7 =
 	{ "Wacom DTU1931", 37832, 30305, 511, 0,
@@ -3295,11 +3292,10 @@ static const struct wacom_features wacom_features_0x57 =
 static const struct wacom_features wacom_features_0x59 = /* Pen */
 	{ "Wacom DTH2242", 95640, 54060, 2047, 63,
 	  DTK, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 6,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5D };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x5D = /* Touch */
 	{ "Wacom DTH2242",       .type = WACOM_24HDT,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x59, .touch_max = 10,
+	  .touch_max = 10,
 	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
 static const struct wacom_features wacom_features_0xCC =
 	{ "Wacom Cintiq 21UX2", 86800, 65200, 2047, 63,
@@ -3312,11 +3308,10 @@ static const struct wacom_features wacom_features_0xFA =
 static const struct wacom_features wacom_features_0x5B =
 	{ "Wacom Cintiq 22HDT", 95440, 53860, 2047, 63,
 	  WACOM_22HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 18,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5e };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x5E =
 	{ "Wacom Cintiq 22HDT", .type = WACOM_24HDT,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5b, .touch_max = 10,
+	  .touch_max = 10,
 	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
 static const struct wacom_features wacom_features_0x90 =
 	{ "Wacom ISDv4 90", 26202, 16325, 255, 0,
@@ -3458,20 +3453,18 @@ static const struct wacom_features wacom_features_0x6004 =
 static const struct wacom_features wacom_features_0x307 =
 	{ "Wacom ISDv5 307", 59152, 33448, 2047, 63,
 	  CINTIQ_HYBRID, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x309 };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x309 =
 	{ "Wacom ISDv5 309", .type = WACOM_24HDT, /* Touch */
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x0307, .touch_max = 10,
+	  .touch_max = 10,
 	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
 static const struct wacom_features wacom_features_0x30A =
 	{ "Wacom ISDv5 30A", 59152, 33448, 2047, 63,
 	  CINTIQ_HYBRID, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x30C };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x30C =
 	{ "Wacom ISDv5 30C", .type = WACOM_24HDT, /* Touch */
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x30A, .touch_max = 10,
+	  .touch_max = 10,
 	  .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE };
 static const struct wacom_features wacom_features_0x318 =
 	{ "Wacom USB Bamboo PAD", 4095, 4095, /* Touch */
@@ -3482,11 +3475,9 @@ static const struct wacom_features wacom_features_0x319 =
 static const struct wacom_features wacom_features_0x325 =
 	{ "Wacom ISDv5 325", 59552, 33848, 2047, 63,
 	  CINTIQ_COMPANION_2, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 11,
-	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET,
-	  .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x326 };
+	  WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET };
 static const struct wacom_features wacom_features_0x326 = /* Touch */
-	{ "Wacom ISDv5 326", .type = HID_GENERIC, .oVid = USB_VENDOR_ID_WACOM,
-	  .oPid = 0x325 };
+	{ "Wacom ISDv5 326", .type = HID_GENERIC };
 static const struct wacom_features wacom_features_0x323 =
 	{ "Wacom Intuos P M", 21600, 13500, 1023, 31,
 	  INTUOSHT, WACOM_INTUOS_RES, WACOM_INTUOS_RES,
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index 5624268..77e7531 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -181,8 +181,6 @@ struct wacom_features {
 	int tilt_fuzz;
 	unsigned quirks;
 	unsigned touch_max;
-	int oVid;
-	int oPid;
 	int pktlen;
 	bool check_for_hid_type;
 	int hid_type;
-- 
2.9.0

Re: [PATCH v3 1/2] HID: wacom: Add WACOM_DEVICETYPE_DIRECT for Cintiqs and similar

[Benjamin Tissoires]

On Jul 22 2016 or thereabouts, Jason Gerecke wrote:
> "Direct" input devices like Cintiqs and Tablet PCs set the INPUT_PROP_DIRECT
> property to notify userspace that the sensor and screen are overlaid. This
> information can also be useful elsewhere within the kernel driver, however,
> so we introduce a new WACOM_DEVICETYPE_DIRECT that signals this to other
> kernel code.
> 
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> Changes from v2:
>  * Moved setting of INPUT_PROP_DIRECT in wacom_setup_pen_input_capabilities
>    and wacom_setup_touch_input_capabilities to occur before the functions
>    exit if a HID_GENERIC device is being probed.
> 
>  * Changed large 'if' to 'switch' statement

This one (1/2 only) is
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

See my comments in the v1 for 2/2.

Cheers,
Benjamin

> 
>  drivers/hid/wacom_wac.c | 58 +++++++++++++++++++++++++++++++++----------------
>  drivers/hid/wacom_wac.h |  1 +
>  2 files changed, 40 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index d2611f3..8621d49 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1742,10 +1742,10 @@ void wacom_wac_usage_mapping(struct hid_device *hdev,
>  {
>  	struct wacom *wacom = hid_get_drvdata(hdev);
>  	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
> +	struct wacom_features *features = &wacom_wac->features;
>  
>  	/* currently, only direct devices have proper hid report descriptors */
> -	__set_bit(INPUT_PROP_DIRECT, wacom_wac->pen_input->propbit);
> -	__set_bit(INPUT_PROP_DIRECT, wacom_wac->touch_input->propbit);
> +	features->device_type |= WACOM_DEVICETYPE_DIRECT;
>  
>  	if (WACOM_PEN_FIELD(field))
>  		return wacom_wac_pen_usage_mapping(hdev, field, usage);
> @@ -2450,6 +2450,33 @@ void wacom_setup_device_quirks(struct wacom *wacom)
>  	if (features->type == REMOTE)
>  		features->device_type = WACOM_DEVICETYPE_PAD;
>  
> +	switch (features->type) {
> +	case PL:
> +	case DTU:
> +	case DTUS:
> +	case DTUSX:
> +	case WACOM_21UX2:
> +	case WACOM_22HD:
> +	case DTK:
> +	case WACOM24_HD:
> +	case WACOM_27QHD:
> +	case CINTIQ_HYBRID:
> +	case CINTIQ_COMPANION_2:
> +	case CINTIQ:
> +	case WACOM_BEE:
> +	case WACOM_13HD:
> +	case WACOM_24HDT:
> +	case WACOM_27QHDT:
> +	case TABLETPC:
> +	case TABLETPCE:
> +	case TABLETPC2FG:
> +	case MTSCREEN:
> +	case MTTPC:
> +	case MTTPC_B:
> +		features->device_type |= WACOM_DEVICETYPE_DIRECT;
> +		break;
> +	}
> +
>  	if (wacom->hdev->bus == BUS_BLUETOOTH)
>  		features->quirks |= WACOM_QUIRK_BATTERY;
>  
> @@ -2483,6 +2510,11 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>  	if (!(features->device_type & WACOM_DEVICETYPE_PEN))
>  		return -ENODEV;
>  
> +	if (features->device_type & WACOM_DEVICETYPE_DIRECT)
> +		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
> +	else
> +		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> +
>  	if (features->type == HID_GENERIC)
>  		/* setup has already been done */
>  		return 0;
> @@ -2501,7 +2533,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>  	input_abs_set_res(input_dev, ABS_X, features->x_resolution);
>  	input_abs_set_res(input_dev, ABS_Y, features->y_resolution);
>  
> -
>  	switch (features->type) {
>  	case GRAPHIRE_BT:
>  		__clear_bit(ABS_MISC, input_dev->absbit);
> @@ -2525,8 +2556,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>  		__set_bit(BTN_TOOL_MOUSE, input_dev->keybit);
>  		__set_bit(BTN_STYLUS, input_dev->keybit);
>  		__set_bit(BTN_STYLUS2, input_dev->keybit);
> -
> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
>  		break;
>  
>  	case WACOM_27QHD:
> @@ -2541,7 +2570,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>  	case CINTIQ_COMPANION_2:
>  		input_set_abs_params(input_dev, ABS_Z, -900, 899, 0, 0);
>  		input_abs_set_res(input_dev, ABS_Z, 287);
> -		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
>  		wacom_setup_cintiq(wacom_wac);
>  		break;
>  
> @@ -2557,8 +2585,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>  		/* fall through */
>  
>  	case INTUOS:
> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> -
>  		wacom_setup_intuos(wacom_wac);
>  		break;
>  
> @@ -2568,8 +2594,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>  	case INTUOSPL:
>  	case INTUOS5S:
>  	case INTUOSPS:
> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> -
>  		input_set_abs_params(input_dev, ABS_DISTANCE, 0,
>  				      features->distance_max,
>  				      features->distance_fuzz, 0);
> @@ -2599,8 +2623,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>  		__set_bit(BTN_TOOL_RUBBER, input_dev->keybit);
>  		__set_bit(BTN_STYLUS, input_dev->keybit);
>  		__set_bit(BTN_STYLUS2, input_dev->keybit);
> -
> -		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
>  		break;
>  
>  	case PTU:
> @@ -2611,16 +2633,12 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
>  		__set_bit(BTN_TOOL_PEN, input_dev->keybit);
>  		__set_bit(BTN_TOOL_RUBBER, input_dev->keybit);
>  		__set_bit(BTN_STYLUS, input_dev->keybit);
> -
> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
>  		break;
>  
>  	case INTUOSHT:
>  	case BAMBOO_PT:
>  	case BAMBOO_PEN:
>  	case INTUOSHT2:
> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> -
>  		if (features->type == INTUOSHT2) {
>  			wacom_setup_basic_pro_pen(wacom_wac);
>  		} else {
> @@ -2651,6 +2669,11 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
>  	if (!(features->device_type & WACOM_DEVICETYPE_TOUCH))
>  		return -ENODEV;
>  
> +	if (features->device_type & WACOM_DEVICETYPE_DIRECT)
> +		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
> +	else
> +		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> +
>  	if (features->type == HID_GENERIC)
>  		/* setup has already been done */
>  		return 0;
> @@ -2685,8 +2708,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
>  	case INTUOSPL:
>  	case INTUOS5S:
>  	case INTUOSPS:
> -		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
> -
>  		input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, features->x_max, 0, 0);
>  		input_set_abs_params(input_dev, ABS_MT_TOUCH_MINOR, 0, features->y_max, 0, 0);
>  		input_mt_init_slots(input_dev, features->touch_max, INPUT_MT_POINTER);
> @@ -2709,7 +2730,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
>  
>  	case TABLETPC:
>  	case TABLETPCE:
> -		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
>  		break;
>  
>  	case INTUOSHT:
> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
> index 267025c..5624268 100644
> --- a/drivers/hid/wacom_wac.h
> +++ b/drivers/hid/wacom_wac.h
> @@ -82,6 +82,7 @@
>  #define WACOM_DEVICETYPE_TOUCH          0x0002
>  #define WACOM_DEVICETYPE_PAD            0x0004
>  #define WACOM_DEVICETYPE_WL_MONITOR     0x0008
> +#define WACOM_DEVICETYPE_DIRECT         0x0010
>  
>  #define WACOM_VENDORDEFINED_PEN		0xff0d0001
>  #define WACOM_G9_PAGE			0xff090000
> -- 
> 2.9.0
> 

[PATCH v4 1/2] HID: wacom: Add WACOM_DEVICETYPE_DIRECT for Cintiqs and similar

[Jason Gerecke]

"Direct" input devices like Cintiqs and Tablet PCs set the INPUT_PROP_DIRECT
property to notify userspace that the sensor and screen are overlaid. This
information can also be useful elsewhere within the kernel driver, however,
so we introduce a new WACOM_DEVICETYPE_DIRECT that signals this to other
kernel code.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
Changes from v3:
 * Changed 'WACOM24_HD' to 'WACOM_24HD' (/me places paper bag over head)

 drivers/hid/wacom_wac.c | 58 +++++++++++++++++++++++++++++++++----------------
 drivers/hid/wacom_wac.h |  1 +
 2 files changed, 40 insertions(+), 19 deletions(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 510a56b..cfefed0 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -1740,10 +1740,10 @@ void wacom_wac_usage_mapping(struct hid_device *hdev,
 {
 	struct wacom *wacom = hid_get_drvdata(hdev);
 	struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+	struct wacom_features *features = &wacom_wac->features;
 
 	/* currently, only direct devices have proper hid report descriptors */
-	__set_bit(INPUT_PROP_DIRECT, wacom_wac->pen_input->propbit);
-	__set_bit(INPUT_PROP_DIRECT, wacom_wac->touch_input->propbit);
+	features->device_type |= WACOM_DEVICETYPE_DIRECT;
 
 	if (WACOM_PEN_FIELD(field))
 		return wacom_wac_pen_usage_mapping(hdev, field, usage);
@@ -2449,6 +2449,33 @@ void wacom_setup_device_quirks(struct wacom *wacom)
 	if (features->type == REMOTE)
 		features->device_type = WACOM_DEVICETYPE_PAD;
 
+	switch (features->type) {
+	case PL:
+	case DTU:
+	case DTUS:
+	case DTUSX:
+	case WACOM_21UX2:
+	case WACOM_22HD:
+	case DTK:
+	case WACOM_24HD:
+	case WACOM_27QHD:
+	case CINTIQ_HYBRID:
+	case CINTIQ_COMPANION_2:
+	case CINTIQ:
+	case WACOM_BEE:
+	case WACOM_13HD:
+	case WACOM_24HDT:
+	case WACOM_27QHDT:
+	case TABLETPC:
+	case TABLETPCE:
+	case TABLETPC2FG:
+	case MTSCREEN:
+	case MTTPC:
+	case MTTPC_B:
+		features->device_type |= WACOM_DEVICETYPE_DIRECT;
+		break;
+	}
+
 	if (wacom->hdev->bus == BUS_BLUETOOTH)
 		features->quirks |= WACOM_QUIRK_BATTERY;
 
@@ -2482,6 +2509,11 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 	if (!(features->device_type & WACOM_DEVICETYPE_PEN))
 		return -ENODEV;
 
+	if (features->device_type & WACOM_DEVICETYPE_DIRECT)
+		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
+	else
+		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
+
 	if (features->type == HID_GENERIC)
 		/* setup has already been done */
 		return 0;
@@ -2500,7 +2532,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 	input_abs_set_res(input_dev, ABS_X, features->x_resolution);
 	input_abs_set_res(input_dev, ABS_Y, features->y_resolution);
 
-
 	switch (features->type) {
 	case GRAPHIRE_BT:
 		__clear_bit(ABS_MISC, input_dev->absbit);
@@ -2524,8 +2555,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 		__set_bit(BTN_TOOL_MOUSE, input_dev->keybit);
 		__set_bit(BTN_STYLUS, input_dev->keybit);
 		__set_bit(BTN_STYLUS2, input_dev->keybit);
-
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
 		break;
 
 	case WACOM_27QHD:
@@ -2540,7 +2569,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 	case CINTIQ_COMPANION_2:
 		input_set_abs_params(input_dev, ABS_Z, -900, 899, 0, 0);
 		input_abs_set_res(input_dev, ABS_Z, 287);
-		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
 		wacom_setup_cintiq(wacom_wac);
 		break;
 
@@ -2556,8 +2584,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 		/* fall through */
 
 	case INTUOS:
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
-
 		wacom_setup_intuos(wacom_wac);
 		break;
 
@@ -2567,8 +2593,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 	case INTUOSPL:
 	case INTUOS5S:
 	case INTUOSPS:
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
-
 		input_set_abs_params(input_dev, ABS_DISTANCE, 0,
 				      features->distance_max,
 				      features->distance_fuzz, 0);
@@ -2598,8 +2622,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 		__set_bit(BTN_TOOL_RUBBER, input_dev->keybit);
 		__set_bit(BTN_STYLUS, input_dev->keybit);
 		__set_bit(BTN_STYLUS2, input_dev->keybit);
-
-		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
 		break;
 
 	case PTU:
@@ -2610,16 +2632,12 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
 		__set_bit(BTN_TOOL_PEN, input_dev->keybit);
 		__set_bit(BTN_TOOL_RUBBER, input_dev->keybit);
 		__set_bit(BTN_STYLUS, input_dev->keybit);
-
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
 		break;
 
 	case INTUOSHT:
 	case BAMBOO_PT:
 	case BAMBOO_PEN:
 	case INTUOSHT2:
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
-
 		if (features->type == INTUOSHT2) {
 			wacom_setup_basic_pro_pen(wacom_wac);
 		} else {
@@ -2650,6 +2668,11 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 	if (!(features->device_type & WACOM_DEVICETYPE_TOUCH))
 		return -ENODEV;
 
+	if (features->device_type & WACOM_DEVICETYPE_DIRECT)
+		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
+	else
+		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
+
 	if (features->type == HID_GENERIC)
 		/* setup has already been done */
 		return 0;
@@ -2684,8 +2707,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 	case INTUOSPL:
 	case INTUOS5S:
 	case INTUOSPS:
-		__set_bit(INPUT_PROP_POINTER, input_dev->propbit);
-
 		input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, features->x_max, 0, 0);
 		input_set_abs_params(input_dev, ABS_MT_TOUCH_MINOR, 0, features->y_max, 0, 0);
 		input_mt_init_slots(input_dev, features->touch_max, INPUT_MT_POINTER);
@@ -2708,7 +2729,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
 
 	case TABLETPC:
 	case TABLETPCE:
-		__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
 		break;
 
 	case INTUOSHT:
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index 9326b60..324c40b 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -82,6 +82,7 @@
 #define WACOM_DEVICETYPE_TOUCH          0x0002
 #define WACOM_DEVICETYPE_PAD            0x0004
 #define WACOM_DEVICETYPE_WL_MONITOR     0x0008
+#define WACOM_DEVICETYPE_DIRECT         0x0010
 
 #define WACOM_VENDORDEFINED_PEN		0xff0d0001
 #define WACOM_G9_PAGE			0xff090000
-- 
2.9.2

[PATCH v4 2/2] HID: wacom: Augment 'oVid' and 'oPid' with heuristics for HID_GENERIC

[Jason Gerecke]

The 'oVid' and 'oPid' variables used by wacom_are_sibling are a hacky
solution to the problem of the driver historically having few good
heuristics to use in determining if two devices should be considered
siblings or not. While it works well enough for explicitly supported
devices, it offers no help for HID_GENERIC devices. Now that we have
a bit more information (e.g. direct/indirect) available to us though,
we should make use of it it to improve the pairing of such devices.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
Changes from v3:
 * No longer remove oVid/oPid checks and variables
 * Modified oVid/oPid checks to recognize HID_ANY_ID as a wildcard
 * Moved path check towards beginning of function as it is common
   between HID_GENERIC and explicitly-supported devices
 * Removed heuristic which required deviecs with different VID/PIDs
   to be direct input.

 drivers/hid/wacom_sys.c | 83 ++++++++++++++++++++++++++++++++++++++++++-------
 drivers/hid/wacom_wac.c |  2 +-
 2 files changed, 72 insertions(+), 13 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index edde881..5e7a564 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -527,36 +527,95 @@ struct wacom_hdev_data {
 static LIST_HEAD(wacom_udev_list);
 static DEFINE_MUTEX(wacom_udev_list_lock);
 
+static bool compare_device_paths(struct hid_device *hdev_a,
+		struct hid_device *hdev_b, char separator)
+{
+	int n1 = strrchr(hdev_a->phys, separator) - hdev_a->phys;
+	int n2 = strrchr(hdev_b->phys, separator) - hdev_b->phys;
+
+	if (n1 != n2 || n1 <= 0 || n2 <= 0)
+		return false;
+
+	return !strncmp(hdev_a->phys, hdev_b->phys, n1);
+}
+
 static bool wacom_are_sibling(struct hid_device *hdev,
 		struct hid_device *sibling)
 {
 	struct wacom *wacom = hid_get_drvdata(hdev);
 	struct wacom_features *features = &wacom->wacom_wac.features;
-	int vid = features->oVid;
-	int pid = features->oPid;
-	int n1,n2;
+	struct wacom *sibling_wacom = hid_get_drvdata(sibling);
+	struct wacom_features *sibling_features = &sibling_wacom->wacom_wac.features;
+	__u32 oVid = features->oVid ? features->oVid : hdev->vendor;
+	__u32 oPid = features->oPid ? features->oPid : hdev->product;
 
-	if (vid == 0 && pid == 0) {
-		vid = hdev->vendor;
-		pid = hdev->product;
+	/* The defined oVid/oPid must match that of the sibling */
+	if (features->oVid != HID_ANY_ID && sibling->vendor != oVid)
+		return false;
+	if (features->oPid != HID_ANY_ID && sibling->product != oPid)
+		return false;
+
+	/*
+	 * Devices with the same VID/PID must share the same physical
+	 * device path, while those with different VID/PID must share
+	 * the same physical parent device path.
+	 */
+	if (hdev->vendor == sibling->vendor && hdev->product == sibling->product) {
+		if (!compare_device_paths(hdev, sibling, '/'))
+			return false;
+	} else {
+		if (!compare_device_paths(hdev, sibling, '.'))
+			return false;
 	}
 
-	if (vid != sibling->vendor || pid != sibling->product)
+	/* Skip the remaining heuristics unless you are a HID_GENERIC device */
+	if (features->type != HID_GENERIC)
+		return true;
+
+	/*
+	 * Direct-input devices may not be siblings of indirect-input
+	 * devices.
+	 */
+	if ((features->device_type & WACOM_DEVICETYPE_DIRECT) &&
+	    !(sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
 		return false;
 
-	/* Compare the physical path. */
-	n1 = strrchr(hdev->phys, '.') - hdev->phys;
-	n2 = strrchr(sibling->phys, '.') - sibling->phys;
-	if (n1 != n2 || n1 <= 0 || n2 <= 0)
+	/*
+	 * Indirect-input devices may not be siblings of direct-input
+	 * devices.
+	 */
+	if (!(features->device_type & WACOM_DEVICETYPE_DIRECT) &&
+	    (sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
+		return false;
+
+	/* Pen devices may only be siblings of touch devices */
+	if ((features->device_type & WACOM_DEVICETYPE_PEN) &&
+	    !(sibling_features->device_type & WACOM_DEVICETYPE_TOUCH))
+		return false;
+
+	/* Touch devices may only be siblings of pen devices */
+	if ((features->device_type & WACOM_DEVICETYPE_TOUCH) &&
+	    !(sibling_features->device_type & WACOM_DEVICETYPE_PEN))
 		return false;
 
-	return !strncmp(hdev->phys, sibling->phys, n1);
+	/*
+	 * No reason could be found for these two devices to NOT be
+	 * siblings, so there's a good chance they ARE siblings
+	 */
+	return true;
 }
 
 static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev)
 {
 	struct wacom_hdev_data *data;
 
+	/* Try to find an already-probed interface from the same device */
+	list_for_each_entry(data, &wacom_udev_list, list) {
+		if (compare_device_paths(hdev, data->dev, '/'))
+			return data;
+	}
+
+	/* Fallback to finding devices that appear to be "siblings" */
 	list_for_each_entry(data, &wacom_udev_list, list) {
 		if (wacom_are_sibling(hdev, data->dev)) {
 			kref_get(&data->kref);
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index cfefed0..1b754b4 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -3521,7 +3521,7 @@ static const struct wacom_features wacom_features_0x343 =
 	  WACOM_DTU_OFFSET, WACOM_DTU_OFFSET };
 
 static const struct wacom_features wacom_features_HID_ANY_ID =
-	{ "Wacom HID", .type = HID_GENERIC };
+	{ "Wacom HID", .type = HID_GENERIC, .oVid = HID_ANY_ID, .oPid = HID_ANY_ID };
 
 #define USB_DEVICE_WACOM(prod)						\
 	HID_DEVICE(BUS_USB, HID_GROUP_WACOM, USB_VENDOR_ID_WACOM, prod),\
-- 
2.9.2

Re: [PATCH v4 2/2] HID: wacom: Augment 'oVid' and 'oPid' with heuristics for HID_GENERIC

[Benjamin Tissoires]

On Aug 08 2016 or thereabouts, Jason Gerecke wrote:
> The 'oVid' and 'oPid' variables used by wacom_are_sibling are a hacky
> solution to the problem of the driver historically having few good
> heuristics to use in determining if two devices should be considered
> siblings or not. While it works well enough for explicitly supported
> devices, it offers no help for HID_GENERIC devices. Now that we have
> a bit more information (e.g. direct/indirect) available to us though,
> we should make use of it it to improve the pairing of such devices.
> 
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> ---
> Changes from v3:
>  * No longer remove oVid/oPid checks and variables
>  * Modified oVid/oPid checks to recognize HID_ANY_ID as a wildcard
>  * Moved path check towards beginning of function as it is common
>    between HID_GENERIC and explicitly-supported devices
>  * Removed heuristic which required deviecs with different VID/PIDs
>    to be direct input.
> 
>  drivers/hid/wacom_sys.c | 83 ++++++++++++++++++++++++++++++++++++++++++-------
>  drivers/hid/wacom_wac.c |  2 +-
>  2 files changed, 72 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index edde881..5e7a564 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -527,36 +527,95 @@ struct wacom_hdev_data {
>  static LIST_HEAD(wacom_udev_list);
>  static DEFINE_MUTEX(wacom_udev_list_lock);
>  
> +static bool compare_device_paths(struct hid_device *hdev_a,
> +		struct hid_device *hdev_b, char separator)
> +{
> +	int n1 = strrchr(hdev_a->phys, separator) - hdev_a->phys;
> +	int n2 = strrchr(hdev_b->phys, separator) - hdev_b->phys;
> +
> +	if (n1 != n2 || n1 <= 0 || n2 <= 0)
> +		return false;
> +
> +	return !strncmp(hdev_a->phys, hdev_b->phys, n1);
> +}
> +
>  static bool wacom_are_sibling(struct hid_device *hdev,
>  		struct hid_device *sibling)
>  {
>  	struct wacom *wacom = hid_get_drvdata(hdev);
>  	struct wacom_features *features = &wacom->wacom_wac.features;
> -	int vid = features->oVid;
> -	int pid = features->oPid;
> -	int n1,n2;
> +	struct wacom *sibling_wacom = hid_get_drvdata(sibling);
> +	struct wacom_features *sibling_features = &sibling_wacom->wacom_wac.features;
> +	__u32 oVid = features->oVid ? features->oVid : hdev->vendor;
> +	__u32 oPid = features->oPid ? features->oPid : hdev->product;
>  
> -	if (vid == 0 && pid == 0) {
> -		vid = hdev->vendor;
> -		pid = hdev->product;
> +	/* The defined oVid/oPid must match that of the sibling */
> +	if (features->oVid != HID_ANY_ID && sibling->vendor != oVid)
> +		return false;
> +	if (features->oPid != HID_ANY_ID && sibling->product != oPid)
> +		return false;
> +
> +	/*
> +	 * Devices with the same VID/PID must share the same physical
> +	 * device path, while those with different VID/PID must share
> +	 * the same physical parent device path.
> +	 */
> +	if (hdev->vendor == sibling->vendor && hdev->product == sibling->product) {
> +		if (!compare_device_paths(hdev, sibling, '/'))
> +			return false;
> +	} else {
> +		if (!compare_device_paths(hdev, sibling, '.'))
> +			return false;
>  	}
>  
> -	if (vid != sibling->vendor || pid != sibling->product)
> +	/* Skip the remaining heuristics unless you are a HID_GENERIC device */
> +	if (features->type != HID_GENERIC)
> +		return true;

I wonder if we shouldn't go for the rest of the tests even for known PIDs
now.
They should be valid for all of our current devices, but that might just
be nitpicking (I can't find a combination of devices that would be
wrongly valid under the current rules but that would be shown as wrong
with those new heuristic).

Anyway, with or without this change, I like the result better:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

> +
> +	/*
> +	 * Direct-input devices may not be siblings of indirect-input
> +	 * devices.
> +	 */
> +	if ((features->device_type & WACOM_DEVICETYPE_DIRECT) &&
> +	    !(sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
>  		return false;
>  
> -	/* Compare the physical path. */
> -	n1 = strrchr(hdev->phys, '.') - hdev->phys;
> -	n2 = strrchr(sibling->phys, '.') - sibling->phys;
> -	if (n1 != n2 || n1 <= 0 || n2 <= 0)
> +	/*
> +	 * Indirect-input devices may not be siblings of direct-input
> +	 * devices.
> +	 */
> +	if (!(features->device_type & WACOM_DEVICETYPE_DIRECT) &&
> +	    (sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
> +		return false;
> +
> +	/* Pen devices may only be siblings of touch devices */
> +	if ((features->device_type & WACOM_DEVICETYPE_PEN) &&
> +	    !(sibling_features->device_type & WACOM_DEVICETYPE_TOUCH))
> +		return false;
> +
> +	/* Touch devices may only be siblings of pen devices */
> +	if ((features->device_type & WACOM_DEVICETYPE_TOUCH) &&
> +	    !(sibling_features->device_type & WACOM_DEVICETYPE_PEN))
>  		return false;
>  
> -	return !strncmp(hdev->phys, sibling->phys, n1);
> +	/*
> +	 * No reason could be found for these two devices to NOT be
> +	 * siblings, so there's a good chance they ARE siblings
> +	 */
> +	return true;
>  }
>  
>  static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev)
>  {
>  	struct wacom_hdev_data *data;
>  
> +	/* Try to find an already-probed interface from the same device */
> +	list_for_each_entry(data, &wacom_udev_list, list) {
> +		if (compare_device_paths(hdev, data->dev, '/'))
> +			return data;
> +	}
> +
> +	/* Fallback to finding devices that appear to be "siblings" */
>  	list_for_each_entry(data, &wacom_udev_list, list) {
>  		if (wacom_are_sibling(hdev, data->dev)) {
>  			kref_get(&data->kref);
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index cfefed0..1b754b4 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -3521,7 +3521,7 @@ static const struct wacom_features wacom_features_0x343 =
>  	  WACOM_DTU_OFFSET, WACOM_DTU_OFFSET };
>  
>  static const struct wacom_features wacom_features_HID_ANY_ID =
> -	{ "Wacom HID", .type = HID_GENERIC };
> +	{ "Wacom HID", .type = HID_GENERIC, .oVid = HID_ANY_ID, .oPid = HID_ANY_ID };
>  
>  #define USB_DEVICE_WACOM(prod)						\
>  	HID_DEVICE(BUS_USB, HID_GROUP_WACOM, USB_VENDOR_ID_WACOM, prod),\
> -- 
> 2.9.2
> 

Re: [PATCH v4 1/2] HID: wacom: Add WACOM_DEVICETYPE_DIRECT for Cintiqs and similar

[Jiri Kosina]

On Mon, 8 Aug 2016, Jason Gerecke wrote:

> "Direct" input devices like Cintiqs and Tablet PCs set the INPUT_PROP_DIRECT
> property to notify userspace that the sensor and screen are overlaid. This
> information can also be useful elsewhere within the kernel driver, however,
> so we introduce a new WACOM_DEVICETYPE_DIRECT that signals this to other
> kernel code.
> 
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Both patches applied to for-4.9/wacom. Thanks,

-- 
Jiri Kosina
SUSE Labs