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

[Dmitry Torokhov]

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);

> +}
> +
> +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.

> +		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).
 
> +
> +#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.

> +
> +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...
 
> +		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.

-- 
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/