Re: [PATCH v4] Input: Add driver for PixArt PS/2 touchpad

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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@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>
> ---
> 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