DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=mime-version:in-reply-to:references:date:message-id:subject:from:to
         :cc:content-type:content-transfer-encoding;
        b=m10fAPp57WCU6y0QI/Ec88XWihSXE2I76p/Mqstf9lvXrHgA77HKqZSjQPCrVXsDYL
         xheNwmOZzL2UQ/KifNrqlZqqk2o65X/Q/H6lHikpXGrUYhPNk10lO8iPbASBzzleoR+9
         nHrIHhZO673hQ70o2LC6MnMGprmxotsB1dnpg=
MIME-Version: 1.0
In-Reply-To: <4C722ECF.5020706@euromail.se>
References: <1282280135-15942-1-git-send-email-peter.hutterer@who-t.net>
	<1282280135-15942-4-git-send-email-peter.hutterer@who-t.net>
	<4C722ECF.5020706@euromail.se>
Date: Mon, 23 Aug 2010 16:11:56 +0200
Message-ID: <AANLkTi=0dsqZg4kTujfzmspcDeoYHnbnGy9JN4y-OLhY@mail.gmail.com>
Subject: Re: [PATCH 3/3] input: add multitouch slot support to w8001.
From: Ping Cheng <pinglinux@gmail.com>
To: Henrik Rydberg <rydberg@euromail.se>
Cc: Peter Hutterer <peter.hutterer@who-t.net>, linux-input@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 7434
Lines: 218

On Mon, Aug 23, 2010 at 10:18 AM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Peter,
>
> thanks for the patches. Comments inline.
>
>> Some serial wacom devices support two-finger touch. Test for this during
>
>> init and parse the touch packets accordingly. Touch packets are
>> processed using Protocol B (MT Slots).
>>
>> Note: there are several wacom versions that do touch but not two-finger
>> touch. These are not catered for here, touch events for these are simply
>> discarded.
>>
>> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
>> CC: Henrik Rydberg <rydberg@euromail.se>
>> ---
>> ?drivers/input/touchscreen/wacom_w8001.c | ? 99 ++++++++++++++++++++++++++++++-
>> ?1 files changed, 96 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
>> index c302cc3..a38a3aa 100644
>> --- a/drivers/input/touchscreen/wacom_w8001.c
>> +++ b/drivers/input/touchscreen/wacom_w8001.c
>> @@ -60,6 +60,17 @@ struct w8001_coord {
>> ? ? ? u8 tilt_y;
>> ?};
>>
>> +/* touch data packet */
>> +struct w8001_touch {
>> + ? ? u8 f1_status;
>> + ? ? u16 f1_x;
>> + ? ? u16 f1_y;
>> + ? ? /* only some tablets have 2FG info */
>> + ? ? u8 f2_status;
>> + ? ? u16 f2_x;
>> + ? ? u16 f2_y;
>> +};
>
>
> No pressure information from this device?

No useful pressure/capacity available for this device.

>
>> +
>> ?/* touch query reply packet */
>> ?struct w8001_touch_query {
>> ? ? ? u8 panel_res;
>> @@ -85,8 +96,18 @@ struct w8001 {
>> ? ? ? char phys[32];
>> ? ? ? int type;
>> ? ? ? unsigned int pktlen;
>> + ? ? unsigned char tracking_id[2];
>> ?};
>>
>> +static int get_next_tracking_id(void)
>> +{
>> + ? ? static unsigned char next_tracking_id;
>> + ? ? next_tracking_id = (next_tracking_id + 1) % 256;
>> + ? ? if (next_tracking_id == 0)
>> + ? ? ? ? ? ? next_tracking_id = 1;
>> + ? ? return next_tracking_id;
>> +}
>
>
> Zero is part of the valid range from 2.6.36. Can be implemented simply as
> (tracking_id++ & 0xfff), see recent MT slots patches.
>
>> +
>> ?static void parse_data(u8 *data, struct w8001_coord *coord)
>> ?{
>> ? ? ? memset(coord, 0, sizeof(*coord));
>> @@ -111,6 +132,26 @@ static void parse_data(u8 *data, struct w8001_coord *coord)
>> ? ? ? coord->tilt_y = data[8] & 0x7F;
>> ?}
>>
>> +static void parse_touch(u8 *data,
>> + ? ? ? ? ? ? ? ? ? ? unsigned int pktlen, struct w8001_touch *touch)
>> +{
>> + ? ? memset(touch, 0, sizeof(*touch));
>> +
>> + ? ? touch->f1_status = data[0] & 0x1;
>> + ? ? touch->f1_x = data[1] << 7;
>> + ? ? touch->f1_x |= data[2];
>> + ? ? touch->f1_y = data[3] << 7;
>> + ? ? touch->f1_y |= data[4];
>
>
> What are data[5] and data[6]? Why is f1_x/y split into several lines?
>
>> +
>> + ? ? if (pktlen >= W8001_PKTLEN_TOUCH2FG) {
>> + ? ? ? ? ? ? touch->f2_status = (data[0] & 0x2) >> 1;
>> + ? ? ? ? ? ? touch->f2_x = data[7] << 7;
>> + ? ? ? ? ? ? touch->f2_x |= data[8];
>> + ? ? ? ? ? ? touch->f2_y = data[9] << 7;
>> + ? ? ? ? ? ? touch->f2_y |= data[10];
>> + ? ? }
>
>
> What are data[11] and data[12]?
>
> Since all needed information is available here, there seems to be little reason
> for the w8001_touch structure. Why not complete the report in this function?
>
>> +}
>> +
>> ?static void parse_touchquery(u8 *data, struct w8001_touch_query *query)
>> ?{
>> ? ? ? memset(query, 0, sizeof(*query));
>> @@ -128,6 +169,46 @@ static void parse_touchquery(u8 *data, struct w8001_touch_query *query)
>> ? ? ? query->y |= (data[2] >> 3) & 0x3;
>> ?}
>>
>> +static void w8001_mt_event(struct input_dev *dev,
>> + ? ? ? ? ? ? ? ? ? ? ? ?int slot, int tid, int x, int y)
>> +{
>> + ? ? input_mt_slot(dev, slot);
>> + ? ? if (tid != 0) {
>> + ? ? ? ? ? ? input_report_abs(dev, ABS_MT_POSITION_X, x);
>> + ? ? ? ? ? ? input_report_abs(dev, ABS_MT_POSITION_Y, y);
>> + ? ? }
>> + ? ? input_report_abs(dev, ABS_MT_TRACKING_ID, tid);
>> + ? ? input_report_abs(dev, ABS_MT_TOOL_TYPE, MT_TOOL_FINGER);
>> + ? ? input_mt_sync(dev);
>> +}
>
>> +
>> +static void w8001_track_fingers(struct w8001 *w8001, struct w8001_touch *touch)
>> +{
>> + ? ? struct input_dev *dev = w8001->dev;
>> +
>> + ? ? if (touch->f1_status) {
>> + ? ? ? ? ? ? if (!w8001->tracking_id[0])
>> + ? ? ? ? ? ? ? ? ? ? w8001->tracking_id[0] = get_next_tracking_id();
>> + ? ? ? ? ? ? w8001_mt_event(dev, 0, w8001->tracking_id[0],
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?touch->f1_x, touch->f1_y);
>> + ? ? } else if (w8001->tracking_id[0]) {
>> + ? ? ? ? ? ? w8001->tracking_id[0] = 0;
>> + ? ? ? ? ? ? w8001_mt_event(dev, 0, 0, touch->f1_x, touch->f1_y);
>> + ? ? }
>
>> +
>> + ? ? if (touch->f2_status) {
>> + ? ? ? ? ? ? if (!w8001->tracking_id[1])
>> + ? ? ? ? ? ? ? ? ? ? w8001->tracking_id[1] = get_next_tracking_id();
>> + ? ? ? ? ? ? w8001_mt_event(dev, 1, w8001->tracking_id[1],
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?touch->f2_x, touch->f2_y);
>> + ? ? } else if (w8001->tracking_id[1]) {
>> + ? ? ? ? ? ? w8001->tracking_id[1] = 0;
>> + ? ? ? ? ? ? w8001_mt_event(dev, 1, 0, touch->f2_x, touch->f2_y);
>> + ? ? }
>> +
>> + ? ? input_sync(dev);
>> +}
>
>
> Apparently, f1 and f2 represent the slots themselves, i.e., the device uses
> slots internally. Please simplify the logic accordingly. There is an example in
> the recent patch for the Bamboo Touch.
>
>> +
>> ?static void report_pen_events(struct w8001 *w8001, struct w8001_coord *coord)
>> ?{
>> ? ? ? struct input_dev *dev = w8001->dev;
>> @@ -172,6 +253,7 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
>> ?{
>> ? ? ? struct w8001 *w8001 = serio_get_drvdata(serio);
>> ? ? ? struct w8001_coord coord;
>> + ? ? struct w8001_touch touch;
>> ? ? ? unsigned char tmp;
>>
>> ? ? ? w8001->data[w8001->idx] = data;
>> @@ -217,10 +299,11 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
>> ? ? ? ? ? ? ? complete(&w8001->cmd_done);
>> ? ? ? ? ? ? ? break;
>>
>> + ? ? /* 2 finger touch packet */
>> ? ? ? case W8001_PKTLEN_TOUCH2FG - 1:
>> - ? ? ? ? ? ? /* ignore two-finger touch packet. */
>> - ? ? ? ? ? ? if (w8001->pktlen == w8001->idx)
>> - ? ? ? ? ? ? ? ? ? ? w8001->idx = 0;
>> + ? ? ? ? ? ? w8001->idx = 0;
>> + ? ? ? ? ? ? parse_touch(w8001->data, w8001->pktlen, &touch);
>> + ? ? ? ? ? ? w8001_track_fingers(w8001, &touch);
>> ? ? ? ? ? ? ? break;
>> ? ? ? }
>>
>> @@ -282,6 +365,16 @@ static int w8001_setup(struct w8001 *w8001)
>> ? ? ? ? ? ? ? ? ? ? ? break;
>> ? ? ? ? ? ? ? case 5:
>> ? ? ? ? ? ? ? ? ? ? ? w8001->pktlen = W8001_PKTLEN_TOUCH2FG;
>> +
>> + ? ? ? ? ? ? ? ? ? ? input_mt_create_slots(dev, 2);
>> + ? ? ? ? ? ? ? ? ? ? input_set_abs_params(dev, ABS_MT_TRACKING_ID,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0, 255, 0, 0);
>> + ? ? ? ? ? ? ? ? ? ? input_set_abs_params(dev, ABS_MT_POSITION_X,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0, touch.x, 0, 0);
>> + ? ? ? ? ? ? ? ? ? ? input_set_abs_params(dev, ABS_MT_POSITION_Y,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0, touch.y, 0, 0);
>> + ? ? ? ? ? ? ? ? ? ? input_set_abs_params(dev, ABS_MT_TOOL_TYPE,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 0, 0, 0, 0);
>> ? ? ? ? ? ? ? ? ? ? ? break;
>> ? ? ? ? ? ? ? }
>> ? ? ? }
>
>
> No information about signal-to-noise ratio (fuzz) for this device?

No.

Ping
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/