Re: [PATCH v2 4/4] hid-multitouch: migrate 3M PCT touch screens to hid-multitouch
From: Benjamin Tissoires
Date: Fri Mar 11 2011 - 02:26:47 EST
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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/