Re: [PATCH 4/4] input: wacom - add 0xE5 (MT device) support
From: Chris Bagwell <hidden>
Date: 2011-12-31 03:34:38
On Wed, Dec 28, 2011 at 1:48 PM, Chris Bagwell [off-list ref] wrote:
On Tue, Dec 27, 2011 at 6:48 PM, Ping Cheng [off-list ref] wrote:quoted
On Wed, Dec 21, 2011 at 8:14 PM, Chris Bagwell [off-list ref] wrote:quoted
On Wed, Dec 21, 2011 at 12:43 PM, Ping Cheng [off-list ref] wrote:quoted
On Sun, Dec 18, 2011 at 6:07 PM, Chris Bagwell [off-list ref] wrote:quoted
On Fri, Dec 16, 2011 at 6:38 PM, Ping Cheng [off-list ref] wrote:quoted
Signed-off-by: Ping Cheng <redacted> --- 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.cb/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.quoted
quoted
features->y_max = get_unaligned_le16(&report[i + 3]); i += 4;@@ -432,7 +429,8 @@ static int wacom_query_tablet_data(structusb_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.quoted
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.quoted
quoted
quoted
quoted
if (TOUCH) /* do TOUCH set's */ if (PEN) /* do PEN set's */quoted
do { rep_data[0] = 3; rep_data[1] = 4;@@ -479,9 +477,9 @@ static int wacom_retrieve_hid_descriptor(structusb_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.cb/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.quoted
quoted
+ + /* 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