On Apr 03 2015 or thereabouts, David Herrmann wrote: > Hi > > On Fri, Apr 3, 2015 at 5:30 PM, Benjamin Tissoires <btiss...@redhat.com> > wrote: > > On Apr 03 2015 or thereabouts, David Herrmann wrote: > >> Hi > >> > >> On Fri, Apr 3, 2015 at 4:38 PM, Benjamin Tissoires <btiss...@redhat.com> > >> wrote: > >> > On Apr 03 2015 or thereabouts, Hans de Goede wrote: > >> >> Hi, > >> >> > >> >> On 03-04-15 15:51, David Herrmann wrote: > >> >> >Hi > >> >> > > >> >> >On Fri, Apr 3, 2015 at 12:07 PM, Hans de Goede <hdego...@redhat.com> > >> >> >wrote: > >> >> >>input_id already (tries to) tag accelerometers as such, but this only > >> >> >>works > >> >> >>for absolute accelerometers. Recent kernels mark accelerometers > >> >> >>through an > >> >> >>input prop. Trust that prop and always tag devices with it with > >> >> >>ID_INPUT_ACCELEROMETER. > >> >> >> > >> >> >>Note that detection by the prop bit works the same as the existing > >> >> >>detection > >> >> >>and will ensure that no other tags get set on the device. > >> >> >> > >> >> >>Signed-off-by: Hans de Goede <hdego...@redhat.com> > >> >> >>--- > >> >> >> src/shared/missing.h | 4 ++++ > >> >> >> src/udev/udev-builtin-input_id.c | 5 +++++ > >> >> >> 2 files changed, 9 insertions(+) > >> >> >> > >> >> >>diff --git a/src/shared/missing.h b/src/shared/missing.h > >> >> >>index 3bdfd8f..4464e35 100644 > >> >> >>--- a/src/shared/missing.h > >> >> >>+++ b/src/shared/missing.h > >> >> >>@@ -944,3 +944,7 @@ static inline int kcmp(pid_t pid1, pid_t pid2, > >> >> >>int type, unsigned long idx1, uns > >> >> >> #ifndef INPUT_PROP_POINTING_STICK > >> >> >> #define INPUT_PROP_POINTING_STICK 0x05 > >> >> >> #endif > >> >> >>+ > >> >> >>+#ifndef INPUT_PROP_ACCELEROMETER > >> >> >>+#define INPUT_PROP_ACCELEROMETER 0x06 > >> >> >>+#endif > >> >> >>diff --git a/src/udev/udev-builtin-input_id.c > >> >> >>b/src/udev/udev-builtin-input_id.c > >> >> >>index d4c38ca..ecfc447 100644 > >> >> >>--- a/src/udev/udev-builtin-input_id.c > >> >> >>+++ b/src/udev/udev-builtin-input_id.c > >> >> >>@@ -136,6 +136,11 @@ static void test_pointers (struct udev_device > >> >> >>*dev, > >> >> >> int is_mouse = 0; > >> >> >> int is_touchpad = 0; > >> >> >> > >> >> >>+ if (test_bit (INPUT_PROP_ACCELEROMETER, bitmask_props)) { > >> >> >>+ udev_builtin_add_property(dev, test, > >> >> >>"ID_INPUT_ACCELEROMETER", "1"); > >> >> >>+ return; > >> >> >>+ } > >> >> >>+ > >> >> > > >> >> >So this property is only set for accelerometer-only devices? > >> >> > >> >> Hmm, good question. Currently I see only one user of it in the kernel: > >> >> > >> >> drivers/hid/wacom_wac.c > >> > > >> > Yep, this is the first and only for now. > >> > > >> >> > >> >> Which most likely does not count as an accelerometer only device, so > >> >> maybe > >> >> my idea to follow the existing accelerometer detection code and > >> >> short-circuit > >> >> the other tests was not such a good idea. > >> >> > >> >> Benjamin, do you have any input on this ? > >> > > >> > See Peter's documentation patch here: > >> > https://patchwork.kernel.org/patch/6102851/ > >> > > >> > If the property is set, "Directional axes on this device (absolute > >> > and/or relative x, y, z) represent accelerometer data. All other axes > >> > retain their meaning. A device must not mix regular directional axes and > >> > accelerometer axes on the same event node." > >> > > >> > So we can have multiple axis, multiple buttons but X,Y,Z will be accel > >> > values. > >> > > >> > The Cintiq 27 QHD has a builtin accelerometer and it is reported by this > >> > property to explicitly say that the X,Y,Z are not stylus data but > >> > accelerometers values. > >> > > >> > For this device, there is no problem because libwacom will set > >> > ID_INPUT_TABLET through a custom udev rule, but for others, the "return" > >> > will be a problem IMO (note that Wacom already breaks the ID_INPUT_* tag > >> > should be set only once per device). > >> > >> Yeah, thanks! > >> > >> Hans, I'm not sure your patch is correct. I mean, just because the > >> input-device exports an accelerometer, doesn't necessarily mean that > >> we should tag it as such, right? Or does the Cintiq 27 QHD export > >> _multiple_ input devices, and the accelerometer has its own evdev > >> node? In that case, I'm fine with it. But otherwise, we should still > >> tag it as tablet, right? > > > > It's a little bit messier than that actually :) > > > > I think (I never had it in my hands, so this is from reading the driver) > > that the 27 QHD (Touch) will show up as: > > - "Wacom Cintiq 27QHD touch Pen" -> regular tablet input (for the > > stylus) which will be seen by udev as a tablet > > - "Wacom Cintiq 27QHD touch Touch" -> regular multitouch screen, > > libwacom will set the ID_INPUT_TABLET tag (no way for udev to detect > > it except lookking at the siblings and this might not be reliable) > > - "Wacom Cintiq 27QHD touch Pad" -> this will have > > INPUT_PROP_ACCELEROMETER, ABS_X|Y|Z, KEY_PROG1|2|3 -> so setting only > > ID_INPUT_ACCELEROMETER for it is fine, there will be no way to > > reliably detect the tablet capability (unless the kernel provides it) > > > > So I think you are both right (and wrong). Hans' patch works with this > > current device, but it won't allow to have other ID_INPUT_* tags set. > > And David is right, but we should not tag as ID_INPUT_TABLET :) > > Ok, so what's the expected behavior? The "touch Touch" device should > be marked as TABLET, the others should not be marked at all? libinput > and friends can thus detect it as master device? > What happens if all are marked as TABLET? > What tagging do you guys expect/want?
What we want is simple: - (multi)touch devices tagged as such (here the "touch Touch") - stylus devices marked as tablet (here the "touch Pen") - accelerometers tagged as such (here the "touch Pad"), in addition to every other ID_INPUT (keyboard, mouse, etc...) Don't bother with marking the other nodes as tablet, libwacom adds a rule which does that based on the VID:PID:name. > > I'm not really sure tagging those devices makes any sense at all. You > need to know how they work to make any use of them. No idea.. maybe I am not requesting to implement a specific udev builtin for such devices. We mostly treat them out of udev through custom rules. As long as udev tags devices on a reliable manner, we are fine and can do whatever needs to be done later. For udev to properly tag tablets, we would need to add a kernel property INPUT_PROP_TABLET and then, yes, the built-ins could do something about that. Until then, the heuristic is fine. > Hans' patch should be applied unchanged. At least we now know the > background story. > I don't think we should return when we see INPUT_PROP_ACCEL. If a keyboard embeds an accelerometer, we will miss it. I think I went too deep in detail for Wacoms/tablets devices and it confused you. Those don't need much processing in udev, we can mostly control everything out the tree. Cheers, Benjamin _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel