[systemd-devel] [PATCH] udev: input_id: tag accelerometers as ID_INPUT_ACCELEROMETER
Hans de Goede
hdegoede at redhat.com
Fri Apr 3 11:55:23 PDT 2015
Hi,
On 03-04-15 20:21, Benjamin Tissoires wrote:
> On Apr 03 2015 or thereabouts, David Herrmann wrote:
>> 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?
>
> 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.
Ok, I'll respin my patch to not have the return then.
Regards,
Hans
More information about the systemd-devel
mailing list