Re: [RFC] OLPC tablet input driver, take two.

["Zephaniah E. Hull" <warp@aehallh.com>]

[-- Attachment #1: Type: text/plain, Size: 4285 bytes --]

On Sun, Sep 10, 2006 at 06:19:31PM -0400, Dmitry Torokhov wrote:
> On Sunday 10 September 2006 16:10, Zephaniah E. Hull wrote:
> > Take two, with most of the items people commented about addressed.
> > 
> 
> Hi Zephaniah,
> 
> I have couple more comments/requests:
> 
> > 
> > +
> > +	if (gs_down) {
> > +		input_report_abs(dev2, ABS_X, gx);
> > +		input_report_abs(dev2, ABS_Y, gy);
> > +	}
> > +	input_report_abs(dev2, ABS_PRESSURE, gz);
> > +
> > +	if (pt_down) {
> > +		input_report_abs(dev, ABS_X, px);
> > +		input_report_abs(dev, ABS_Y, py);
> > +	}
> > +
> > +	input_sync(dev);
> 
> Please add input_sync(dev2);

Whoops, bizarrely it still worked, but fixed.
> 
> > +}
> > +
> > +static psmouse_ret_t olpc_process_byte(struct psmouse *psmouse, struct pt_regs *regs)
> > +{
> > +	struct olpc_data *priv = psmouse->private;
> > +	psmouse_ret_t ret = PSMOUSE_BAD_DATA;
> > +
> > +	if ((psmouse->packet[0] & priv->i->mask0) != priv->i->byte0) {
> > +		ret = PSMOUSE_BAD_DATA;
> 
> It looks like you can kill "ret = PSMOUSE_BAD_DATA" assignments since you
> initialize ret with it.

Done.
> 
> > +		goto out;
> > +	}
> > +
> > +	/* Bytes 2 - 9 should have 0 in the highest bit */
> > +	if (psmouse->pktcnt >= 2 && psmouse->pktcnt <= 9 &&
> > +		(psmouse->packet[psmouse->pktcnt - 1] & 0x80)) {
> > +	    ret = PSMOUSE_BAD_DATA;
> > +	    goto out;
> > +	}
> 
> I'd like to have standard identation throughout the driver (and input
> sybsystem in general).

Whoops, fixed.
>  
> > +
> > +#ifndef _OLPC_H
> > +#define _OLPC_H
> > +
> > +int olpc_detect(struct psmouse *psmouse, int set_properties);
> > +int olpc_init(struct psmouse *psmouse);
> > +
> > +struct olpc_model_info {
> > +        unsigned char signature[3];
> > +        unsigned char byte0, mask0;
> > +        unsigned char flags;
> > +};
> 
> Hard TABs for identation please.

Done.
> 
> > +
> > +struct olpc_data {
> > +	struct input_dev *dev2;		/* Relative device */
> > +	char name[32];			/* Name */
> > +	char phys[32];			/* Phys */
> > +	const struct olpc_model_info *i;/* Info */
> > +};
> > +
> > +
> > +#endif
> > diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
> > index 8bc9f51..20060b0 100644
> > --- a/drivers/input/mouse/psmouse-base.c
> > +++ b/drivers/input/mouse/psmouse-base.c
> > @@ -26,6 +26,7 @@
> >  #include "synaptics.h"
> >  #include "logips2pp.h"
> >  #include "alps.h"
> > +#include "olpc.h"
> >  #include "lifebook.h"
> >  #include "trackpoint.h"
> >  
> > @@ -616,6 +617,15 @@ static int psmouse_extensions(struct psm
> >   */
> >  			max_proto = PSMOUSE_IMEX;
> >  		}
> > +		ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_RESET_DIS);
> 
> Do we have to do 2nd reset here? Plus logic seems a bit fuzzy here -
> if ALPS is detected but initizliztion fails it will start OLPC detection
> which is probably not what you wanted...

Reset is _probably_ not necessary, I'll verify.

However the logic is the same as for all the others, if init succeeds,
it returns PSMOUSE_ALPS, if it doesn't then it continues on to the next,
which happens to be olpc, admittedly it would be more obvious that it's
doing the same thing if it was in its own if, but.
>  
> > +		if (olpc_detect(psmouse, set_properties) == 0) {
> > +			if (!set_properties || olpc_init(psmouse) == 0)
> > +				return PSMOUSE_OLPC;
> > +/*
> > + * Init failed, try basic relative protocols
> > + */
> > +			max_proto = PSMOUSE_IMEX;
> > +		}
> >  	}
> >  
> >  	if (max_proto > PSMOUSE_IMEX && genius_detect(psmouse, set_properties) == 0)
> > @@ -726,6 +736,13 @@ static struct psmouse_protocol psmouse_p
> >  		.detect		= trackpoint_detect,
> >  	},
> >  	{
> > +		.type		= PSMOUSE_OLPC,
> > +		.name		= "OLPC",
> > +		.alias		= "olpc",
> > +		.maxproto	= 1,
> 
> Do not set maxproto on speciality protocols. It is meant to limit highest
> version of standard protocols to be probed/used by a device.

Fixed.


Thanks a ton, I have some extra testing to do and then I'll send out a
fixed copy.

Zephaniah E. Hull.
> 
> -- 
> Dmitry
> 

-- 
	  1024D/E65A7801 Zephaniah E. Hull <warp@aehallh.com>
	   92ED 94E4 B1E6 3624 226D  5727 4453 008B E65A 7801
	    CCs of replies from mailing lists are requested.

"I am ecstatic that some moron re-invented a 1995 windows fuckup."
        -- Alan Cox

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]