From: Jason Gerecke <hidden> Date: 2016-07-11 18:07:25
"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(-)
@@ -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){caseGRAPHIRE_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;caseWACOM_27QHD:
@@ -2556,7 +2573,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,caseCINTIQ_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 */caseINTUOS:-__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,caseINTUOSPL:caseINTUOS5S:caseINTUOSPS:-__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;casePTU:
@@ -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;caseINTUOSHT:caseBAMBOO_PT:caseBAMBOO_PEN:caseINTUOSHT2:-__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){caseINTUOS5:caseINTUOS5L:
@@ -2700,8 +2711,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,caseINTUOSPL:caseINTUOS5S:caseINTUOSPS:-__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,caseTABLETPC:caseTABLETPCE:-__set_bit(INPUT_PROP_DIRECT,input_dev->propbit);break;caseINTUOSHT:
From: Jason Gerecke <hidden> Date: 2016-07-11 18:07:21
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(-)
@@ -532,31 +532,65 @@ static bool wacom_are_sibling(struct hid_device *hdev,{structwacom*wacom=hid_get_drvdata(hdev);structwacom_features*features=&wacom->wacom_wac.features;-intvid=features->oVid;-intpid=features->oPid;-intn1,n2;+structwacom*sibling_wacom=hid_get_drvdata(sibling);+structwacom_features*sibling_features=&sibling_wacom->wacom_wac.features;+intn1,n2;-if(vid==0&&pid==0){-vid=hdev->vendor;-pid=hdev->product;+/* Compare the physical path. Require devices with the same+*PIDtosharethesamedevice,anddeviceswithdifferent+*PIDstoshareparentdevices.+*/+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)returnfalse;-/* 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))+returnfalse;++if(hdev->vendor!=sibling->vendor||hdev->product!=sibling->product){+if(!(features->device_type&WACOM_DEVICETYPE_DIRECT))+returnfalse;+}++if((features->device_type&WACOM_DEVICETYPE_DIRECT)!=+(sibling_features->device_type&WACOM_DEVICETYPE_DIRECT))returnfalse;-return!strncmp(hdev->phys,sibling->phys,n1);+if((features->device_type&WACOM_DEVICETYPE_PEN)&&+!(sibling_features->device_type&WACOM_DEVICETYPE_TOUCH))+returnfalse;++if((features->device_type&WACOM_DEVICETYPE_TOUCH)&&+!(sibling_features->device_type&WACOM_DEVICETYPE_PEN))+returnfalse;++returntrue;}staticstructwacom_hdev_data*wacom_get_hdev_data(structhid_device*hdev){structwacom_hdev_data*data;+/* Try to find an already-probed interface from the same device */+list_for_each_entry(data,&wacom_udev_list,list){+intn1,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))+returndata;+}++/* 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);
From: Benjamin Tissoires <hidden> Date: 2016-07-12 09:05:16
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.
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?
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
quoted hunk
+
+ /* 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);
From: Jason Gerecke <hidden> Date: 2016-07-20 16:36:38
On 07/12/2016 02:05 AM, Benjamin Tissoires wrote:
On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
quoted
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.
quoted
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).
quoted
* 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 :)
quoted
* 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.
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?
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;
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.
quoted
+
+ 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....
quoted
+
+ /* 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);
--
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
From: Benjamin Tissoires <hidden> Date: 2016-07-25 09:02:23
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:
quoted
On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
quoted
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.
quoted
quoted
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
quoted
quoted
* 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
quoted
quoted
* 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.
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?
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;
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.
quoted
quoted
+
+ 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.
quoted
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....
quoted
quoted
+
+ /* 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);
--
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
From: Jason Gerecke <hidden> Date: 2016-08-03 17:13:50
On Mon, Jul 25, 2016 at 2:02 AM, Benjamin Tissoires
[off-list ref] 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:
quoted
On 07/12/2016 02:05 AM, Benjamin Tissoires wrote:
quoted
On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
quoted
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?
quoted
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.
quoted
quoted
quoted
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
quoted
quoted
quoted
* 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.
quoted
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.
quoted
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....
quoted
quoted
quoted
* 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.
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?
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;
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.
quoted
quoted
+
+ 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.
quoted
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....
quoted
quoted
+
+ /* 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);
From: Jason Gerecke <hidden> Date: 2016-08-06 20:39:19
On 08/03/2016 10:13 AM, Jason Gerecke wrote:
On Mon, Jul 25, 2016 at 2:02 AM, Benjamin Tissoires
[off-list ref] wrote:
quoted
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:
quoted
On 07/12/2016 02:05 AM, Benjamin Tissoires wrote:
quoted
On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
quoted
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?
quoted
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.
quoted
quoted
quoted
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
quoted
quoted
quoted
* 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.
quoted
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.
quoted
quoted
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....
From: Benjamin Tissoires <hidden> Date: 2016-08-08 16:36:24
On Aug 05 2016 or thereabouts, Jason Gerecke wrote:
On 08/03/2016 10:13 AM, Jason Gerecke wrote:
quoted
On Mon, Jul 25, 2016 at 2:02 AM, Benjamin Tissoires
[off-list ref] wrote:
quoted
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:
quoted
On 07/12/2016 02:05 AM, Benjamin Tissoires wrote:
quoted
On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
quoted
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?
quoted
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.
quoted
quoted
quoted
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
quoted
quoted
quoted
* 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.
quoted
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.
quoted
quoted
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....
From: Jason Gerecke <hidden> Date: 2016-08-08 17:41:12
On 08/08/2016 09:36 AM, Benjamin Tissoires wrote:
On Aug 05 2016 or thereabouts, Jason Gerecke wrote:
quoted
On 08/03/2016 10:13 AM, Jason Gerecke wrote:
quoted
On Mon, Jul 25, 2016 at 2:02 AM, Benjamin Tissoires
[off-list ref] wrote:
quoted
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:
quoted
On 07/12/2016 02:05 AM, Benjamin Tissoires wrote:
quoted
On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
quoted
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?
quoted
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.
quoted
quoted
quoted
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
quoted
quoted
quoted
* 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.
quoted
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.
quoted
quoted
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.
quoted
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....
From: Benjamin Tissoires <hidden> Date: 2016-08-08 17:56:43
On Aug 08 2016 or thereabouts, Jason Gerecke wrote:
On 08/08/2016 09:36 AM, Benjamin Tissoires wrote:
quoted
On Aug 05 2016 or thereabouts, Jason Gerecke wrote:
quoted
On 08/03/2016 10:13 AM, Jason Gerecke wrote:
quoted
On Mon, Jul 25, 2016 at 2:02 AM, Benjamin Tissoires
[off-list ref] wrote:
quoted
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:
quoted
On 07/12/2016 02:05 AM, Benjamin Tissoires wrote:
quoted
On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
quoted
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?
quoted
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.
quoted
quoted
quoted
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
quoted
quoted
quoted
* 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.
quoted
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.
quoted
quoted
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).
quoted
quoted
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....
From: Benjamin Tissoires <hidden> Date: 2016-07-12 07:36:58
On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
quoted hunk
"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(-)
@@ -1757,8 +1757,10 @@ void wacom_wac_usage_mapping(struct hid_device *hdev,{structwacom*wacom=hid_get_drvdata(hdev);structwacom_wac*wacom_wac=&wacom->wacom_wac;+structwacom_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 <redacted>
Cheers,
Benjamin
@@ -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:
From: Jason Gerecke <hidden> Date: 2016-07-20 17:48:39
On 07/12/2016 12:36 AM, Benjamin Tissoires wrote:
On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
quoted
"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(-)
@@ -1757,8 +1757,10 @@ void wacom_wac_usage_mapping(struct hid_device *hdev,{structwacom*wacom=hid_get_drvdata(hdev);structwacom_wac*wacom_wac=&wacom->wacom_wac;+structwacom_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 <redacted>
Cheers,
Benjamin
@@ -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:
--
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
From: Benjamin Tissoires <hidden> Date: 2016-07-22 09:09:22
On Jul 20 2016 or thereabouts, Jason Gerecke wrote:
On 07/12/2016 12:36 AM, Benjamin Tissoires wrote:
quoted
On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
quoted
"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(-)
@@ -1757,8 +1757,10 @@ void wacom_wac_usage_mapping(struct hid_device *hdev,{structwacom*wacom=hid_get_drvdata(hdev);structwacom_wac*wacom_wac=&wacom->wacom_wac;+structwacom_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....
quoted
Rest looks good to me.
Reviewed-by: Benjamin Tissoires <redacted>
Cheers,
Benjamin
@@ -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:
--
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
On Fri, Jul 22, 2016 at 2:09 AM, Benjamin Tissoires
[off-list ref] wrote:
On Jul 20 2016 or thereabouts, Jason Gerecke wrote:
quoted
On 07/12/2016 12:36 AM, Benjamin Tissoires wrote:
quoted
On Jul 11 2016 or thereabouts, Jason Gerecke wrote:
quoted
"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(-)
@@ -1757,8 +1757,10 @@ void wacom_wac_usage_mapping(struct hid_device *hdev,{structwacom*wacom=hid_get_drvdata(hdev);structwacom_wac*wacom_wac=&wacom->wacom_wac;+structwacom_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.
quoted
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.
quoted
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....
quoted
Rest looks good to me.
Reviewed-by: Benjamin Tissoires <redacted>
Cheers,
Benjamin
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
From: Jason Gerecke <hidden> Date: 2016-07-21 16:12:09
"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 <redacted>
---
drivers/hid/wacom_wac.c | 40 ++++++++++++++++++++++++----------------
drivers/hid/wacom_wac.h | 1 +
2 files changed, 25 insertions(+), 16 deletions(-)
@@ -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){caseGRAPHIRE_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;caseWACOM_27QHD:
@@ -2541,7 +2558,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,caseCINTIQ_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 */caseINTUOS:-__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,caseINTUOSPL:caseINTUOS5S:caseINTUOSPS:-__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;casePTU:
@@ -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;caseINTUOSHT:caseBAMBOO_PT:caseBAMBOO_PEN:caseINTUOSHT2:-__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){caseINTUOS5:caseINTUOS5L:
@@ -2685,8 +2696,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,caseINTUOSPL:caseINTUOS5S:caseINTUOSPS:-__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,caseTABLETPC:caseTABLETPCE:-__set_bit(INPUT_PROP_DIRECT,input_dev->propbit);break;caseINTUOSHT:
From: Jason Gerecke <hidden> Date: 2016-07-21 16:12:11
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(-)
@@ -527,36 +527,92 @@ struct wacom_hdev_data {staticLIST_HEAD(wacom_udev_list);staticDEFINE_MUTEX(wacom_udev_list_lock);+staticboolcompare_device_paths(structhid_device*hdev_a,+structhid_device*hdev_b,charseparator)+{+intn1=strrchr(hdev_a->phys,separator)-hdev_a->phys;+intn2=strrchr(hdev_b->phys,separator)-hdev_b->phys;++if(n1!=n2||n1<=0||n2<=0)+returnfalse;++return!strncmp(hdev_a->phys,hdev_b->phys,n1);+}+staticboolwacom_are_sibling(structhid_device*hdev,structhid_device*sibling){structwacom*wacom=hid_get_drvdata(hdev);structwacom_features*features=&wacom->wacom_wac.features;-intvid=features->oVid;-intpid=features->oPid;-intn1,n2;+structwacom*sibling_wacom=hid_get_drvdata(sibling);+structwacom_features*sibling_features=&sibling_wacom->wacom_wac.features;-if(vid==0&&pid==0){-vid=hdev->vendor;-pid=hdev->product;+/*+*DeviceswithdifferentVID/PIDsmaynotbesiblingsunless+*theyaredirectinputdevices.+*/+if(hdev->vendor!=sibling->vendor||hdev->product!=sibling->product){+if(!(features->device_type&WACOM_DEVICETYPE_DIRECT))+returnfalse;}-if(vid!=sibling->vendor||pid!=sibling->product)+/*+*Direct-inputdevicesmaynotbesiblingsofindirect-input+*devices.+*/+if((features->device_type&WACOM_DEVICETYPE_DIRECT)&&+!(sibling_features->device_type&WACOM_DEVICETYPE_DIRECT))returnfalse;-/* Compare the physical path. */-n1=strrchr(hdev->phys,'.')-hdev->phys;-n2=strrchr(sibling->phys,'.')-sibling->phys;-if(n1!=n2||n1<=0||n2<=0)+/*+*Indirect-inputdevicesmaynotbesiblingsofdirect-input+*devices.+*/+if(!(features->device_type&WACOM_DEVICETYPE_DIRECT)&&+(sibling_features->device_type&WACOM_DEVICETYPE_DIRECT))returnfalse;-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))+returnfalse;++/* Touch devices may only be siblings of pen devices */+if((features->device_type&WACOM_DEVICETYPE_TOUCH)&&+!(sibling_features->device_type&WACOM_DEVICETYPE_PEN))+returnfalse;++/*+*DeviceswiththesameVID/PIDmustsharethesamephysical+*devicepath,whilethosewithdifferentVID/PIDmustshare+*thesamephysicalparentdevicepath.+*/+if(hdev->vendor==sibling->vendor&&hdev->product==sibling->product){+if(!compare_device_paths(hdev,sibling,'/'))+returnfalse;+}else{+if(!compare_device_paths(hdev,sibling,'.'))+returnfalse;+}++/*+*NoreasoncouldbefoundforthesetwodevicestoNOTbe+*siblings,sothere'sagoodchancetheyAREsiblings+*/+returntrue;}staticstructwacom_hdev_data*wacom_get_hdev_data(structhid_device*hdev){structwacom_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,'/'))+returndata;+}++/* 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);
From: Jason Gerecke <hidden> Date: 2016-07-22 23:15:08
"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 <redacted>
---
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(-)
@@ -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 */return0;
@@ -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){caseGRAPHIRE_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;caseWACOM_27QHD:
@@ -2541,7 +2570,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,caseCINTIQ_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 */caseINTUOS:-__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,caseINTUOSPL:caseINTUOS5S:caseINTUOSPS:-__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;casePTU:
@@ -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;caseINTUOSHT:caseBAMBOO_PT:caseBAMBOO_PEN:caseINTUOSHT2:-__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 */return0;
@@ -2685,8 +2708,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,caseINTUOSPL:caseINTUOS5S:caseINTUOSPS:-__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,caseTABLETPC:caseTABLETPCE:-__set_bit(INPUT_PROP_DIRECT,input_dev->propbit);break;caseINTUOSHT:
From: Jason Gerecke <hidden> Date: 2016-07-22 23:15:11
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(-)
@@ -527,36 +527,92 @@ struct wacom_hdev_data {staticLIST_HEAD(wacom_udev_list);staticDEFINE_MUTEX(wacom_udev_list_lock);+staticboolcompare_device_paths(structhid_device*hdev_a,+structhid_device*hdev_b,charseparator)+{+intn1=strrchr(hdev_a->phys,separator)-hdev_a->phys;+intn2=strrchr(hdev_b->phys,separator)-hdev_b->phys;++if(n1!=n2||n1<=0||n2<=0)+returnfalse;++return!strncmp(hdev_a->phys,hdev_b->phys,n1);+}+staticboolwacom_are_sibling(structhid_device*hdev,structhid_device*sibling){structwacom*wacom=hid_get_drvdata(hdev);structwacom_features*features=&wacom->wacom_wac.features;-intvid=features->oVid;-intpid=features->oPid;-intn1,n2;+structwacom*sibling_wacom=hid_get_drvdata(sibling);+structwacom_features*sibling_features=&sibling_wacom->wacom_wac.features;-if(vid==0&&pid==0){-vid=hdev->vendor;-pid=hdev->product;+/*+*DeviceswithdifferentVID/PIDsmaynotbesiblingsunless+*theyaredirectinputdevices.+*/+if(hdev->vendor!=sibling->vendor||hdev->product!=sibling->product){+if(!(features->device_type&WACOM_DEVICETYPE_DIRECT))+returnfalse;}-if(vid!=sibling->vendor||pid!=sibling->product)+/*+*Direct-inputdevicesmaynotbesiblingsofindirect-input+*devices.+*/+if((features->device_type&WACOM_DEVICETYPE_DIRECT)&&+!(sibling_features->device_type&WACOM_DEVICETYPE_DIRECT))returnfalse;-/* Compare the physical path. */-n1=strrchr(hdev->phys,'.')-hdev->phys;-n2=strrchr(sibling->phys,'.')-sibling->phys;-if(n1!=n2||n1<=0||n2<=0)+/*+*Indirect-inputdevicesmaynotbesiblingsofdirect-input+*devices.+*/+if(!(features->device_type&WACOM_DEVICETYPE_DIRECT)&&+(sibling_features->device_type&WACOM_DEVICETYPE_DIRECT))returnfalse;-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))+returnfalse;++/* Touch devices may only be siblings of pen devices */+if((features->device_type&WACOM_DEVICETYPE_TOUCH)&&+!(sibling_features->device_type&WACOM_DEVICETYPE_PEN))+returnfalse;++/*+*DeviceswiththesameVID/PIDmustsharethesamephysical+*devicepath,whilethosewithdifferentVID/PIDmustshare+*thesamephysicalparentdevicepath.+*/+if(hdev->vendor==sibling->vendor&&hdev->product==sibling->product){+if(!compare_device_paths(hdev,sibling,'/'))+returnfalse;+}else{+if(!compare_device_paths(hdev,sibling,'.'))+returnfalse;+}++/*+*NoreasoncouldbefoundforthesetwodevicestoNOTbe+*siblings,sothere'sagoodchancetheyAREsiblings+*/+returntrue;}staticstructwacom_hdev_data*wacom_get_hdev_data(structhid_device*hdev){structwacom_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,'/'))+returndata;+}++/* 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);
From: Benjamin Tissoires <hidden> Date: 2016-07-25 09:51:17
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 <redacted>
---
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 <redacted>
See my comments in the v1 for 2/2.
Cheers,
Benjamin
@@ -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 */return0;
@@ -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){caseGRAPHIRE_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;caseWACOM_27QHD:
@@ -2541,7 +2570,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,caseCINTIQ_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 */caseINTUOS:-__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,caseINTUOSPL:caseINTUOS5S:caseINTUOSPS:-__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;casePTU:
@@ -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;caseINTUOSHT:caseBAMBOO_PT:caseBAMBOO_PEN:caseINTUOSHT2:-__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 */return0;
@@ -2685,8 +2708,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,caseINTUOSPL:caseINTUOS5S:caseINTUOSPS:-__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,caseTABLETPC:caseTABLETPCE:-__set_bit(INPUT_PROP_DIRECT,input_dev->propbit);break;caseINTUOSHT:
From: Jason Gerecke <hidden> Date: 2016-08-08 19:06:46
"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 <redacted>
---
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(-)
@@ -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 */return0;
@@ -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){caseGRAPHIRE_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;caseWACOM_27QHD:
@@ -2540,7 +2569,6 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,caseCINTIQ_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 */caseINTUOS:-__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,caseINTUOSPL:caseINTUOS5S:caseINTUOSPS:-__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;casePTU:
@@ -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;caseINTUOSHT:caseBAMBOO_PT:caseBAMBOO_PEN:caseINTUOSHT2:-__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 */return0;
@@ -2684,8 +2707,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,caseINTUOSPL:caseINTUOS5S:caseINTUOSPS:-__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,caseTABLETPC:caseTABLETPCE:-__set_bit(INPUT_PROP_DIRECT,input_dev->propbit);break;caseINTUOSHT:
From: Jason Gerecke <hidden> Date: 2016-08-08 19:06:48
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(-)
@@ -527,36 +527,95 @@ struct wacom_hdev_data {staticLIST_HEAD(wacom_udev_list);staticDEFINE_MUTEX(wacom_udev_list_lock);+staticboolcompare_device_paths(structhid_device*hdev_a,+structhid_device*hdev_b,charseparator)+{+intn1=strrchr(hdev_a->phys,separator)-hdev_a->phys;+intn2=strrchr(hdev_b->phys,separator)-hdev_b->phys;++if(n1!=n2||n1<=0||n2<=0)+returnfalse;++return!strncmp(hdev_a->phys,hdev_b->phys,n1);+}+staticboolwacom_are_sibling(structhid_device*hdev,structhid_device*sibling){structwacom*wacom=hid_get_drvdata(hdev);structwacom_features*features=&wacom->wacom_wac.features;-intvid=features->oVid;-intpid=features->oPid;-intn1,n2;+structwacom*sibling_wacom=hid_get_drvdata(sibling);+structwacom_features*sibling_features=&sibling_wacom->wacom_wac.features;+__u32oVid=features->oVid?features->oVid:hdev->vendor;+__u32oPid=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)+returnfalse;+if(features->oPid!=HID_ANY_ID&&sibling->product!=oPid)+returnfalse;++/*+*DeviceswiththesameVID/PIDmustsharethesamephysical+*devicepath,whilethosewithdifferentVID/PIDmustshare+*thesamephysicalparentdevicepath.+*/+if(hdev->vendor==sibling->vendor&&hdev->product==sibling->product){+if(!compare_device_paths(hdev,sibling,'/'))+returnfalse;+}else{+if(!compare_device_paths(hdev,sibling,'.'))+returnfalse;}-if(vid!=sibling->vendor||pid!=sibling->product)+/* Skip the remaining heuristics unless you are a HID_GENERIC device */+if(features->type!=HID_GENERIC)+returntrue;++/*+*Direct-inputdevicesmaynotbesiblingsofindirect-input+*devices.+*/+if((features->device_type&WACOM_DEVICETYPE_DIRECT)&&+!(sibling_features->device_type&WACOM_DEVICETYPE_DIRECT))returnfalse;-/* Compare the physical path. */-n1=strrchr(hdev->phys,'.')-hdev->phys;-n2=strrchr(sibling->phys,'.')-sibling->phys;-if(n1!=n2||n1<=0||n2<=0)+/*+*Indirect-inputdevicesmaynotbesiblingsofdirect-input+*devices.+*/+if(!(features->device_type&WACOM_DEVICETYPE_DIRECT)&&+(sibling_features->device_type&WACOM_DEVICETYPE_DIRECT))+returnfalse;++/* Pen devices may only be siblings of touch devices */+if((features->device_type&WACOM_DEVICETYPE_PEN)&&+!(sibling_features->device_type&WACOM_DEVICETYPE_TOUCH))+returnfalse;++/* Touch devices may only be siblings of pen devices */+if((features->device_type&WACOM_DEVICETYPE_TOUCH)&&+!(sibling_features->device_type&WACOM_DEVICETYPE_PEN))returnfalse;-return!strncmp(hdev->phys,sibling->phys,n1);+/*+*NoreasoncouldbefoundforthesetwodevicestoNOTbe+*siblings,sothere'sagoodchancetheyAREsiblings+*/+returntrue;}staticstructwacom_hdev_data*wacom_get_hdev_data(structhid_device*hdev){structwacom_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,'/'))+returndata;+}++/* 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);
From: Benjamin Tissoires <hidden> Date: 2016-08-08 19:53:02
On Aug 08 2016 or thereabouts, Jason Gerecke wrote:
quoted hunk
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(-)
@@ -527,36 +527,95 @@ struct wacom_hdev_data {staticLIST_HEAD(wacom_udev_list);staticDEFINE_MUTEX(wacom_udev_list_lock);+staticboolcompare_device_paths(structhid_device*hdev_a,+structhid_device*hdev_b,charseparator)+{+intn1=strrchr(hdev_a->phys,separator)-hdev_a->phys;+intn2=strrchr(hdev_b->phys,separator)-hdev_b->phys;++if(n1!=n2||n1<=0||n2<=0)+returnfalse;++return!strncmp(hdev_a->phys,hdev_b->phys,n1);+}+staticboolwacom_are_sibling(structhid_device*hdev,structhid_device*sibling){structwacom*wacom=hid_get_drvdata(hdev);structwacom_features*features=&wacom->wacom_wac.features;-intvid=features->oVid;-intpid=features->oPid;-intn1,n2;+structwacom*sibling_wacom=hid_get_drvdata(sibling);+structwacom_features*sibling_features=&sibling_wacom->wacom_wac.features;+__u32oVid=features->oVid?features->oVid:hdev->vendor;+__u32oPid=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)+returnfalse;+if(features->oPid!=HID_ANY_ID&&sibling->product!=oPid)+returnfalse;++/*+*DeviceswiththesameVID/PIDmustsharethesamephysical+*devicepath,whilethosewithdifferentVID/PIDmustshare+*thesamephysicalparentdevicepath.+*/+if(hdev->vendor==sibling->vendor&&hdev->product==sibling->product){+if(!compare_device_paths(hdev,sibling,'/'))+returnfalse;+}else{+if(!compare_device_paths(hdev,sibling,'.'))+returnfalse;}-if(vid!=sibling->vendor||pid!=sibling->product)+/* Skip the remaining heuristics unless you are a HID_GENERIC device */+if(features->type!=HID_GENERIC)+returntrue;
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 <redacted>
Cheers,
Benjamin
quoted hunk
+
+ /*
+ * 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);
"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 <redacted>
Both patches applied to for-4.9/wacom. Thanks,
--
Jiri Kosina
SUSE Labs