Hi Éric,
Thanks for your comments.
Reply below inline, thank you.
jj
On Wed, 31 Aug 2011 14:50:59 +0200, Éric Piel <E.A.B.Piel@xxxxxxxxxx> wrote:
> Op 29-08-11 10:28, JJ Ding schreef:
> > v4 hardware is a true multitouch capable touchpad (up to 5 fingers).
> > The packet format is quite complex, please see protocol document for
> > reference.
> Hi,
>
> It's great that you add support for another version!
>
> Looks good. Just a few comment (inline).
>
> Cheers,
> Éric
>
>
> >
> > Signed-off-by: JJ Ding<jj_ding@xxxxxxxxxx>
> > ---
> > Documentation/input/elantech.txt | 170 ++++++++++++++++++++++++++
> > drivers/input/mouse/elantech.c | 247 ++++++++++++++++++++++++++++++++++----
> > drivers/input/mouse/elantech.h | 29 ++++-
> > 3 files changed, 420 insertions(+), 26 deletions(-)
> >
> > diff --git a/Documentation/input/elantech.txt b/Documentation/input/elantech.txt
> > index cee08ee..f63115a 100644
> > --- a/Documentation/input/elantech.txt
> > +++ b/Documentation/input/elantech.txt
> > @@ -32,6 +32,12 @@ Contents
> > 6.2 Native absolute mode 6 byte packet format
> > 6.2.1 One/Three finger touch
> > 6.2.2 Two finger touch
> > + 7. Hardware version 4
> > + 7.1 Registers
> > + 7.2 Native absolute mode 6 byte packet format
> > + 7.2.1 Status packet
> > + 7.2.2 Head packet
> > + 7.2.3 Motion packet
> >
> >
> >
> > @@ -573,3 +579,167 @@ The packet format is exactly the same for two finger touch, except the hardware
> > sends two 6 byte packets. The first packet contains data for the first finger,
> > the second packet has data for the second finger. So for two finger touch a
> > total of 12 bytes are sent.
> > +
> > +/////////////////////////////////////////////////////////////////////////////
> > +
> > +7. Hardware version 4
> > + ==================
> > +
> > +7.1 Registers
> > + ~~~~~~~~~
> > +* reg_07
> > +
> > + bit 7 6 5 4 3 2 1 0
> > + 0 0 0 0 0 0 0 A
> > +
> > + A: 1 = enable absolute tracking
> > +
> > +7.2 Native absolute mode 6 byte packet format
> > + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +v4 hardware is a true multitouch touchpad, capable of tracking up to 5 fingers.
> > +Unfortunately, due to PS/2's limited bandwidth, its packet format is rather
> > +complex.
> > +
> > +Whenever the numbers or identities of the fingers changes, the hardware sends a
> > +status packet to indicate how many and which fingers is on touchpad, followed by
> > +head packets or motion packets. A head packet contains data of finger id, finger
> > +position (absolute x, y values), width, and presure. A motion packet contains
> Typo: pres_s_ure
Fixed.
> > +two fingers' position delta.
> > +
> :
> > diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
> > index c4ceefd..0d3936d 100644
> > --- a/drivers/input/mouse/elantech.c
> > +++ b/drivers/input/mouse/elantech.c
> > @@ -84,12 +84,6 @@ static int elantech_read_reg(struct psmouse *psmouse, unsigned char reg,
> > unsigned char param[3];
> > int rc = 0;
> >
> > - if (reg< 0x10 || reg> 0x26)
> > - return -1;
> > -
> > - if (reg> 0x11&& reg< 0x20)
> > - return -1;
> > -
> You could still leave a check of reg being between 0x07 and 0x26, it's
> better than nothing.
OK, I will add them back.
> :
> >
> > +static void elantech_mt_sync(struct psmouse *psmouse)
> > +{
> > + struct input_dev *dev = psmouse->dev;
> > + unsigned char *packet = psmouse->packet;
> > +
> > + input_report_key(dev, BTN_LEFT, packet[0]& 0x01);
> > + input_report_key(dev, BTN_RIGHT, packet[0]& 0x02);
> > + input_mt_report_pointer_emulation(dev, true);
> > + input_sync(dev);
> > +}
> The function naming is a bit strange. If you put _mt_, I expect there is
> only code related to multitouch. Maybe rename to:
> elantech_input_sync_v4()
Changed.
>
> > +
> > +static void process_packet_status(struct psmouse *psmouse)
> :
> > +static void process_packet_head(struct psmouse *psmouse)
> :
> > +static void process_packet_motion(struct psmouse *psmouse)
> Maybe rename these function to *_v4(), so that it's clear it's not for
> v3 hardware or any other version.
Changed.
> :
> > @@ -645,10 +807,11 @@ static int elantech_set_absolute_mode(struct psmouse *psmouse)
> >
> > static int set_range(struct psmouse *psmouse, unsigned int *x_min,
> > unsigned int *y_min, unsigned int *x_max,
> > - unsigned int *y_max)
> > + unsigned int *y_max, unsigned int *width)
> > {
> > struct elantech_data *etd = psmouse->private;
> > unsigned char param[3];
> > + unsigned char traces = 0;
> Don't initialize it.
Fixed.
>
> > int i;
> >
> > switch (etd->hw_version) {
> > @@ -677,12 +840,16 @@ static int set_range(struct psmouse *psmouse, unsigned int *x_min,
> > }
> > break;
> >
> > + case 4:
> > + traces = etd->capabilities[1];
> > + /* pass through */
> > case 3:
> > if (synaptics_send_cmd(psmouse, ETP_FW_ID_QUERY, param))
> > return -1;
> >
> > *x_max = (0x0f& param[0])<< 8 | param[1];
> > *y_max = (0xf0& param[0])<< 4 | param[2];
> > + *width = *x_max / (traces - 1);
> > break;
> > }
> width is used only for firmware 4, right? If so then this code is too
> tricky. Order normally the cases, and duplicate the few common lines.
> Maintainability is more important than saving a couple of bytes :-)
Yes, width is only used for firmware 4. I will change this for better
maintainability.
> Also, what happens if the firmware returns 1 in etd->capabilities[1]?
> Make sure a division by zero is _impossible_. Please add a check on sane
> values for "traces", and bail out if it's not within these limits:
> if ((traces < 2) || (traces > *x_max))
> return -1;
Thanks for reminding me this. I will fix it.
> Éric
>
> --
> 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
--
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
