Re: [PATCH] input: Synaptics USB device driver

[Dmitry Torokhov]

On Thu, Jan 05, 2012 at 05:39:57AM +0000, Jan Steinhoff wrote:
> On Wed, 4 Jan 2012 01:41:03 -0800
> Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote:
> > Om, so I have been looking at the driver and the following seems to be
> > still working with my Lenovo composite touchpad/stick.
> > 
> > Changes:
> > 
> > - the stick is reported in relative mode; we might consider addig
> >   sensitivity control similar to trackpoint driver. Press-to-select -
> >   maybe in driver, or in client; undecided.
> > 
> > - got rid of BTN_MISC/BTN_MIDDLE option (at least for now);
> 
> Please make BTN_MIDDLE the default in this case for the cPad. This
> button is actually located where the middle button should be. Only if
> one uses the cPad's background display it obtains a "special" meaning.
> (Then pressing it should launch a menu on the background display and the
> touchpad works as a touchscreen for this display.)

OK, let's change it to MTN_MIDDLE. We may consider changing it to
BTN_MISC if cPad display support is merged and active.

> 
> > - added devices to HID blacklist;
> 
> I think it is more flexible and save to use the manual driver binding
> capability of the USB core instead:
> 
> http://lwn.net/Articles/143397/
> 
> Which driver to use for which device can then be set, e.g., with the
> help of udev config files.

However it is more work for distributions and users. As it stands now
adding blacklist and compiling synaptics_usb does not require any
additional userspace support (everyone packages and installs synaptics X
driver nowadays).

> 
> Blacklisting has the big disadvantage that the device will not work at
> all if the synaptics-usb kernel module is not loaded/present, whereas
> otherwise usbhid is used as the default. At least the corresponding
> part in the blacklist should be enclosed in
> #ifdef CONFIG_MOUSE_SYNAPTICS_USB ... #endif

This is a good idea, I'll add ifdefs.

> 
> Further, as noted in the initial comment, not all devices have been
> tested with this driver. This is another reason to allow the user to
> choose which driver to use.

I'll remove touchscreen from blacklist/device table as it really should
generate different set of events; the rest I hope we'll be able to fix
reasonably quickly if there are complaints.

> 
> > - runtime PM (hopefully I got it right);
> > 
> > - open/close.
> 
> I do not understand why needs_remote_wakeup is flipped in open/close.
> As far as I know, the device can not autosuspend after
> usb_autopm_get_interface was called anyway?

We do usb_autopm_put_interface as last action in open() so the device
will be suspended and needs wakeups.

> 
> > Still TODO:
> > 
> > - query device for supported rages instead of having module
> > parameters.
> 
> At least for the cPad the device does unfortunately not provide this
> information. I tried to read the HID descriptors once but just got
> errors. However, the defaults work quite well for all tested devices.
> The module parameters are actually meant to just provide a simple
> workaround in case one of the untested devices (especially the
> touchscreen) might need them.

OK, so why don't we replace module parameters with default ranges and
maybe later Synaptics guys will tell us how to query ranges properly?
Default ranges worked reasonably well for PS/2 version for several
years.

Thanks.

-- 
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/