Re: [PATCH v2 3/9] HID: multitouch: add support for Nexio 42" panel

["Henrik Rydberg" <rydberg@euromail.se>]

Hi Benjamin,

> > Why not an index here?
> 
> Just because an index is not sufficient. You need two things: an index
> within the field, and the actual field (a pointer to a struct
> hid_field). Each .value is local to a field, and even if in most of
> the case, the contact count is alone in its field, it would mean to
> take the risk that a new device does not follow this logic.

The field value is passed to process_mt_event() in a fairly
straight-forward fashion, I was imagining that behavior could be
copied somehow.

> So the actual pointer to the contact count value seemed to be the
> shortest way to do it. But it can be easily changed.

Keeping a pointer into the core structure creates unwanted
dependencies to the scope of that value, making an eventual core
refactoring even harder, not to mention trickier to debug. So even
though it looks neat in the code, it pushes the problem forward.

> >> @@ -251,6 +257,9 @@ static ssize_t mt_set_quirks(struct device *dev,
> >>
> >>       td->mtclass.quirks = val;
> >>
> >> +     if (!td->contactcount)
> >> +             td->mtclass.quirks &= ~MT_QUIRK_CONTACT_CNT_ACCURATE;
> >> +
> >
> > Why override the overrider here?
> 
> This callback is called from the user-space through the sysfs
> attribute. So, it is not called in the same time that the
> mt_post_parse function. This is just to avoid a user setting this
> quirk once the device is up and running leading to a potential oops.

Yes, but the quirk _is_ user modifiable. Hence, the problem lies in
equating the user-modifiable quirk with the branch control of the
program.

> > An index into the the struct actually passed in mt_report() feels safer.
> 
> again, you need to store "field" and "usage->usage_index". Agree, it
> would be safer but it will take more space... :)

If you think the code change is not only correct, but also moves the
whole code base in a good direction, by all means.

> >> @@ -750,11 +765,15 @@ static void mt_post_parse_default_settings(struct mt_device *td)
> >>  static void mt_post_parse(struct mt_device *td)
> >>  {
> >>       struct mt_fields *f = td->fields;
> >> +     struct mt_class *cls = &td->mtclass;
> >>
> >>       if (td->touches_by_report > 0) {
> >>               int field_count_per_touch = f->length / td->touches_by_report;
> >>               td->last_slot_field = f->usages[field_count_per_touch - 1];
> >>       }
> >> +
> >> +     if (!td->contactcount)
> >> +             cls->quirks &= ~MT_QUIRK_CONTACT_CNT_ACCURATE;
> >
> > Since MT_QUIRK_CONTACT_CNT_ACCURATE is a quirk, modifiable by the
> > user, it should probably not validate num_expected in the code. Better
> > use the contact count index or something equivalent for that.
> 
> As when the user changes the quirk, we validate it, this is not required.

True, barring the comments above.

Thanks,
Henrik