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

[David Herrmann]

Hi

On Fri, Apr 3, 2015 at 5:30 PM, Benjamin Tissoires <btissoir at redhat.com> wrote:
> 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 :)

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?

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
Hans' patch should be applied unchanged. At least we now know the
background story.

Thanks
David