Hi Binbin,
On Thu, Jul 04, 2024 at 08:52:43PM +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>
> ---
> V4:
> - Thanks Dmitry for the review.
> - Just return what ps2_command() reports, instead of replacing it with
> -EIO;
> - Refact pixart_read_tp_mode/pixart_read_tp_type(), to separate mode
> value and errors/success;
> - Pass the INPUT_MT_POINTER flag to input_mt_init_slots() and remove
> some redundant code, like the call to input_mt_report_finger_count()
> and the setting of bits in the touchpad section.
Thank you for making the change. I noticed a couple more things that I
fixed up on my side and applied. Please take a look and shout if you
see something wrong.
> +
> +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]);
We have a nice FIELD_GET() macro that operates on a bitmask and value,
so I changed CONTACT_CNT(m) to CONTACT_CNT_MASK and converted this to:
unsigned int contact_cnt = FIELD_GET(CONTACT_CNT_MASK, pkt[0]);
Same for SLOT_ID_MASK, ABS_Y_MASK, and ABS_X_MASK.
> + 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];
That's way too many of pkt[3 * i + offset] repetitions, I made
const u8 *p = &pkt[3 * i];
temporary.
<...>
> +
> +static int pixart_reconnect(struct psmouse *psmouse)
> +{
> + u8 mode;
> + int error;
> + struct ps2dev *ps2dev = &psmouse->ps2dev;
> +
> + pixart_reset(psmouse);
> + error = pixart_read_tp_mode(ps2dev, &mode);
> + if (error)
> + return error;
> +
> + if (mode != PIXART_MODE_ABS)
> + return mode;
This should be "return -EIO".
Thanks.
--
Dmitry
