Two small comments below. Am 27.10.2014 um 10:40 schrieb Hans de Goede: > Hi Matthias, > > On 10/25/2014 02:01 PM, Mathias Gottschlag wrote: >> Most of the protocol for these touchpads has been reverse engineered. This >> commit adds a basic multitouch-capable driver. >> >> A lot of the protocol is still unknown. Especially, we don't know how to >> identify the device yet apart from the PNP ID. The code currently compares >> known good values with all data from the touchpad which could be usable for >> identification, so by design even minor variations will cause the driver >> to fail to detect the device and fall back to compatibility mode. >> >> The previous workaround for these devices has been left in place in case >> the driver is not compiled into the kernel or in case some other device >> with the same PNP ID is not recognized by the driver yet still has the same >> problems with the device probing code. >> --- >> >> Hi, >> >> this patch is related to the following bugs: >> https://bugzilla.redhat.com/show_bug.cgi?id=1110011 >> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1372609 >> >> It has been tested on the Asus UX303LN, UX303LA and K750L laptops. The >> latter is affected by bug 1110011 above and used to require additional >> flags before the touchpad would work. This behaviour should have been >> fixed by the previous FocalTech patch, but is still present after this >> patch. I have not yet gotten any feedback yet, but my theory is that the >> original patch failed to fix the kernel on that device. I have to admit >> though that I haven't 100% understood that patch. >> >> When reviewing, keep in mind that this is my first linux patch. I am >> using the code for quite some time now without issues, but I might have >> overlooked quite some problems. > > First of all many thanks for working on this. > > First one overall comment on the patch, I don't really like the new > detect function you are adding, and I don't see a need for it, pnp-id > detection is what windows uses, and it should be 100% reliable (famous last > words). So I would prefer to stick with with only the pnp-id detection. > > I do agree that keeping the pnp-id detection (and falling back to bare) even > when focaltech support is not compiled in is something which we should do. > > Note that likewise specifying psmouse.proto=bare should runtime disable > your new code if compiled in so the detection blurb in psmouse-base.c should > look something like this: > > /* Always check for focaltech, this is safe as it uses pnp-id matching */ > if (psmouse_do_detect(focaltech_detect, psmouse, set_properties) == 0) { > if (max_proto > PSMOUSE_IMEX) { > if (!set_properties || focaltech_init(psmouse) == 0) { > if (focaltech_supported()) > return PSMOUSE_FOCALTECH; > /* > * Note that we need to also restrict > * psmouse_max_proto so that psmouse_initialize() > * does not try to reset rate and resolution, > * because even that upsets the device. > */ > psmouse_max_proto = PSMOUSE_PS2; > return PSMOUSE_PS2; > } > } > } > > Where focaltech_supported() is a simple: > > #ifdef CONFIG_MOUSE_PS2_FOCALTECH > bool focaltech_supported(void) { > return true; > } > #else > bool focaltech_supported(void) { > return false; > } > #endif > > in focaltech.c (see synaptics.c, which defines synaptics_supported() in the > same way. > > > > > >> >> Regards, >> Mathias >> >> drivers/input/mouse/Kconfig | 10 ++ >> drivers/input/mouse/focaltech.c | 326 >> ++++++++++++++++++++++++++++++++++++- >> drivers/input/mouse/focaltech.h | 61 +++++++ >> drivers/input/mouse/psmouse-base.c | 15 ++ >> drivers/input/mouse/psmouse.h | 1 + >> 5 files changed, 406 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/input/mouse/Kconfig b/drivers/input/mouse/Kconfig >> index 366fc7a..db973e5 100644 >> --- a/drivers/input/mouse/Kconfig >> +++ b/drivers/input/mouse/Kconfig >> @@ -146,6 +146,16 @@ config MOUSE_PS2_OLPC >> If unsure, say N. >> +config MOUSE_PS2_FOCALTECH >> + bool "FocalTech PS/2 mouse protocol extension" if EXPERT >> + default y >> + depends on MOUSE_PS2 >> + help >> + Say Y here if you have a FocalTech PS/2 TouchPad connected to >> + your system. >> + >> + If unsure, say Y. >> + >> config MOUSE_SERIAL >> tristate "Serial mouse" >> select SERIO >> diff --git a/drivers/input/mouse/focaltech.c >> b/drivers/input/mouse/focaltech.c >> index f4d657e..ada97cc 100644 >> --- a/drivers/input/mouse/focaltech.c >> +++ b/drivers/input/mouse/focaltech.c >> @@ -2,6 +2,7 @@ >> * Focaltech TouchPad PS/2 mouse driver >> * >> * Copyright (c) 2014 Red Hat Inc. >> + * Copyright (c) 2014 Mathias Gottschlag <mgottschlag@xxxxxxxxx> >> * >> * This program is free software; you can redistribute it and/or modify >> * it under the terms of the GNU General Public License as published by >> @@ -13,15 +14,14 @@ >> * Hans de Goede <hdegoede@xxxxxxxxxx> >> */ >> -/* >> - * The Focaltech PS/2 touchpad protocol is unknown. This drivers deals with >> - * detection only, to avoid further detection attempts confusing the >> touchpad >> - * this way it at least works in PS/2 mouse compatibility mode. >> - */ >> #include <linux/device.h> >> #include <linux/libps2.h> >> +#include <linux/input/mt.h> >> +#include <linux/serio.h> >> +#include <linux/slab.h> >> #include "psmouse.h" >> +#include "focaltech.h" >> static const char * const focaltech_pnp_ids[] = { >> "FLT0101", >> @@ -30,7 +30,14 @@ static const char * const focaltech_pnp_ids[] = { >> NULL >> }; >> -int focaltech_detect(struct psmouse *psmouse, bool set_properties) >> +/* >> + * Even if the kernel is built without support for Focaltech PS/2 >> touchpads (or >> + * when the real driver fails to recognize the device), we still have >> to detect >> + * them in order to avoid further detection attempts confusing the >> touchpad. >> + * This way it at least works in PS/2 mouse compatibility mode. >> + */ >> + >> +int focaltech_detect_fallback(struct psmouse *psmouse, bool set_properties) >> { >> if (!psmouse_matches_pnp_id(psmouse, focaltech_pnp_ids)) >> return -ENODEV; >> @@ -43,10 +50,315 @@ int focaltech_detect(struct psmouse *psmouse, bool >> set_properties) >> return 0; >> } >> -int focaltech_init(struct psmouse *psmouse) >> +int focaltech_init_fallback(struct psmouse *psmouse) >> +{ >> + ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_RESET_DIS); >> + psmouse_reset(psmouse); >> + >> + return 0; >> +} >> + > > As discussed please keep this named focaltech_init, and make this version > #ifndef CONFIG_MOUSE_PS2_FOCALTECH (or put it in the #else block. > > Also you may want to make this use the newly defined focaltech_reset() > function (which would then be outside of #ifdef CONFIG_MOUSE_PS2_FOCALTECH) > >> +#ifdef CONFIG_MOUSE_PS2_FOCALTECH >> + >> +static int focaltech_read_register(struct ps2dev *ps2dev, int reg, >> + unsigned char *param) >> +{ >> + if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETSCALE11)) >> + return -1; >> + param[0] = 0; >> + if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES)) >> + return -1; >> + if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES)) >> + return -1; >> + if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES)) >> + return -1; >> + param[0] = reg; >> + if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES)) >> + return -1; >> + if (ps2_command(ps2dev, param, PSMOUSE_CMD_GETINFO)) >> + return -1; >> + return 0; >> +} >> + > > If you drop the register contents check, you can drop this as well, > may be useful for future reference though, so may make it #if 0 ... #endif. > Dmitry ? I've left this one in, I think my comment about the touchpad size below was correct. > > > >> +int focaltech_detect(struct psmouse *psmouse, bool set_properties) >> +{ >> + struct ps2dev *ps2dev = &psmouse->ps2dev; >> + unsigned char param[3]; >> + >> + if (!psmouse_matches_pnp_id(psmouse, focaltech_pnp_ids)) >> + return -ENODEV; >> + >> + /* >> + * We don't know how to identify the device, so we just check the >> + * content of all registers. >> + */ >> + if (focaltech_read_register(ps2dev, 0, param)) >> + return -EIO; >> + if (param[0] != 0x69 || param[1] != 0x80 || param[2] != 0x80) >> + return -ENODEV; >> + if (focaltech_read_register(ps2dev, 1, param)) >> + return -EIO; >> + if (param[0] != 0x36 || param[1] != 0x53 || param[2] != 0x03) >> + return -ENODEV; >> + if (focaltech_read_register(ps2dev, 2, param)) >> + return -EIO; >> + /* 0x13 and 0x0d might be the touchpad size? */ >> + if (param[0] != 0x00 || param[1] != 0x13 || param[2] != 0x0d) >> + return -ENODEV; >> + if (focaltech_read_register(ps2dev, 5, param)) >> + return -EIO; >> + if (param[0] != 0x0b || param[1] != 0x03 || param[2] != 0x00) >> + return -ENODEV; >> + if (focaltech_read_register(ps2dev, 6, param)) >> + return -EIO; >> + if (param[0] != 0x23 || param[1] != 0xbd || param[2] != 0xf8) >> + return -ENODEV; >> + >> + if (set_properties) { >> + psmouse->vendor = "FocalTech"; >> + psmouse->name = "FocalTech Touchpad"; >> + } >> + >> + return 0; >> +} >> + >> +static void focaltech_report_state(struct psmouse *psmouse) >> +{ >> + int i; >> + struct focaltech_data *priv = psmouse->private; >> + struct focaltech_hw_state *state = &priv->state; >> + struct input_dev *dev = psmouse->dev; >> + int finger_count = 0; >> + >> + for (i = 0; i < FOC_MAX_FINGERS; i++) { >> + struct focaltech_finger_state *finger = &state->fingers[i]; >> + int active = finger->active && finger->valid; >> + input_mt_slot(dev, i); >> + input_mt_report_slot_state(dev, MT_TOOL_FINGER, active); >> + if (active) { >> + finger_count++; >> + input_report_abs(dev, ABS_MT_POSITION_X, finger->x); >> + input_report_abs(dev, ABS_MT_POSITION_Y, >> + focaltech_invert_y(finger->y)); >> + } >> + } >> + input_mt_report_pointer_emulation(dev, false); >> + input_mt_report_finger_count(dev, finger_count); > > These 2 lines should be replace with: > input_mt_report_pointer_emulation(dev, finger_count); > > So that BTN_TOUCH properly gets set when at least one finger is down (this is > mandatory when not reporting pressure). > > Also I see that you are not using input_mt_assign_slots, so I take it that > if put 2 fingers down and then lift the first finger down, the second finger > stays in slot->fingers[1] ? Because if it does not then you need to use > input_mt_assign_slots. Yes, the touchpad keeps track of which touch corresponds to which finger. > > >> + >> + input_report_key(psmouse->dev, BTN_LEFT, state->pressed); >> + input_sync(psmouse->dev); >> +} >> + >> +static void process_touch_packet(struct focaltech_hw_state *state, >> + unsigned char *packet) >> +{ >> + int i; >> + unsigned char fingers = packet[1]; >> + >> + state->pressed = (packet[0] >> 4) & 1; >> + /* the second byte contains a bitmap of all fingers touching the pad */ >> + for (i = 0; i < FOC_MAX_FINGERS; i++) { >> + if ((fingers & 0x1) && !state->fingers[i].active) { >> + /* we do not have a valid position for the finger yet */ >> + state->fingers[i].valid = 0; >> + } >> + state->fingers[i].active = fingers & 0x1; >> + fingers >>= 1; >> + } >> +} >> + >> +static void process_abs_packet(struct focaltech_hw_state *state, >> + unsigned char *packet) >> +{ >> + unsigned int finger = (packet[1] >> 4) - 1; >> + >> + state->pressed = (packet[0] >> 4) & 1; >> + if (finger >= FOC_MAX_FINGERS) >> + return; >> + /* >> + * packet[5] contains some kind of tool size in the most significant >> + * nibble. 0xff is a special value (latching) that signals a large >> + * contact area. >> + */ >> + if (packet[5] == 0xff) >> + return; > > Maybe set state->fingers[finger].valid = 0 here ? > >> + state->fingers[finger].x = ((packet[1] & 0xf) << 8) | packet[2]; >> + state->fingers[finger].y = (packet[3] << 8) | packet[4]; >> + state->fingers[finger].valid = 1; >> +} >> +static void process_rel_packet(struct focaltech_hw_state *state, >> + unsigned char *packet) >> +{ >> + int finger1 = ((packet[0] >> 4) & 0x7) - 1; >> + int finger2 = ((packet[3] >> 4) & 0x7) - 1; >> + >> + state->pressed = packet[0] >> 7; >> + if (finger1 < FOC_MAX_FINGERS) { >> + state->fingers[finger1].x += (char)packet[1]; >> + state->fingers[finger1].y += (char)packet[2]; >> + } >> + /* >> + * If there is an odd number of fingers, the last relative packet only >> + * contains one finger. In this case, the second finger index in the >> + * packet is 0 (we subtract 1 in the lines above to create array >> + * indices). >> + */ >> + if (finger2 != -1 && finger2 < FOC_MAX_FINGERS) { >> + state->fingers[finger2].x += (char)packet[4]; >> + state->fingers[finger2].y += (char)packet[5]; >> + } >> +} >> + >> +static void focaltech_process_packet(struct psmouse *psmouse) >> +{ >> + struct focaltech_data *priv = psmouse->private; >> + unsigned char *packet = psmouse->packet; >> + >> + switch (packet[0] & 0xf) { >> + case FOC_TOUCH: >> + process_touch_packet(&priv->state, packet); >> + break; >> + case FOC_ABS: >> + process_abs_packet(&priv->state, packet); >> + break; >> + case FOC_REL: >> + process_rel_packet(&priv->state, packet); >> + break; >> + default: >> + psmouse_err(psmouse, "Unknown packet type: %02x", packet[0]); >> + break; >> + } >> + >> + focaltech_report_state(psmouse); >> +} >> + >> +static psmouse_ret_t focaltech_process_byte(struct psmouse *psmouse) >> { >> + if (psmouse->pktcnt >= 6) { /* Full packet received */ >> + focaltech_process_packet(psmouse); >> + return PSMOUSE_FULL_PACKET; >> + } >> + /* >> + * we might want to do some validation of the data here, but we do not >> + * know the protocoll well enough >> + */ >> + return PSMOUSE_GOOD_DATA; >> +} >> + >> +static void focaltech_reset(struct psmouse *psmouse) >> +{ >> + psmouse_info(psmouse, "focaltech_reset"); >> ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_RESET_DIS); >> psmouse_reset(psmouse); >> +} >> + >> +static int focaltech_switch_protocol(struct psmouse *psmouse) >> +{ >> + struct ps2dev *ps2dev = &psmouse->ps2dev; >> + unsigned char param[3]; >> + >> + param[0] = 0; >> + if (ps2_command(ps2dev, param, 0x10f8)) >> + return -EIO; >> + if (ps2_command(ps2dev, param, 0x10f8)) >> + return -EIO; >> + if (ps2_command(ps2dev, param, 0x10f8)) >> + return -EIO; >> + param[0] = 1; >> + if (ps2_command(ps2dev, param, 0x10f8)) >> + return -EIO; >> + if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETSCALE11)) >> + return -EIO; >> + >> + if (ps2_command(ps2dev, param, PSMOUSE_CMD_ENABLE)) >> + return -EIO; >> return 0; >> } >> + >> +static void focaltech_disconnect(struct psmouse *psmouse) >> +{ >> + focaltech_reset(psmouse); >> + kfree(psmouse->private); >> + psmouse->private = NULL; >> +} >> + >> +static int focaltech_reconnect(struct psmouse *psmouse) >> +{ >> + focaltech_reset(psmouse); >> + if (focaltech_switch_protocol(psmouse)) { >> + psmouse_err(psmouse, >> + "Unable to initialize the device."); >> + return -1; >> + } >> + return 0; >> +} >> + >> +static void set_input_params(struct psmouse *psmouse) >> +{ >> + struct input_dev *dev = psmouse->dev; >> + >> + __set_bit(EV_KEY, dev->evbit); >> + __set_bit(EV_ABS, dev->evbit); >> + input_set_abs_params(dev, ABS_X, 0, FOC_MAX_X, 0, 0); >> + input_set_abs_params(dev, ABS_Y, 0, FOC_MAX_Y, 0, 0); >> + input_mt_init_slots(dev, 5, INPUT_MT_POINTER); >> + input_set_abs_params(dev, ABS_MT_POSITION_X, 0, FOC_MAX_X, 0, 0); >> + input_set_abs_params(dev, ABS_MT_POSITION_Y, 0, FOC_MAX_Y, 0, 0); >> + __set_bit(BTN_TOUCH, dev->keybit); >> + __set_bit(BTN_TOOL_FINGER, dev->keybit); >> + __set_bit(BTN_TOOL_DOUBLETAP, dev->keybit); >> + __set_bit(BTN_TOOL_TRIPLETAP, dev->keybit); >> + __set_bit(BTN_TOOL_QUADTAP, dev->keybit); >> + __set_bit(BTN_TOOL_QUINTTAP, dev->keybit); >> + __clear_bit(EV_REL, dev->evbit); >> + __clear_bit(REL_X, dev->relbit); >> + __clear_bit(REL_Y, dev->relbit); >> + __set_bit(BTN_LEFT, dev->keybit); >> + __clear_bit(BTN_RIGHT, dev->keybit); >> + __clear_bit(BTN_MIDDLE, dev->keybit); >> + __set_bit(INPUT_PROP_BUTTONPAD, dev->propbit); >> +} >> + >> +int focaltech_init(struct psmouse *psmouse) >> +{ >> + struct focaltech_data *priv; >> + >> + focaltech_reset(psmouse); >> + if (focaltech_switch_protocol(psmouse)) { >> + focaltech_reset(psmouse); >> + psmouse_info(psmouse, >> + "Unable to initialize the device."); >> + return -ENOSYS; >> + } >> + >> + psmouse->private = priv = kzalloc(sizeof(struct focaltech_data), >> GFP_KERNEL); >> + if (!priv) >> + return -ENOMEM; >> + >> + set_input_params(psmouse); >> + >> + psmouse->protocol_handler = focaltech_process_byte; >> + psmouse->pktsize = 6; >> + psmouse->disconnect = focaltech_disconnect; >> + psmouse->reconnect = focaltech_reconnect; >> + psmouse->cleanup = focaltech_reset; >> + /* resync is not supported yet */ >> + psmouse->resync_time = 0; >> + >> + return 0; >> +} >> + >> +#else /* CONFIG_MOUSE_PS2_FOCALTECH */ >> + >> +int focaltech_detect(struct psmouse *psmouse, bool set_properties) >> +{ >> + return -ENOSYS; >> +} >> + >> +int focaltech_init(struct psmouse *psmouse) >> +{ >> + return -ENOSYS; >> +} >> + >> +#endif /* CONFIG_MOUSE_PS2_FOCALTECH */ >> diff --git a/drivers/input/mouse/focaltech.h >> b/drivers/input/mouse/focaltech.h >> index 498650c..3c553b2 100644 >> --- a/drivers/input/mouse/focaltech.h >> +++ b/drivers/input/mouse/focaltech.h >> @@ -2,6 +2,7 @@ >> * Focaltech TouchPad PS/2 mouse driver >> * >> * Copyright (c) 2014 Red Hat Inc. >> + * Copyright (c) 2014 Mathias Gottschlag <mgottschlag@xxxxxxxxx> >> * >> * This program is free software; you can redistribute it and/or modify >> * it under the terms of the GNU General Public License as published by >> @@ -16,7 +17,67 @@ >> #ifndef _FOCALTECH_H >> #define _FOCALTECH_H >> +/* >> + * Packet types - the numbers are not consecutive, so we might be missing >> + * something here. >> + */ >> +#define FOC_TOUCH 0x3 /* bitmap of active fingers */ >> +#define FOC_ABS 0x6 /* absolute position of one finger */ >> +#define FOC_REL 0x9 /* relative position of 1-2 fingers */ >> + >> +#define FOC_MAX_FINGERS 5 >> + >> +#define FOC_MAX_X 2431 >> +#define FOC_MAX_Y 1663 >> + >> +static inline int focaltech_invert_y(int y) >> +{ >> + return FOC_MAX_Y - y; >> +} >> + >> +/* >> + * Current state of a single finger on the touchpad. >> + */ >> +struct focaltech_finger_state { >> + /* the touchpad has generated a touch event for the finger */ >> + bool active; >> + /* >> + * The touchpad has sent position data for the finger. Touch event >> + * packages reset this flag for new fingers, and there is a time >> + * between the first touch event and the following absolute position >> + * packet for the finger where the touchpad has declared the finger to >> + * be valid, but we do not have any valid position yet. >> + */ >> + bool valid; >> + /* absolute position (from the bottom left corner) of the finger */ >> + unsigned int x; >> + unsigned int y; >> +}; >> + >> +/* >> + * Description of the current state of the touchpad hardware. >> + */ >> +struct focaltech_hw_state { >> + /* >> + * The touchpad tracks the positions of the fingers for us, the array >> + * indices correspond to the finger indices returned in the report >> + * packages. >> + */ >> + struct focaltech_finger_state fingers[FOC_MAX_FINGERS]; >> + /* >> + * True if the clickpad has been pressed. >> + */ >> + bool pressed; >> +}; >> + >> +struct focaltech_data { >> + struct focaltech_hw_state state; >> +}; >> + >> int focaltech_detect(struct psmouse *psmouse, bool set_properties); >> int focaltech_init(struct psmouse *psmouse); >> +int focaltech_detect_fallback(struct psmouse *psmouse, bool >> set_properties); >> +int focaltech_init_fallback(struct psmouse *psmouse); >> + >> #endif >> diff --git a/drivers/input/mouse/psmouse-base.c >> b/drivers/input/mouse/psmouse-base.c >> index 26994f6..2e2b98d 100644 >> --- a/drivers/input/mouse/psmouse-base.c >> +++ b/drivers/input/mouse/psmouse-base.c >> @@ -726,6 +726,12 @@ static int psmouse_extensions(struct psmouse *psmouse, >> /* Always check for focaltech, this is safe as it uses pnp-id matching */ >> if (psmouse_do_detect(focaltech_detect, psmouse, set_properties) == 0) { >> if (!set_properties || focaltech_init(psmouse) == 0) { >> + return PSMOUSE_FOCALTECH; >> + } >> + } >> + if (psmouse_do_detect(focaltech_detect_fallback, >> + psmouse, set_properties) == 0) { >> + if (!set_properties || focaltech_init_fallback(psmouse) == 0) { >> /* >> * Not supported yet, use bare protocol. >> * Note that we need to also restrict >> @@ -1063,6 +1069,15 @@ static const struct psmouse_protocol >> psmouse_protocols[] = { >> .alias = "cortps", >> .detect = cortron_detect, >> }, >> +#ifdef CONFIG_MOUSE_PS2_FOCALTECH >> + { >> + .type = PSMOUSE_FOCALTECH, >> + .name = "FocalTechPS/2", >> + .alias = "focaltech", >> + .detect = focaltech_detect, >> + .init = focaltech_init, >> + }, >> +#endif >> { >> .type = PSMOUSE_AUTO, >> .name = "auto", >> diff --git a/drivers/input/mouse/psmouse.h b/drivers/input/mouse/psmouse.h >> index f4cf664..c2ff137 100644 >> --- a/drivers/input/mouse/psmouse.h >> +++ b/drivers/input/mouse/psmouse.h >> @@ -96,6 +96,7 @@ enum psmouse_type { >> PSMOUSE_FSP, >> PSMOUSE_SYNAPTICS_RELATIVE, >> PSMOUSE_CYPRESS, >> + PSMOUSE_FOCALTECH, >> PSMOUSE_AUTO /* This one should always be last */ >> }; > > Overall this looks very good, many thanks for your work on this! > > Regards, > > Hans > -- 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
