Re: [PATCH 4/5] input: wacom: Add support for the Bamboo Touch trackpad (rev4)

[Ping Cheng <pinglinux@gmail.com>]

Hi Henrik,

I know this patchset is under Linus tree and I've acked it already.
But, I am trying to catch up with the fast moving MT train and to make
sure the train is moving to my destination :).

More comments in line.

Ping

On Sat, Sep 4, 2010 at 6:43 AM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Add support for the Bamboo Touch trackpad, and make it work with
> both the Synaptics X Driver and the Multitouch X Driver. The device
> uses MT slots internally, so the choice of protocol is a given.
>
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---
>  drivers/input/tablet/wacom_wac.c |   88 ++++++++++++++++++++++++++++++++++++++
>  drivers/input/tablet/wacom_wac.h |    3 +
>  2 files changed, 91 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/input/tablet/wacom_wac.c b/drivers/input/tablet/wacom_wac.c
> index ca18b4d..97295aa 100644
> --- a/drivers/input/tablet/wacom_wac.c
> +++ b/drivers/input/tablet/wacom_wac.c
> @@ -855,6 +855,54 @@ static int wacom_tpc_irq(struct wacom_wac *wacom, size_t len)
>        return retval;
>  }
>
> +static int wacom_bpt_irq(struct wacom_wac *wacom, size_t len)
> +{
> +       static int trkid;
> +       struct input_dev *input = wacom->input;
> +       unsigned char *data = wacom->data;
> +       int sp = 0, sx = 0, sy = 0, count = 0;
> +       int i;
> +
> +       if (len != WACOM_PKGLEN_BBTOUCH)
> +               return 0;
> +
> +       for (i = 0; i < 2; i++) {
> +               int p = data[9 * i + 2];
> +               input_mt_slot(input, i);
> +               if (p) {
> +                       int x = get_unaligned_be16(&data[9 * i + 3]) & 0x7ff;
> +                       int y = get_unaligned_be16(&data[9 * i + 5]) & 0x7ff;
> +                       input_report_abs(input, ABS_MT_PRESSURE, p);
> +                       input_report_abs(input, ABS_MT_POSITION_X, x);
> +                       input_report_abs(input, ABS_MT_POSITION_Y, y);
> +                       if (wacom->id[i] < 0)
> +                               wacom->id[i] = trkid++ & MAX_TRACKING_ID;

Why do we need an arbitrary MAX_TRACKING_ID when the device can tell
us how many IDs we can have and it tracks the individual fingers for
us? In this case, there are only two ID/fingers and the ID can be
retrieved from the raw data. I must be missing something in the
defintion of MAX_TRACKING_ID.

> +                       if (!count++)
> +                               sp = p, sx = x, sy = y;
> +               } else {
> +                       wacom->id[i] = -1;
> +               }
> +               input_report_abs(input, ABS_MT_TRACKING_ID, wacom->id[i]);
> +       }
> +
> +       input_report_key(input, BTN_TOUCH, count > 0);
> +       input_report_key(input, BTN_TOOL_FINGER, count == 1);
> +       input_report_key(input, BTN_TOOL_DOUBLETAP, count == 2);
> +
> +       input_report_abs(input, ABS_PRESSURE, sp);
> +       input_report_abs(input, ABS_X, sx);
> +       input_report_abs(input, ABS_Y, sy);
> +
> +       input_report_key(input, BTN_LEFT, (data[1] & 0x08) != 0);
> +       input_report_key(input, BTN_FORWARD, (data[1] & 0x04) != 0);
> +       input_report_key(input, BTN_BACK, (data[1] & 0x02) != 0);
> +       input_report_key(input, BTN_RIGHT, (data[1] & 0x01) != 0);

This implementation impairs the value of those buttons since I know a
lot of users want them configurable. If we can not or do not pass a
generic set to the userland, we will need to make it configurable in
the kernel driver as Dmitry suggested (if I remember it correctly).
Which approach, in the userland or the kernel, do you like, Henrik?
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html