Re: [RFC v3 3/5] hid-multitouch: support for PixCir-based panels

[Henrik Rydberg]

> >> +static int mt_compute_slot(struct mt_device *td)
> >> +{
> >> +     struct mt_class *cls = td->mtclass;
> >> +
> >> +     if (cls->quirks & MT_QUIRK_SLOT_IS_CONTACTID)
> >> +             return slot_is_contactid(td);
> >
> > No real need for a function call here...
> 
> just to prepare for the other patches

I mean the line could simply read

	return td->curdata.contactid;

[...]
> >> +static int mt_event(struct hid_device *hid, struct hid_field *field,
> >> +                             struct hid_usage *usage, __s32 value)
> >> +{
> >> +     struct mt_device *td = hid_get_drvdata(hid);
> >> +
> >> +     if (hid->claimed & HID_CLAIMED_INPUT) {
> >> +             switch (usage->hid) {
> >> +             case HID_DG_INRANGE:
> >> +                     break;
> >> +             case HID_DG_TIPSWITCH:
> >> +                     td->curvalid = value;
> >
> > I do not think curvalid should depend on the touch state (which is
> > what tipswitch is). Either move to INRANGE, or simply set to true.
> 
> INRANGE is not all the time used as curvalid as you are saying.
> At least for Stantum, curvalid is given by CONFIDENCE.

Yes.

> The solution of giving the value true to curvalid is not working too:
> Cypress devices can send their touches over 2 reports of 7 touches,
> even if it handles 10 touches.

Yes.
 
> Solution:
> 1) td->curvalid = td->num_received < td->mtclass->maxcontacts;
> (not sure it will work for all devices)

For the ones where NOT_SEEN_MEANS_UP, it ought to be true. For egalax,
it is not true.

> 
> or 2) introduce MT_QUIRK_VALID_IS_INRANGE and MT_QUIRK_VALID_IS_CONFIDENCE

Yes, I believe this is a good way to go.
 
> or 3) let curvalid=value and disable the quirk
> MT_QUIRK_NOT_SEEN_MEANS_UP (adding a FixMe comment above)

This would mean setting curvalid = touch_state again, which according
to the discussion is precisely what we should not do. We know we need
to check whether the incoming packet represents data is whether it is
just filling up the transfer protocol size. We just need to identify,
per device, how that is done.

> >> +                     if (value)
> >> +                             td->num_expected = value - 1;
> >
> > The - 1 should be dropped here.
> 
> Here is the really tricky one:
> 
> if I drop -1,
> 
> the test below will be:
>  if (field->index == td->last_field_index
>       && td->num_received >= td->num_expected)
> 
> but during the init of the device, the kernel receive 1 event though
> the driver is not ready.

So the current code really needs to also check whether the driver is
ready to events. Adding a state for it would be appropriate. As it
stands, an unrelated syntatic oddity is covering up this fact.

> 
> During this time, num_expected is at 0, and when we receive the
> last_field_index: segfault!
> 
> To sum up, no, we can't.

Yes we can. :-)

> >> +     if (field->index == td->last_field_index
> >> +             && td->num_received > td->num_expected)
> >
> > A ">=" here.

still applies then.

Thanks,
Henrik
--
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/