On 05/10/12(Fri) 20:40, Stefan Sperling wrote:
> On Fri, Oct 05, 2012 at 01:52:08PM +0200, Martin Pieuchot wrote:
> > My first impression at looking the code is that the v2 and v3 use
> > totally different packet formats and I don't see a real benefit of
> > having only one entry in the protocol table if you need to check
> > for the hardware version in every function.
> >
> > I would suggest to use 3 different entries in the protocol table and a
> > enable_elantech_common() function so you can get rid of the hw_version
> > check and the protocol->packetsize hack for the v1.
> >
> > I got the same feeling when you set the absolute mode: every version
> > use different magic ps2 commands with different magic values, I'm not
> > sure adding generic read/write functions help to understand nor save
> > some lines of code.
> >
> > Other than that, it looks to me that the flags ELANTECH_F_HW_V1_OLD and
> > ELANTECH_F_PARITY_REVERSED are redundant.
>
> All good points. And the parity flag was even wrongly defined...
>
> New version, hopefully better. Also fixes softc->elantech memory leak
> in error paths (which synaptics and alps have, too, btw.)
>
> Works for me with version 3, other versions still untested AFAIK.
I don't have any Elantech hardware to test but I can tell you that this
version doesn't introduce any regression with my ALPS touchpad. It could
be nice to hear from other touchpad owners too ;)
> +int
> +pms_ioctl_elantech(struct pms_softc *sc, u_long cmd, caddr_t data, int flag,
> + struct proc *p)
> +{
> + struct elantech_softc *elantech = sc->elantech;
> + struct wsmouse_calibcoords *wsmc = (struct wsmouse_calibcoords *)data;
> + int wsmode;
> +
> + switch (cmd) {
> + case WSMOUSEIO_GTYPE:
> + *(u_int *)data = WSMOUSE_TYPE_SYNAPTICS;
Here you may want to define a WSMOUSE_TYPE_ELANTECH, it is used in the
synaptics(4) driver to describe which "features" are supported by this
touchpad, see xenocara/driver/xf86-input-synaptics/src/wsconscomm.c
But if you do so, you'll obviously need to rebuild the X driver too ;)
> +void
> +pms_proc_elantech_v3(struct pms_softc *sc)
> +{
> + const u_char debounce_pkt[] = { 0xc4, 0xff, 0xff, 0x02, 0xff, 0xff };
> + struct elantech_softc *elantech = sc->elantech;
> + u_int buttons;
> + int fingers, x, y, w, z;
> +
> + /* The hardware sends this packet when in debounce state.
> + * The packet should be ignored. */
> + if (!memcmp(sc->packet, debounce_pkt, sizeof(debounce_pkt)))
> + return;
> +
> + buttons = ((sc->packet[0] & 0x01 ? WSMOUSE_BUTTON(1) : 0) |
> + ((sc->packet[0] & 0x02) ? WSMOUSE_BUTTON(3): 0));
> + x = y = z = 0;
> + w = -1; /* corresponds to no finger, see synaptics */
> + fingers = (sc->packet[0] & 0xc0) >> 6;
> + if (fingers == 2) {
> + /* Two-finger touch causes two packets -- a head packet
> + * and a tail packet. */
> + if ((sc->packet[0] & 0x0c) == 0x04 &&
> + (sc->packet[3] & 0xfc) == 0x02) {
> + /* head packet */
> + x = ((sc->packet[1] & 0x0f) << 8 | sc->packet[2]);
> + y = ((sc->packet[4] & 0x0f) << 8 | sc->packet[5]);
> + } else if ((sc->packet[0] & 0x0c) == 0x0c &&
> + (sc->packet[3] & 0xce) == 0x0c) {
> + /* tail packet */
> + x = ((sc->packet[1] & 0x0f) << 8 | sc->packet[2]);
> + y = ((sc->packet[4] & 0x0f) << 8 | sc->packet[5]);
Is it a typo or the 'x' and 'y' calculation are the same in both cases?
> + }
> + w = 0; /* force 2 fingers in synaptics */
This isn't related to your diff, but we should add defines for
w = {-1, 0, 3, 4} because these values come from the synaptics devices.
> + } else if (fingers == 1 || fingers == 3) {
> + x = (sc->packet[1] & 0x0f) << 8 | sc->packet[2];
> + y = ((sc->packet[4] & 0x0f) << 8) | sc->packet[5];
> + w = (fingers == 3 ? 1 : 4); /* values for synaptics */
> + }
Same here for the 'x' and 'y', maybe you just need to convert the value
you call 'fingers' to the corresponding 'w' (synaptics finger value).
> +void
> +elantech_send_input(struct pms_softc *sc, u_int buttons, int x, int y, int z,
> + int w)
> + {
> + struct elantech_softc *elantech = sc->elantech;
> + int dx, dy;
> +
> + if (elantech->wsmode == WSMOUSE_NATIVE) {
> + if ((x > 0 && y > 0) || buttons)
I think this condition is always true, or does the elantech touchpads
generate packets without position nor buttons pressed? Plus in case of
the v3 if x or y are null you explicitly force them to their previous
know position.
> +/* Hardware version 1 has hard-coded axis range values.
> + * X axis range is 0 to 576, Y axis range is 0 to 384.
> + * Edge offset accounts for bezel around the touchpad. */
> +#define ELANTECH_V1_EDGE_OFFSET 32
> +#define ELANTECH_V1_X_MIN (0 + ELANTECH_V1_EDGE_OFFSET)
> +#define ELANTECH_V1_X_MAX (576 - ELANTECH_V1_EDGE_OFFSET)
> +#define ELANTECH_V1_Y_MIN (0 + ELANTECH_V1_EDGE_OFFSET)
> +#define ELANTECH_V1_Y_MAX (384 - ELANTECH_V1_EDGE_OFFSET)
Some nitpicking, can you use just one space after "#define" to be
coherent with the rest of the file ;)
Apart from that, does your touchpad respond to the ELANTECH_QUE_FW_ID
query before being in absolute mode? Because if it does you can remove
the set_absolute_mode_vN() call from get_hwinfo_vN() and turn the latter
into a version independent function.
This would allow you to simplify the enable_elantech_vN() by sharing
the get_hwinfo() code and merging the set_absolute_mode_vN() code in it.
Martin
