Hi Henrik-
Thanks very much for your detailed review...
On Wed, 2012-11-21 at 20:20 +0100, Henrik Rydberg wrote:
> Reading the patch, it looks very much like a typical semi-mt device to
> me. Any good reason not to handle it that way?
Well, the device does properly report contact-counts up to 5 fingers
(though only two actual contact positions). If I understand SEMI_MT
correctly (and I might not) using it restricts us to reporting only up
to 2 fingers, which seems undesirable.
That said, my forthcoming PATCH v3 does implement the SEMI_MT /
2-contacts-only method as you requested (I think) -- and includes one
more proposed patch to re-enable >2 contacts.
PATCH v3 also implements the majority of your other suggestions,
including replacing all the custom slot tracking code with
input_mt_assign_slots. Large swaths of code have been eliminated
(thanks!).
Below are notes regarding the few suggestions which I have not
addressed.
Thanks again for your help, and in advance for your review of PATCH v3.
-Kamal
> > +static int cypress_read_vital_statistics(struct psmouse *psmouse)
> > +{
>> [...]
> > +
> > + if (!cytp->tp_max_pressure ||
> > + (cytp->tp_max_pressure < cytp->tp_min_pressure) ||
> > + (!cytp->tp_width || !cytp->tp_high) ||
> > + (!cytp->tp_max_abs_x) ||
> > + (cytp->tp_max_abs_x < cytp->tp_width) ||
> > + (!cytp->tp_max_abs_y) ||
> > + (cytp->tp_max_abs_y < cytp->tp_high))
> > + return -1;
>
> Odd parenthesis. Can tp_min_pressure be zero? Don't we trust the
> values coming from the device?
I do not know, so I have left this as is.
> > +static psmouse_ret_t cypress_validate_byte(struct psmouse *psmouse)
> > +{
> > + int contact_cnt;
> > + int index = psmouse->pktcnt - 1;
> > + unsigned char *packet = psmouse->packet;
> > + struct cytp_data *cytp = psmouse->private;
> > +
> > + if (index < 0 || index > cytp->pkt_size)
> > + return PSMOUSE_BAD_DATA;
> > +
> > + if ((index == 0) && ((packet[0] & 0xfc) == 0)) {
> > + /* call packet process for reporting finger leave. */
> > + cypress_process_packet(psmouse, 1);
> > + return PSMOUSE_FULL_PACKET;
> > + }
>
> Odd placement given the function's name?
Do you mean the call to cypress_process_packet? I agree that it is odd.
Left it as is. :-)
> > diff --git a/drivers/input/mouse/cypress_ps2.h b/drivers/input/mouse/cypress_ps2.h
>> [...]
> > +
> > +/* bits of first byte response of E9h-Status Request command. */
> > +#define RESP_BTN_RIGHT_BIT 0x01
> > +#define RESP_BTN_MIDDLE_BIT 0x02
> > +#define RESP_BTN_LEFT_BIT 0x04
> > +#define RESP_SCALING_BIT 0x10
> > +#define RESP_ENABLE_BIT 0x20
> > +#define RESP_REMOTE_BIT 0x40
> > +#define RESP_SMBUS_BIT 0x80
> > +
> > +#define CYTP_MAX_CONTACTS 5
> > +#define CYTP_MAX_MT_SLOTS 16
>
> Are all these values used?
Some of the #define's are not used, but I'm reluctant to remove them as
they might be useful for later versions of the driver. I left them as
is, pending your advice.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
