lkml.org 
[lkml]   [2011]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 4/4] hid-multitouch: migrate 3M PCT touch screens to hid-multitouch
From
Hi Henrik,

On Thu, Mar 10, 2011 at 16:52, Henrik Rydberg <rydberg@euromail.se> 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@gmail.com>
>>   *  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@enac.fr>
>> + *    Copyright (c) 2010      Henrik Rydberg <rydberg@euromail.se>
>> + *    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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2011-03-11 08:29    [from the cache]
©2003-2011 Jasper Spaans