Hi Henrik, On Thu, Mar 10, 2011 at 16:52, Henrik Rydberg <rydberg@xxxxxxxxxxx> wrote: > Hi Benjamin, > >> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c >> index 02a77f9..0b92dfc 100644 >> --- a/drivers/hid/hid-multitouch.c >> +++ b/drivers/hid/hid-multitouch.c >> @@ -5,6 +5,11 @@ >> * Copyright (c) 2010-2011 Benjamin Tissoires <benjamin.tissoires@xxxxxxxxx> >> * Copyright (c) 2010-2011 Ecole Nationale de l'Aviation Civile, France >> * >> + * based on hid-3m-pct.c copyrighted as follows: >> + * Copyright (c) 2009-2010 Stephane Chatty <chatty@xxxxxxx> >> + * Copyright (c) 2010 Henrik Rydberg <rydberg@xxxxxxxxxxx> >> + * Copyright (c) 2010 Canonical, Ltd. >> + * >> */ >> >> /* >> @@ -62,6 +67,8 @@ struct mt_class { >> __s32 name; /* MT_CLS */ >> __s32 quirks; >> __s32 sn_move; /* Signal/noise ratio for move events */ >> + __s32 sn_width; /* Signal/noise ratio for width events */ >> + __s32 sn_height; /* Signal/noise ratio for height events */ >> __s32 sn_pressure; /* Signal/noise ratio for pressure events */ >> __u8 maxcontacts; >> }; >> @@ -72,6 +79,7 @@ struct mt_class { >> #define MT_CLS_DUAL_INRANGE_CONTACTNUMBER 3 >> #define MT_CLS_CYPRESS 4 >> #define MT_CLS_STANTUM 5 >> +#define MT_CLS_3M 6 >> >> /* >> * these device-dependent functions determine what slot corresponds >> @@ -123,6 +131,12 @@ struct mt_class mt_classes[] = { >> .maxcontacts = 10 }, >> { .name = MT_CLS_STANTUM, >> .quirks = MT_QUIRK_VALID_IS_CONFIDENCE }, > > I realize several of the entries are missing maxcontacts now, so all > patches needs to be checked... This is really annoying. The idea behind the auto-detection was to simplify the writing of the driver and to let the device inform us on its capabilities. It's not a bug, it's a feature in this particular case. My idea was to remove all those .maxcontact (for instance, I know that it works for cypress, stantum and 3M) but I didn't want to bother my testers about such a little change. Can I remember you that you where the first complaining about the maxcontact? You asked if it could not be given by the device. And as we where in a hurry, we didn't include it and said that we will do it later. > >> + { .name = MT_CLS_3M, >> + .quirks = MT_QUIRK_VALID_IS_CONFIDENCE | >> + MT_QUIRK_SLOT_IS_CONTACTID, >> + .sn_move = 2048, >> + .sn_width = 128, >> + .sn_height = 128 }, > > And this one does too And this one would be false: should I write 10 or 60? As it depends on the device, we'll have to let the device decide. If you remember the commit message of the auto-detection patch, I have written that we keep the .maxcontact field in case the device _lies_. And 3M does not lie. >> >> { } >> }; >> @@ -206,11 +220,15 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, >> case HID_DG_WIDTH: >> hid_map_usage(hi, usage, bit, max, >> EV_ABS, ABS_MT_TOUCH_MAJOR); >> + set_abs(hi->input, ABS_MT_TOUCH_MAJOR, field, >> + cls->sn_width); >> td->last_slot_field = usage->hid; >> return 1; >> case HID_DG_HEIGHT: >> hid_map_usage(hi, usage, bit, max, >> EV_ABS, ABS_MT_TOUCH_MINOR); >> + set_abs(hi->input, ABS_MT_TOUCH_MINOR, field, >> + cls->sn_height); >> field->logical_maximum = 1; >> field->logical_minimum = 0; > > These limits are not right - I doubt they are for any device. I was a little surprise too while looking at these. But This is not related to ABS_MT_TOUCH_MINOR, but ABS_MT_ORIENTATION. And if I'm forced to modify those values this way it's because _you_ made me introduce the set_abs function which takes in parameter the field. Thus, it's more complicated to introduce new fields and usages. > >> set_abs(hi->input, ABS_MT_ORIENTATION, field, 0); >> @@ -307,11 +325,18 @@ static void mt_emit_event(struct mt_device *td, struct input_dev *input) >> input_mt_report_slot_state(input, MT_TOOL_FINGER, >> s->touch_state); >> if (s->touch_state) { >> + /* this finger is on the screen */ >> + int wide = (s->w > s->h); >> + /* divided by two to match visual scale of touch */ >> + int major = max(s->w, s->h) >> 1; >> + int minor = min(s->w, s->h) >> 1; >> + >> input_event(input, EV_ABS, ABS_MT_POSITION_X, s->x); >> input_event(input, EV_ABS, ABS_MT_POSITION_Y, s->y); >> + input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide); >> input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p); >> - input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, s->w); >> - input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, s->h); >> + input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, major); >> + input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, minor); >> } >> s->seen_in_this_frame = false; >> >> @@ -481,6 +506,14 @@ static void mt_remove(struct hid_device *hdev) >> >> static const struct hid_device_id mt_devices[] = { >> >> + /* 3M panels */ >> + { .driver_data = MT_CLS_3M, >> + HID_USB_DEVICE(USB_VENDOR_ID_3M, >> + USB_DEVICE_ID_3M1968) }, >> + { .driver_data = MT_CLS_3M, >> + HID_USB_DEVICE(USB_VENDOR_ID_3M, >> + USB_DEVICE_ID_3M2256) }, >> + >> /* Cando panels */ >> { .driver_data = MT_CLS_DUAL_INRANGE_CONTACTNUMBER, >> HID_USB_DEVICE(USB_VENDOR_ID_CANDO, >> -- >> 1.7.4 >> > > Also, this line needs to be added in case no feature reports are sent: > > @@ -481,6 +481,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id) > return -ENOMEM; > } > td->mtclass = mtclass; > + td->maxcontacts = mtclass->maxcontacts; > td->inputmode = -1; > hid_set_drvdata(hdev, td); > So: 1) If a device does not send the feature report related to maxcontact, then it won't pass the Win7 certification, then we will need to write a special driver for it. 2) This is wrong because I do not want to add in all those classes the field maxcontact. So I'll revert the MT_DEFAULT_MAXCONTACT, and I will add this sort of line just before allocating the slots. Oh, and I'll remove the .maxcontact = 2 for MT_CLS_DEFAULT. Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html