On Wed, Dec 29, 2010 at 02:30:24PM -0800, Ping Cheng wrote:
> Dmitry,
>
> Thank you for your effort in updating the patch. Most changes work for
> me. I do have comments related to raw touch data scaling for MT and
> single touch legacy client support. Details are inline.
>
> Ping
>
> On Tue, Dec 28, 2010 at 11:40 PM, Dmitry Torokhov
> <dmitry.torokhov@xxxxxxxxx> wrote:
> > Hi Ping,
> >
> > On Fri, Dec 17, 2010 at 09:37:54AM -0800, Ping Cheng wrote:
> >> Signed-off-by: Ping Cheng <pingc@xxxxxxxxx>
> >> ---
> >> Âdrivers/input/touchscreen/wacom_w8001.c | Â 89 ++++++++++++++++++++++++++++---
> >> Â1 files changed, 82 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
> >> index 59664a8..763eb8f 100644
> >> --- a/drivers/input/touchscreen/wacom_w8001.c
> >> +++ b/drivers/input/touchscreen/wacom_w8001.c
> >> @@ -3,6 +3,7 @@
> >> Â *
> >> Â * Copyright (c) 2008 Jaya Kumar
> >> Â * Copyright (c) 2010 Red Hat, Inc.
> >> + * Copyright (c) 2010 Ping Cheng, Wacom. <pingc@xxxxxxxxx>
> >> Â *
> >> Â * This file is subject to the terms and conditions of the GNU General Public
> >> Â * License. See the file COPYING in the main directory of this archive for
> >> @@ -86,6 +87,12 @@ struct w8001 {
> >> Â Â Â char phys[32];
> >> Â Â Â int type;
> >> Â Â Â unsigned int pktlen;
> >> + Â Â bool pen_in_prox;
> >> + Â Â bool has_touch;
> >
> > We already have type, why do we need these 2 fields a well?
> >
> > Actually, I tried massaging the patch a bit, could you please tell me if
> > the patch below still works for you?
> >
> > Thanks.
> >
> > --
> > Dmitry
> >
> >
> > Input: wacom_w8001 - add one finger touch support
> >
> > From: Ping Cheng <pinglinux@xxxxxxxxx>
> >
> > Signed-off-by: Ping Cheng <pingc@xxxxxxxxx>
> > Signed-off-by: Dmitry Torokhov <dtor@xxxxxxx>
> > ---
> >
> > Âdrivers/input/touchscreen/wacom_w8001.c | Â123 +++++++++++++++++++++++++------
> > Â1 files changed, 101 insertions(+), 22 deletions(-)
> >
> >
> > diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
> > index 4774c09..ce8c8e1 100644
> > --- a/drivers/input/touchscreen/wacom_w8001.c
> > +++ b/drivers/input/touchscreen/wacom_w8001.c
> > @@ -3,6 +3,7 @@
> > Â*
> > Â* Copyright (c) 2008 Jaya Kumar
> > Â* Copyright (c) 2010 Red Hat, Inc.
> > + * Copyright (c) 2010 Ping Cheng, Wacom. <pingc@xxxxxxxxx>
> > Â*
> > Â* This file is subject to the terms and conditions of the GNU General Public
> > Â* License. See the file COPYING in the main directory of this archive for
> > @@ -63,11 +64,11 @@ struct w8001_coord {
> >
> > Â/* touch query reply packet */
> > Âstruct w8001_touch_query {
> > + Â Â Â u16 x;
> > + Â Â Â u16 y;
> > Â Â Â Âu8 panel_res;
> > Â Â Â Âu8 capacity_res;
> > Â Â Â Âu8 sensor_id;
> > - Â Â Â u16 x;
> > - Â Â Â u16 y;
> > Â};
> >
> > Â/*
> > @@ -86,9 +87,13 @@ struct w8001 {
> > Â Â Â Âchar phys[32];
> > Â Â Â Âint type;
> > Â Â Â Âunsigned int pktlen;
> > + Â Â Â u16 max_touch_x;
> > + Â Â Â u16 max_touch_y;
> > + Â Â Â u16 max_pen_x;
> > + Â Â Â u16 max_pen_y;
> > Â};
> >
> > -static void parse_data(u8 *data, struct w8001_coord *coord)
> > +static void parse_pen_data(u8 *data, struct w8001_coord *coord)
> > Â{
> > Â Â Â Âmemset(coord, 0, sizeof(*coord));
> >
> > @@ -112,7 +117,14 @@ static void parse_data(u8 *data, struct w8001_coord *coord)
> > Â Â Â Âcoord->tilt_y = data[8] & 0x7F;
> > Â}
> >
> > -static void parse_touch(struct w8001 *w8001)
> > +static void parse_single_touch(u8 *data, struct w8001_coord *coord)
> > +{
> > + Â Â Â coord->x = (data[1] << 7) | data[2];
> > + Â Â Â coord->y = (data[3] << 7) | data[4];
> > + Â Â Â coord->tsw = data[0] & 0x01;
> > +}
> > +
> > +static void parse_multi_touch(struct w8001 *w8001)
> > Â{
> > Â Â Â Âstruct input_dev *dev = w8001->dev;
> > Â Â Â Âunsigned char *data = w8001->data;
> > @@ -124,8 +136,8 @@ static void parse_touch(struct w8001 *w8001)
> > Â Â Â Â Â Â Â Âinput_mt_slot(dev, i);
> > Â Â Â Â Â Â Â Âinput_mt_report_slot_state(dev, MT_TOOL_FINGER, touch);
> > Â Â Â Â Â Â Â Âif (touch) {
> > - Â Â Â Â Â Â Â Â Â Â Â int x = (data[6 * i + 1] << 7) | (data[6 * i + 2]);
> > - Â Â Â Â Â Â Â Â Â Â Â int y = (data[6 * i + 3] << 7) | (data[6 * i + 4]);
> > + Â Â Â Â Â Â Â Â Â Â Â int x = (data[6 * i + 1] << 7) | data[6 * i + 2];
> > + Â Â Â Â Â Â Â Â Â Â Â int y = (data[6 * i + 3] << 7) | data[6 * i + 4];
>
> Since you scaled MT max_touch_x/y to max_pen_x/y, you would need to
> scale the x and y here as well. I didn't scale MT touch data since MT
> protocol can report touch resolution on its own. See more comments
> below.
Hmm, it depends on whether there are multitouch devices that are also
penabled. Are there such devices? If not then we won't actually scale
anyting.
>
> > Â Â Â Â Â Â Â Â Â Â Â Â/* data[5,6] and [11,12] is finger capacity */
> >
> > Â Â Â Â Â Â Â Â Â Â Â Âinput_report_abs(dev, ABS_MT_POSITION_X, x);
> > @@ -151,6 +163,15 @@ static void parse_touchquery(u8 *data, struct w8001_touch_query *query)
> > Â Â Â Âquery->y = data[5] << 9;
> > Â Â Â Âquery->y |= data[6] << 2;
> > Â Â Â Âquery->y |= (data[2] >> 3) & 0x3;
> > +
> > + Â Â Â /* Early days' single-finger touch models need the following defaults */
> > + Â Â Â if (!query->x && !query->y) {
> > + Â Â Â Â Â Â Â query->x = 1024;
> > + Â Â Â Â Â Â Â query->y = 1024;
> > + Â Â Â Â Â Â Â if (query->panel_res)
> > + Â Â Â Â Â Â Â Â Â Â Â query->x = query->y = (1 << query->panel_res);
> > + Â Â Â Â Â Â Â query->panel_res = 10;
> > + Â Â Â }
> > Â}
> >
> > Âstatic void report_pen_events(struct w8001 *w8001, struct w8001_coord *coord)
> > @@ -160,16 +181,15 @@ static void report_pen_events(struct w8001 *w8001, struct w8001_coord *coord)
> > Â Â Â Â/*
> > Â Â Â Â * We have 1 bit for proximity (rdy) and 3 bits for tip, side,
> > Â Â Â Â * side2/eraser. If rdy && f2 are set, this can be either pen + side2,
> > - Â Â Â Â* or eraser. assume
> > + Â Â Â Â* or eraser. Assume:
> > Â Â Â Â * - if dev is already in proximity and f2 is toggled â pen + side2
> > Â Â Â Â * - if dev comes into proximity with f2 set â eraser
> > Â Â Â Â * If f2 disappears after assuming eraser, fake proximity out for
> > Â Â Â Â * eraser and in for pen.
> > Â Â Â Â */
> >
> > - Â Â Â if (!w8001->type) {
> > - Â Â Â Â Â Â Â w8001->type = coord->f2 ? BTN_TOOL_RUBBER : BTN_TOOL_PEN;
> > - Â Â Â } else if (w8001->type == BTN_TOOL_RUBBER) {
> > + Â Â Â switch (w8001->type) {
> > + Â Â Â case BTN_TOOL_RUBBER:
> > Â Â Â Â Â Â Â Âif (!coord->f2) {
> > Â Â Â Â Â Â Â Â Â Â Â Âinput_report_abs(dev, ABS_PRESSURE, 0);
> > Â Â Â Â Â Â Â Â Â Â Â Âinput_report_key(dev, BTN_TOUCH, 0);
> > @@ -179,8 +199,21 @@ static void report_pen_events(struct w8001 *w8001, struct w8001_coord *coord)
> > Â Â Â Â Â Â Â Â Â Â Â Âinput_sync(dev);
> > Â Â Â Â Â Â Â Â Â Â Â Âw8001->type = BTN_TOOL_PEN;
> > Â Â Â Â Â Â Â Â}
> > - Â Â Â } else {
> > + Â Â Â Â Â Â Â break;
> > +
> > + Â Â Â case BTN_TOOL_FINGER:
> > + Â Â Â Â Â Â Â input_report_key(dev, BTN_TOUCH, 0);
> > + Â Â Â Â Â Â Â input_report_key(dev, BTN_TOOL_FINGER, 0);
> > + Â Â Â Â Â Â Â input_sync(dev);
> > + Â Â Â Â Â Â Â /* fall through */
> > +
> > + Â Â Â case KEY_RESERVED:
> > + Â Â Â Â Â Â Â w8001->type = coord->f2 ? BTN_TOOL_RUBBER : BTN_TOOL_PEN;
> > + Â Â Â Â Â Â Â break;
> > +
> > + Â Â Â default:
> > Â Â Â Â Â Â Â Âinput_report_key(dev, BTN_STYLUS2, coord->f2);
> > + Â Â Â Â Â Â Â break;
> > Â Â Â Â}
> >
> > Â Â Â Âinput_report_abs(dev, ABS_X, coord->x);
> > @@ -192,7 +225,30 @@ static void report_pen_events(struct w8001 *w8001, struct w8001_coord *coord)
> > Â Â Â Âinput_sync(dev);
> >
> > Â Â Â Âif (!coord->rdy)
> > - Â Â Â Â Â Â Â w8001->type = 0;
> > + Â Â Â Â Â Â Â w8001->type = KEY_RESERVED;
> > +}
> > +
> > +static void report_single_touch(struct w8001 *w8001, struct w8001_coord *coord)
> > +{
> > + Â Â Â struct input_dev *dev = w8001->dev;
> > + Â Â Â unsigned int x = coord->x;
> > + Â Â Â unsigned int y = coord->y;
> > +
> > + Â Â Â /* scale to pen maximum */
> > + Â Â Â if (w8001->max_pen_x && w8001->max_touch_x)
> > + Â Â Â Â Â Â Â x = x * w8001->max_pen_x / w8001->max_touch_x;
> > +
> > + Â Â Â if (w8001->max_pen_y && w8001->max_touch_y)
> > + Â Â Â Â Â Â Â y = y * w8001->max_pen_y / w8001->max_touch_y;
> > +
> > + Â Â Â input_report_abs(dev, ABS_X, x);
> > + Â Â Â input_report_abs(dev, ABS_Y, y);
> > + Â Â Â input_report_key(dev, BTN_TOUCH, coord->tsw);
> > + Â Â Â input_report_key(dev, BTN_TOOL_FINGER, coord->tsw);
> > +
> > + Â Â Â input_sync(dev);
> > +
> > + Â Â Â w8001->type = coord->tsw ? BTN_TOOL_FINGER : KEY_RESERVED;
> > Â}
> >
> > Âstatic irqreturn_t w8001_interrupt(struct serio *serio,
> > @@ -213,9 +269,18 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
> >
> > Â Â Â Âcase W8001_PKTLEN_TOUCH93 - 1:
> > Â Â Â Âcase W8001_PKTLEN_TOUCH9A - 1:
> > - Â Â Â Â Â Â Â /* ignore one-finger touch packet. */
> > - Â Â Â Â Â Â Â if (w8001->pktlen == w8001->idx)
> > + Â Â Â Â Â Â Â tmp = w8001->data[0] & W8001_TOUCH_BYTE;
> > + Â Â Â Â Â Â Â if (tmp != W8001_TOUCH_BYTE)
> > + Â Â Â Â Â Â Â Â Â Â Â break;
> > +
> > + Â Â Â Â Â Â Â if (w8001->pktlen == w8001->idx) {
> > Â Â Â Â Â Â Â Â Â Â Â Âw8001->idx = 0;
> > + Â Â Â Â Â Â Â Â Â Â Â if (w8001->type != BTN_TOOL_PEN &&
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â w8001->type != BTN_TOOL_RUBBER) {
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â parse_single_touch(w8001->data, &coord);
> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â report_single_touch(w8001, &coord);
> > + Â Â Â Â Â Â Â Â Â Â Â }
> > + Â Â Â Â Â Â Â }
> > Â Â Â Â Â Â Â Âbreak;
> >
> > Â Â Â Â/* Pen coordinates packet */
> > @@ -224,18 +289,18 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
> > Â Â Â Â Â Â Â Âif (unlikely(tmp == W8001_TAB_BYTE))
> > Â Â Â Â Â Â Â Â Â Â Â Âbreak;
> >
> > - Â Â Â Â Â Â Â tmp = (w8001->data[0] & W8001_TOUCH_BYTE);
> > + Â Â Â Â Â Â Â tmp = w8001->data[0] & W8001_TOUCH_BYTE;
> > Â Â Â Â Â Â Â Âif (tmp == W8001_TOUCH_BYTE)
> > Â Â Â Â Â Â Â Â Â Â Â Âbreak;
> >
> > Â Â Â Â Â Â Â Âw8001->idx = 0;
> > - Â Â Â Â Â Â Â parse_data(w8001->data, &coord);
> > + Â Â Â Â Â Â Â parse_pen_data(w8001->data, &coord);
> > Â Â Â Â Â Â Â Âreport_pen_events(w8001, &coord);
> > Â Â Â Â Â Â Â Âbreak;
> >
> > Â Â Â Â/* control packet */
> > Â Â Â Âcase W8001_PKTLEN_TPCCTL - 1:
> > - Â Â Â Â Â Â Â tmp = (w8001->data[0] & W8001_TOUCH_MASK);
> > + Â Â Â Â Â Â Â tmp = w8001->data[0] & W8001_TOUCH_MASK;
> > Â Â Â Â Â Â Â Âif (tmp == W8001_TOUCH_BYTE)
> > Â Â Â Â Â Â Â Â Â Â Â Âbreak;
> >
> > @@ -248,7 +313,7 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
> > Â Â Â Â/* 2 finger touch packet */
> > Â Â Â Âcase W8001_PKTLEN_TOUCH2FG - 1:
> > Â Â Â Â Â Â Â Âw8001->idx = 0;
> > - Â Â Â Â Â Â Â parse_touch(w8001);
> > + Â Â Â Â Â Â Â parse_multi_touch(w8001);
> > Â Â Â Â Â Â Â Âbreak;
> > Â Â Â Â}
> >
> > @@ -278,6 +343,7 @@ static int w8001_setup(struct w8001 *w8001)
> > Â{
> > Â Â Â Âstruct input_dev *dev = w8001->dev;
> > Â Â Â Âstruct w8001_coord coord;
> > + Â Â Â struct w8001_touch_query touch;
> > Â Â Â Âint error;
> >
> > Â Â Â Âerror = w8001_command(w8001, W8001_CMD_STOP, false);
> > @@ -289,11 +355,15 @@ static int w8001_setup(struct w8001 *w8001)
> > Â Â Â Â/* penabled? */
> > Â Â Â Âerror = w8001_command(w8001, W8001_CMD_QUERY, true);
> > Â Â Â Âif (!error) {
> > + Â Â Â Â Â Â Â __set_bit(BTN_TOUCH, dev->keybit);
> > Â Â Â Â Â Â Â Â__set_bit(BTN_TOOL_PEN, dev->keybit);
> > Â Â Â Â Â Â Â Â__set_bit(BTN_TOOL_RUBBER, dev->keybit);
> > Â Â Â Â Â Â Â Â__set_bit(BTN_STYLUS, dev->keybit);
> > Â Â Â Â Â Â Â Â__set_bit(BTN_STYLUS2, dev->keybit);
> > - Â Â Â Â Â Â Â parse_data(w8001->response, &coord);
> > +
> > + Â Â Â Â Â Â Â parse_pen_data(w8001->response, &coord);
> > + Â Â Â Â Â Â Â w8001->max_pen_x = coord.x;
> > + Â Â Â Â Â Â Â w8001->max_pen_y = coord.y;
> >
> > Â Â Â Â Â Â Â Âinput_set_abs_params(dev, ABS_X, 0, coord.x, 0, 0);
> > Â Â Â Â Â Â Â Âinput_set_abs_params(dev, ABS_Y, 0, coord.y, 0, 0);
> > @@ -312,24 +382,34 @@ static int w8001_setup(struct w8001 *w8001)
> > Â Â Â Â * second byte is empty, which indicates touch is not supported.
> > Â Â Â Â */
> > Â Â Â Âif (!error && w8001->response[1]) {
> > - Â Â Â Â Â Â Â struct w8001_touch_query touch;
> > + Â Â Â Â Â Â Â __set_bit(BTN_TOUCH, dev->keybit);
> > + Â Â Â Â Â Â Â __set_bit(BTN_TOOL_FINGER, dev->keybit);
>
> I do not think we want to set BTN_TOUCH and BTN_TOOL_FINGER for MT
> device since we do not need to support single touch for this MT
> device. Please refer to my comments for your other patch in the next
> email.
I guess for touchscreens we may say that we may omit BTN_TOOL_FINGER if
finger is the only "tool" that is supported, to keep in line with
current touchscreen drivers. BTN_TOUCH is needed for legacy.
>
> > Â Â Â Â Â Â Â Âparse_touchquery(w8001->response, &touch);
> > + Â Â Â Â Â Â Â w8001->max_touch_x = touch.x;
> > + Â Â Â Â Â Â Â w8001->max_touch_y = touch.y;
> > +
> > + Â Â Â Â Â Â Â /* scale to pen maximum */
> > + Â Â Â Â Â Â Â if (w8001->max_pen_x && w8001->max_pen_y) {
> > + Â Â Â Â Â Â Â Â Â Â Â touch.x = w8001->max_pen_x;
> > + Â Â Â Â Â Â Â Â Â Â Â touch.y = w8001->max_pen_y;
> > + Â Â Â Â Â Â Â }
>
> If we scale touch for both single and MT, we need to scale the raw
> (x,y) for both too. I purposely did not want to scale MT touch since
> they can be supported in different resolution from pen by MT protocol
> now.
I am really not sure if this is good thing that MT has it's own
min/max/resolution. It is more implementation atrifact that it has...
My gut feelig is that we shoudl keep it in sync with ST values,
especially if we want to perform pointer emulation.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
