* [PATCH 4/4] input: wacom - add 0xE5 (MT device) support @ 2011-12-17 0:38 Ping Cheng 2011-12-19 2:07 ` Chris Bagwell 0 siblings, 1 reply; 6+ messages in thread From: Ping Cheng @ 2011-12-17 0:38 UTC (permalink / raw) To: linux-input; +Cc: Ping Cheng, Ping Cheng Signed-off-by: Ping Cheng <pingc@wacom.com> --- drivers/input/tablet/wacom_sys.c | 26 ++++++------ drivers/input/tablet/wacom_wac.c | 82 +++++++++++++++++++++++++++++++++++++- drivers/input/tablet/wacom_wac.h | 8 ++++ 3 files changed, 101 insertions(+), 15 deletions(-) diff --git a/drivers/input/tablet/wacom_sys.c b/drivers/input/tablet/wacom_sys.c index b9736fb..eee06ef 100644 --- a/drivers/input/tablet/wacom_sys.c +++ b/drivers/input/tablet/wacom_sys.c @@ -297,6 +297,10 @@ static int wacom_parse_hid(struct usb_interface *intf, features->pktlen = WACOM_PKGLEN_TPC2FG; features->touch_max = 2; } + + if (features->type == MTSCREEN) + features->pktlen = WACOM_PKGLEN_MTOUCH; + if (features->type == BAMBOO_PT) { /* need to reset back */ features->pktlen = WACOM_PKGLEN_BBTOUCH; @@ -331,18 +335,15 @@ static int wacom_parse_hid(struct usb_interface *intf, case HID_USAGE_Y: if (usage == WCM_DESKTOP) { if (finger) { - features->device_type = BTN_TOOL_FINGER; - if (features->type == TABLETPC2FG) { - /* need to reset back */ - features->pktlen = WACOM_PKGLEN_TPC2FG; + int type = features->type; + + if (type == TABLETPC2FG || type == MTSCREEN) { features->y_max = get_unaligned_le16(&report[i + 3]); features->y_phy = get_unaligned_le16(&report[i + 6]); i += 7; - } else if (features->type == BAMBOO_PT) { - /* need to reset back */ - features->pktlen = WACOM_PKGLEN_BBTOUCH; + } else if (type == BAMBOO_PT) { features->y_phy = get_unaligned_le16(&report[i + 3]); features->y_max = @@ -356,10 +357,6 @@ static int wacom_parse_hid(struct usb_interface *intf, i += 4; } } else if (pen) { - /* penabled only accepts exact bytes of data */ - if (features->type == TABLETPC2FG) - features->pktlen = WACOM_PKGLEN_GRAPHIRE; - features->device_type = BTN_TOOL_PEN; features->y_max = get_unaligned_le16(&report[i + 3]); i += 4; @@ -432,7 +429,8 @@ static int wacom_query_tablet_data(struct usb_interface *intf, struct wacom_feat /* ask to report tablet data if it is MT Tablet PC or * not a Tablet PC */ - if (features->type == TABLETPC2FG) { + if (((features->type == TABLETPC2FG) || (features->type == MTSCREEN)) + && features->device_type != BTN_TOOL_PEN) { do { rep_data[0] = 3; rep_data[1] = 4; @@ -479,9 +477,9 @@ static int wacom_retrieve_hid_descriptor(struct usb_interface *intf, features->pressure_fuzz = 0; features->distance_fuzz = 0; - /* only Tablet PCs and Bamboo P&T need to retrieve the info */ + /* only devices that support touch need to retrieve the info */ if ((features->type != TABLETPC) && (features->type != TABLETPC2FG) && - (features->type != BAMBOO_PT)) + (features->type != BAMBOO_PT) && (features->type != MTSCREEN)) goto out; if (usb_get_extra_descriptor(interface, HID_DEVICET_HID, &hid_desc)) { diff --git a/drivers/input/tablet/wacom_wac.c b/drivers/input/tablet/wacom_wac.c index 8dc185d..1a887a1 100644 --- a/drivers/input/tablet/wacom_wac.c +++ b/drivers/input/tablet/wacom_wac.c @@ -729,6 +729,73 @@ static int wacom_intuos_irq(struct wacom_wac *wacom) return 1; } +static int wacom_mt_touch(struct wacom_wac *wacom) +{ + struct wacom_features *features = &wacom->features; + struct input_dev *input = wacom->input; + char *data = wacom->data; + struct input_mt_slot *mt; + int i, id = -1, j = 0, k = 4; + int x = 0, y = 0; + int current_num_contacts = data[2]; + int contacts_to_send = 0; + bool touch = false; + + /* reset the counter by the first packet */ + if (current_num_contacts) { + features->num_contacts = current_num_contacts; + features->num_contacts_left = current_num_contacts; + } + + /* There are at most 5 contacts per packet */ + contacts_to_send = min(5, (int)features->num_contacts_left); + + for (i = 0; i < contacts_to_send; i++) { + id = le16_to_cpup((__le16 *)&data[k]); + + /* is there an existing slot for this contact? */ + for (j = 0; j < features->touch_max; j++) { + mt = &input->mt[j]; + if (input_mt_get_value(mt, ABS_MT_TRACKING_ID) == id ) + break; + } + + /* no. then find an unused slot to fill */ + if (j >= features->touch_max) { + for (j = 0; j < features->touch_max; j++) { + mt = &input->mt[j]; + if (input_mt_get_value(mt, ABS_MT_TRACKING_ID) == -1 ) + break; + } + } + + touch = data[k - 1] & 0x1; + input_mt_slot(input, j); + if (touch) { + x = le16_to_cpup((__le16 *)&data[k + 6]); + y = le16_to_cpup((__le16 *)&data[k + 8]); + + input_report_abs(input, ABS_MT_POSITION_X, x); + input_report_abs(input, ABS_MT_POSITION_Y, y); + } else + id = -1; + + /* keep the id from firmware since we need + * it to process future events + */ + input_report_abs(input, ABS_MT_TRACKING_ID, id); + + k += WACOM_BYTES_PER_MT_PACKET; + } + + input_mt_report_pointer_emulation(input, true); + + features->num_contacts_left -= contacts_to_send; + if (features->num_contacts_left < 0) + features->num_contacts_left = 0; + return 1; +} + static int wacom_tpc_mt_touch(struct wacom_wac *wacom) { struct input_dev *input = wacom->input; @@ -767,6 +834,10 @@ static int wacom_tpc_single_touch(struct wacom_wac *wacom, size_t len) bool prox; int x = 0, y = 0; + if ((wacom->features.touch_max > 1) || + (len > WACOM_PKGLEN_TPC2FG)) + return 0; + if (!wacom->shared->stylus_in_proximity) { if (len == WACOM_PKGLEN_TPC1FG) { prox = data[0] & 0x01; @@ -847,6 +918,9 @@ static int wacom_tpc_irq(struct wacom_wac *wacom, size_t len) case WACOM_REPORT_TPCST: return wacom_tpc_single_touch(wacom, len); + case WACOM_REPORT_TPCMT: + return wacom_mt_touch(wacom); + case WACOM_REPORT_PENABLED: return wacom_tpc_pen(wacom); } @@ -1088,6 +1162,7 @@ void wacom_wac_irq(struct wacom_wac *wacom_wac, size_t len) case TABLETPC: case TABLETPC2FG: + case MTSCREEN: sync = wacom_tpc_irq(wacom_wac, len); break; @@ -1156,7 +1231,7 @@ void wacom_setup_device_quirks(struct wacom_features *features) /* these device have multiple inputs */ if (features->type == TABLETPC || features->type == TABLETPC2FG || - features->type == BAMBOO_PT) + features->type == BAMBOO_PT || features->type == MTSCREEN) features->quirks |= WACOM_QUIRK_MULTI_INPUT; /* quirk for bamboo touch with 2 low res touches */ @@ -1330,6 +1405,7 @@ void wacom_setup_input_capabilities(struct input_dev *input_dev, break; case TABLETPC2FG: + case MTSCREEN: if (features->device_type == BTN_TOOL_FINGER) { input_mt_init_slots(input_dev, features->touch_max); @@ -1629,6 +1705,9 @@ static const struct wacom_features wacom_features_0xE2 = static const struct wacom_features wacom_features_0xE3 = { "Wacom ISDv4 E3", WACOM_PKGLEN_TPC2FG, 26202, 16325, 255, 0, TABLETPC2FG, WACOM_INTUOS_RES, WACOM_INTUOS_RES }; +static const struct wacom_features wacom_features_0xE5 = + { "Wacom ISDv4 E5", WACOM_PKGLEN_MTOUCH, 26202, 16325, 255, + 0, MTSCREEN, WACOM_INTUOS_RES, WACOM_INTUOS_RES }; static const struct wacom_features wacom_features_0xE6 = { "Wacom ISDv4 E6", WACOM_PKGLEN_TPC2FG, 27760, 15694, 255, 0, TABLETPC2FG, WACOM_INTUOS_RES, WACOM_INTUOS_RES }; @@ -1784,6 +1863,7 @@ const struct usb_device_id wacom_ids[] = { { USB_DEVICE_WACOM(0x9F) }, { USB_DEVICE_WACOM(0xE2) }, { USB_DEVICE_WACOM(0xE3) }, + { USB_DEVICE_WACOM(0xE5) }, { USB_DEVICE_WACOM(0xE6) }, { USB_DEVICE_WACOM(0x47) }, { USB_DEVICE_WACOM(0xF4) }, diff --git a/drivers/input/tablet/wacom_wac.h b/drivers/input/tablet/wacom_wac.h index 1c9dc3e..265b09f 100644 --- a/drivers/input/tablet/wacom_wac.h +++ b/drivers/input/tablet/wacom_wac.h @@ -24,6 +24,10 @@ #define WACOM_PKGLEN_BBTOUCH 20 #define WACOM_PKGLEN_BBTOUCH3 64 #define WACOM_PKGLEN_BBPEN 10 +#define WACOM_PKGLEN_MTOUCH 62 + +/* wacom data size per MT contact */ +#define WACOM_BYTES_PER_MT_PACKET 11 /* device IDs */ #define STYLUS_DEVICE_ID 0x02 @@ -39,6 +43,7 @@ #define WACOM_REPORT_INTUOSPAD 12 #define WACOM_REPORT_TPC1FG 6 #define WACOM_REPORT_TPC2FG 13 +#define WACOM_REPORT_TPCMT 13 #define WACOM_REPORT_TPCHID 15 #define WACOM_REPORT_TPCST 16 @@ -68,6 +73,7 @@ enum { WACOM_MO, TABLETPC, TABLETPC2FG, + MTSCREEN, MAX_TYPE }; @@ -92,6 +98,8 @@ struct wacom_features { int distance_fuzz; unsigned quirks; unsigned touch_max; + unsigned num_contacts; + unsigned num_contacts_left; }; struct wacom_shared { -- 1.7.6.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 4/4] input: wacom - add 0xE5 (MT device) support 2011-12-17 0:38 [PATCH 4/4] input: wacom - add 0xE5 (MT device) support Ping Cheng @ 2011-12-19 2:07 ` Chris Bagwell [not found] ` <CAF8JNhJS-b0hktyXiJ-T+8HheRXwW8yD0FFsgmMK=CnvSiFJOA@mail.gmail.com> 0 siblings, 1 reply; 6+ messages in thread From: Chris Bagwell @ 2011-12-19 2:07 UTC (permalink / raw) To: Ping Cheng; +Cc: linux-input, Ping Cheng On Fri, Dec 16, 2011 at 6:38 PM, Ping Cheng <pinglinux@gmail.com> wrote: > Signed-off-by: Ping Cheng <pingc@wacom.com> > --- > drivers/input/tablet/wacom_sys.c | 26 ++++++------ > drivers/input/tablet/wacom_wac.c | 82 +++++++++++++++++++++++++++++++++++++- > drivers/input/tablet/wacom_wac.h | 8 ++++ > 3 files changed, 101 insertions(+), 15 deletions(-) > > diff --git a/drivers/input/tablet/wacom_sys.c b/drivers/input/tablet/wacom_sys.c > index b9736fb..eee06ef 100644 > --- a/drivers/input/tablet/wacom_sys.c > +++ b/drivers/input/tablet/wacom_sys.c > @@ -297,6 +297,10 @@ static int wacom_parse_hid(struct usb_interface *intf, > features->pktlen = WACOM_PKGLEN_TPC2FG; > features->touch_max = 2; > } > + > + if (features->type == MTSCREEN) > + features->pktlen = WACOM_PKGLEN_MTOUCH; > + > if (features->type == BAMBOO_PT) { > /* need to reset back */ > features->pktlen = WACOM_PKGLEN_BBTOUCH; > @@ -331,18 +335,15 @@ static int wacom_parse_hid(struct usb_interface *intf, > case HID_USAGE_Y: > if (usage == WCM_DESKTOP) { > if (finger) { > - features->device_type = BTN_TOOL_FINGER; > - if (features->type == TABLETPC2FG) { > - /* need to reset back */ > - features->pktlen = WACOM_PKGLEN_TPC2FG; > + int type = features->type; > + > + if (type == TABLETPC2FG || type == MTSCREEN) { > features->y_max = > get_unaligned_le16(&report[i + 3]); > features->y_phy = > get_unaligned_le16(&report[i + 6]); > i += 7; > - } else if (features->type == BAMBOO_PT) { > - /* need to reset back */ > - features->pktlen = WACOM_PKGLEN_BBTOUCH; > + } else if (type == BAMBOO_PT) { > features->y_phy = > get_unaligned_le16(&report[i + 3]); > features->y_max = > @@ -356,10 +357,6 @@ static int wacom_parse_hid(struct usb_interface *intf, > i += 4; > } > } else if (pen) { > - /* penabled only accepts exact bytes of data */ > - if (features->type == TABLETPC2FG) > - features->pktlen = WACOM_PKGLEN_GRAPHIRE; > - features->device_type = BTN_TOOL_PEN; All makes sense. Getting rid of duplicate code since X/Y will always be present. Maybe worth a 1 line comment now that its cleaned up so someone doesn't let it creep back in. > features->y_max = > get_unaligned_le16(&report[i + 3]); > i += 4; > @@ -432,7 +429,8 @@ static int wacom_query_tablet_data(struct usb_interface *intf, struct wacom_feat > > /* ask to report tablet data if it is MT Tablet PC or > * not a Tablet PC */ > - if (features->type == TABLETPC2FG) { > + if (((features->type == TABLETPC2FG) || (features->type == MTSCREEN)) > + && features->device_type != BTN_TOOL_PEN) { Glad to see this device_type check. I thought it was probably needed. I'll have to trust you on this one but its different than Bamboo's. The set on Bamboo is on Pen and causes it to start sending pen packets. IIR, the touch packets are always sending. So this is opposite and enables something related to touch? Assuming you say yes, do you mind changing that to an affirmative check for TOUCH so it reads: if (TOUCH) /* do TOUCH set's */ if (PEN) /* do PEN set's */ > do { > rep_data[0] = 3; > rep_data[1] = 4; > @@ -479,9 +477,9 @@ static int wacom_retrieve_hid_descriptor(struct usb_interface *intf, > features->pressure_fuzz = 0; > features->distance_fuzz = 0; > > - /* only Tablet PCs and Bamboo P&T need to retrieve the info */ > + /* only devices that support touch need to retrieve the info */ > if ((features->type != TABLETPC) && (features->type != TABLETPC2FG) && > - (features->type != BAMBOO_PT)) > + (features->type != BAMBOO_PT) && (features->type != MTSCREEN)) > goto out; > > if (usb_get_extra_descriptor(interface, HID_DEVICET_HID, &hid_desc)) { > diff --git a/drivers/input/tablet/wacom_wac.c b/drivers/input/tablet/wacom_wac.c > index 8dc185d..1a887a1 100644 > --- a/drivers/input/tablet/wacom_wac.c > +++ b/drivers/input/tablet/wacom_wac.c > @@ -729,6 +729,73 @@ static int wacom_intuos_irq(struct wacom_wac *wacom) > return 1; > } > > +static int wacom_mt_touch(struct wacom_wac *wacom) > +{ > + struct wacom_features *features = &wacom->features; > + struct input_dev *input = wacom->input; > + char *data = wacom->data; > + struct input_mt_slot *mt; > + int i, id = -1, j = 0, k = 4; > + int x = 0, y = 0; > + int current_num_contacts = data[2]; > + int contacts_to_send = 0; > + bool touch = false; > + > + /* reset the counter by the first packet */ > + if (current_num_contacts) { > + features->num_contacts = current_num_contacts; > + features->num_contacts_left = current_num_contacts; > + } It seems that the saved contact values are only processed in case were data[2] indicates no contact counts in packet. I would think that packets with no contacts could be ignored (return) unless there is some data that must be implied. Why is it not ignored? > + > + /* There are at most 5 contacts per packet */ > + contacts_to_send = min(5, (int)features->num_contacts_left); Its not obvious to me if data[2] contains touches in packet or total touches on screen. > + > + for (i = 0; i < contacts_to_send; i++) { > + id = le16_to_cpup((__le16 *)&data[k]); > + > + /* is there an existing slot for this contact? */ > + for (j = 0; j < features->touch_max; j++) { > + mt = &input->mt[j]; > + if (input_mt_get_value(mt, ABS_MT_TRACKING_ID) == id ) > + break; > + } > + > + /* no. then find an unused slot to fill */ > + if (j >= features->touch_max) { > + for (j = 0; j < features->touch_max; j++) { > + mt = &input->mt[j]; > + if (input_mt_get_value(mt, ABS_MT_TRACKING_ID) == -1 ) > + break; > + } > + } This may need a little more thought since hid-multitouch doesn't query old value. Is there a case were a touch is removed and new touch added in same packet? Probably that would never send the -1 to user land. Thats all. Chris > + > + touch = data[k - 1] & 0x1; > + input_mt_slot(input, j); > + if (touch) { > + x = le16_to_cpup((__le16 *)&data[k + 6]); > + y = le16_to_cpup((__le16 *)&data[k + 8]); > + > + input_report_abs(input, ABS_MT_POSITION_X, x); > + input_report_abs(input, ABS_MT_POSITION_Y, y); > + } else > + id = -1; > + > + /* keep the id from firmware since we need > + * it to process future events > + */ > + input_report_abs(input, ABS_MT_TRACKING_ID, id); > + > + k += WACOM_BYTES_PER_MT_PACKET; > + } > + > + input_mt_report_pointer_emulation(input, true); > + > + features->num_contacts_left -= contacts_to_send; > + if (features->num_contacts_left < 0) > + features->num_contacts_left = 0; Same comment as above. Could use a comment on why storing this would be needed. > + return 1; > +} > + > static int wacom_tpc_mt_touch(struct wacom_wac *wacom) > { > struct input_dev *input = wacom->input; > @@ -767,6 +834,10 @@ static int wacom_tpc_single_touch(struct wacom_wac *wacom, size_t len) > bool prox; > int x = 0, y = 0; > > + if ((wacom->features.touch_max > 1) || > + (len > WACOM_PKGLEN_TPC2FG)) > + return 0; > + > if (!wacom->shared->stylus_in_proximity) { > if (len == WACOM_PKGLEN_TPC1FG) { > prox = data[0] & 0x01; > @@ -847,6 +918,9 @@ static int wacom_tpc_irq(struct wacom_wac *wacom, size_t len) > case WACOM_REPORT_TPCST: > return wacom_tpc_single_touch(wacom, len); > > + case WACOM_REPORT_TPCMT: > + return wacom_mt_touch(wacom); > + > case WACOM_REPORT_PENABLED: > return wacom_tpc_pen(wacom); > } > @@ -1088,6 +1162,7 @@ void wacom_wac_irq(struct wacom_wac *wacom_wac, size_t len) > > case TABLETPC: > case TABLETPC2FG: > + case MTSCREEN: > sync = wacom_tpc_irq(wacom_wac, len); > break; > > @@ -1156,7 +1231,7 @@ void wacom_setup_device_quirks(struct wacom_features *features) > > /* these device have multiple inputs */ > if (features->type == TABLETPC || features->type == TABLETPC2FG || > - features->type == BAMBOO_PT) > + features->type == BAMBOO_PT || features->type == MTSCREEN) > features->quirks |= WACOM_QUIRK_MULTI_INPUT; > > /* quirk for bamboo touch with 2 low res touches */ > @@ -1330,6 +1405,7 @@ void wacom_setup_input_capabilities(struct input_dev *input_dev, > break; > > case TABLETPC2FG: > + case MTSCREEN: > if (features->device_type == BTN_TOOL_FINGER) { > > input_mt_init_slots(input_dev, features->touch_max); > @@ -1629,6 +1705,9 @@ static const struct wacom_features wacom_features_0xE2 = > static const struct wacom_features wacom_features_0xE3 = > { "Wacom ISDv4 E3", WACOM_PKGLEN_TPC2FG, 26202, 16325, 255, > 0, TABLETPC2FG, WACOM_INTUOS_RES, WACOM_INTUOS_RES }; > +static const struct wacom_features wacom_features_0xE5 = > + { "Wacom ISDv4 E5", WACOM_PKGLEN_MTOUCH, 26202, 16325, 255, > + 0, MTSCREEN, WACOM_INTUOS_RES, WACOM_INTUOS_RES }; > static const struct wacom_features wacom_features_0xE6 = > { "Wacom ISDv4 E6", WACOM_PKGLEN_TPC2FG, 27760, 15694, 255, > 0, TABLETPC2FG, WACOM_INTUOS_RES, WACOM_INTUOS_RES }; > @@ -1784,6 +1863,7 @@ const struct usb_device_id wacom_ids[] = { > { USB_DEVICE_WACOM(0x9F) }, > { USB_DEVICE_WACOM(0xE2) }, > { USB_DEVICE_WACOM(0xE3) }, > + { USB_DEVICE_WACOM(0xE5) }, > { USB_DEVICE_WACOM(0xE6) }, > { USB_DEVICE_WACOM(0x47) }, > { USB_DEVICE_WACOM(0xF4) }, > diff --git a/drivers/input/tablet/wacom_wac.h b/drivers/input/tablet/wacom_wac.h > index 1c9dc3e..265b09f 100644 > --- a/drivers/input/tablet/wacom_wac.h > +++ b/drivers/input/tablet/wacom_wac.h > @@ -24,6 +24,10 @@ > #define WACOM_PKGLEN_BBTOUCH 20 > #define WACOM_PKGLEN_BBTOUCH3 64 > #define WACOM_PKGLEN_BBPEN 10 > +#define WACOM_PKGLEN_MTOUCH 62 > + > +/* wacom data size per MT contact */ > +#define WACOM_BYTES_PER_MT_PACKET 11 > > /* device IDs */ > #define STYLUS_DEVICE_ID 0x02 > @@ -39,6 +43,7 @@ > #define WACOM_REPORT_INTUOSPAD 12 > #define WACOM_REPORT_TPC1FG 6 > #define WACOM_REPORT_TPC2FG 13 > +#define WACOM_REPORT_TPCMT 13 > #define WACOM_REPORT_TPCHID 15 > #define WACOM_REPORT_TPCST 16 > > @@ -68,6 +73,7 @@ enum { > WACOM_MO, > TABLETPC, > TABLETPC2FG, > + MTSCREEN, > MAX_TYPE > }; > > @@ -92,6 +98,8 @@ struct wacom_features { > int distance_fuzz; > unsigned quirks; > unsigned touch_max; > + unsigned num_contacts; > + unsigned num_contacts_left; > }; > > struct wacom_shared { > -- > 1.7.6.4 > > -- > 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 -- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <CAF8JNhJS-b0hktyXiJ-T+8HheRXwW8yD0FFsgmMK=CnvSiFJOA@mail.gmail.com>]
* Re: [PATCH 4/4] input: wacom - add 0xE5 (MT device) support [not found] ` <CAF8JNhJS-b0hktyXiJ-T+8HheRXwW8yD0FFsgmMK=CnvSiFJOA@mail.gmail.com> @ 2011-12-22 4:14 ` Chris Bagwell 2011-12-28 0:48 ` Ping Cheng 0 siblings, 1 reply; 6+ messages in thread From: Chris Bagwell @ 2011-12-22 4:14 UTC (permalink / raw) To: Ping Cheng; +Cc: linux-input On Wed, Dec 21, 2011 at 12:43 PM, Ping Cheng <pinglinux@gmail.com> wrote: > On Sun, Dec 18, 2011 at 6:07 PM, Chris Bagwell <chris@cnpbagwell.com> wrote: >> >> On Fri, Dec 16, 2011 at 6:38 PM, Ping Cheng <pinglinux@gmail.com> wrote: >> > Signed-off-by: Ping Cheng <pingc@wacom.com> >> > --- >> > drivers/input/tablet/wacom_sys.c | 26 ++++++------ >> > drivers/input/tablet/wacom_wac.c | 82 >> > +++++++++++++++++++++++++++++++++++++- >> > drivers/input/tablet/wacom_wac.h | 8 ++++ >> > 3 files changed, 101 insertions(+), 15 deletions(-) >> > >> > diff --git a/drivers/input/tablet/wacom_sys.c >> > b/drivers/input/tablet/wacom_sys.c >> > index b9736fb..eee06ef 100644 >> > --- a/drivers/input/tablet/wacom_sys.c >> > +++ b/drivers/input/tablet/wacom_sys.c >> > @@ -297,6 +297,10 @@ static int wacom_parse_hid(struct usb_interface >> > *intf, >> > features->pktlen >> > = WACOM_PKGLEN_TPC2FG; >> > >> > features->touch_max = 2; >> > } >> > + >> > + if (features->type == >> > MTSCREEN) >> > + features->pktlen >> > = WACOM_PKGLEN_MTOUCH; >> > + >> > if (features->type == >> > BAMBOO_PT) { >> > /* need to reset >> > back */ >> > features->pktlen >> > = WACOM_PKGLEN_BBTOUCH; >> > @@ -331,18 +335,15 @@ static int wacom_parse_hid(struct usb_interface >> > *intf, >> > case HID_USAGE_Y: >> > if (usage == WCM_DESKTOP) { >> > if (finger) { >> > - features->device_type = >> > BTN_TOOL_FINGER; >> > - if (features->type == >> > TABLETPC2FG) { >> > - /* need to reset >> > back */ >> > - features->pktlen >> > = WACOM_PKGLEN_TPC2FG; >> > + int type = >> > features->type; >> > + >> > + if (type == TABLETPC2FG >> > || type == MTSCREEN) { >> > features->y_max = >> > >> > get_unaligned_le16(&report[i + 3]); >> > features->y_phy = >> > >> > get_unaligned_le16(&report[i + 6]); >> > i += 7; >> > - } else if >> > (features->type == BAMBOO_PT) { >> > - /* need to reset >> > back */ >> > - features->pktlen >> > = WACOM_PKGLEN_BBTOUCH; >> > + } else if (type == >> > BAMBOO_PT) { >> > features->y_phy = >> > >> > get_unaligned_le16(&report[i + 3]); >> > features->y_max = >> > @@ -356,10 +357,6 @@ static int wacom_parse_hid(struct usb_interface >> > *intf, >> > i += 4; >> > } >> > } else if (pen) { >> > - /* penabled only accepts >> > exact bytes of data */ >> > - if (features->type == >> > TABLETPC2FG) >> > - features->pktlen >> > = WACOM_PKGLEN_GRAPHIRE; >> > - features->device_type = >> > BTN_TOOL_PEN; >> >> All makes sense. Getting rid of duplicate code since X/Y will always >> be present. Maybe worth a 1 line comment now that its cleaned up so >> someone doesn't let it creep back in. > > > I'll add some comments in v2. > >> >> > features->y_max = >> > >> > get_unaligned_le16(&report[i + 3]); >> > i += 4; >> > @@ -432,7 +429,8 @@ static int wacom_query_tablet_data(struct >> > usb_interface *intf, struct wacom_feat >> > >> > /* ask to report tablet data if it is MT Tablet PC or >> > * not a Tablet PC */ >> > - if (features->type == TABLETPC2FG) { >> > + if (((features->type == TABLETPC2FG) || (features->type == >> > MTSCREEN)) >> > + && features->device_type != BTN_TOOL_PEN) { >> >> Glad to see this device_type check. I thought it was probably needed. >> >> I'll have to trust you on this one but its different than Bamboo's. >> The set on Bamboo is on Pen and causes it to start sending pen >> packets. IIR, the touch packets are always sending. So this is >> opposite and enables something related to touch? > > > There are two types' of devices: tabletPC or non-tabletPC. For all tabletPC > devices that support more than one finger/contact, a set call is needed. For > all non-tabletPC devices, a set call is needed. Bamboo is a non-tabletPC > device. > >> >> Assuming you say yes, do you mind changing that to an affirmative >> check for TOUCH so it reads: > > > Well, it does not depend on whether it is touch or pen. It depends on the > type of the device as explained above. Do you still want me to use your > below logic? Let me ask this way. On Bamboo there are 2 interfaces, 1 pen and 1 touch. I need to set feature report 2 to a specific value on the pen interface to get useful packets. If I set feature report 2 to something on touch interface, I get an error response back from device. This aligns with HID descriptor because only the pen interface declares that feature report #2. I'm sure its similar for Tablet PC's. Its also writing to feature report #2 but unique data values. What ever the values are, it probably also doesn't make sense to write to both pen interface and touch interface on Tablet PC. If the feature report is only on one of two interfaces I do suggest changing the if() to something like below to make it more obvious ("if (TOUCH)" is more obvious that your looking for specific interface then "if (!PEN)"). If its on both and only 1 needs to be written to then that would probably be a good comment as well. > >> >> >> if (TOUCH) >> /* do TOUCH set's */ >> if (PEN) >> /* do PEN set's */ >> >> >> > do { >> > rep_data[0] = 3; >> > rep_data[1] = 4; >> > @@ -479,9 +477,9 @@ static int wacom_retrieve_hid_descriptor(struct >> > usb_interface *intf, >> > features->pressure_fuzz = 0; >> > features->distance_fuzz = 0; >> > >> > - /* only Tablet PCs and Bamboo P&T need to retrieve the info */ >> > + /* only devices that support touch need to retrieve the info */ >> > if ((features->type != TABLETPC) && (features->type != >> > TABLETPC2FG) && >> > - (features->type != BAMBOO_PT)) >> > + (features->type != BAMBOO_PT) && (features->type != >> > MTSCREEN)) >> > goto out; >> > >> > if (usb_get_extra_descriptor(interface, HID_DEVICET_HID, >> > &hid_desc)) { >> > diff --git a/drivers/input/tablet/wacom_wac.c >> > b/drivers/input/tablet/wacom_wac.c >> > index 8dc185d..1a887a1 100644 >> > --- a/drivers/input/tablet/wacom_wac.c >> > +++ b/drivers/input/tablet/wacom_wac.c >> > @@ -729,6 +729,73 @@ static int wacom_intuos_irq(struct wacom_wac >> > *wacom) >> > return 1; >> > } >> > >> > +static int wacom_mt_touch(struct wacom_wac *wacom) >> > +{ >> > + struct wacom_features *features = &wacom->features; >> > + struct input_dev *input = wacom->input; >> > + char *data = wacom->data; >> > + struct input_mt_slot *mt; >> > + int i, id = -1, j = 0, k = 4; >> > + int x = 0, y = 0; >> > + int current_num_contacts = data[2]; >> > + int contacts_to_send = 0; >> > + bool touch = false; >> > + >> > + /* reset the counter by the first packet */ >> > + if (current_num_contacts) { >> > + features->num_contacts = current_num_contacts; >> > + features->num_contacts_left = current_num_contacts; >> > + } >> >> It seems that the saved contact values are only processed in case were >> data[2] indicates no contact counts in packet. I would think that >> packets with no contacts could be ignored (return) unless there is >> some data that must be implied. Why is it not ignored? > > > current_num_contacts is only reported, hence updated, by the first set of > data. Later touch data will be processed as long as number of contacts does > not exceed current_num_contacts. > >> >> >> > + >> > + /* There are at most 5 contacts per packet */ >> > + contacts_to_send = min(5, (int)features->num_contacts_left); >> >> Its not obvious to me if data[2] contains touches in packet or total >> touches on screen. > > > Total touches on the screen. OK. I understand now. It would be nice to add that to above comment (the fact that num_contacts_left is total on screen). > >> >> > + >> > + for (i = 0; i < contacts_to_send; i++) { >> > + id = le16_to_cpup((__le16 *)&data[k]); >> > + >> > + /* is there an existing slot for this contact? */ >> > + for (j = 0; j < features->touch_max; j++) { >> > + mt = &input->mt[j]; >> > + if (input_mt_get_value(mt, ABS_MT_TRACKING_ID) >> > == id ) >> > + break; >> > + } >> > + >> > + /* no. then find an unused slot to fill */ >> > + if (j >= features->touch_max) { >> > + for (j = 0; j < features->touch_max; j++) { >> > + mt = &input->mt[j]; >> > + if (input_mt_get_value(mt, >> > ABS_MT_TRACKING_ID) == -1 ) >> > + break; >> > + } >> > + } >> >> This may need a little more thought since hid-multitouch doesn't query >> old value. Is there a case were a touch is removed and new touch >> added in same packet? Probably that would never send the -1 to user >> land. > > > Once a touch leaves the screen, a -1 will always be sent to indicate the > touch is out. When a new touch is added, it may use any available packet. > Hence the one used by the just removed touch can also be used. Do I get your > question properly? I'm thinking of this sequence inside 1 packet (probably not easy to produce in the wild): * 1st location in packet shows Finger with ID of 100 as not touching any more. * Loops threw and finds matching ID and sets to -1. * 2nd location in packet shows Finger with ID of 101 as touching. Its new. * Loops threw to find 1st slot with -1 and uses one we just cleared. * Sync window occurs. User only sees Contact ID change from 100 and 101. > >> >> > + >> > + touch = data[k - 1] & 0x1; >> > + input_mt_slot(input, j); >> > + if (touch) { >> > + x = le16_to_cpup((__le16 *)&data[k + 6]); >> > + y = le16_to_cpup((__le16 *)&data[k + 8]); >> > + >> > + input_report_abs(input, ABS_MT_POSITION_X, x); >> > + input_report_abs(input, ABS_MT_POSITION_Y, y); >> > + } else >> > + id = -1; >> > + >> > + /* keep the id from firmware since we need >> > + * it to process future events >> > + */ >> > + input_report_abs(input, ABS_MT_TRACKING_ID, id); >> > + >> > + k += WACOM_BYTES_PER_MT_PACKET; >> > + } >> > + >> > + input_mt_report_pointer_emulation(input, true); >> > + >> > + features->num_contacts_left -= contacts_to_send; >> > + if (features->num_contacts_left < 0) >> > + features->num_contacts_left = 0; >> >> Same comment as above. Could use a comment on why storing this would be >> needed. > > > If you understood the value of current_num_contacts, it would be easier to > see the need for num_contacts_left. I'll add a comment for > current_num_contacts. I guess I still don't see why its needed or how its being used. Now that I understand that current_num_contacts is total touches, I do understand you need to know when a packet contains 5 full touches and when it contains less than 5. For example if 6 touches then 1st packet could have 5 and next packet could have 1. So you need to know to stop looping after 1. But I do not see how this num_contacts_left helps that. It looks like it would set contacts_to_send to 5 in both packets. Chris > > Thank you for the review. > > Ping -- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 4/4] input: wacom - add 0xE5 (MT device) support 2011-12-22 4:14 ` Chris Bagwell @ 2011-12-28 0:48 ` Ping Cheng 2011-12-28 19:48 ` Chris Bagwell 0 siblings, 1 reply; 6+ messages in thread From: Ping Cheng @ 2011-12-28 0:48 UTC (permalink / raw) To: Chris Bagwell; +Cc: linux-input On Wed, Dec 21, 2011 at 8:14 PM, Chris Bagwell <chris@cnpbagwell.com> wrote: > > On Wed, Dec 21, 2011 at 12:43 PM, Ping Cheng <pinglinux@gmail.com> wrote: > > On Sun, Dec 18, 2011 at 6:07 PM, Chris Bagwell <chris@cnpbagwell.com> wrote: > >> > >> On Fri, Dec 16, 2011 at 6:38 PM, Ping Cheng <pinglinux@gmail.com> wrote: > >> > Signed-off-by: Ping Cheng <pingc@wacom.com> > >> > --- > >> > drivers/input/tablet/wacom_sys.c | 26 ++++++------ > >> > drivers/input/tablet/wacom_wac.c | 82 > >> > +++++++++++++++++++++++++++++++++++++- > >> > drivers/input/tablet/wacom_wac.h | 8 ++++ > >> > 3 files changed, 101 insertions(+), 15 deletions(-) > >> > > >> > diff --git a/drivers/input/tablet/wacom_sys.c > >> > b/drivers/input/tablet/wacom_sys.c > >> > index b9736fb..eee06ef 100644 > >> > --- a/drivers/input/tablet/wacom_sys.c > >> > +++ b/drivers/input/tablet/wacom_sys.c > >> > @@ -297,6 +297,10 @@ static int wacom_parse_hid(struct usb_interface > >> > *intf, > >> > features->pktlen > >> > = WACOM_PKGLEN_TPC2FG; > >> > > >> > features->touch_max = 2; > >> > } > >> > + > >> > + if (features->type == > >> > MTSCREEN) > >> > + features->pktlen > >> > = WACOM_PKGLEN_MTOUCH; > >> > + > >> > if (features->type == > >> > BAMBOO_PT) { > >> > /* need to reset > >> > back */ > >> > features->pktlen > >> > = WACOM_PKGLEN_BBTOUCH; > >> > @@ -331,18 +335,15 @@ static int wacom_parse_hid(struct usb_interface > >> > *intf, > >> > case HID_USAGE_Y: > >> > if (usage == WCM_DESKTOP) { > >> > if (finger) { > >> > - features->device_type = > >> > BTN_TOOL_FINGER; > >> > - if (features->type == > >> > TABLETPC2FG) { > >> > - /* need to reset > >> > back */ > >> > - features->pktlen > >> > = WACOM_PKGLEN_TPC2FG; > >> > + int type = > >> > features->type; > >> > + > >> > + if (type == TABLETPC2FG > >> > || type == MTSCREEN) { > >> > features->y_max = > >> > > >> > get_unaligned_le16(&report[i + 3]); > >> > features->y_phy = > >> > > >> > get_unaligned_le16(&report[i + 6]); > >> > i += 7; > >> > - } else if > >> > (features->type == BAMBOO_PT) { > >> > - /* need to reset > >> > back */ > >> > - features->pktlen > >> > = WACOM_PKGLEN_BBTOUCH; > >> > + } else if (type == > >> > BAMBOO_PT) { > >> > features->y_phy = > >> > > >> > get_unaligned_le16(&report[i + 3]); > >> > features->y_max = > >> > @@ -356,10 +357,6 @@ static int wacom_parse_hid(struct usb_interface > >> > *intf, > >> > i += 4; > >> > } > >> > } else if (pen) { > >> > - /* penabled only accepts > >> > exact bytes of data */ > >> > - if (features->type == > >> > TABLETPC2FG) > >> > - features->pktlen > >> > = WACOM_PKGLEN_GRAPHIRE; > >> > - features->device_type = > >> > BTN_TOOL_PEN; > >> > >> All makes sense. Getting rid of duplicate code since X/Y will always > >> be present. Maybe worth a 1 line comment now that its cleaned up so > >> someone doesn't let it creep back in. > > > > > > I'll add some comments in v2. > > > >> > >> > features->y_max = > >> > > >> > get_unaligned_le16(&report[i + 3]); > >> > i += 4; > >> > @@ -432,7 +429,8 @@ static int wacom_query_tablet_data(struct > >> > usb_interface *intf, struct wacom_feat > >> > > >> > /* ask to report tablet data if it is MT Tablet PC or > >> > * not a Tablet PC */ > >> > - if (features->type == TABLETPC2FG) { > >> > + if (((features->type == TABLETPC2FG) || (features->type == > >> > MTSCREEN)) > >> > + && features->device_type != BTN_TOOL_PEN) { > >> > >> Glad to see this device_type check. I thought it was probably needed. > >> > >> I'll have to trust you on this one but its different than Bamboo's. > >> The set on Bamboo is on Pen and causes it to start sending pen > >> packets. IIR, the touch packets are always sending. So this is > >> opposite and enables something related to touch? > > > > > > There are two types' of devices: tabletPC or non-tabletPC. For all tabletPC > > devices that support more than one finger/contact, a set call is needed. For > > all non-tabletPC devices, a set call is needed. Bamboo is a non-tabletPC > > device. > > > >> > >> Assuming you say yes, do you mind changing that to an affirmative > >> check for TOUCH so it reads: > > > > > > Well, it does not depend on whether it is touch or pen. It depends on the > > type of the device as explained above. Do you still want me to use your > > below logic? > > Let me ask this way. On Bamboo there are 2 interfaces, 1 pen and 1 > touch. I need to set feature report 2 to a specific value on the pen > interface to get useful packets. If I set feature report 2 to > something on touch interface, I get an error response back from > device. This aligns with HID descriptor because only the pen > interface declares that feature report #2. > > I'm sure its similar for Tablet PC's. Its also writing to feature > report #2 but unique data values. What ever the values are, it > probably also doesn't make sense to write to both pen interface and > touch interface on Tablet PC. > > If the feature report is only on one of two interfaces I do suggest > changing the if() to something like below to make it more obvious ("if > (TOUCH)" is more obvious that your looking for specific interface then > "if (!PEN)"). If its on both and only 1 needs to be written to then > that would probably be a good comment as well. I see your point. I'll use TOUCH/PEN for the if-statement and add a comment. > >> > >> if (TOUCH) > >> /* do TOUCH set's */ > >> if (PEN) > >> /* do PEN set's */ > >> > >> > >> > do { > >> > rep_data[0] = 3; > >> > rep_data[1] = 4; > >> > @@ -479,9 +477,9 @@ static int wacom_retrieve_hid_descriptor(struct > >> > usb_interface *intf, > >> > features->pressure_fuzz = 0; > >> > features->distance_fuzz = 0; > >> > > >> > - /* only Tablet PCs and Bamboo P&T need to retrieve the info */ > >> > + /* only devices that support touch need to retrieve the info */ > >> > if ((features->type != TABLETPC) && (features->type != > >> > TABLETPC2FG) && > >> > - (features->type != BAMBOO_PT)) > >> > + (features->type != BAMBOO_PT) && (features->type != > >> > MTSCREEN)) > >> > goto out; > >> > > >> > if (usb_get_extra_descriptor(interface, HID_DEVICET_HID, > >> > &hid_desc)) { > >> > diff --git a/drivers/input/tablet/wacom_wac.c > >> > b/drivers/input/tablet/wacom_wac.c > >> > index 8dc185d..1a887a1 100644 > >> > --- a/drivers/input/tablet/wacom_wac.c > >> > +++ b/drivers/input/tablet/wacom_wac.c > >> > @@ -729,6 +729,73 @@ static int wacom_intuos_irq(struct wacom_wac > >> > *wacom) > >> > return 1; > >> > } > >> > > >> > +static int wacom_mt_touch(struct wacom_wac *wacom) > >> > +{ > >> > + struct wacom_features *features = &wacom->features; > >> > + struct input_dev *input = wacom->input; > >> > + char *data = wacom->data; > >> > + struct input_mt_slot *mt; > >> > + int i, id = -1, j = 0, k = 4; > >> > + int x = 0, y = 0; > >> > + int current_num_contacts = data[2]; > >> > + int contacts_to_send = 0; > >> > + bool touch = false; > >> > + > >> > + /* reset the counter by the first packet */ > >> > + if (current_num_contacts) { > >> > + features->num_contacts = current_num_contacts; > >> > + features->num_contacts_left = current_num_contacts; > >> > + } > >> > >> It seems that the saved contact values are only processed in case were > >> data[2] indicates no contact counts in packet. I would think that > >> packets with no contacts could be ignored (return) unless there is > >> some data that must be implied. Why is it not ignored? > > > > > > current_num_contacts is only reported, hence updated, by the first set of > > data. Later touch data will be processed as long as number of contacts does > > not exceed current_num_contacts. > > > >> > >> > >> > + > >> > + /* There are at most 5 contacts per packet */ > >> > + contacts_to_send = min(5, (int)features->num_contacts_left); > >> > >> Its not obvious to me if data[2] contains touches in packet or total > >> touches on screen. > > > > > > Total touches on the screen. > > OK. I understand now. It would be nice to add that to above comment > (the fact that num_contacts_left is total on screen). num_contacts_left is normally not the total on the screen. It is the number of contacts that need to be processed. num_contacts is the total on the screen. contacts_to_send is the number of contacts we are going to send. > >> > >> > + > >> > + for (i = 0; i < contacts_to_send; i++) { > >> > + id = le16_to_cpup((__le16 *)&data[k]); > >> > + > >> > + /* is there an existing slot for this contact? */ > >> > + for (j = 0; j < features->touch_max; j++) { > >> > + mt = &input->mt[j]; > >> > + if (input_mt_get_value(mt, ABS_MT_TRACKING_ID) > >> > == id ) > >> > + break; > >> > + } > >> > + > >> > + /* no. then find an unused slot to fill */ > >> > + if (j >= features->touch_max) { > >> > + for (j = 0; j < features->touch_max; j++) { > >> > + mt = &input->mt[j]; > >> > + if (input_mt_get_value(mt, > >> > ABS_MT_TRACKING_ID) == -1 ) > >> > + break; > >> > + } > >> > + } > >> > >> This may need a little more thought since hid-multitouch doesn't query > >> old value. Is there a case were a touch is removed and new touch > >> added in same packet? Probably that would never send the -1 to user > >> land. > > > > > > Once a touch leaves the screen, a -1 will always be sent to indicate the > > touch is out. When a new touch is added, it may use any available packet. > > Hence the one used by the just removed touch can also be used. Do I get your > > question properly? > > I'm thinking of this sequence inside 1 packet (probably not easy to > produce in the wild): > > * 1st location in packet shows Finger with ID of 100 as not touching any more. > * Loops threw and finds matching ID and sets to -1. > * 2nd location in packet shows Finger with ID of 101 as touching. Its new. > * Loops threw to find 1st slot with -1 and uses one we just cleared. > * Sync window occurs. User only sees Contact ID change from 100 and 101. Oh, I know where you are. You are in the real world, not in the firmware ;). The firmware can not tell if the second finger (101) is the same finger as the first one (100) or not. All it can do is: report a finger out then a finger in. So, in any moment if there is one finger on the screen, it report one finger. If none, it reports none. If, by any chance, the second one (101) touches before the first one out, we get two finger data before one if out. Your case won't happen, at least in theory. > >> > + > >> > + touch = data[k - 1] & 0x1; > >> > + input_mt_slot(input, j); > >> > + if (touch) { > >> > + x = le16_to_cpup((__le16 *)&data[k + 6]); > >> > + y = le16_to_cpup((__le16 *)&data[k + 8]); > >> > + > >> > + input_report_abs(input, ABS_MT_POSITION_X, x); > >> > + input_report_abs(input, ABS_MT_POSITION_Y, y); > >> > + } else > >> > + id = -1; > >> > + > >> > + /* keep the id from firmware since we need > >> > + * it to process future events > >> > + */ > >> > + input_report_abs(input, ABS_MT_TRACKING_ID, id); > >> > + > >> > + k += WACOM_BYTES_PER_MT_PACKET; > >> > + } > >> > + > >> > + input_mt_report_pointer_emulation(input, true); > >> > + > >> > + features->num_contacts_left -= contacts_to_send; > >> > + if (features->num_contacts_left < 0) > >> > + features->num_contacts_left = 0; > >> > >> Same comment as above. Could use a comment on why storing this would be > >> needed. > > > > > > If you understood the value of current_num_contacts, it would be easier to > > see the need for num_contacts_left. I'll add a comment for > > current_num_contacts. > > I guess I still don't see why its needed or how its being used. > > Now that I understand that current_num_contacts is total touches, I do > understand you need to know when a packet contains 5 full touches and > when it contains less than 5. For example if 6 touches then 1st > packet could have 5 and next packet could have 1. So you need to know > to stop looping after 1. But I do not see how this num_contacts_left > helps that. It looks like it would set contacts_to_send to 5 in both > packets. Why do you think contacts_to_send could be 5? If num_contacts_left is 1, min(5, (int)features->num_contacts_left) should return 1, which is the contacts_to_send. Do I miss something? Ping -- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 4/4] input: wacom - add 0xE5 (MT device) support 2011-12-28 0:48 ` Ping Cheng @ 2011-12-28 19:48 ` Chris Bagwell 2011-12-31 3:34 ` Chris Bagwell 0 siblings, 1 reply; 6+ messages in thread From: Chris Bagwell @ 2011-12-28 19:48 UTC (permalink / raw) To: Ping Cheng; +Cc: linux-input On Tue, Dec 27, 2011 at 6:48 PM, Ping Cheng <pinglinux@gmail.com> wrote: > On Wed, Dec 21, 2011 at 8:14 PM, Chris Bagwell <chris@cnpbagwell.com> wrote: >> >> On Wed, Dec 21, 2011 at 12:43 PM, Ping Cheng <pinglinux@gmail.com> wrote: >> > On Sun, Dec 18, 2011 at 6:07 PM, Chris Bagwell <chris@cnpbagwell.com> wrote: >> >> >> >> On Fri, Dec 16, 2011 at 6:38 PM, Ping Cheng <pinglinux@gmail.com> wrote: >> >> > Signed-off-by: Ping Cheng <pingc@wacom.com> >> >> > --- >> >> > drivers/input/tablet/wacom_sys.c | 26 ++++++------ >> >> > drivers/input/tablet/wacom_wac.c | 82 >> >> > +++++++++++++++++++++++++++++++++++++- >> >> > drivers/input/tablet/wacom_wac.h | 8 ++++ >> >> > 3 files changed, 101 insertions(+), 15 deletions(-) >> >> > >> >> > diff --git a/drivers/input/tablet/wacom_sys.c >> >> > b/drivers/input/tablet/wacom_sys.c >> >> > index b9736fb..eee06ef 100644 >> >> > --- a/drivers/input/tablet/wacom_sys.c >> >> > +++ b/drivers/input/tablet/wacom_sys.c >> >> > @@ -297,6 +297,10 @@ static int wacom_parse_hid(struct usb_interface >> >> > *intf, >> >> > features->pktlen >> >> > = WACOM_PKGLEN_TPC2FG; >> >> > >> >> > features->touch_max = 2; >> >> > } >> >> > + >> >> > + if (features->type == >> >> > MTSCREEN) >> >> > + features->pktlen >> >> > = WACOM_PKGLEN_MTOUCH; >> >> > + >> >> > if (features->type == >> >> > BAMBOO_PT) { >> >> > /* need to reset >> >> > back */ >> >> > features->pktlen >> >> > = WACOM_PKGLEN_BBTOUCH; >> >> > @@ -331,18 +335,15 @@ static int wacom_parse_hid(struct usb_interface >> >> > *intf, >> >> > case HID_USAGE_Y: >> >> > if (usage == WCM_DESKTOP) { >> >> > if (finger) { >> >> > - features->device_type = >> >> > BTN_TOOL_FINGER; >> >> > - if (features->type == >> >> > TABLETPC2FG) { >> >> > - /* need to reset >> >> > back */ >> >> > - features->pktlen >> >> > = WACOM_PKGLEN_TPC2FG; >> >> > + int type = >> >> > features->type; >> >> > + >> >> > + if (type == TABLETPC2FG >> >> > || type == MTSCREEN) { >> >> > features->y_max = >> >> > >> >> > get_unaligned_le16(&report[i + 3]); >> >> > features->y_phy = >> >> > >> >> > get_unaligned_le16(&report[i + 6]); >> >> > i += 7; >> >> > - } else if >> >> > (features->type == BAMBOO_PT) { >> >> > - /* need to reset >> >> > back */ >> >> > - features->pktlen >> >> > = WACOM_PKGLEN_BBTOUCH; >> >> > + } else if (type == >> >> > BAMBOO_PT) { >> >> > features->y_phy = >> >> > >> >> > get_unaligned_le16(&report[i + 3]);distracted me. >> >> > features->y_max = >> >> > @@ -356,10 +357,6 @@ static int wacom_parse_hid(struct usb_interface >> >> > *intf, >> >> > i += 4; >> >> > } >> >> > } else if (pen) { >> >> > - /* penabled only accepts >> >> > exact bytes of data */ >> >> > - if (features->type == >> >> > TABLETPC2FG) >> >> > - features->pktlen >> >> > = WACOM_PKGLEN_GRAPHIRE; >> >> > - features->device_type = >> >> > BTN_TOOL_PEN; >> >> >> >> All makes sense. Getting rid of duplicate code since X/Y will always >> >> be present. Maybe worth a 1 line comment now that its cleaned up so >> >> someone doesn't let it creep back in. >> > >> > >> > I'll add some comments in v2. >> > >> >> >> >> > features->y_max = >> >> > >> >> > get_unaligned_le16(&report[i + 3]); >> >> > i += 4; >> >> > @@ -432,7 +429,8 @@ static int wacom_query_tablet_data(struct >> >> > usb_interface *intf, struct wacom_feat >> >> > >> >> > /* ask to report tablet data if it is MT Tablet PC or >> >> > * not a Tablet PC */ >> >> > - if (features->type == TABLETPC2FG) { >> >> > + if (((features->type == TABLETPC2FG) || (features->type == >> >> > MTSCREEN)) >> >> > + && features->device_type != BTN_TOOL_PEN) { >> >> >> >> Glad to see this device_type check. I thought it was probably needed. >> >> >> >> I'll have to trust you on this one but its different than Bamboo's. >> >> The set on Bamboo is on Pen and causes it to start sending pen >> >> packets. IIR, the touch packets are always sending. So this is >> >> opposite and enables something related to touch? >> > >> > >> > There are two types' of devices: tabletPC or non-tabletPC. For all tabletPC >> > devices that support more than one finger/contact, a set call is needed. For >> > all non-tabletPC devices, a set call is needed. Bamboo is a non-tabletPC >> > device. >> > >> >> >> >> Assuming you say yes, do you mind changing that to an affirmative >> >> check for TOUCH so it reads: >> > >> > >> > Well, it does not depend on whether it is touch or pen. It depends on the >> > type of the device as explained above. Do you still want me to use your >> > below logic? >> >> Let me ask this way. On Bamboo there are 2 interfaces, 1 pen and 1 >> touch. I need to set feature report 2 to a specific value on the pen >> interface to get useful packets. If I set feature report 2 to >> something on touch interface, I get an error response back from >> device. This aligns with HID descriptor because only the pen >> interface declares that feature report #2. >> >> I'm sure its similar for Tablet PC's. Its also writing to feature >> report #2 but unique data values. What ever the values are, it >> probably also doesn't make sense to write to both pen interface and >> touch interface on Tablet PC. >> >> If the feature report is only on one of two interfaces I do suggest >> changing the if() to something like below to make it more obvious ("if >> (TOUCH)" is more obvious that your looking for specific interface then >> "if (!PEN)"). If its on both and only 1 needs to be written to then >> that would probably be a good comment as well. > > I see your point. I'll use TOUCH/PEN for the if-statement and add a comment. > Thanks. >> >> >> >> if (TOUCH) >> >> /* do TOUCH set's */ >> >> if (PEN) >> >> /* do PEN set's */ >> >> >> >> >> >> > do { >> >> > rep_data[0] = 3; >> >> > rep_data[1] = 4; >> >> > @@ -479,9 +477,9 @@ static int wacom_retrieve_hid_descriptor(struct >> >> > usb_interface *intf, >> >> > features->pressure_fuzz = 0; >> >> > features->distance_fuzz = 0; >> >> > >> >> > - /* only Tablet PCs and Bamboo P&T need to retrieve the info */ >> >> > + /* only devices that support touch need to retrieve the info */ >> >> > if ((features->type != TABLETPC) && (features->type != >> >> > TABLETPC2FG) && >> >> > - (features->type != BAMBOO_PT)) >> >> > + (features->type != BAMBOO_PT) && (features->type != >> >> > MTSCREEN)) >> >> > goto out; >> >> > >> >> > if (usb_get_extra_descriptor(interface, HID_DEVICET_HID, >> >> > &hid_desc)) { >> >> > diff --git a/drivers/input/tablet/wacom_wac.c >> >> > b/drivers/input/tablet/wacom_wac.c >> >> > index 8dc185d..1a887a1 100644 >> >> > --- a/drivers/input/tablet/wacom_wac.c >> >> > +++ b/drivers/input/tablet/wacom_wac.c >> >> > @@ -729,6 +729,73 @@ static int wacom_intuos_irq(struct wacom_wac >> >> > *wacom) >> >> > return 1; >> >> > } >> >> > >> >> > +static int wacom_mt_touch(struct wacom_wac *wacom) >> >> > +{ >> >> > + struct wacom_features *features = &wacom->features; >> >> > + struct input_dev *input = wacom->input; >> >> > + char *data = wacom->data; >> >> > + struct input_mt_slot *mt; >> >> > + int i, id = -1, j = 0, k = 4; >> >> > + int x = 0, y = 0; >> >> > + int current_num_contacts = data[2]; >> >> > + int contacts_to_send = 0; >> >> > + bool touch = false; >> >> > + >> >> > + /* reset the counter by the first packet */ >> >> > + if (current_num_contacts) { >> >> > + features->num_contacts = current_num_contacts; >> >> > + features->num_contacts_left = current_num_contacts; >> >> > + } >> >> >> >> It seems that the saved contact values are only processed in case were >> >> data[2] indicates no contact counts in packet. I would think that >> >> packets with no contacts could be ignored (return) unless there is >> >> some data that must be implied. Why is it not ignored? >> > >> > >> > current_num_contacts is only reported, hence updated, by the first set of >> > data. Later touch data will be processed as long as number of contacts does >> > not exceed current_num_contacts. >> > >> >> >> >> >> >> > + >> >> > + /* There are at most 5 contacts per packet */ >> >> > + contacts_to_send = min(5, (int)features->num_contacts_left); >> >> >> >> Its not obvious to me if data[2] contains touches in packet or total >> >> touches on screen. >> > >> > >> > Total touches on the screen. >> >> OK. I understand now. It would be nice to add that to above comment >> (the fact that num_contacts_left is total on screen). > > num_contacts_left is normally not the total on the screen. It is the > number of contacts that need to be processed. num_contacts is the > total on the screen. contacts_to_send is the number of contacts we are > going to send. OK. With understanding that data[2] is # of touches in packet and not total touches on screen then my questions change slightly. I assume if user put 6 touches a screen and moves around you'd get 2 packets with movement; 1 with data[2] == 5 and 1 with data[2] == 1. There is this code then: int current_num_contacts = data[2]; if (current_num_contacts) { features->num_contacts = current_num_contacts; features->num_contacts_left = current_num_contacts; } So it seems to me that num_contacts can never reach 6 (total touches on screen). Maybe that needs to be a += or something? > >> >> >> >> > + >> >> > + for (i = 0; i < contacts_to_send; i++) { >> >> > + id = le16_to_cpup((__le16 *)&data[k]); >> >> > + >> >> > + /* is there an existing slot for this contact? */ >> >> > + for (j = 0; j < features->touch_max; j++) { >> >> > + mt = &input->mt[j]; >> >> > + if (input_mt_get_value(mt, ABS_MT_TRACKING_ID) >> >> > == id ) >> >> > + break; >> >> > + } >> >> > + >> >> > + /* no. then find an unused slot to fill */ >> >> > + if (j >= features->touch_max) { >> >> > + for (j = 0; j < features->touch_max; j++) { >> >> > + mt = &input->mt[j]; >> >> > + if (input_mt_get_value(mt, >> >> > ABS_MT_TRACKING_ID) == -1 ) >> >> > + break; >> >> > + } >> >> > + } >> >> >> >> This may need a little more thought since hid-multitouch doesn't query >> >> old value. Is there a case were a touch is removed and new touch >> >> added in same packet? Probably that would never send the -1 to user >> >> land. >> > >> > >> > Once a touch leaves the screen, a -1 will always be sent to indicate the >> > touch is out. When a new touch is added, it may use any available packet. >> > Hence the one used by the just removed touch can also be used. Do I get your >> > question properly? >> >> I'm thinking of this sequence inside 1 packet (probably not easy to >> produce in the wild): >> >> * 1st location in packet shows Finger with ID of 100 as not touching any more. >> * Loops threw and finds matching ID and sets to -1. >> * 2nd location in packet shows Finger with ID of 101 as touching. Its new. >> * Loops threw to find 1st slot with -1 and uses one we just cleared. >> * Sync window occurs. User only sees Contact ID change from 100 and 101. > > Oh, I know where you are. You are in the real world, not in the > firmware ;). The firmware can not tell if the second finger (101) is > the same finger as the first one (100) or not. All it can do is: > report a finger out then a finger in. So, in any moment if there is > one finger on the screen, it report one finger. If none, it reports > none. If, by any chance, the second one (101) touches before the first > one out, we get two finger data before one if out. > > Your case won't happen, at least in theory. OK. I only wanted to bring up the possibility. Depending on firmware design its possible that never could happen. > >> >> > + >> >> > + touch = data[k - 1] & 0x1; >> >> > + input_mt_slot(input, j); >> >> > + if (touch) { >> >> > + x = le16_to_cpup((__le16 *)&data[k + 6]); >> >> > + y = le16_to_cpup((__le16 *)&data[k + 8]); >> >> > + >> >> > + input_report_abs(input, ABS_MT_POSITION_X, x); >> >> > + input_report_abs(input, ABS_MT_POSITION_Y, y); >> >> > + } else >> >> > + id = -1; >> >> > + >> >> > + /* keep the id from firmware since we need >> >> > + * it to process future events >> >> > + */ >> >> > + input_report_abs(input, ABS_MT_TRACKING_ID, id); >> >> > + >> >> > + k += WACOM_BYTES_PER_MT_PACKET; >> >> > + } >> >> > + >> >> > + input_mt_report_pointer_emulation(input, true); >> >> > + >> >> > + features->num_contacts_left -= contacts_to_send; >> >> > + if (features->num_contacts_left < 0) >> >> > + features->num_contacts_left = 0; >> >> >> >> Same comment as above. Could use a comment on why storing this would be >> >> needed. >> > >> > >> > If you understood the value of current_num_contacts, it would be easier to >> > see the need for num_contacts_left. I'll add a comment for >> > current_num_contacts. >> >> I guess I still don't see why its needed or how its being used. >> >> Now that I understand that current_num_contacts is total touches, I do >> understand you need to know when a packet contains 5 full touches and >> when it contains less than 5. For example if 6 touches then 1st >> packet could have 5 and next packet could have 1. So you need to know >> to stop looping after 1. But I do not see how this num_contacts_left >> helps that. It looks like it would set contacts_to_send to 5 in both >> packets. > > Why do you think contacts_to_send could be 5? If num_contacts_left is > 1, min(5, (int)features->num_contacts_left) should return 1, which is > the contacts_to_send. Do I miss something? I think it stemmed from my misunderstanding of what data[2] contained. If it contains 1 for 2nd packet with 6th finger then num_contacts_left would be 1 as well and it would work. With my new understanding that data[2] is touches in packet (1 to 5) then it seems all this num_contacts and num_contacts_left is trying to compute total touches on the screen but then its never used by anything. I'd get rid of it and just keep the camp of data[2] to <= 5/contacts_to_send part. Chris > > Ping -- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 4/4] input: wacom - add 0xE5 (MT device) support 2011-12-28 19:48 ` Chris Bagwell @ 2011-12-31 3:34 ` Chris Bagwell 0 siblings, 0 replies; 6+ messages in thread From: Chris Bagwell @ 2011-12-31 3:34 UTC (permalink / raw) To: Ping Cheng; +Cc: linux-input On Wed, Dec 28, 2011 at 1:48 PM, Chris Bagwell <chris@cnpbagwell.com> wrote: > On Tue, Dec 27, 2011 at 6:48 PM, Ping Cheng <pinglinux@gmail.com> wrote: >> On Wed, Dec 21, 2011 at 8:14 PM, Chris Bagwell <chris@cnpbagwell.com> wrote: >>> >>> On Wed, Dec 21, 2011 at 12:43 PM, Ping Cheng <pinglinux@gmail.com> wrote: >>> > On Sun, Dec 18, 2011 at 6:07 PM, Chris Bagwell <chris@cnpbagwell.com> wrote: >>> >> >>> >> On Fri, Dec 16, 2011 at 6:38 PM, Ping Cheng <pinglinux@gmail.com> wrote: >>> >> > Signed-off-by: Ping Cheng <pingc@wacom.com> >>> >> > --- >>> >> > drivers/input/tablet/wacom_sys.c | 26 ++++++------ >>> >> > drivers/input/tablet/wacom_wac.c | 82 >>> >> > +++++++++++++++++++++++++++++++++++++- >>> >> > drivers/input/tablet/wacom_wac.h | 8 ++++ >>> >> > 3 files changed, 101 insertions(+), 15 deletions(-) >>> >> > >>> >> > diff --git a/drivers/input/tablet/wacom_sys.c >>> >> > b/drivers/input/tablet/wacom_sys.c >>> >> > index b9736fb..eee06ef 100644 >>> >> > --- a/drivers/input/tablet/wacom_sys.c >>> >> > +++ b/drivers/input/tablet/wacom_sys.c >>> >> > @@ -297,6 +297,10 @@ static int wacom_parse_hid(struct usb_interface >>> >> > *intf, >>> >> > features->pktlen >>> >> > = WACOM_PKGLEN_TPC2FG; >>> >> > >>> >> > features->touch_max = 2; >>> >> > } >>> >> > + >>> >> > + if (features->type == >>> >> > MTSCREEN) >>> >> > + features->pktlen >>> >> > = WACOM_PKGLEN_MTOUCH; >>> >> > + >>> >> > if (features->type == >>> >> > BAMBOO_PT) { >>> >> > /* need to reset >>> >> > back */ >>> >> > features->pktlen >>> >> > = WACOM_PKGLEN_BBTOUCH; >>> >> > @@ -331,18 +335,15 @@ static int wacom_parse_hid(struct usb_interface >>> >> > *intf, >>> >> > case HID_USAGE_Y: >>> >> > if (usage == WCM_DESKTOP) { >>> >> > if (finger) { >>> >> > - features->device_type = >>> >> > BTN_TOOL_FINGER; >>> >> > - if (features->type == >>> >> > TABLETPC2FG) { >>> >> > - /* need to reset >>> >> > back */ >>> >> > - features->pktlen >>> >> > = WACOM_PKGLEN_TPC2FG; >>> >> > + int type = >>> >> > features->type; >>> >> > + >>> >> > + if (type == TABLETPC2FG >>> >> > || type == MTSCREEN) { >>> >> > features->y_max = >>> >> > >>> >> > get_unaligned_le16(&report[i + 3]); >>> >> > features->y_phy = >>> >> > >>> >> > get_unaligned_le16(&report[i + 6]); >>> >> > i += 7; >>> >> > - } else if >>> >> > (features->type == BAMBOO_PT) { >>> >> > - /* need to reset >>> >> > back */ >>> >> > - features->pktlen >>> >> > = WACOM_PKGLEN_BBTOUCH; >>> >> > + } else if (type == >>> >> > BAMBOO_PT) { >>> >> > features->y_phy = >>> >> > >>> >> > get_unaligned_le16(&report[i + 3]);distracted me. >>> >> > features->y_max = >>> >> > @@ -356,10 +357,6 @@ static int wacom_parse_hid(struct usb_interface >>> >> > *intf, >>> >> > i += 4; >>> >> > } >>> >> > } else if (pen) { >>> >> > - /* penabled only accepts >>> >> > exact bytes of data */ >>> >> > - if (features->type == >>> >> > TABLETPC2FG) >>> >> > - features->pktlen >>> >> > = WACOM_PKGLEN_GRAPHIRE; >>> >> > - features->device_type = >>> >> > BTN_TOOL_PEN; >>> >> >>> >> All makes sense. Getting rid of duplicate code since X/Y will always >>> >> be present. Maybe worth a 1 line comment now that its cleaned up so >>> >> someone doesn't let it creep back in. >>> > >>> > >>> > I'll add some comments in v2. >>> > >>> >> >>> >> > features->y_max = >>> >> > >>> >> > get_unaligned_le16(&report[i + 3]); >>> >> > i += 4; >>> >> > @@ -432,7 +429,8 @@ static int wacom_query_tablet_data(struct >>> >> > usb_interface *intf, struct wacom_feat >>> >> > >>> >> > /* ask to report tablet data if it is MT Tablet PC or >>> >> > * not a Tablet PC */ >>> >> > - if (features->type == TABLETPC2FG) { >>> >> > + if (((features->type == TABLETPC2FG) || (features->type == >>> >> > MTSCREEN)) >>> >> > + && features->device_type != BTN_TOOL_PEN) { >>> >> >>> >> Glad to see this device_type check. I thought it was probably needed. >>> >> >>> >> I'll have to trust you on this one but its different than Bamboo's. >>> >> The set on Bamboo is on Pen and causes it to start sending pen >>> >> packets. IIR, the touch packets are always sending. So this is >>> >> opposite and enables something related to touch? >>> > >>> > >>> > There are two types' of devices: tabletPC or non-tabletPC. For all tabletPC >>> > devices that support more than one finger/contact, a set call is needed. For >>> > all non-tabletPC devices, a set call is needed. Bamboo is a non-tabletPC >>> > device. >>> > >>> >> >>> >> Assuming you say yes, do you mind changing that to an affirmative >>> >> check for TOUCH so it reads: >>> > >>> > >>> > Well, it does not depend on whether it is touch or pen. It depends on the >>> > type of the device as explained above. Do you still want me to use your >>> > below logic? >>> >>> Let me ask this way. On Bamboo there are 2 interfaces, 1 pen and 1 >>> touch. I need to set feature report 2 to a specific value on the pen >>> interface to get useful packets. If I set feature report 2 to >>> something on touch interface, I get an error response back from >>> device. This aligns with HID descriptor because only the pen >>> interface declares that feature report #2. >>> >>> I'm sure its similar for Tablet PC's. Its also writing to feature >>> report #2 but unique data values. What ever the values are, it >>> probably also doesn't make sense to write to both pen interface and >>> touch interface on Tablet PC. >>> >>> If the feature report is only on one of two interfaces I do suggest >>> changing the if() to something like below to make it more obvious ("if >>> (TOUCH)" is more obvious that your looking for specific interface then >>> "if (!PEN)"). If its on both and only 1 needs to be written to then >>> that would probably be a good comment as well. >> >> I see your point. I'll use TOUCH/PEN for the if-statement and add a comment. >> > > Thanks. > >>> >> >>> >> if (TOUCH) >>> >> /* do TOUCH set's */ >>> >> if (PEN) >>> >> /* do PEN set's */ >>> >> >>> >> >>> >> > do { >>> >> > rep_data[0] = 3; >>> >> > rep_data[1] = 4; >>> >> > @@ -479,9 +477,9 @@ static int wacom_retrieve_hid_descriptor(struct >>> >> > usb_interface *intf, >>> >> > features->pressure_fuzz = 0; >>> >> > features->distance_fuzz = 0; >>> >> > >>> >> > - /* only Tablet PCs and Bamboo P&T need to retrieve the info */ >>> >> > + /* only devices that support touch need to retrieve the info */ >>> >> > if ((features->type != TABLETPC) && (features->type != >>> >> > TABLETPC2FG) && >>> >> > - (features->type != BAMBOO_PT)) >>> >> > + (features->type != BAMBOO_PT) && (features->type != >>> >> > MTSCREEN)) >>> >> > goto out; >>> >> > >>> >> > if (usb_get_extra_descriptor(interface, HID_DEVICET_HID, >>> >> > &hid_desc)) { >>> >> > diff --git a/drivers/input/tablet/wacom_wac.c >>> >> > b/drivers/input/tablet/wacom_wac.c >>> >> > index 8dc185d..1a887a1 100644 >>> >> > --- a/drivers/input/tablet/wacom_wac.c >>> >> > +++ b/drivers/input/tablet/wacom_wac.c >>> >> > @@ -729,6 +729,73 @@ static int wacom_intuos_irq(struct wacom_wac >>> >> > *wacom) >>> >> > return 1; >>> >> > } >>> >> > >>> >> > +static int wacom_mt_touch(struct wacom_wac *wacom) >>> >> > +{ >>> >> > + struct wacom_features *features = &wacom->features; >>> >> > + struct input_dev *input = wacom->input; >>> >> > + char *data = wacom->data; >>> >> > + struct input_mt_slot *mt; >>> >> > + int i, id = -1, j = 0, k = 4; >>> >> > + int x = 0, y = 0; >>> >> > + int current_num_contacts = data[2]; >>> >> > + int contacts_to_send = 0; >>> >> > + bool touch = false; >>> >> > + >>> >> > + /* reset the counter by the first packet */ >>> >> > + if (current_num_contacts) { >>> >> > + features->num_contacts = current_num_contacts; >>> >> > + features->num_contacts_left = current_num_contacts; >>> >> > + } >>> >> >>> >> It seems that the saved contact values are only processed in case were >>> >> data[2] indicates no contact counts in packet. I would think that >>> >> packets with no contacts could be ignored (return) unless there is >>> >> some data that must be implied. Why is it not ignored? >>> > >>> > >>> > current_num_contacts is only reported, hence updated, by the first set of >>> > data. Later touch data will be processed as long as number of contacts does >>> > not exceed current_num_contacts. >>> > >>> >> >>> >> >>> >> > + >>> >> > + /* There are at most 5 contacts per packet */ >>> >> > + contacts_to_send = min(5, (int)features->num_contacts_left); >>> >> >>> >> Its not obvious to me if data[2] contains touches in packet or total >>> >> touches on screen. >>> > >>> > >>> > Total touches on the screen. >>> >>> OK. I understand now. It would be nice to add that to above comment >>> (the fact that num_contacts_left is total on screen). >> >> num_contacts_left is normally not the total on the screen. It is the >> number of contacts that need to be processed. num_contacts is the >> total on the screen. contacts_to_send is the number of contacts we are >> going to send. > > OK. With understanding that data[2] is # of touches in packet and not > total touches on screen then my questions change slightly. > > I assume if user put 6 touches a screen and moves around you'd get 2 > packets with movement; 1 with data[2] == 5 and 1 with data[2] == 1. > There is this code then: > > int current_num_contacts = data[2]; > if (current_num_contacts) { > features->num_contacts = current_num_contacts; > features->num_contacts_left = current_num_contacts; > } > > So it seems to me that num_contacts can never reach 6 (total touches > on screen). Maybe that needs to be a += or something? I get it now. In first touch packet received, data[2] will contain the total touches to be reported. There will (data[2] / 5) additional packets following this packet and they will all have data[2] set to 0. So I understand how your logic is meant to work with that firmware behaviour. If you could throw in something about data[2] == 0 being a continuation packet then I think it would help future readers. So that wraps up all the concerns/questions/comments I had on the patches. Chris -- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-12-31 3:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-12-17 0:38 [PATCH 4/4] input: wacom - add 0xE5 (MT device) support Ping Cheng
2011-12-19 2:07 ` Chris Bagwell
[not found] ` <CAF8JNhJS-b0hktyXiJ-T+8HheRXwW8yD0FFsgmMK=CnvSiFJOA@mail.gmail.com>
2011-12-22 4:14 ` Chris Bagwell
2011-12-28 0:48 ` Ping Cheng
2011-12-28 19:48 ` Chris Bagwell
2011-12-31 3:34 ` Chris Bagwell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).