Re: [PATCH v4 8/8] Input: elantech - add v4 hardware support

[JJ Ding <jj_ding@emc.com.tw>]

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@tudelft.nl> 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@emc.com.tw>
> > ---
> >   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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html