by Benjamin Tissoires

[permalink] [raw]

Subject: Re: [Linuxwacom-devel] [PATCH 1/5] Input - wacom: create a separate input device for pads

On Thu, Jul 10, 2014 at 8:18 PM, Jason Gerecke <[email protected]> wrote:
> On Mon, Jun 23, 2014 at 1:57 PM, Benjamin Tissoires
> <[email protected]> wrote:
>> Currently, the pad events are sent through the stylus input device
>> for the Intuos/Cintiqs, and through the touch input device for the
>> Bamboos.
>>
>> To differentiate the buttons pressed on the pad from the ones pressed
>> on the stylus, the Intuos/Cintiq uses MISC_SERIAL and ABS_MISC. This
>> lead to a multiplexing of the events into one device, which are then
>> splitted out in xf86-input-wacom. Bamboos are not using MISC events
>> because the pad is attached to the touch interface, and only BTN_TOUCH
>> is used for the finger (and DOUBLE_TAP, etc...). However, the user space
>> driver still splits out the pad from the touch interface in the same
>> way it does for the pro line devices.
>>
>> The other problem we can see with this fact is that some of the Intuos
>> and Cintiq have a wheel, and the effective range of the reported values
>> is [0..71]. Unfortunately, the airbrush stylus also sends wheel events
>> (there is a small wheel on it), but in the range [0..1023]. From the user
>> space point of view it is kind of difficult to understand that because
>> the wheel on the pad are quite common, while the airbrush tool is not.
>>
>> A solution to fix all of these problems is to split out the pad device
>> from the stylus/touch. This decision makes more sense because the pad is
>> not linked to the absolute position of the finger or pen, and usually, the
>> events from the pad are filtered out by the compositor, which then convert
>> them into actions or keyboard shortcuts.
>>
>> For backward compatibility with current xf86-input-wacom, the pad devices
>> still present the ABS_X, ABS_Y and ABS_MISC events, but they can be
>> completely ignored in the new implementation.
>>
>> Signed-off-by: Benjamin Tissoires <[email protected]>
>> ---
>> drivers/input/tablet/wacom.h | 2 ++
>> drivers/input/tablet/wacom_sys.c | 63 +++++++++++++++++++++++++++++++++++-----
>> drivers/input/tablet/wacom_wac.c | 27 ++++++++++++++++-
>> drivers/input/tablet/wacom_wac.h | 2 ++
>> 4 files changed, 85 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/input/tablet/wacom.h b/drivers/input/tablet/wacom.h
>> index 9ebf0ed..caa59ca 100644
>> --- a/drivers/input/tablet/wacom.h
>> +++ b/drivers/input/tablet/wacom.h
>> @@ -136,4 +136,6 @@ void wacom_wac_irq(struct wacom_wac *wacom_wac, size_t len);
>> void wacom_setup_device_quirks(struct wacom_features *features);
>> int wacom_setup_input_capabilities(struct input_dev *input_dev,
>> struct wacom_wac *wacom_wac);
>> +int wacom_setup_pad_input_capabilities(struct input_dev *input_dev,
>> + struct wacom_wac *wacom_wac);
>> #endif
>> diff --git a/drivers/input/tablet/wacom_sys.c b/drivers/input/tablet/wacom_sys.c
>> index c993eee..b9bf37e 100644
>> --- a/drivers/input/tablet/wacom_sys.c
>> +++ b/drivers/input/tablet/wacom_sys.c
>> @@ -135,6 +135,9 @@ static int wacom_open(struct input_dev *dev)
>>
>> mutex_lock(&wacom->lock);
>>
>> + if (wacom->open)
>> + goto out;
>> +
>> if (usb_submit_urb(wacom->irq, GFP_KERNEL)) {
>> retval = -EIO;
>> goto out;
>> @@ -157,9 +160,14 @@ static void wacom_close(struct input_dev *dev)
>> autopm_error = usb_autopm_get_interface(wacom->intf);
>>
>> mutex_lock(&wacom->lock);
>> + if (!wacom->open)
>> + goto out;
>> +
>> usb_kill_urb(wacom->irq);
>> wacom->open = false;
>> wacom->intf->needs_remote_wakeup = 0;
>> +
>> +out:
>> mutex_unlock(&wacom->lock);
>>
>> if (!autopm_error)
>> @@ -1109,19 +1117,16 @@ static void wacom_destroy_battery(struct wacom *wacom)
>> }
>> }
>>
>> -static int wacom_register_input(struct wacom *wacom)
>> +static struct input_dev *wacom_allocate_input(struct wacom *wacom)
>> {
>> struct input_dev *input_dev;
>> struct usb_interface *intf = wacom->intf;
>> struct usb_device *dev = interface_to_usbdev(intf);
>> struct wacom_wac *wacom_wac = &(wacom->wacom_wac);
>> - int error;
>>
>> input_dev = input_allocate_device();
>> - if (!input_dev) {
>> - error = -ENOMEM;
>> - goto fail1;
>> - }
>> + if (!input_dev)
>> + return NULL;
>>
>> input_dev->name = wacom_wac->name;
>> input_dev->phys = wacom->phys;
>> @@ -1131,21 +1136,59 @@ static int wacom_register_input(struct wacom *wacom)
>> usb_to_input_id(dev, &input_dev->id);
>> input_set_drvdata(input_dev, wacom);
>>
>> + return input_dev;
>> +}
>> +
>> +static int wacom_register_input(struct wacom *wacom)
>> +{
>> + struct input_dev *input_dev, *pad_input_dev;
>> + struct wacom_wac *wacom_wac = &(wacom->wacom_wac);
>> + int error;
>> +
>> + input_dev = wacom_allocate_input(wacom);
>> + pad_input_dev = wacom_allocate_input(wacom);
>> + if (!input_dev || !pad_input_dev) {
>> + error = -ENOMEM;
>> + goto fail1;
>> + }
>> +
>> wacom_wac->input = input_dev;
>> + wacom_wac->pad_input = pad_input_dev;
>> + wacom_wac->pad_input->name = wacom_wac->pad_name;
>> +
>> error = wacom_setup_input_capabilities(input_dev, wacom_wac);
>> if (error)
>> - goto fail1;
>> + goto fail2;
>>
>> error = input_register_device(input_dev);
>> if (error)
>> goto fail2;
>>
>> + error = wacom_setup_pad_input_capabilities(pad_input_dev, wacom_wac);
>> + if (error) {
>> + /* no pad in use on this interface */
>> + input_free_device(pad_input_dev);
>> + wacom_wac->pad_input = NULL;
>> + pad_input_dev = NULL;
>> + } else {
>> + error = input_register_device(pad_input_dev);
>> + if (error)
>> + goto fail3;
>> + }
>> +
>> return 0;
>>
>> +fail3:
>> + input_unregister_device(input_dev);
>> + input_dev = NULL;
>> fail2:
>> - input_free_device(input_dev);
>> wacom_wac->input = NULL;
>> + wacom_wac->pad_input = NULL;
>> fail1:
>> + if (input_dev)
>> + input_free_device(input_dev);
>> + if (pad_input_dev)
>> + input_free_device(pad_input_dev);
>> return error;
>> }
>>
>> @@ -1364,6 +1407,8 @@ static int wacom_probe(struct usb_interface *intf, const struct usb_device_id *i
>> wacom_calculate_res(features);
>>
>> strlcpy(wacom_wac->name, features->name, sizeof(wacom_wac->name));
>> + snprintf(wacom_wac->pad_name, sizeof(wacom_wac->pad_name),
>> + "%s Pad", features->name);
>
> This may cause some heartburn for some users that have xsetwacom
> scripts (or use similar tools that need a device name). Our X driver
> already appends a " pad" suffix to the device name, so this results in
> X devices now having the double-suffix " Pad pad". I agree with adding
> the suffix here though, so I think I'll write a patch to fix this in
> xf86-input-wacom.

Yeah, but as there was already some part of the code which appended
such suffixes in some cases, I think fixing this in xf86-input-wacom
is the best solution.

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....
>
>>
>> if (features->quirks & WACOM_QUIRK_MULTI_INPUT) {
>> struct usb_device *other_dev;
>> @@ -1438,6 +1483,8 @@ static void wacom_disconnect(struct usb_interface *intf)
>> cancel_work_sync(&wacom->work);
>> if (wacom->wacom_wac.input)
>> input_unregister_device(wacom->wacom_wac.input);
>> + if (wacom->wacom_wac.pad_input)
>> + input_unregister_device(wacom->wacom_wac.pad_input);
>> wacom_destroy_battery(wacom);
>> wacom_destroy_leds(wacom);
>> usb_free_urb(wacom->irq);
>> diff --git a/drivers/input/tablet/wacom_wac.c b/drivers/input/tablet/wacom_wac.c
>> index 977d05c..4b16a34 100644
>> --- a/drivers/input/tablet/wacom_wac.c
>> +++ b/drivers/input/tablet/wacom_wac.c
>> @@ -1489,8 +1489,11 @@ void wacom_wac_irq(struct wacom_wac *wacom_wac, size_t len)
>> break;
>> }
>>
>> - if (sync)
>> + if (sync) {
>> input_sync(wacom_wac->input);
>> + if (wacom_wac->pad_input)
>> + input_sync(wacom_wac->pad_input);
>> + }
>> }
>>
>> static void wacom_setup_cintiq(struct wacom_wac *wacom_wac)
>> @@ -1939,6 +1942,28 @@ int wacom_setup_input_capabilities(struct input_dev *input_dev,
>> return 0;
>> }
>>
>> +int wacom_setup_pad_input_capabilities(struct input_dev *input_dev,
>> + struct wacom_wac *wacom_wac)
>> +{
>> + struct wacom_features *features = &wacom_wac->features;
>> +
>> + input_dev->evbit[0] |= BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
>> +
>> + /* kept for making legacy xf86-input-wacom working with the wheels */
>> + __set_bit(ABS_MISC, input_dev->absbit);
>> +
>> + /* kept for making legacy xf86-input-wacom accepting the pad */
>> + input_set_abs_params(input_dev, ABS_X, 0, 1, 0, 0);
>> + input_set_abs_params(input_dev, ABS_Y, 0, 1, 0, 0);
>> +
>> + switch (features->type) {
>> + default:
>> + /* no pad supported */
>> + return 1;
>> + }
>> + return 0;
>> +}
>> +
>> static const struct wacom_features wacom_features_0x00 =
>> { "Wacom Penpartner", WACOM_PKGLEN_PENPRTN, 5040, 3780, 255,
>> 0, PENPARTNER, WACOM_PENPRTN_RES, WACOM_PENPRTN_RES };
>> diff --git a/drivers/input/tablet/wacom_wac.h b/drivers/input/tablet/wacom_wac.h
>> index b2c9a9c..f48164c 100644
>> --- a/drivers/input/tablet/wacom_wac.h
>> +++ b/drivers/input/tablet/wacom_wac.h
>> @@ -150,6 +150,7 @@ struct wacom_shared {
>>
>> struct wacom_wac {
>> char name[WACOM_NAME_MAX];
>> + char pad_name[WACOM_NAME_MAX];
>> unsigned char *data;
>> int tool[2];
>> int id[2];
>> @@ -157,6 +158,7 @@ struct wacom_wac {
>> struct wacom_features features;
>> struct wacom_shared *shared;
>> struct input_dev *input;
>> + struct input_dev *pad_input;
>> int pid;
>> int battery_capacity;
>> int num_contacts_left;
>> --
>> 1.9.0
>>
>
> ------------------------------------------------------------------------------
> Open source business process management suite built on Java and Eclipse
> Turn processes into business applications with Bonita BPM Community Edition
> Quickly connect people, data, and systems into organized workflows
> Winner of BOSSIE, CODIE, OW2 and Gartner awards
> http://p.sf.net/sfu/Bonitasoft
> _______________________________________________
> Linuxwacom-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel