Hi Henrik-
Thanks again for your review. The forthcoming PATCH v4 includes the
majority of your change requests, except where noted below (and
considering my email "Subject: SEMI_MT vs. simulated mt")...
On Mon, 2012-12-03 at 08:45 +0100, Henrik Rydberg wrote:
> > + /*
> > + * send extension command 0xE8 or 0xF3,
> > + * if send extension command failed,
> > + * try to send recovery command to make
> > + * trackpad device return to ready wait command state.
> > + * It alwasy success based on this recovery commands.
>
> -EPARSE
I don't understand the meaning of that comment either. Perhaps the
original author Dudley Du can explain it.
> > +static int cypress_read_vital_statistics(struct psmouse *psmouse)
>
> Why do you call this statistics?
Perhaps "cypress_read_tp_metrics" would be a better name. Any thoughts
on this, Dudley?
> > +/*
> > + * reset trackpad device to standard relative mode.
> > + * This is also the defalut mode when trackpad powered on.
> > + */
> > +static void cypress_reset(struct psmouse *psmouse)
> > +{
> > + struct cytp_data *cytp = psmouse->private;
> > +
> > + psmouse_reset(psmouse);
> > +
> > + CYTP_SET_MODE_BIT(CYTP_BIT_STANDARD_REL);
> > + CYTP_SET_PACKET_SIZE(3);
> > +
> > + cytp->prev_contact_cnt = 0;
> > +}
>
> I suppose it is, but is it necessary to reset to default mode?
I left this unchanged, as I think you and Dmitry agreed.
> > +
> > +static int cypress_set_input_params(struct input_dev *input,
> > + struct cytp_data *cytp)
> > +{
> > + int ret;
> > +
> > + if (cytp->mode & CYTP_BIT_ABS_MASK) {
>
> It seems the device will always operate in this mode, so please simplify.
I #ifdef'd out the unused relative-mode code, as opposed to deleting it.
Will that be acceptable?
> > + /* Remove HSCROLL bit */
> > + if (report_data->contact_cnt == 4)
> > + header_byte &= ~(ABS_HSCROLL_BIT);
>
> Why conditionally?
Dudley, can you answer this? I left this (and all the scroll code)
unchanged.
> > +#if 0
> > + /* FIXME: cypress_reconnect() never works...
> > + * just let psmouse re-init() us for now.
> > + */
> > + psmouse->reconnect = cypress_reconnect;
> > +#endif
>
> This needs to be removed or fixed, of course.
Per Dudley, this does work in his disconnect/reconnect scenario, so I
re-enabled it. We will continue to investigate why it fails
(harmlessly) in my suspend/resume scenario.
> > + int tp_max_abs_x; /* Max X absolution units can be reported. */
> > + int tp_max_abs_y; /* Max Y absolution units can be reported. */
>
> I do not think we are seeking absolution here. :-)
Ha! Speak for yourself, Henrik! ;-) I'm starting to feel like maybe I
should be seeking absolution here!
-Kamal
--
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
