Re: [PATCH 2.6.11-rc3] IBM Trackpoint support

[Vojtech Pavlik <vojtech@suse.cz>]

On Thu, Feb 03, 2005 at 07:34:16PM -0500, Dmitry Torokhov wrote:
> On Thursday 03 February 2005 17:43, Stephen Evanchik wrote:
> > Vojtech,
> > 
> > Here is a patch that exposes the IBM TrackPoint's extended properties
> > as well as scroll wheel emulation.
> > 
> > 
> 
> Hi,
> 
> Very nice although I have a couple of comments.
> 
> >  /*
> > + * Try to initialize the IBM TrackPoint
> > + */
> > +	if (max_proto > PSMOUSE_PS2 && trackpoint_init(psmouse) == 0) {
> > +		psmouse->vendor = "IBM";
> > +		psmouse->name = "TrackPoint";
> > + 
> > +		return PSMOUSE_PS2;
> 
> Why PSMOUSE_PS2? Reconnect will surely not like it.

Indeed. IIRC this patch killed wheel mouse detection in ubuntu.

> 
> > +int tp_sens = TP_DEF_SENS;
> > +module_param_named(sens, tp_sens, uint, 0);
> > +MODULE_PARM_DESC(sens, "Sensitivity");
> ....
> > +int tp_mb_scroll = TP_DEF_MB_SCROLL;
> > +module_param_named(mb_scroll, tp_mb_scroll, uint, 0);
> > +MODULE_PARM_DESC(mb_scroll, "Scroll with middle button");
> 
> All of this should be handled via sysfs dynamically, we don't need any
> more pmouse module parameters.

Exactly.

> > +        remove_proc_entry("neg_inertia", tp_dir);
> > +        remove_proc_entry("speed", tp_dir);
> > +        remove_proc_entry("sensitivity", tp_dir);
> > +        remove_proc_entry("trackpoint", NULL);
> > +}
> 
> No new proc stuff please (it will be visible from sysfs when you redo the
> module parameters).

... automatically, even ...

> > +
> > +		tp->scrolling = 0;
> > +		tp->mb_was_down = 0;
> > +	}
> > +        else if(tp->scrolling) {
> > +
> > +		/* Vertical scrolling */
> > +		diff = (int) ((packet[0] << 3) & 0x100) - (int) packet[2];
> > +                if( diff < -2 ) {
> > +			input_report_rel(&psmouse->dev, REL_WHEEL, 1);
> > +		}
> > +		else if(diff > 2) {
> > +			input_report_rel(&psmouse->dev, REL_WHEEL, -1);
> > +		}
> > +
> > +                /* Horizontal scrolling */
> > +                diff = (int) packet[1] - (int) ((packet[0] << 4) & 0x100);
> > +                if( diff < -2) {
> > +                        input_report_rel(&psmouse->dev, REL_HWHEEL, 1);
> > +                }
> > +                else if( diff > 2) {
> > +                        input_report_rel(&psmouse->dev, REL_HWHEEL, -1);
> > +                }
> > +
> > +		packet[1] &= 0x00;
> > +		packet[2] &= 0x00;
> > +	}
> 
> Looks like whitespace damage (tabs vs spaces) plus it should be "} else {"
> on one line.

What a shame that this thing doesn't have a raw mode where it'd report
the pressure ...

> > +int trackpoint_reconnect(struct psmouse *psmouse)
> > +{
> > +	unsigned char toggle;
> > +	struct trackpoint_data *tp = psmouse->private;
> > +
> > +	/* Push the config to the device */
> > +	
> 
> I'd like to verify that it still recognized as trackpoint - suspends often
> play tricks on PS/2 devices.
> 
> > +
> > +	printk("IBM TrackPoint firmware: 0x%02X\n", param[1]);
> 
> KERN_INFO?

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR