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

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

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