Re: [PATCH v2 2/3] input: Cypress PS/2 Trackpad psmouse driver

[Kamal Mostafa <kamal@canonical.com>]

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.