> > @@ -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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
