On Tue, Jan 11, 2011 at 2:11 PM, Henrik Rydberg <rydberg@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
