[systemd-devel] [PATCH] udev: input_id: tag accelerometers as ID_INPUT_ACCELEROMETER

[Benjamin Tissoires]

On Apr 03 2015 or thereabouts, David Herrmann wrote:
> Hi
> 
> On Fri, Apr 3, 2015 at 4:38 PM, Benjamin Tissoires <btissoir at 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 <hdegoede at 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 <hdegoede at 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 :)

Cheers,
Benjamin