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

quoted

quoted

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.

The point is that this is not generally needed, but necessitated by a
bug in the firmware. Problems like this is why we have quirks.

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.

The comment here was about using specific code paths enabled by quirks.

quoted

quoted

quoted

               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.

Fine. Since we will always want the pressure to have zero intercept, I
think it is ok to always execute that line, but I still think it is
better to leave it conditional.

quoted

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?

Maybe at bit long, I guess you aren't really serious.

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