Re: [PATCH] hid-multitouch: changes from the review process

[Benjamin Tissoires <benjamin.tissoires@enac.fr>]

On Tue, Jan 11, 2011 at 2:11 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
> Hi Benjamin,
>
> seems we are cloing in now, although there is still an outstanding
> issue with the setting of curvalid, as replied in your previous
> mail. Some comments on this patch, too.
>
>>  struct mt_class mt_classes[] = {
>> -     { 0, 0, 0, 10 },                             /* MT_CLS_DEFAULT */
>> -     { MT_QUIRK_SLOT_IS_CONTACTID, 0, 0, 2 },     /* MT_CLS_DUAL1 */
>> -     { MT_QUIRK_SLOT_IS_CONTACTNUMBER, 0, 0, 10 },    /* MT_CLS_DUAL2 */
>> -     { MT_QUIRK_CYPRESS | MT_QUIRK_NOT_SEEN_MEANS_UP, 0, 0, 10 }, /* MT_CLS_CYPRESS */
>> +     { .name = MT_CLS_DEFAULT,
>> +             .quirks = 0,
>
> Please do not zero-initialize.
>
>> +             .sn_move = 0,
>> +             .sn_pressure = 0,
>> +             .maxcontacts = 10 },
>> +     { .name = MT_CLS_DUAL1,
>> +             .quirks = MT_QUIRK_SLOT_IS_CONTACTID,
>> +             .sn_move = 0,
>> +             .sn_pressure = 0,
>> +             .maxcontacts = 2 },
>> +     { .name = MT_CLS_DUAL2,
>> +             .quirks = MT_QUIRK_SLOT_IS_CONTACTNUMBER,
>> +             .sn_move = 0,
>> +             .sn_pressure = 0,
>> +             .maxcontacts = 2 },
>> +     { .name = MT_CLS_CYPRESS,
>> +             .quirks = MT_QUIRK_CYPRESS,
>> +             .sn_move = 0,
>> +             .sn_pressure = 0,
>> +             .maxcontacts = 10 },
>> +
>> +     { }
>>  };
>
> So no device is marked as NOT_SEEN_MEANS_UP, although allegedly some devices should...

Well, my fault: I was sure that Cypress devices need the
NOT_SEEN_MEANS_UP, but in fact, it was another one. We (with Stephane)
don't recall it at the moment, but Stephane already saw some devices
that required this quirk.

mea culpa

>
>> @@ -282,11 +297,10 @@ static void mt_emit_event(struct mt_device *td, struct input_dev *input)
>>
>>       for (i = 0; i < td->mtclass->maxcontacts; ++i) {
>>               struct mt_slot *s = &(td->slots[i]);
>> -             if ((td->mtclass->quirks & MT_QUIRK_NOT_SEEN_MEANS_UP) &&
>> -                     !s->seen_in_this_frame) {
>> +             if (!s->seen_in_this_frame) {
>>                       /*
>> -                      * this slot does not contain useful data,
>> -                      * notify its closure
>> +                      * FixMe: use MT_QUIRK_NOT_SEEN_MEANS_UP here.
>> +                      * This requires to change the curvalid logic.
>>                        */
>>                       s->touch_state = false;
>>               }
>
> If we were to push this broken behavior, simply to avoid changing the
> currently tested code, we would only push the problem of testing onto
> the next cycle, with a larger risk of introducing regressions. I
> really think we need to sort this out now.

I'll do the quirk way

>
>> @@ -392,9 +407,16 @@ static void mt_set_input_mode(struct hid_device *hdev)
>>
>>  static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>  {
>> -     int ret;
>> +     int ret, i;
>>       struct mt_device *td;
>> -     struct mt_class *mtclass = mt_classes + id->driver_data;
>> +     struct mt_class *mtclass = mt_classes; /* MT_CLS_DEFAULT */
>> +
>> +     for (i = 0; mt_classes[i].name ; i++) {
>> +             if (id->driver_data == mt_classes[i].name) {
>> +                     mtclass = &(mt_classes[i]);
>> +                     break;
>> +             }
>> +     }
>
> Nice.
>
Thanks,
Benjamin
--
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