Re: [PATCH v3] [input-hid] Add hid-egalax driver to the unified hid-multitouch framework.

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

> > @@ -147,11 +163,15 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> >  {
> >        struct mt_device *td = hid_get_drvdata(hdev);
> >        struct mt_class *cls = td->mtclass;
> > +       __s32 quirks = cls->quirks;
> > +
> >        switch (usage->hid & HID_USAGE_PAGE) {
> >
> >        case HID_UP_GENDESK:
> >                switch (usage->hid) {
> >                case HID_GD_X:
> > +                       if (quirks & MT_QUIRK_EGALAX_XYZ_FIXUP)
> > +                               field->logical_maximum = 32760;
> 
> Please do not add numerical constants in a generic code. If I want to
> override the values for another device, I'll have to rename your quirk
> to MT_QUIRK_EGALAX_XYZ_FIXUP_WITH_0_32760_0_32760_0_x and add mine.
> These values have to be moved in the MT_CLS.

It is a specific value used for a specific code path, no need to
over-generalize.  The quirk name could be specialized even more, i
suppose, but the point is that it makes the hid-multitouch driver do
exactly what is done in the hid-egalax driver, and it is easy to prove
that there are no side effects for other devices.

> >                case HID_DG_TIPPRESSURE:
> > +                       if (quirks & MT_QUIRK_EGALAX_XYZ_FIXUP)
> > +                               field->logical_minimum = 0;
> 
> No really need for the test and the quirk. In addition, here you do
> not override logical_max. This shows that the quirk is not
> MT_QUIRK_EGALAX_XYZ_FIXUP but the combination of
> MT_QUIRK_EGALAX_XY_FIXUP and  MT_QUIRK_EGALAX_Z_MIN_FIXUP.
> Just drop it here.

The added code path simply reflects what is in the hid-egalax
driver. It is true that the statement _probably_ works for other
devices as well, but it cannot be proven just by looking at the
code. Also, we have no known additional case where it is needed.

I am trying to think of a better name for the quirk, but frankly, I
think it already says what it needs to say.

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