From patchwork Wed Apr 20 09:54:07 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Benjamin Tissoires X-Patchwork-Id: 721411 X-Patchwork-Delegate: jikos@jikos.cz Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by demeter1.kernel.org (8.14.4/8.14.3) with ESMTP id p3K9sabv011031 for ; Wed, 20 Apr 2011 09:54:37 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753040Ab1DTJyK (ORCPT ); Wed, 20 Apr 2011 05:54:10 -0400 Received: from mail-wy0-f174.google.com ([74.125.82.174]:64096 "EHLO mail-wy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751899Ab1DTJyI (ORCPT ); Wed, 20 Apr 2011 05:54:08 -0400 Received: by wya21 with SMTP id 21so449113wya.19 for ; Wed, 20 Apr 2011 02:54:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=7RS4/7rBfaZbHHqHGEGUlsciR1H/TV69oyHN1i5UT7c=; b=mLuAbvXSU+CH3p1E8OdZ1nsYGePZLVWpLd7RR5Qs8TwtKiUm4mbVigRhnq0dDplxSI Ubc9y0nWnOMNXwjOre4GcliO0rzhevQGlTcaHuySQUEw16DVpa1k7DQzDD15j+mKO+hH avpaJahLDim+TSxGryzsJy/0gQGFuMgjJFTc0= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; b=EMXh1lm/77/YHdoEKJ/DWZ4zVn/BdSTddSlOhjf7nSzjhEkce0M2oSYiA7PhYrAsxR J17stJQdYWKbzzV7Hh8Ch8vLERjuI6Xfhw/E1IGrnLNtbbgWB02YFuDRg96h8r48t3xl 9hIFnTigisgiN7Kfpym71ZXWa/VgXh7uh0pM8= MIME-Version: 1.0 Received: by 10.227.71.200 with SMTP id i8mr7284403wbj.176.1303293247076; Wed, 20 Apr 2011 02:54:07 -0700 (PDT) Received: by 10.227.155.207 with HTTP; Wed, 20 Apr 2011 02:54:07 -0700 (PDT) In-Reply-To: <20110420094700.GA2258@polaris.bitmath.org> References: <1303291155-3148-1-git-send-email-salt@salt.com.tw> <20110420094700.GA2258@polaris.bitmath.org> Date: Wed, 20 Apr 2011 11:54:07 +0200 X-Google-Sender-Auth: FW58HioS9geIi05qzno-gk3rY_s Message-ID: Subject: Re: [PATCH] HID: add support for PenMount dual-touch panel From: Benjamin Tissoires To: Henrik Rydberg , Richard Nauber Cc: PenMount , Dmitry Torokhov , Jiri Kosina , Stephane Chatty , linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, PenMount Sender: linux-input-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Wed, 20 Apr 2011 09:54:37 +0000 (UTC) Hi Henrik, On Wed, Apr 20, 2011 at 11:47, Henrik Rydberg wrote: > Hi, > > thanks for the patch. Please find comments inline. > > On Wed, Apr 20, 2011 at 05:19:15PM +0800, PenMount wrote: >> This patch adds PenMount support to hid-multitouch. >> A new class MT_CLS_CONFIDENCE_SERIAL_MODE is defined for PenMount, >> since it uses HID_DG_CONFIDENCE as the valid flag. >> A new quirk MT_QUIRK_SERIAL_REPORT_MODE is defined, >> since PenMount uses serial reporting mode and there is no >> HID_DG_CONTACTCOUNT usage in it's report descriptor. >> >> Signed-off-by: PenMount >> Reviewed-by: Benjamin Tissoires >> --- >>  drivers/hid/Kconfig          |    1 + >>  drivers/hid/hid-core.c       |    1 + >>  drivers/hid/hid-ids.h        |    3 +++ >>  drivers/hid/hid-multitouch.c |   14 ++++++++++++++ >>  4 files changed, 19 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig >> index 996ae3a..8058cf1 100644 >> --- a/drivers/hid/Kconfig >> +++ b/drivers/hid/Kconfig >> @@ -313,6 +313,7 @@ config HID_MULTITOUCH >>         - Cypress TrueTouch panels >>         - Hanvon dual touch panels >>         - IrTouch Infrared USB panels >> +       - PenMount dual touch panels >>         - Pixcir dual touch panels >>         - 'Sensing Win7-TwoFinger' panel by GeneralTouch >>            - eGalax dual-touch panels, including the >> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c >> index c3d6626..6e31b9f 100644 >> --- a/drivers/hid/hid-core.c >> +++ b/drivers/hid/hid-core.c >> @@ -1438,6 +1438,7 @@ static const struct hid_device_id hid_have_special_driver[] = { >>       { HID_USB_DEVICE(USB_VENDOR_ID_NTRIG, USB_DEVICE_ID_NTRIG_TOUCH_SCREEN_18) }, >>       { HID_USB_DEVICE(USB_VENDOR_ID_ORTEK, USB_DEVICE_ID_ORTEK_PKB1700) }, >>       { HID_USB_DEVICE(USB_VENDOR_ID_ORTEK, USB_DEVICE_ID_ORTEK_WKB2000) }, >> +     { HID_USB_DEVICE(USB_VENDOR_ID_PENMOUNT, USB_DEVICE_ID_PENMOUNT_PCI) }, >>       { HID_USB_DEVICE(USB_VENDOR_ID_PETALYNX, USB_DEVICE_ID_PETALYNX_MAXTER_REMOTE) }, >>       { HID_USB_DEVICE(USB_VENDOR_ID_QUANTA, USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH) }, >>       { HID_USB_DEVICE(USB_VENDOR_ID_QUANTA, USB_DEVICE_ID_PIXART_IMAGING_INC_OPTICAL_TOUCH_SCREEN) }, >> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h >> index d485894..a4d0505 100644 >> --- a/drivers/hid/hid-ids.h >> +++ b/drivers/hid/hid-ids.h >> @@ -487,6 +487,9 @@ >>  #define USB_VENDOR_ID_PETALYNX               0x18b1 >>  #define USB_DEVICE_ID_PETALYNX_MAXTER_REMOTE 0x0037 >> >> +#define USB_VENDOR_ID_PENMOUNT               0x14e1 >> +#define USB_DEVICE_ID_PENMOUNT_PCI   0x3500 >> + >>  #define USB_VENDOR_ID_PHILIPS                0x0471 >>  #define USB_DEVICE_ID_PHILIPS_IEEE802154_DONGLE 0x0617 >> >> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c >> index 0175f85..c88d96d 100644 >> --- a/drivers/hid/hid-multitouch.c >> +++ b/drivers/hid/hid-multitouch.c >> @@ -50,6 +50,7 @@ MODULE_LICENSE("GPL"); >>  #define MT_QUIRK_VALID_IS_INRANGE    (1 << 4) >>  #define MT_QUIRK_VALID_IS_CONFIDENCE (1 << 5) >>  #define MT_QUIRK_EGALAX_XYZ_FIXUP    (1 << 6) >> +#define MT_QUIRK_SERIAL_REPORT_MODE  (1 << 7) >> >>  struct mt_slot { >>       __s32 x, y, p, w, h; >> @@ -89,6 +90,7 @@ struct mt_class { >>  #define MT_CLS_EGALAX                                5 >>  #define MT_CLS_STANTUM                               6 >>  #define MT_CLS_3M                            7 >> +#define MT_CLS_CONFIDENCE_SERIAL_MODE                8 >> >>  #define MT_DEFAULT_MAXCONTACT        10 >> >> @@ -156,6 +158,9 @@ struct mt_class mt_classes[] = { >>               .sn_move = 2048, >>               .sn_width = 128, >>               .sn_height = 128 }, >> +     { .name = MT_CLS_CONFIDENCE_SERIAL_MODE, >> +             .quirks = MT_QUIRK_VALID_IS_CONFIDENCE | >> +                     MT_QUIRK_SERIAL_REPORT_MODE }, >> >>       { } >>  }; >> @@ -196,6 +201,10 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, >>       struct mt_class *cls = td->mtclass; >>       __s32 quirks = cls->quirks; >> >> +     if ((quirks & MT_QUIRK_SERIAL_REPORT_MODE) && >> +             (!td->last_field_index)) >> +             td->last_field_index = field->report->maxfield - 1; >> + > > Are the events not emitted automatically per touch, if the lines above are omitted? > >>       switch (usage->hid & HID_USAGE_PAGE) { >> >>       case HID_UP_GENDESK: >> @@ -585,6 +594,11 @@ static const struct hid_device_id mt_devices[] = { >>               HID_USB_DEVICE(USB_VENDOR_ID_IRTOUCHSYSTEMS, >>                       USB_DEVICE_ID_IRTOUCH_INFRARED_USB) }, >> >> +     /* PenMount panels */ >> +     {  .driver_data = MT_CLS_CONFIDENCE_SERIAL_MODE, >> +             HID_USB_DEVICE(USB_VENDOR_ID_PENMOUNT, >> +                     USB_DEVICE_ID_PENMOUNT_PCI) }, >> + >>       /* PixCir-based panels */ >>       { .driver_data = MT_CLS_DUAL_INRANGE_CONTACTID, >>               HID_USB_DEVICE(USB_VENDOR_ID_HANVON, >> -- >> 1.7.4.1 >> > > Benjamin: Since field->index == 0 is in use, it seems the driver could > execute event emission wrongly under some circumstances (when > last_field_index == 0). Is that related to the reason for the quirk in > this patch? In fact, I have a second patch just on top of this one. It has been tested by Ryan with PenMount devices. I was waiting for Penmount to be included first as it does not impact any other drivers. I inline the code here, but my gmail client will destroy tabs (so I attached it too).... Can you test it against 3M? Richard, can you test it (after applying the PenMount patch) too? Thanks, Benjamin From ac6b68d27b70e91846f2179d15804144dfab8bc1 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Tue, 19 Apr 2011 11:49:44 +0200 Subject: [PATCH] HID: hid-multitouch: refactor last_field_index the current implementation requires the devices to report HID_DG_CONTACTCOUNT to set the last_field_index value. However, devices reporting in serial mode (DWAV and PenMount) do not send this field. Other devices (3M) add other fields in the reports descriptor that are not multitouch related at the end, thus the need to add a special case in the default case when handling events. A first work around has been set up but with PenMount devices, we have reached the limit. The idea is to calculate the last_field_index by relying only on multitouch fields the device send. This allows us to remove the handling of non-multitouch events in hid-multitouch, and guarantee that the function mt_emit_event is always called. Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-multitouch.c | 37 +++++++++++++++---------------------- 1 files changed, 15 insertions(+), 22 deletions(-) case HID_UP_GENDESK: @@ -219,6 +213,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, /* touchscreen emulation */ set_abs(hi->input, ABS_X, field, cls->sn_move); td->last_slot_field = usage->hid; + td->last_field_index = field->index; return 1; case HID_GD_Y: if (quirks & MT_QUIRK_EGALAX_XYZ_FIXUP) @@ -230,6 +225,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, /* touchscreen emulation */ set_abs(hi->input, ABS_Y, field, cls->sn_move); td->last_slot_field = usage->hid; + td->last_field_index = field->index; return 1; } return 0; @@ -238,18 +234,22 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, switch (usage->hid) { case HID_DG_INRANGE: td->last_slot_field = usage->hid; + td->last_field_index = field->index; return 1; case HID_DG_CONFIDENCE: td->last_slot_field = usage->hid; + td->last_field_index = field->index; return 1; case HID_DG_TIPSWITCH: hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH); input_set_capability(hi->input, EV_KEY, BTN_TOUCH); td->last_slot_field = usage->hid; + td->last_field_index = field->index; return 1; case HID_DG_CONTACTID: input_mt_init_slots(hi->input, td->maxcontacts); td->last_slot_field = usage->hid; + td->last_field_index = field->index; return 1; case HID_DG_WIDTH: hid_map_usage(hi, usage, bit, max, @@ -257,6 +257,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, set_abs(hi->input, ABS_MT_TOUCH_MAJOR, field, cls->sn_width); td->last_slot_field = usage->hid; + td->last_field_index = field->index; return 1; case HID_DG_HEIGHT: hid_map_usage(hi, usage, bit, max, @@ -266,6 +267,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, input_set_abs_params(hi->input, ABS_MT_ORIENTATION, 0, 1, 0, 0); td->last_slot_field = usage->hid; + td->last_field_index = field->index; return 1; case HID_DG_TIPPRESSURE: if (quirks & MT_QUIRK_EGALAX_XYZ_FIXUP) @@ -278,13 +280,15 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, set_abs(hi->input, ABS_PRESSURE, field, cls->sn_pressure); td->last_slot_field = usage->hid; + td->last_field_index = field->index; return 1; case HID_DG_CONTACTCOUNT: - td->last_field_index = field->report->maxfield - 1; + td->last_field_index = field->index; return 1; case HID_DG_CONTACTMAX: /* we don't set td->last_slot_field as contactcount and * contact max are global to the report */ + td->last_field_index = field->index; return -1; } /* let hid-input decide for the others */ @@ -433,23 +437,12 @@ static int mt_event(struct hid_device *hid, struct hid_field *field, break; default: - if (td->last_field_index - && field->index == td->last_field_index) - /* we reach here when the last field in the - * report is not related to multitouch. - * This is not good. As a temporary solution, - * we trigger our mt event completion and - * ignore the field. - */ - break; /* fallback to the generic hidinput handling */ return 0; } if (usage->hid == td->last_slot_field) { mt_complete_slot(td); - if (!td->last_field_index) - mt_emit_event(td, field->hidinput->input); } if (field->index == td->last_field_index @@ -595,7 +588,7 @@ static const struct hid_device_id mt_devices[] = { USB_DEVICE_ID_IRTOUCH_INFRARED_USB) }, /* PenMount panels */ - { .driver_data = MT_CLS_CONFIDENCE_SERIAL_MODE, + { .driver_data = MT_CLS_CONFIDENCE, HID_USB_DEVICE(USB_VENDOR_ID_PENMOUNT, USB_DEVICE_ID_PENMOUNT_PCI) }, From ac6b68d27b70e91846f2179d15804144dfab8bc1 Mon Sep 17 00:00:00 2001 From: Benjamin Tissoires Date: Tue, 19 Apr 2011 11:49:44 +0200 Subject: [PATCH] HID: hid-multitouch: refactor last_field_index the current implementation requires the devices to report HID_DG_CONTACTCOUNT to set the last_field_index value. However, devices reporting in serial mode (DWAV and PenMount) do not send this field. Other devices (3M) add other fields in the reports descriptor that are not multitouch related at the end, thus the need to add a special case in the default case when handling events. A first work around has been set up but with PenMount devices, we have reached the limit. The idea is to calculate the last_field_index by relying only on multitouch fields the device send. This allows us to remove the handling of non-multitouch events in hid-multitouch, and guarantee that the function mt_emit_event is always called. Signed-off-by: Benjamin Tissoires --- drivers/hid/hid-multitouch.c | 37 +++++++++++++++---------------------- 1 files changed, 15 insertions(+), 22 deletions(-) diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 5d4ba79..51b5d27 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -50,7 +50,6 @@ MODULE_LICENSE("GPL"); #define MT_QUIRK_VALID_IS_INRANGE (1 << 4) #define MT_QUIRK_VALID_IS_CONFIDENCE (1 << 5) #define MT_QUIRK_EGALAX_XYZ_FIXUP (1 << 6) -#define MT_QUIRK_SERIAL_REPORT_MODE (1 << 7) struct mt_slot { __s32 x, y, p, w, h; @@ -90,7 +89,7 @@ struct mt_class { #define MT_CLS_EGALAX 5 #define MT_CLS_STANTUM 6 #define MT_CLS_3M 7 -#define MT_CLS_CONFIDENCE_SERIAL_MODE 8 +#define MT_CLS_CONFIDENCE 8 #define MT_DEFAULT_MAXCONTACT 10 @@ -158,9 +157,8 @@ struct mt_class mt_classes[] = { .sn_move = 2048, .sn_width = 128, .sn_height = 128 }, - { .name = MT_CLS_CONFIDENCE_SERIAL_MODE, - .quirks = MT_QUIRK_VALID_IS_CONFIDENCE | - MT_QUIRK_SERIAL_REPORT_MODE }, + { .name = MT_CLS_CONFIDENCE, + .quirks = MT_QUIRK_VALID_IS_CONFIDENCE }, { } }; @@ -201,10 +199,6 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, struct mt_class *cls = td->mtclass; __s32 quirks = cls->quirks; - if ((quirks & MT_QUIRK_SERIAL_REPORT_MODE) && - (!td->last_field_index)) - td->last_field_index = field->report->maxfield - 1; - switch (usage->hid & HID_USAGE_PAGE) { case HID_UP_GENDESK: @@ -219,6 +213,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, /* touchscreen emulation */ set_abs(hi->input, ABS_X, field, cls->sn_move); td->last_slot_field = usage->hid; + td->last_field_index = field->index; return 1; case HID_GD_Y: if (quirks & MT_QUIRK_EGALAX_XYZ_FIXUP) @@ -230,6 +225,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, /* touchscreen emulation */ set_abs(hi->input, ABS_Y, field, cls->sn_move); td->last_slot_field = usage->hid; + td->last_field_index = field->index; return 1; } return 0; @@ -238,18 +234,22 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, switch (usage->hid) { case HID_DG_INRANGE: td->last_slot_field = usage->hid; + td->last_field_index = field->index; return 1; case HID_DG_CONFIDENCE: td->last_slot_field = usage->hid; + td->last_field_index = field->index; return 1; case HID_DG_TIPSWITCH: hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH); input_set_capability(hi->input, EV_KEY, BTN_TOUCH); td->last_slot_field = usage->hid; + td->last_field_index = field->index; return 1; case HID_DG_CONTACTID: input_mt_init_slots(hi->input, td->maxcontacts); td->last_slot_field = usage->hid; + td->last_field_index = field->index; return 1; case HID_DG_WIDTH: hid_map_usage(hi, usage, bit, max, @@ -257,6 +257,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, set_abs(hi->input, ABS_MT_TOUCH_MAJOR, field, cls->sn_width); td->last_slot_field = usage->hid; + td->last_field_index = field->index; return 1; case HID_DG_HEIGHT: hid_map_usage(hi, usage, bit, max, @@ -266,6 +267,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, input_set_abs_params(hi->input, ABS_MT_ORIENTATION, 0, 1, 0, 0); td->last_slot_field = usage->hid; + td->last_field_index = field->index; return 1; case HID_DG_TIPPRESSURE: if (quirks & MT_QUIRK_EGALAX_XYZ_FIXUP) @@ -278,13 +280,15 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, set_abs(hi->input, ABS_PRESSURE, field, cls->sn_pressure); td->last_slot_field = usage->hid; + td->last_field_index = field->index; return 1; case HID_DG_CONTACTCOUNT: - td->last_field_index = field->report->maxfield - 1; + td->last_field_index = field->index; return 1; case HID_DG_CONTACTMAX: /* we don't set td->last_slot_field as contactcount and * contact max are global to the report */ + td->last_field_index = field->index; return -1; } /* let hid-input decide for the others */ @@ -433,23 +437,12 @@ static int mt_event(struct hid_device *hid, struct hid_field *field, break; default: - if (td->last_field_index - && field->index == td->last_field_index) - /* we reach here when the last field in the - * report is not related to multitouch. - * This is not good. As a temporary solution, - * we trigger our mt event completion and - * ignore the field. - */ - break; /* fallback to the generic hidinput handling */ return 0; } if (usage->hid == td->last_slot_field) { mt_complete_slot(td); - if (!td->last_field_index) - mt_emit_event(td, field->hidinput->input); } if (field->index == td->last_field_index @@ -595,7 +588,7 @@ static const struct hid_device_id mt_devices[] = { USB_DEVICE_ID_IRTOUCH_INFRARED_USB) }, /* PenMount panels */ - { .driver_data = MT_CLS_CONFIDENCE_SERIAL_MODE, + { .driver_data = MT_CLS_CONFIDENCE, HID_USB_DEVICE(USB_VENDOR_ID_PENMOUNT, USB_DEVICE_ID_PENMOUNT_PCI) }, -- 1.7.4.2