lkml.org 
[lkml]   [2011]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] hid-multitouch: changes from the review process
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...

> @@ -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.

> @@ -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,
Henrik


\
 
 \ /
  Last update: 2011-01-11 14:15    [W:0.067 / U:1.216 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site