From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Binbin Zhou <zhoubinbin@loongson.cn>
Cc: Binbin Zhou <zhoubb.aaron@gmail.com>,
Huacai Chen <chenhuacai@loongson.cn>,
Jon Xie <jon_xie@pixart.com>, Jay Lee <jay_lee@pixart.com>,
Huacai Chen <chenhuacai@kernel.org>,
linux-input@vger.kernel.org, Xiaotian Wu <wuxiaotian@loongson.cn>
Subject: Re: [PATCH v3] Input: Add driver for PixArt PS/2 touchpad
Date: Wed, 3 Jul 2024 14:33:05 -0700 [thread overview]
Message-ID: <ZoXDkcn7O5a8a3l3@google.com> (raw)
In-Reply-To: <20240701094953.3195501-1-zhoubinbin@loongson.cn>
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@pixart.com>
> Signed-off-by: Jon Xie <jon_xie@pixart.com>
> Co-developed-by: Jay Lee <jay_lee@pixart.com>
> Signed-off-by: Jay Lee <jay_lee@pixart.com>
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
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@loongson.cn/
>
> 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@loongson.cn/
>
> 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@pixart.com>
> + * Jay Lee <jay_lee@pixart.com>
> + * Further cleanup and restructuring by:
> + * Binbin Zhou <zhoubinbin@loongson.cn>
> + *
> + * 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?
> +{
> + 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.
> +
> + 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.
> +
> + 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.
> +
> + /* Absolute position */
> + __set_bit(EV_ABS, dev->evbit);
No need to call __set_bit(EV_ABS, dev->evbit) when using
input_set_abs_params().
> + 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
next prev parent reply other threads:[~2024-07-03 21:33 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-07-01 9:49 [PATCH v3] Input: Add driver for PixArt PS/2 touchpad Binbin Zhou
2024-07-03 21:33 ` Dmitry Torokhov [this message]
2024-07-04 7:05 ` Binbin Zhou
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZoXDkcn7O5a8a3l3@google.com \
--to=dmitry.torokhov@gmail.com \
--cc=chenhuacai@kernel.org \
--cc=chenhuacai@loongson.cn \
--cc=jay_lee@pixart.com \
--cc=jon_xie@pixart.com \
--cc=linux-input@vger.kernel.org \
--cc=wuxiaotian@loongson.cn \
--cc=zhoubb.aaron@gmail.com \
--cc=zhoubinbin@loongson.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).