On Wed, Mar 9, 2011 at 10:23, Henrik Rydberg <rydberg@xxxxxxxxxxx> wrote:
>> > @@ -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.
Or it's a generic code path for generic devices: if someone want to do
the same thing that have been done in hid-egalax (correct the logical
min/max reported by the device), this could be done.
Concerning the prove, I'm pretty sure we can manage to prove that both
solutions are equivalent as the current code already relies on the
fact that static struct are null initialized.
>
>> > 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 didn't told to remove the test just by looking at the code. I'm
taking in account all the devices that have been reported to us:
http://lii-enac.fr/en/architecture/linux-input/multitouch-devices.html
Among all of them, only asus, egalax (capacitive and resistive),
mosart (the same as asus), and stantum reports pressure.
Asus and mosart set only the logical maximum -> logical minimum is
therefore at 0.
Stantum does not set the min/max -> logical minimum is at 0 too.
EGalax set the minimum at 1 -> in your patch, you explained that was a
bug for the upper layers.
>
> I am trying to think of a better name for the quirk, but frankly, I
> think it already says what it needs to say.
What about MT_QUIRK_XYZ_CORRECTIONS_FOR_SPECIAL_EGALAX_DEVICES?
Cheers,
Benjamin
--
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
