On Wed, Jul 24, 2024 at 10:31 AM Dmitry Torokhov
[off-list ref] wrote:

Hi Binbin,

On Thu, Jul 04, 2024 at 08:52:43PM +0800, Binbin Zhou wrote:

quoted

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 <redacted>
Signed-off-by: Jon Xie <redacted>
Co-developed-by: Jay Lee <redacted>
Signed-off-by: Jay Lee <redacted>
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.

Hi Dmitry:

When I tried to fix it, I found that you had already fixed it for me
and merged it into your input/next branch. Thank you for your
corrections.
Also thank you very much for reviewing this patch series. I also
learned a lot about the input subsystem from it.

Of course I have tested it and it is good.

Thanks.
Binbin

quoted

+
+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.

quoted

+     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.

<...>

quoted

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