Messages in this thread |  | | From | Dmitry Torokhov <> | Subject | Re: [PATCH 2.6.11-rc3] IBM Trackpoint support | Date | Thu, 3 Feb 2005 19:34:16 -0500 |
| |
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.
> +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.
> +__obsolete_setup("pts="); > +__obsolete_setup("backup="); > +__obsolete_setup("draghyst=");
What are these? They can't be obsolete as the module has never been part of the vanilla kernel.
> + > +/* > + * Device IO: read, write and toggle bit > + */ > +static void trackpoint_command(struct ps2dev *ps2dev, unsigned char cmd) > +{ > + ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, TP_COMMAND)); > + ps2_command(ps2dev, NULL, MAKE_PS2_CMD(0, 0, cmd)); > +} > +
Error checking would be nice.
> + > +#ifdef CONFIG_PROC_FS ... > +{ > + remove_proc_entry("scroll_delay", tp_dir); > + remove_proc_entry("scroll", tp_dir); > + remove_proc_entry("transparent", tp_dir); > + remove_proc_entry("ext_dev", tp_dir); > + remove_proc_entry("mb", tp_dir); > + remove_proc_entry("skip_back", tp_dir); > + remove_proc_entry("ptson", tp_dir); > + remove_proc_entry("jenks_curv", tp_dir); > + remove_proc_entry("z_time", tp_dir); > + remove_proc_entry("up_thresh", tp_dir); > + remove_proc_entry("thresh", tp_dir); > + remove_proc_entry("min_drag", tp_dir); > + remove_proc_entry("drag_hyst", tp_dir); > + remove_proc_entry("backup", tp_dir); > + 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).
> + > + 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.
> +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?
-- Dmitry - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |