On Wed, Jul 24, 2024 at 10:31 AM Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote:
>
> 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.
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
>
> > +
> > +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
