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