Hi Dmitry: Thanks for your detailed revierw. On Thu, Jul 4, 2024 at 3:33 AM Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote: > > Hi Binbin, > > On Mon, Jul 01, 2024 at 05:49:53PM +0800, Binbin Zhou wrote: > > This patch introduces a driver for the PixArt PS/2 touchpad, which > > supports both clickpad and touchpad types. > > > > At the same time, we extended the single data packet length to 16, > > because according to the current PixArt hardware and FW design, we need > > 11 bytes/15 bytes to represent the complete three-finger/four-finger data. > > > > Co-developed-by: Jon Xie <jon_xie@xxxxxxxxxx> > > Signed-off-by: Jon Xie <jon_xie@xxxxxxxxxx> > > Co-developed-by: Jay Lee <jay_lee@xxxxxxxxxx> > > Signed-off-by: Jay Lee <jay_lee@xxxxxxxxxx> > > Signed-off-by: Binbin Zhou <zhoubinbin@xxxxxxxxxxx> > > A few more comments below... > > > --- > > V3: > > - Rebased on input/next; > > - Added comment to msleep() in pixart_reset() as suggested by Aseda, no > > functional change, thanks. > > > > Link to V2: > > https://lore.kernel.org/all/20240624065359.2985060-1-zhoubinbin@xxxxxxxxxxx/ > > > > V2: > > - Rebased on input/next; > > - Merge two patches from the V1 patchset; > > - Initialize local variables to prevent random garbage; > > - Remove some noisy debug message; > > - Check ps2_command() return value; > > - Use macros to represent bit operations for better readability, such > > as abs_x; > > - Remove the code related to rel packets, for the normal > > intellimouse detection is well in PixArt. > > > > Link to V1: > > https://lore.kernel.org/all/cover.1715224143.git.zhoubinbin@xxxxxxxxxxx/ > > > > drivers/input/mouse/Kconfig | 12 ++ > > drivers/input/mouse/Makefile | 1 + > > drivers/input/mouse/pixart_ps2.c | 269 +++++++++++++++++++++++++++++ > > drivers/input/mouse/pixart_ps2.h | 36 ++++ > > drivers/input/mouse/psmouse-base.c | 17 ++ > > drivers/input/mouse/psmouse.h | 3 +- > > 6 files changed, 337 insertions(+), 1 deletion(-) > > create mode 100644 drivers/input/mouse/pixart_ps2.c > > create mode 100644 drivers/input/mouse/pixart_ps2.h > > > > diff --git a/drivers/input/mouse/Kconfig b/drivers/input/mouse/Kconfig > > index 833b643f0616..8a27a20d04b0 100644 > > --- a/drivers/input/mouse/Kconfig > > +++ b/drivers/input/mouse/Kconfig > > @@ -69,6 +69,18 @@ config MOUSE_PS2_LOGIPS2PP > > > > If unsure, say Y. > > > > +config MOUSE_PS2_PIXART > > + bool "PixArt PS/2 touchpad protocol extension" if EXPERT > > + default y > > + depends on MOUSE_PS2 > > + help > > + This driver supports the PixArt PS/2 touchpad found in some > > + laptops. > > + Say Y here if you have a PixArt PS/2 TouchPad connected to > > + your system. > > + > > + If unsure, say Y. > > + > > config MOUSE_PS2_SYNAPTICS > > bool "Synaptics PS/2 mouse protocol extension" if EXPERT > > default y > > diff --git a/drivers/input/mouse/Makefile b/drivers/input/mouse/Makefile > > index a1336d5bee6f..563029551529 100644 > > --- a/drivers/input/mouse/Makefile > > +++ b/drivers/input/mouse/Makefile > > @@ -32,6 +32,7 @@ psmouse-$(CONFIG_MOUSE_PS2_ELANTECH) += elantech.o > > psmouse-$(CONFIG_MOUSE_PS2_OLPC) += hgpk.o > > psmouse-$(CONFIG_MOUSE_PS2_LOGIPS2PP) += logips2pp.o > > psmouse-$(CONFIG_MOUSE_PS2_LIFEBOOK) += lifebook.o > > +psmouse-$(CONFIG_MOUSE_PS2_PIXART) += pixart_ps2.o > > psmouse-$(CONFIG_MOUSE_PS2_SENTELIC) += sentelic.o > > psmouse-$(CONFIG_MOUSE_PS2_TRACKPOINT) += trackpoint.o > > psmouse-$(CONFIG_MOUSE_PS2_TOUCHKIT) += touchkit_ps2.o > > diff --git a/drivers/input/mouse/pixart_ps2.c b/drivers/input/mouse/pixart_ps2.c > > new file mode 100644 > > index 000000000000..a089beb2b00e > > --- /dev/null > > +++ b/drivers/input/mouse/pixart_ps2.c > > @@ -0,0 +1,269 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Pixart Touchpad Controller 1336U PS2 driver > > + * > > + * Author: Jon Xie <jon_xie@xxxxxxxxxx> > > + * Jay Lee <jay_lee@xxxxxxxxxx> > > + * Further cleanup and restructuring by: > > + * Binbin Zhou <zhoubinbin@xxxxxxxxxxx> > > + * > > + * Copyright (C) 2021-2024 Pixart Imaging. > > + * Copyright (C) 2024 Loongson Technology Corporation Limited. > > + * > > + */ > > + > > +#include <linux/delay.h> > > +#include <linux/device.h> > > +#include <linux/libps2.h> > > +#include <linux/input/mt.h> > > +#include <linux/serio.h> > > +#include <linux/slab.h> > > + > > +#include "pixart_ps2.h" > > + > > +static int pixart_read_tp_mode(struct psmouse *psmouse) > > Maybe make it > > static int pixart_read_tp_mode(struct ps2dev *ps2dev, u8 *mode) > > to separate mode value and errors/success? Yes, that would be much more clearer. > > > +{ > > + struct ps2dev *ps2dev = &psmouse->ps2dev; > > + u8 param[1] = { 0 }; > > + > > + if (ps2_command(ps2dev, param, PIXART_CMD_REPORT_FORMAT)) > > + return -EIO; > > Just return what ps2_command() reported, no need to change everything to > -EIO. > OK. > > + > > + return (param[0] == 1) ? PIXART_MODE_ABS : PIXART_MODE_REL; > > +} > > + > > +static int pixart_read_tp_type(struct psmouse *psmouse) > > Same here: > > static int pixart_read_tp_type(struct ps2dev *ps2dev, u8 *type) > > > +{ > > + struct ps2dev *ps2dev = &psmouse->ps2dev; > > + u8 param[3] = { 0 }; > > + > > + param[0] = 0x0a; > > + if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRATE)) > > + return -EIO; > > Same here and below, do not clobber with -EIO. > > > + > > + param[0] = 0x0; > > + if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES)) > > + return -EIO; > > + if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES)) > > + return -EIO; > > + if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES)) > > + return -EIO; > > + > > + param[0] = 0x03; > > + if (ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES)) > > + return -EIO; > > + > > + if (ps2_command(ps2dev, param, PSMOUSE_CMD_GETINFO)) > > + return -EIO; > > + > > + return (param[0] == 0x0e) ? PIXART_TYPE_TOUCHPAD : PIXART_TYPE_CLICKPAD; > > +} > > + > > +static void pixart_reset(struct psmouse *psmouse) > > +{ > > + ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_RESET_DIS); > > + > > + /* according to PixArt, 100ms is required for the upcoming reset */ > > + msleep(100); > > + psmouse_reset(psmouse); > > +} > > + > > +static void pixart_process_packet(struct psmouse *psmouse) > > +{ > > + struct pixart_data *priv = psmouse->private; > > + struct input_dev *dev = psmouse->dev; > > + int i, id, fingers = 0, abs_x, abs_y; > > + u8 *pkt = psmouse->packet; > > + u8 contact_cnt = CONTACT_CNT(pkt[0]); > > + bool tip; > > + > > + for (i = 0; i < contact_cnt; i++) { > > + id = SLOT_ID_MASK(pkt[3 * i + 3]); > > + abs_y = ABS_Y_MASK(pkt[3 * i + 3]) | pkt[3 * i + 1]; > > + abs_x = ABS_X_MASK(pkt[3 * i + 3]) | pkt[3 * i + 2]; > > + > > + if (i == PIXART_MAX_FINGERS - 1) > > + tip = pkt[14] & BIT(1); > > + else > > + tip = pkt[3 * contact_cnt + 1] & (0x01 << (2 * i + 1)); > > BIT(2 * i + 1)? > > > + > > + input_mt_slot(dev, id); > > + if (input_mt_report_slot_state(dev, MT_TOOL_FINGER, tip)) { > > + fingers++; > > + input_report_abs(dev, ABS_MT_POSITION_Y, abs_y); > > + input_report_abs(dev, ABS_MT_POSITION_X, abs_x); > > + } > > + } > > + > > + input_mt_sync_frame(dev); > > + input_mt_report_finger_count(dev, fingers); > > I believe if you pass INPUT_MT_POINTER flag to input_mt_init_slots() > below you will not need to call input_mt_report_finger_count() because > input_mt_sync_frame() will do it for you. Sorry, this is my fault, I didn't fully understand these APIs before. I will remove the redundant code. > > > + > > + if (priv->type == PIXART_TYPE_CLICKPAD) { > > + input_report_key(dev, BTN_LEFT, pkt[0] & 0x03); > > + } else { > > + input_report_key(dev, BTN_LEFT, pkt[0] & 0x01); > > + input_report_key(dev, BTN_RIGHT, pkt[0] & 0x02); > > + } > > + > > + input_sync(dev); > > +} > > + > > +static psmouse_ret_t pixart_protocol_handler(struct psmouse *psmouse) > > +{ > > + u8 *pkt = psmouse->packet; > > + u8 contact_cnt = CONTACT_CNT(pkt[0]); > > + > > + if (contact_cnt > PIXART_MAX_FINGERS || ((pkt[0] & 0x8c) != 0x80)) > > + return PSMOUSE_BAD_DATA; > > + > > + if (contact_cnt == PIXART_MAX_FINGERS && psmouse->pktcnt < psmouse->pktsize) > > + return PSMOUSE_GOOD_DATA; > > + > > + if (contact_cnt == 0 && psmouse->pktcnt < 5) > > + return PSMOUSE_GOOD_DATA; > > + > > + if (psmouse->pktcnt < (3 * contact_cnt + 2)) > > + return PSMOUSE_GOOD_DATA; > > + > > + pixart_process_packet(psmouse); > > + > > + return PSMOUSE_FULL_PACKET; > > +} > > + > > +static void pixart_disconnect(struct psmouse *psmouse) > > +{ > > + pixart_reset(psmouse); > > + kfree(psmouse->private); > > + psmouse->private = NULL; > > +} > > + > > +static int pixart_reconnect(struct psmouse *psmouse) > > +{ > > + int mode; > > + > > + pixart_reset(psmouse); > > + mode = pixart_read_tp_mode(psmouse); > > error = pixart_read_tp_mode(&psmouse->ps2dev, &mode); > if (error) > return error; > > > + if (mode != PIXART_MODE_ABS) > > + return mode; > return -ENODEV; > > + > > + if (ps2_command(&psmouse->ps2dev, NULL, PIXART_CMD_SWITCH_PROTO)) > > + return -EIO; > > error = ps2_command(&psmouse->ps2dev, NULL, PIXART_CMD_SWITCH_PROTO); > if (error) > return error; > > > + > > + return 0; > > +} > > + > > +static int pixart_set_input_params(struct input_dev *dev, struct pixart_data *priv) > > +{ > > + /* No relative support */ > > + __clear_bit(EV_REL, dev->evbit); > > + __clear_bit(REL_X, dev->relbit); > > + __clear_bit(REL_Y, dev->relbit); > > + __clear_bit(BTN_MIDDLE, dev->keybit); > > + > > + /* Buttons */ > > + __set_bit(EV_KEY, dev->evbit); > > + __set_bit(BTN_LEFT, dev->keybit); > > + if (priv->type == PIXART_TYPE_CLICKPAD) > > + __set_bit(INPUT_PROP_BUTTONPAD, dev->propbit); > > + else > > + __set_bit(BTN_RIGHT, dev->keybit); > > + > > + /* Touchpad */ > > + __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(INPUT_PROP_POINTER, dev->propbit); > > If you pass INPUT_MT_POINTER to input_mt_init_slots() then you will not > need to set any of these bits in the "touchpad" section. Yes, this is already done in input_mt_init_slots(). I will drop it. > > > + > > + /* Absolute position */ > > + __set_bit(EV_ABS, dev->evbit); > > No need to call __set_bit(EV_ABS, dev->evbit) when using > input_set_abs_params(). > OK, I will drop it. Thanks. Binbin > > > + input_set_abs_params(dev, ABS_X, 0, PIXART_PAD_WIDTH, 0, 0); > > + input_set_abs_params(dev, ABS_Y, 0, PIXART_PAD_HEIGHT, 0, 0); > > + > > + input_set_abs_params(dev, ABS_MT_POSITION_X, 0, PIXART_PAD_WIDTH, 0, 0); > > + input_set_abs_params(dev, ABS_MT_POSITION_Y, 0, PIXART_PAD_HEIGHT, 0, 0); > > + > > + return input_mt_init_slots(dev, PIXART_SLOTS_NUM, 0); > > +} > > + > > +static int pixart_query_hardware(struct psmouse *psmouse) > > +{ > > + struct pixart_data *priv = psmouse->private; > > + > > + priv->type = pixart_read_tp_type(psmouse); > > + if (priv->type < 0) > > + return -EIO; > > + > > + priv->mode = pixart_read_tp_mode(psmouse); > > + if (priv->mode < 0) > > + return -EIO; > > + > > + return 0; > > +} > > + > > +int pixart_detect(struct psmouse *psmouse, bool set_properties) > > +{ > > + int type; > > + > > + pixart_reset(psmouse); > > + type = pixart_read_tp_type(psmouse); > > + if (type < 0) > > + return -EIO; > > + > > + if (set_properties) { > > + psmouse->vendor = "PixArt"; > > + psmouse->name = (type == PIXART_TYPE_TOUCHPAD) ? > > + "touchpad" : "clickpad"; > > + } > > + > > + return 0; > > +} > > + > > +int pixart_init(struct psmouse *psmouse) > > +{ > > + struct pixart_data *priv; > > + > > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + psmouse->private = priv; > > + pixart_reset(psmouse); > > + > > + if (pixart_query_hardware(psmouse)) { > > + psmouse_err(psmouse, "Unable to query PixArt touchpad hardware.\n"); > > + goto err_exit; > > + } > > + > > + /* Relative mode follows standard PS/2 mouse protocol */ > > + if (priv->mode != PIXART_MODE_ABS) > > + goto err_exit; > > + > > + /* Set absolute mode */ > > + if (ps2_command(&psmouse->ps2dev, NULL, PIXART_CMD_SWITCH_PROTO)) { > > + psmouse_err(psmouse, "init: Unable to initialize PixArt absolute mode.\n"); > > + goto err_exit; > > + } > > + > > + if (pixart_set_input_params(psmouse->dev, priv) < 0) { > > + psmouse_err(psmouse, "init: Unable to set input params.\n"); > > + goto err_exit; > > + } > > + > > + psmouse->pktsize = 15; > > + psmouse->protocol_handler = pixart_protocol_handler; > > + psmouse->disconnect = pixart_disconnect; > > + psmouse->reconnect = pixart_reconnect; > > + psmouse->cleanup = pixart_reset; > > + /* resync is not supported yet */ > > + psmouse->resync_time = 0; > > + > > + return 0; > > + > > +err_exit: > > + pixart_reset(psmouse); > > + kfree(priv); > > + psmouse->private = NULL; > > + return -EIO; > > +} > > diff --git a/drivers/input/mouse/pixart_ps2.h b/drivers/input/mouse/pixart_ps2.h > > new file mode 100644 > > index 000000000000..f7f16cb9fbb0 > > --- /dev/null > > +++ b/drivers/input/mouse/pixart_ps2.h > > @@ -0,0 +1,36 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +#ifndef _PIXART_PS2_H > > +#define _PIXART_PS2_H > > + > > +#include "psmouse.h" > > + > > +#define PIXART_PAD_WIDTH 1023 > > +#define PIXART_PAD_HEIGHT 579 > > +#define PIXART_MAX_FINGERS 4 > > +#define PIXART_SLOTS_NUM PIXART_MAX_FINGERS > > + > > +#define PIXART_CMD_REPORT_FORMAT 0x01d8 > > +#define PIXART_CMD_SWITCH_PROTO 0x00de > > + > > +#define PIXART_MODE_REL 0 > > +#define PIXART_MODE_ABS 1 > > + > > +#define PIXART_TYPE_CLICKPAD 0 > > +#define PIXART_TYPE_TOUCHPAD 1 > > + > > +#define CONTACT_CNT(m) (((m) & GENMASK(6, 4)) >> 4) > > +#define SLOT_ID_MASK(m) ((m) & GENMASK(2, 0)) > > +#define ABS_Y_MASK(m) (((m) & GENMASK(5, 4)) << 4) > > +#define ABS_X_MASK(m) (((m) & GENMASK(7, 6)) << 2) > > + > > +struct pixart_data { > > + int mode; > > + int type; > > + int x_max; > > + int y_max; > > +}; > > + > > +int pixart_detect(struct psmouse *psmouse, bool set_properties); > > +int pixart_init(struct psmouse *psmouse); > > + > > +#endif /* _PIXART_PS2_H */ > > diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c > > index a0aac76b1e41..41af3460077d 100644 > > --- a/drivers/input/mouse/psmouse-base.c > > +++ b/drivers/input/mouse/psmouse-base.c > > @@ -36,6 +36,7 @@ > > #include "focaltech.h" > > #include "vmmouse.h" > > #include "byd.h" > > +#include "pixart_ps2.h" > > > > #define DRIVER_DESC "PS/2 mouse driver" > > > > @@ -905,6 +906,15 @@ static const struct psmouse_protocol psmouse_protocols[] = { > > .detect = byd_detect, > > .init = byd_init, > > }, > > +#endif > > +#ifdef CONFIG_MOUSE_PS2_PIXART > > + { > > + .type = PSMOUSE_PIXART, > > + .name = "PixArtPS/2", > > + .alias = "pixart", > > + .detect = pixart_detect, > > + .init = pixart_init, > > + }, > > #endif > > { > > .type = PSMOUSE_AUTO, > > @@ -1172,6 +1182,13 @@ static int psmouse_extensions(struct psmouse *psmouse, > > return ret; > > } > > > > + /* Try PixArt touchpad */ > > + if (max_proto > PSMOUSE_IMEX && > > + psmouse_try_protocol(psmouse, PSMOUSE_PIXART, &max_proto, > > + set_properties, true)) { > > + return PSMOUSE_PIXART; > > + } > > + > > if (max_proto > PSMOUSE_IMEX) { > > if (psmouse_try_protocol(psmouse, PSMOUSE_GENPS, > > &max_proto, set_properties, true)) > > diff --git a/drivers/input/mouse/psmouse.h b/drivers/input/mouse/psmouse.h > > index 4d8acfe0d82a..23f7fa7243cb 100644 > > --- a/drivers/input/mouse/psmouse.h > > +++ b/drivers/input/mouse/psmouse.h > > @@ -69,6 +69,7 @@ enum psmouse_type { > > PSMOUSE_BYD, > > PSMOUSE_SYNAPTICS_SMBUS, > > PSMOUSE_ELANTECH_SMBUS, > > + PSMOUSE_PIXART, > > PSMOUSE_AUTO /* This one should always be last */ > > }; > > > > @@ -94,7 +95,7 @@ struct psmouse { > > const char *vendor; > > const char *name; > > const struct psmouse_protocol *protocol; > > - unsigned char packet[8]; > > + unsigned char packet[16]; > > unsigned char badbyte; > > unsigned char pktcnt; > > unsigned char pktsize; > > -- > > 2.43.0 > > > > Thanks. > > -- > Dmitry
