input: Synaptics USB device driver
Subsystems (2)
PATCH 1/1 input: Synaptics USB device driver
From: Jan Steinhoff <[email protected]>
This patch adds a driver for Synaptics USB touchpad or pointing stick devices. These USB devices emulate an USB mouse by default, so one can also use the usbhid driver. However, in combination with special user space drivers this kernel driver allows one to customize the behaviour of the device.
An extended version of this driver with support for the cPad background display can be found at
<http://jan-steinhoff.de/linux/synaptics-usb.html>.
--- drivers/input/mouse/Kconfig | 14 + drivers/input/mouse/Makefile | 1 + drivers/input/mouse/synaptics_usb.c | 593 +++++++++++++++++++++++++++++++++++ drivers/input/mouse/synaptics_usb.h | 66 ++++ 4 files changed, 674 insertions(+), 0 deletions(-) create mode 100644 drivers/input/mouse/synaptics_usb.c create mode 100644 drivers/input/mouse/synaptics_usb.h
drivers/input/mouse/Kconfig +14 -0
@@ -322,4 +322,18 @@ config MOUSE_SYNAPTICS_I2C To compile this driver as a module, choose M here: the module will be called synaptics_i2c. +config MOUSE_SYNAPTICS_USB + tristate "Synaptics USB device support" + depends on USB + help + Say Y here if you want to use a Synaptics USB touchpad or pointing stick. + These Synaptics USB devices emulate an USB mouse by default, so you can + also use the usbhid driver. However, in combination with special X.Org + drivers this kernel driver allows you to customize the behaviour of the + device. More information can be found at: + <http://jan-steinhoff.de/linux/synaptics-usb.html> + + To compile this driver as a module, choose M here: the + module will be called synaptics_usb. + endif
drivers/input/mouse/Makefile +1 -0
@@ -18,6 +18,7 @@ obj-$(CONFIG_MOUSE_PXA930_TRKBALL) += pxa930_trkball.o obj-$(CONFIG_MOUSE_RISCPC) += rpcmouse.o obj-$(CONFIG_MOUSE_SERIAL) += sermouse.o obj-$(CONFIG_MOUSE_SYNAPTICS_I2C) += synaptics_i2c.o +obj-$(CONFIG_MOUSE_SYNAPTICS_USB) += synaptics_usb.o obj-$(CONFIG_MOUSE_VSXXXAA) += vsxxxaa.o psmouse-objs := psmouse-base.o synaptics.o
drivers/input/mouse/synaptics_usb.c new file +593 -0
593 lines not on this page · raw message
drivers/input/mouse/synaptics_usb.h new file +66 -0
66 lines not on this page · raw message
Signature · 1 line
1.7.5.4
Am Dienstag, 3. Januar 2012, 19:40:33 schrieb Jan Steinhoff:
Quoted 7 lines
From: Jan Steinhoff <[email protected]>
This patch adds a driver for Synaptics USB touchpad or pointing stick devices. These USB devices emulate an USB mouse by default, so one can also use the usbhid driver. However, in combination with special user space drivers this kernel driver allows one to customize the behaviour of the device.
Hi,
thank you for this driver. There are a few issues which I have commented on in the text.
Regards
Oliver
[..]
drivers/input/mouse/synaptics_usb.c line 195
Quoted 13 lines
drivers/input/mouse/synaptics_usb.c new file +13
-0+input_report_abs (idev,ABS_PRESSURE , pressure); + +input_report_key (idev,BTN_LEFT , data[1 ] &0x04 ); +input_report_key (idev,BTN_RIGHT , data[1 ] &0x01 ); +input_report_key (idev,BTN_MIDDLE , data[1 ] &0x02 ); +if (synusb->has_display) +input_report_key (idev, btn_middle ?BTN_MIDDLE :BTN_MISC , + data[1 ] &0x08 ); + +input_sync (idev); +resubmit: + res =usb_submit_urb (urb,GFP_ATOMIC ); +if (res)
You are racing with disconnect, suspend and pre_reset. This may lead to a spurious error message. The correct check is (res && res != -EPERM)
[..]
drivers/input/mouse/synaptics_usb.c line 287
Quoted 12 lines
drivers/input/mouse/synaptics_usb.c new file +12
-0+ +/* +* initialization of usb data structures +*/ + +static int synusb_setup_iurb (struct synusb *synusb, +struct usb_endpoint_descriptor *endpoint) +{ +char *buf; + +if (endpoint->wMaxPacketSize <8 ) +return 0 ;
How could this happen?
drivers/input/mouse/synaptics_usb.c line 296
Quoted 9 lines
drivers/input/mouse/synaptics_usb.c new file +9
-0+if (synusb->iurb) { +synusb_warn (synusb,"Found more than one possible " +"int in endpoint" ); +return 0 ; + } + synusb->iurb =usb_alloc_urb (0 ,GFP_KERNEL ); +if (synusb->iurb ==NULL ) +return -ENOMEM ; + buf = usb_alloc_coherent(synusb->udev, 8, GFP_ATOMIC,
No need for an atomic allocation here.
Quoted 7 lines
drivers/input/mouse/synaptics_usb.c new file +7
-0+ &synusb->iurb->transfer_dma); +if (buf ==NULL ) +return -ENOMEM ; + usb_fill_int_urb(synusb->iurb, synusb->udev, + usb_rcvintpipe(synusb->udev, + endpoint->bEndpointAddress), + buf, 8, synusb_input_callback,
drivers/input/mouse/synaptics_usb.c line 313
Quoted 10 lines
drivers/input/mouse/synaptics_usb.c new file +10
-0+ synusb, endpoint->bInterval); + synusb->iurb->transfer_flags |=URB_NO_TRANSFER_DMA_MAP ; +return 0 ; +} + +static inline int synusb_match_endpoint (struct usb_endpoint_descriptor *ep) +{ +if (((ep->bEndpointAddress &USB_DIR_IN ) ==USB_DIR_IN ) && + ((ep->bmAttributes &USB_ENDPOINT_XFERTYPE_MASK ) + ==USB_ENDPOINT_XFER_INT ))
We have macros for such checks. Please use them.
Quoted 5 lines
drivers/input/mouse/synaptics_usb.c new file +5
-0+return 0 ; + +return -ENODEV ; +} +
[..]
drivers/input/mouse/synaptics_usb.c line 473
Quoted 10 lines
drivers/input/mouse/synaptics_usb.c new file +10
-0+static void synusb_disconnect (struct usb_interface *interface) +{ +struct synusb *synusb; + + synusb =usb_get_intfdata (interface); +usb_set_intfdata (interface,NULL ); + +cancel_delayed_work_sync (&synusb->isubmit); + +usb_kill_urb (synusb->iurb);
Code duplication. If you define draw_down(), use it.
Quoted 15 lines
drivers/input/mouse/synaptics_usb.c new file +15
-0+input_unregister_device (synusb->idev); + synusb->idev =NULL ; + +kref_put (&synusb->kref, synusb_delete); + +dev_info (&interface->dev,"Synaptics device disconnected\n" ); +} + + +/* +* suspend code +*/ + +static void synusb_draw_down (struct synusb *synusb) +{
drivers/input/mouse/synaptics_usb.c line 498
Quoted 10 lines
drivers/input/mouse/synaptics_usb.c new file +10
-0+cancel_delayed_work_sync (&synusb->isubmit); +usb_kill_urb (synusb->iurb); +} + +static int synusb_suspend (struct usb_interface *intf,pm_message_t message) +{ +struct synusb *synusb =usb_get_intfdata (intf); + +if (synusb ==NULL ) +return 0 ;
How can this happen?
drivers/input/mouse/synaptics_usb.c line 510
Quoted 12 lines
drivers/input/mouse/synaptics_usb.c new file +12
-0+ +synusb_draw_down (synusb); + +return 0 ; +} + +static int synusb_resume (struct usb_interface *intf) +{ +struct synusb *synusb =usb_get_intfdata (intf); +int res; + + res =usb_submit_urb (synusb->iurb,GFP_ATOMIC );
GFP_NOIO
Quoted 12 lines
drivers/input/mouse/synaptics_usb.c new file +12
-0+if (res) +synusb_err (synusb,"submit int in urb failed during resume " +"with result %d" , res); + +return res; +} + +static int synusb_pre_reset (struct usb_interface *intf) +{ +struct synusb *synusb =usb_get_intfdata (intf); + +synusb_draw_down (synusb);
drivers/input/mouse/synaptics_usb.c line 532
Quoted 10 lines
drivers/input/mouse/synaptics_usb.c new file +10
-0+ +return 0 ; +} + +static int synusb_post_reset (struct usb_interface *intf) +{ +struct synusb *synusb =usb_get_intfdata (intf); +int res; + + res =usb_submit_urb (synusb->iurb,GFP_ATOMIC );
GFP_NOIO
drivers/input/mouse/synaptics_usb.c line 545
Quoted 13 lines
drivers/input/mouse/synaptics_usb.c new file +13
-0+if (res) +synusb_err (synusb,"submit int in urb failed in during " +"post_reset with result %d" , res); + +return res; +} + +static int synusb_reset_resume (struct usb_interface *intf) +{ +struct synusb *synusb =usb_get_intfdata (intf); +int res; + + res =usb_submit_urb (synusb->iurb,GFP_ATOMIC );
GFP_NOIO
Quoted 12 lines
drivers/input/mouse/synaptics_usb.c new file +12
-0+if (res) +synusb_err (synusb,"submit int in urb failed during " +"reset-resume with result %d" , res); + +return res; +} + +/* the id table is filled via sysfs, so usbhid is always the default driver */ +static struct usb_device_id synusb_idtable[] = { { } }; +MODULE_DEVICE_TABLE (usb, synusb_idtable); + +static struct usb_driver synusb_driver = {
drivers/input/mouse/synaptics_usb.c line 567
Quoted 10 lines
drivers/input/mouse/synaptics_usb.c new file +10
-0+ .name = "synaptics_usb", + .probe = synusb_probe, + .disconnect = synusb_disconnect, + .id_table = synusb_idtable, + .suspend = synusb_suspend, + .resume = synusb_resume, + .pre_reset = synusb_pre_reset, + .post_reset = synusb_post_reset, + .reset_resume = synusb_reset_resume, + .supports_autosuspend = 1,
Yet you do not manage the busy state. Do you really want to play ping-pong with the power state?
Quoted 9 lines · On Wed, 4 Jan 2012 09:25:59 +0100 Oliver Neukum…
Am Dienstag, 3. Januar 2012, 19:40:33 schrieb Jan Steinhoff:
Quoted 7 lines
From: Jan Steinhoff <[email protected]>
This patch adds a driver for Synaptics USB touchpad or pointing stick devices. These USB devices emulate an USB mouse by default, so one can also use the usbhid driver. However, in combination with special user space drivers this kernel driver allows one to customize the behaviour of the device.
Quoted 4 lines
Hi,
thank you for this driver. There are a few issues which I have commented on in the text.
Hi,
I agree with most of the comments, but I have some further questions/remarks below.
Best,
Jan
[...]
Quoted 15 lines
[..]
drivers/input/mouse/synaptics_usb.c line 287
Quoted 12 lines
drivers/input/mouse/synaptics_usb.c new file +7
-0+ +/* +* initialization of usb data structures +*/ + +static int synusb_setup_iurb (struct synusb *synusb, + struct usb_endpoint_descriptor*endpoint) +{
drivers/input/mouse/synaptics_usb.c new file +4
-0+char *buf; + +if (endpoint->wMaxPacketSize <8 ) +return 0 ;How could this happen?
The id table can be modified via sysfs, so it is easily possible to try this driver with officially unsupported devices. (At least for Synaptics devices, there is a good chance this driver works just the same.) The condition I put there was the only one I could think of to select when it is not worth trying.
[...]
Quoted 8 lines
drivers/input/mouse/synaptics_usb.c line 498
Quoted 6 lines
+static int
synusb_suspend(struct usb_interface *intf, pm_message_t message)+{drivers/input/mouse/synaptics_usb.c new file +4
-0+struct synusb *synusb =usb_get_intfdata (intf); + +if (synusb ==NULL ) +return 0 ;How can this happen?
According to the Documentation this can not happen. However, this check is present in usb-skeleton.c, so I put it there just in case I overlooked something.
[...]
Quoted 6 lines
Quoted 6 lines
drivers/input/mouse/synaptics_usb.c new file +6
-0+static struct usb_driver synusb_driver = { + .name ="synaptics_usb" , + .probe = synusb_probe, + .disconnect = synusb_disconnect, + .id_table = synusb_idtable, + .suspend = synusb_suspend,
Quoted 8 lines
drivers/input/mouse/synaptics_usb.c line 567
Quoted 5 lines
drivers/input/mouse/synaptics_usb.c new file +5
-0+ .resume = synusb_resume, + .pre_reset = synusb_pre_reset, + .post_reset = synusb_post_reset, + .reset_resume = synusb_reset_resume, + .supports_autosuspend = 1,Yet you do not manage the busy state. Do you really want to play ping-pong with the power state?
The last time I tested autosuspend its timer was reseted "automatically" every time an int urb arrived. Isn't this all that is needed for an input device?
The autosuspend delay should of course be set to a reasonably large value. Then there should be no ping-pong with the power state while the device is actually used/touched. On the other hand, if the pad is not touched for a while, then no int urb arrives and the device will autosuspend (and, thanks to remote wakeup, autoresume nicely when touched again).
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 3 Jan 2012, Jan Steinhoff wrote:
[ ... snip ... ]
Quoted 9 lines
drivers/input/mouse/synaptics_usb.c new file +9
-0+static int synusb_setup_iurb (struct synusb *synusb, +struct usb_endpoint_descriptor *endpoint) +{ +char *buf; + +if (endpoint->wMaxPacketSize <8 ) +return 0 ; +if (synusb->iurb) { + synusb_warn(synusb, "Found more than one possible "
drivers/input/mouse/synaptics_usb.c line 299
Quoted 10 lines
drivers/input/mouse/synaptics_usb.c new file +10
-0+ "int in endpoint"); +return 0 ; + } + synusb->iurb =usb_alloc_urb (0 ,GFP_KERNEL ); +if (synusb->iurb ==NULL ) +return -ENOMEM ; + buf =usb_alloc_coherent (synusb->udev,8 ,GFP_ATOMIC , + &synusb->iurb->transfer_dma); +if (buf ==NULL ) +return -ENOMEM ;
You seem to leak synusb->iurb here.
Signature · 2 lines
Jiri Kosina SUSE Labs
Am Mittwoch, 4. Januar 2012, 09:55:19 schrieb Jiri Kosina:
Quoted 16 lines · Jan Steinhoff
[ ... snip ... ]
Quoted 13 lines
drivers/input/mouse/synaptics_usb.c new file +13
-0+static int synusb_setup_iurb (struct synusb *synusb, +struct usb_endpoint_descriptor *endpoint) +{ +char *buf; + +if (endpoint->wMaxPacketSize <8 ) +return 0 ; +if (synusb->iurb) { +synusb_warn (synusb,"Found more than one possible " +"int in endpoint" ); +return 0 ; + } + synusb->iurb =usb_alloc_urb (0 ,GFP_KERNEL );
Quoted 8 lines
drivers/input/mouse/synaptics_usb.c line 299
Quoted 6 lines
drivers/input/mouse/synaptics_usb.c new file +6
-0+if (synusb->iurb ==NULL ) +return -ENOMEM ; + buf =usb_alloc_coherent (synusb->udev,8 ,GFP_ATOMIC , + &synusb->iurb->transfer_dma); +if (buf ==NULL ) +return -ENOMEM ;You seem to leak
synusb->iurbhere.
Seems so, but doesn't. It gets freed when the refcount hits 0. Now, whether this is a good technique is another question.
Regards
Oliver
Signature · 10 lines
- - - SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) Maxfeldstraße 5 90409 Nürnberg Germany - - - -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov
maintainerQuoted 26 lines · Oliver Neukum
Am Mittwoch, 4. Januar 2012, 09:55:19 schrieb Jiri Kosina:
Quoted 16 lines · Jan Steinhoff
[ ... snip ... ]
Quoted 13 lines
(quoted diff) +13
-0+static int synusb_setup_iurb(struct synusb *synusb, + struct usb_endpoint_descriptor *endpoint) +{ + char *buf; + + if (endpoint->wMaxPacketSize < 8) + return 0; + if (synusb->iurb) { + synusb_warn(synusb, "Found more than one possible " + "int in endpoint"); + return 0; + } + synusb->iurb = usb_alloc_urb(0, GFP_KERNEL);Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ if (synusb->iurb == NULL) + return -ENOMEM; + buf = usb_alloc_coherent(synusb->udev, 8, GFP_ATOMIC, + &synusb->iurb->transfer_dma); + if (buf == NULL) + return -ENOMEM;You seem to leak
synusb->iurbhere.
Quoted 2 lines
Seems so, but doesn't. It gets freed when the refcount hits 0. Now, whether this is a good technique is another question.
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);
- added devices to HID blacklist;
- runtime PM (hopefully I got it right);
- open/close.
Still TODO:
- query device for supported rages instead of having module parameters.
Thanks.
Signature · 28 lines
Dmitry Input: Synaptics USB device driver From: Jan Steinhoff <[email protected]> This patch adds a driver for Synaptics USB touchpad or pointing stick devices. These USB devices emulate an USB mouse by default, so one can also use the usbhid driver. However, in combination with special user space drivers this kernel driver allows one to customize the behaviour of the device. An extended version of this driver with support for the cPad background display can be found at <http://jan-steinhoff.de/linux/synaptics-usb.html>. Signed-off-by: Jan Steinhoff <[email protected]> Signed-off-by: Dmitry Torokhov <[email protected]> --- drivers/hid/hid-core.c | 9 + drivers/hid/hid-ids.h | 11 + drivers/input/mouse/Kconfig | 14 + drivers/input/mouse/Makefile | 1 drivers/input/mouse/synaptics_usb.c | 529 +++++++++++++++++++++++++++++++++++ 5 files changed, 564 insertions(+), 0 deletions(-) create mode 100644 drivers/input/mouse/synaptics_usb.c
drivers/hid/hid-core.c +9 -0
@@ -1892,6 +1892,15 @@ static const struct hid_device_id hid_ignore_list[] = { { HID_USB_DEVICE(USB_VENDOR_ID_PANJIT, 0x0004) }, { HID_USB_DEVICE(USB_VENDOR_ID_PHILIPS, USB_DEVICE_ID_PHILIPS_IEEE802154_DONGLE) }, { HID_USB_DEVICE(USB_VENDOR_ID_POWERCOM, USB_DEVICE_ID_POWERCOM_UPS) }, + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_TP) }, + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_INT_TP) }, + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_CPAD) }, + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_TS) }, + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_STICK) }, + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_WP) }, + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_COMP_TP) }, + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_WTP) }, + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_DPAD) }, { HID_USB_DEVICE(USB_VENDOR_ID_VERNIER, USB_DEVICE_ID_VERNIER_LABPRO) }, { HID_USB_DEVICE(USB_VENDOR_ID_VERNIER, USB_DEVICE_ID_VERNIER_GOTEMP) }, { HID_USB_DEVICE(USB_VENDOR_ID_VERNIER, USB_DEVICE_ID_VERNIER_SKIP) },
drivers/hid/hid-ids.h +11 -0
@@ -633,6 +633,17 @@#define USB_DEVICE_ID_SYMBOL_SCANNER_1 0x0800#define USB_DEVICE_ID_SYMBOL_SCANNER_2 0x1300 +#define USB_VENDOR_ID_SYNAPTICS 0x06cb +#define USB_DEVICE_ID_SYNAPTICS_TP 0x0001 +#define USB_DEVICE_ID_SYNAPTICS_INT_TP 0x0002 +#define USB_DEVICE_ID_SYNAPTICS_CPAD 0x0003 +#define USB_DEVICE_ID_SYNAPTICS_TS 0x0006 +#define USB_DEVICE_ID_SYNAPTICS_STICK 0x0007 +#define USB_DEVICE_ID_SYNAPTICS_WP 0x0008 +#define USB_DEVICE_ID_SYNAPTICS_COMP_TP 0x0009 +#define USB_DEVICE_ID_SYNAPTICS_WTP 0x0010 +#define USB_DEVICE_ID_SYNAPTICS_DPAD 0x0013 +#define USB_VENDOR_ID_THRUSTMASTER 0x044f#define USB_VENDOR_ID_TOPSEED 0x0766
drivers/input/mouse/Kconfig +14 -0
@@ -322,4 +322,18 @@ config MOUSE_SYNAPTICS_I2C To compile this driver as a module, choose M here: the module will be called synaptics_i2c. +config MOUSE_SYNAPTICS_USB + tristate "Synaptics USB device support" + depends on USB + help + Say Y here if you want to use a Synaptics USB touchpad or pointing stick. + These Synaptics USB devices emulate an USB mouse by default, so you can + also use the usbhid driver. However, in combination with special X.Org + drivers this kernel driver allows you to customize the behaviour of the + device. More information can be found at: + <http://jan-steinhoff.de/linux/synaptics-usb.html> + + To compile this driver as a module, choose M here: the + module will be called synaptics_usb. + endif
drivers/input/mouse/Makefile +1 -0
@@ -18,6 +18,7 @@ obj-$(CONFIG_MOUSE_PXA930_TRKBALL) += pxa930_trkball.o obj-$(CONFIG_MOUSE_RISCPC) += rpcmouse.o obj-$(CONFIG_MOUSE_SERIAL) += sermouse.o obj-$(CONFIG_MOUSE_SYNAPTICS_I2C) += synaptics_i2c.o +obj-$(CONFIG_MOUSE_SYNAPTICS_USB) += synaptics_usb.o obj-$(CONFIG_MOUSE_VSXXXAA) += vsxxxaa.o psmouse-objs := psmouse-base.o synaptics.o
drivers/input/mouse/synaptics_usb.c new file +529 -0
529 lines not on this page · raw message
Am Mittwoch, 4. Januar 2012, 10:41:03 schrieb Dmitry Torokhov:
Quoted 5 lines
drivers/input/mouse/synaptics_usb.c new file +5
-0+static void synusb_irq (struct urb *urb) +{ +struct synusb *synusb = urb->context; +int error; +
drivers/input/mouse/synaptics_usb.c line 210
Quoted 10 lines
drivers/input/mouse/synaptics_usb.c new file +10
-0+/* Check our status in case we need to bail out early. */ +switch (urb->status) { +case 0 : +usb_mark_last_busy (synusb->udev); +break ; + +case -EOVERFLOW : +dev_err (&synusb->intf->dev, +"%s: OVERFLOW with data length %d, actual length is %d\n" , + __func__,SYNUSB_RECV_SIZE , urb->actual_length);
Do you really want to fall through here?
Quoted 25 lines
drivers/input/mouse/synaptics_usb.c new file +25
-0+ +/* Device went away so don't keep trying to read from it. */ +case -ECONNRESET : +case -ENOENT : +case -ESHUTDOWN : +return ; + +default : + goto resubmit; +break ; + } + +if (synusb->flags &SYNUSB_STICK ) +synusb_report_stick (synusb); +else +synusb_report_touchpad (synusb); + +resubmit: + error =usb_submit_urb (urb,GFP_ATOMIC ); +if (error && error != -EPERM ) +dev_err (&synusb->intf->dev, +"%s - usb_submit_urb failed with result: %d" , + __func__, error); +} +
[..]
Quoted 66 lines
drivers/input/mouse/synaptics_usb.c new file +66
-0+static int synusb_open (struct input_dev *dev) +{ +struct synusb *synusb =input_get_drvdata (dev); +int retval =0 ; + +dev_err (&synusb->intf->dev,"%s\n" , __func__); + +if (usb_autopm_get_interface (synusb->intf) <0 ) +return -EIO ; + +if (usb_submit_urb (synusb->urb,GFP_KERNEL )) { +dev_err (&synusb->intf->dev, +"%s - usb_submit_urb failed" , __func__); + retval = -EIO ; + goto out; + } + + synusb->intf->needs_remote_wakeup =1 ; + +out: +usb_autopm_put_interface (synusb->intf); +dev_err (&synusb->intf->dev,"%s done: %d\n" , __func__, retval); +return retval; +} + +static void synusb_close (struct input_dev *dev) +{ +struct synusb *synusb =input_get_drvdata (dev); +int autopm_error; + + autopm_error =usb_autopm_get_interface (synusb->intf); + +usb_kill_urb (synusb->urb); + synusb->intf->needs_remote_wakeup =0 ; + +if (!autopm_error) +usb_autopm_put_interface (synusb->intf); +} + +static int synusb_probe (struct usb_interface *intf, +const struct usb_device_id *id) +{ +struct usb_device *udev =interface_to_usbdev (intf); +struct usb_endpoint_descriptor *ep; +struct synusb *synusb; +struct input_dev *input_dev; +unsigned int intf_num = intf->cur_altsetting->desc.bInterfaceNumber; +unsigned int altsetting =min (intf->num_altsetting,1U ); +int error; + + error =usb_set_interface (udev, intf_num, altsetting); +if (error) { +dev_err (&udev->dev, +"Can not set alternate setting to %i, error: %i" , + altsetting, error); +return error; + } + + ep =synusb_get_in_endpoint (intf->cur_altsetting); +if (!ep) +return -ENODEV ; + + synusb =kzalloc (sizeof (*synusb),GFP_KERNEL ); + input_dev =input_allocate_device (); +if (!synusb || !input_dev) { + error = -ENOMEM ;
drivers/input/mouse/synaptics_usb.c line 330
Quoted 10 lines
drivers/input/mouse/synaptics_usb.c new file +10
-0+ goto err_free_mem; + } + + synusb->udev = udev; + synusb->intf = intf; + synusb->input = input_dev; + + synusb->flags = id->driver_info; +if (test_bit (SYNUSB_STICK , &synusb->flags) && +test_bit (SYNUSB_STICK , &synusb->flags)) {
??? Voodoo ???
Quoted 17 lines
drivers/input/mouse/synaptics_usb.c new file +17
-0+/* +* This is a combo device, we need to leave only proper +* capability, depending on the interface. +*/ +clear_bit (intf_num ==1 ?SYNUSB_TOUCHPAD :SYNUSB_STICK , + &synusb->flags); + } + + synusb->urb =usb_alloc_urb (0 ,GFP_KERNEL ); +if (!synusb->urb) { + error = -ENOMEM ; + goto err_free_mem; + } + + synusb->data =usb_alloc_coherent (udev,SYNUSB_RECV_SIZE ,GFP_KERNEL , + &synusb->urb->transfer_dma); +if (!synusb->data) {
drivers/input/mouse/synaptics_usb.c line 357
Quoted 10 lines
drivers/input/mouse/synaptics_usb.c new file +10
-0+ error = -ENOMEM ; + goto err_free_urb; + } + +usb_fill_int_urb (synusb->urb, udev, +usb_rcvintpipe (udev, ep->bEndpointAddress), + synusb->data,SYNUSB_RECV_SIZE , + synusb_irq, synusb, + ep->bInterval); + synusb->urb->transfer_flags |=URB_NO_TRANSFER_DMA_MAP ;
According to the comment in the original driver you must submit the URB. Are you sure not doing so is save?
Quoted 80 lines
drivers/input/mouse/synaptics_usb.c new file +80
-0+ +if (udev->manufacturer) +strlcpy (synusb->name, udev->manufacturer, +sizeof (synusb->name)); + +if (udev->product) { +if (udev->manufacturer) +strlcat (synusb->name," " ,sizeof (synusb->name)); +strlcat (synusb->name, udev->product,sizeof (synusb->name)); + } + +if (!strlen (synusb->name)) +snprintf (synusb->name,sizeof (synusb->name), +"USB Synaptics Device %04x:%04x" , +le16_to_cpu (udev->descriptor.idVendor), +le16_to_cpu (udev->descriptor.idProduct)); + +if (synusb->flags &SYNUSB_STICK ) +strlcat (synusb->name," (Stick) " ,sizeof (synusb->name)); + +usb_make_path (udev, synusb->phys,sizeof (synusb->phys)); +strlcat (synusb->phys,"/input0" ,sizeof (synusb->phys)); + + input_dev->name = synusb->name;// synusb_get_name(synusb); + input_dev->phys = synusb->phys; +usb_to_input_id (udev, &input_dev->id); + input_dev->dev.parent = &synusb->intf->dev; + + input_dev->open = synusb_open; + input_dev->close = synusb_close; + +input_set_drvdata (input_dev, synusb); + +__set_bit (EV_ABS , input_dev->evbit); +__set_bit (EV_KEY , input_dev->evbit); + +if (synusb->flags &SYNUSB_STICK ) { +__set_bit (EV_REL , input_dev->evbit); +__set_bit (REL_X , input_dev->relbit); +__set_bit (REL_Y , input_dev->relbit); +input_set_abs_params (input_dev,ABS_PRESSURE ,0 ,127 ,0 ,0 ); + }else { +input_set_abs_params (input_dev,ABS_X , xmin, xmax,0 ,0 ); +input_set_abs_params (input_dev,ABS_Y , ymin, ymax,0 ,0 ); +input_set_abs_params (input_dev,ABS_PRESSURE ,0 ,255 ,0 ,0 ); +input_set_abs_params (input_dev,ABS_TOOL_WIDTH ,0 ,15 ,0 ,0 ); +__set_bit (BTN_TOUCH , input_dev->keybit); +__set_bit (BTN_TOOL_FINGER , input_dev->keybit); +__set_bit (BTN_TOOL_DOUBLETAP , input_dev->keybit); +__set_bit (BTN_TOOL_TRIPLETAP , input_dev->keybit); + } + +__set_bit (BTN_LEFT , input_dev->keybit); +__set_bit (BTN_RIGHT , input_dev->keybit); +__set_bit (BTN_MIDDLE , input_dev->keybit); +if (synusb->flags &SYNUSB_AUXDISPLAY ) +__set_bit (BTN_MISC , input_dev->keybit); + + error =input_register_device (input_dev); +if (error) { +dev_err (&udev->dev,"Failed to register input device, error %d\n" , + error); + goto err_free_dma; + } + +usb_set_intfdata (intf, synusb); +return 0 ; + +err_free_dma: +usb_free_coherent (udev,SYNUSB_RECV_SIZE , synusb->data, + synusb->urb->transfer_dma); +err_free_urb: +usb_free_urb (synusb->urb); +err_free_mem: +input_free_device (input_dev); +kfree (synusb); + +return error; +} +
[..]
drivers/input/mouse/synaptics_usb.c line 508
Quoted 8 lines
drivers/input/mouse/synaptics_usb.c new file +8
-0+static struct usb_driver synusb_driver = { + .name ="synaptics_usb" , + .probe = synusb_probe, + .disconnect = synusb_disconnect, + .id_table = synusb_idtable, + .suspend = synusb_suspend, + .resume = synusb_resume, + .reset_resume = synusb_reset_resume,
You've killed the support for reset. That is not cool.
drivers/input/mouse/synaptics_usb.c line 510
Quoted 2 lines
drivers/input/mouse/synaptics_usb.c new file +2
-0+ .supports_autosuspend = 1, +};
Regards
Oliver
Dmitry Torokhov
maintainerQuoted 12 lines · Oliver Neukum
Am Mittwoch, 4. Januar 2012, 10:41:03 schrieb Dmitry Torokhov:
Quoted 9 lines
drivers/input/mouse/synaptics_usb.c new file +9
-0+static void synusb_irq (struct urb *urb) +{ +struct synusb *synusb = urb->context; +int error; + +/* Check our status in case we need to bail out early. */ +switch (urb->status) { +case 0 : +usb_mark_last_busy (synusb->udev);
Quoted 8 lines
drivers/input/mouse/synaptics_usb.c line 210
Quoted 6 lines
drivers/input/mouse/synaptics_usb.c new file +6
-0+break ; + +case -EOVERFLOW : +dev_err (&synusb->intf->dev, +"%s: OVERFLOW with data length %d, actual length is %d\n" , + __func__,SYNUSB_RECV_SIZE , urb->actual_length);Do you really want to fall through here?
Probably not.
Quoted 98 lines
Quoted 26 lines
drivers/input/mouse/synaptics_usb.c new file +25
-0+ +/* Device went away so don't keep trying to read from it. */ +case -ECONNRESET : +case -ENOENT : +case -ESHUTDOWN : +return ; + +default : + goto resubmit; +break ; + } + +if (synusb->flags &SYNUSB_STICK ) +synusb_report_stick (synusb); +else +synusb_report_touchpad (synusb); + +resubmit: + error =usb_submit_urb (urb,GFP_ATOMIC ); +if (error && error != -EPERM ) +dev_err (&synusb->intf->dev, +"%s - usb_submit_urb failed with result: %d" , + __func__, error); +} +[..]
Quoted 70 lines
drivers/input/mouse/synaptics_usb.c new file +70
-0+static int synusb_open (struct input_dev *dev) +{ +struct synusb *synusb =input_get_drvdata (dev); +int retval =0 ; + +dev_err (&synusb->intf->dev,"%s\n" , __func__); + +if (usb_autopm_get_interface (synusb->intf) <0 ) +return -EIO ; + +if (usb_submit_urb (synusb->urb,GFP_KERNEL )) { +dev_err (&synusb->intf->dev, +"%s - usb_submit_urb failed" , __func__); + retval = -EIO ; + goto out; + } + + synusb->intf->needs_remote_wakeup =1 ; + +out: +usb_autopm_put_interface (synusb->intf); +dev_err (&synusb->intf->dev,"%s done: %d\n" , __func__, retval); +return retval; +} + +static void synusb_close (struct input_dev *dev) +{ +struct synusb *synusb =input_get_drvdata (dev); +int autopm_error; + + autopm_error =usb_autopm_get_interface (synusb->intf); + +usb_kill_urb (synusb->urb); + synusb->intf->needs_remote_wakeup =0 ; + +if (!autopm_error) +usb_autopm_put_interface (synusb->intf); +} + +static int synusb_probe (struct usb_interface *intf, +const struct usb_device_id *id) +{ +struct usb_device *udev =interface_to_usbdev (intf); +struct usb_endpoint_descriptor *ep; +struct synusb *synusb; +struct input_dev *input_dev; +unsigned int intf_num = intf->cur_altsetting->desc.bInterfaceNumber; +unsigned int altsetting =min (intf->num_altsetting,1U ); +int error; + + error =usb_set_interface (udev, intf_num, altsetting); +if (error) { +dev_err (&udev->dev, +"Can not set alternate setting to %i, error: %i" , + altsetting, error); +return error; + } + + ep =synusb_get_in_endpoint (intf->cur_altsetting); +if (!ep) +return -ENODEV ; + + synusb =kzalloc (sizeof (*synusb),GFP_KERNEL ); + input_dev =input_allocate_device (); +if (!synusb || !input_dev) { + error = -ENOMEM ; + goto err_free_mem; + } + + synusb->udev = udev;
Quoted 8 lines
drivers/input/mouse/synaptics_usb.c line 330
Quoted 6 lines
drivers/input/mouse/synaptics_usb.c new file +6
-0+ synusb->intf = intf; + synusb->input = input_dev; + + synusb->flags = id->driver_info; +if (test_bit (SYNUSB_STICK , &synusb->flags) && +test_bit (SYNUSB_STICK , &synusb->flags)) {??? Voodoo ???
Yeah, 2nd should be test_bit(SYNUSB_TOUCHPAD, ...)
Quoted 23 lines
Quoted 23 lines
drivers/input/mouse/synaptics_usb.c new file +22
-0+/* +* This is a combo device, we need to leave only proper +* capability, depending on the interface. +*/ +clear_bit (intf_num ==1 ?SYNUSB_TOUCHPAD :SYNUSB_STICK , + &synusb->flags); + } + + synusb->urb =usb_alloc_urb (0 ,GFP_KERNEL ); +if (!synusb->urb) { + error = -ENOMEM ; + goto err_free_mem; + } + + synusb->data =usb_alloc_coherent (udev,SYNUSB_RECV_SIZE ,GFP_KERNEL , + &synusb->urb->transfer_dma); +if (!synusb->data) { + error = -ENOMEM ; + goto err_free_urb; + } + + usb_fill_int_urb(synusb->urb, udev,
Quoted 8 lines
drivers/input/mouse/synaptics_usb.c line 357
Quoted 5 lines
drivers/input/mouse/synaptics_usb.c new file +5
-0+usb_rcvintpipe (udev, ep->bEndpointAddress), + synusb->data,SYNUSB_RECV_SIZE , + synusb_irq, synusb, + ep->bInterval); + synusb->urb->transfer_flags |=URB_NO_TRANSFER_DMA_MAP ;According to the comment in the original driver you must submit the URB. Are you sure not doing so is save?
Seems to work here...
Quoted 83 lines
Quoted 81 lines
drivers/input/mouse/synaptics_usb.c new file +80
-0+ +if (udev->manufacturer) +strlcpy (synusb->name, udev->manufacturer, +sizeof (synusb->name)); + +if (udev->product) { +if (udev->manufacturer) +strlcat (synusb->name," " ,sizeof (synusb->name)); +strlcat (synusb->name, udev->product,sizeof (synusb->name)); + } + +if (!strlen (synusb->name)) +snprintf (synusb->name,sizeof (synusb->name), +"USB Synaptics Device %04x:%04x" , +le16_to_cpu (udev->descriptor.idVendor), +le16_to_cpu (udev->descriptor.idProduct)); + +if (synusb->flags &SYNUSB_STICK ) +strlcat (synusb->name," (Stick) " ,sizeof (synusb->name)); + +usb_make_path (udev, synusb->phys,sizeof (synusb->phys)); +strlcat (synusb->phys,"/input0" ,sizeof (synusb->phys)); + + input_dev->name = synusb->name;// synusb_get_name(synusb); + input_dev->phys = synusb->phys; +usb_to_input_id (udev, &input_dev->id); + input_dev->dev.parent = &synusb->intf->dev; + + input_dev->open = synusb_open; + input_dev->close = synusb_close; + +input_set_drvdata (input_dev, synusb); + +__set_bit (EV_ABS , input_dev->evbit); +__set_bit (EV_KEY , input_dev->evbit); + +if (synusb->flags &SYNUSB_STICK ) { +__set_bit (EV_REL , input_dev->evbit); +__set_bit (REL_X , input_dev->relbit); +__set_bit (REL_Y , input_dev->relbit); +input_set_abs_params (input_dev,ABS_PRESSURE ,0 ,127 ,0 ,0 ); + }else { +input_set_abs_params (input_dev,ABS_X , xmin, xmax,0 ,0 ); +input_set_abs_params (input_dev,ABS_Y , ymin, ymax,0 ,0 ); +input_set_abs_params (input_dev,ABS_PRESSURE ,0 ,255 ,0 ,0 ); +input_set_abs_params (input_dev,ABS_TOOL_WIDTH ,0 ,15 ,0 ,0 ); +__set_bit (BTN_TOUCH , input_dev->keybit); +__set_bit (BTN_TOOL_FINGER , input_dev->keybit); +__set_bit (BTN_TOOL_DOUBLETAP , input_dev->keybit); +__set_bit (BTN_TOOL_TRIPLETAP , input_dev->keybit); + } + +__set_bit (BTN_LEFT , input_dev->keybit); +__set_bit (BTN_RIGHT , input_dev->keybit); +__set_bit (BTN_MIDDLE , input_dev->keybit); +if (synusb->flags &SYNUSB_AUXDISPLAY ) +__set_bit (BTN_MISC , input_dev->keybit); + + error =input_register_device (input_dev); +if (error) { +dev_err (&udev->dev,"Failed to register input device, error %d\n" , + error); + goto err_free_dma; + } + +usb_set_intfdata (intf, synusb); +return 0 ; + +err_free_dma: +usb_free_coherent (udev,SYNUSB_RECV_SIZE , synusb->data, + synusb->urb->transfer_dma); +err_free_urb: +usb_free_urb (synusb->urb); +err_free_mem: +input_free_device (input_dev); +kfree (synusb); + +return error; +} +[..]
Quoted 10 lines
drivers/input/mouse/synaptics_usb.c line 508
Quoted 8 lines
drivers/input/mouse/synaptics_usb.c new file +8
-0+static struct usb_driver synusb_driver = { + .name ="synaptics_usb" , + .probe = synusb_probe, + .disconnect = synusb_disconnect, + .id_table = synusb_idtable, + .suspend = synusb_suspend, + .resume = synusb_resume, + .reset_resume = synusb_reset_resume,You've killed the support for reset. That is not cool.
OK.
Thanks.
Signature · 1 line
Dmitry
Quoted 5 lines · On Wed, 4 Jan 2012 02:05:53 -0800 Dmitry Torokhov <[email protected]>…
Quoted 4 lines · Oliver Neukum
Am Mittwoch, 4. Januar 2012, 10:41:03 schrieb Dmitry Torokhov:
Quoted 3 lines
drivers/input/mouse/synaptics_usb.c new file +3
-0+static int synusb_probe (struct usb_interface *intf, +const struct usb_device_id *id) +{
[...]
Quoted 26 lines
Quoted 17 lines
Quoted 17 lines
drivers/input/mouse/synaptics_usb.c new file +7
-0+ synusb->urb =usb_alloc_urb (0 ,GFP_KERNEL ); +if (!synusb->urb) { + error = -ENOMEM ; + goto err_free_mem; + } + + synusb->data = usb_alloc_coherent(udev,
SYNUSB_RECV_SIZE,GFP_KERNEL,
+&synusb->urb->transfer_dma);drivers/input/mouse/synaptics_usb.c new file +7
-0+if (!synusb->data) { + error = -ENOMEM ; + goto err_free_urb; + } + + usb_fill_int_urb(synusb->urb, udev, + usb_rcvintpipe(udev,Quoted 8 lines
drivers/input/mouse/synaptics_usb.c line 357
Quoted 5 lines
ep->bEndpointAddress),drivers/input/mouse/synaptics_usb.c new file +4
-0+ synusb->data,SYNUSB_RECV_SIZE , + synusb_irq, synusb, + ep->bInterval); + synusb->urb->transfer_flags |=URB_NO_TRANSFER_DMA_MAP ;According to the comment in the original driver you must submit the URB. Are you sure not doing so is save?
Quoted 1 line
Seems to work here...
Let me comment on this issue in more detail, so you can better judge what is the "right" solution here.
Both ways actually work. But, at least for the cPad, the device will pretend it got reconnected if the int urb is not fetched (while not suspended, of course). This means it would disconnect if no input device is opened (AFAIK most X.org input drivers close the devices when one switches to console) and one touches the pad (actually, a slight breeze of air might be enough, it is very sensitive!). Another annoying side effect in the case of the cPad is that the background light flashes on every reconnect.
Dmitry Torokhov
maintainerQuoted 39 lines · Jan Steinhoff
Quoted 5 lines · On Wed, 4 Jan 2012 02:05:53 -0800 Dmitry Torokhov <[email protected]>…
Quoted 4 lines · Oliver Neukum
Am Mittwoch, 4. Januar 2012, 10:41:03 schrieb Dmitry Torokhov:
Quoted 3 lines
drivers/input/mouse/synaptics_usb.c new file +3
-0+static int synusb_probe (struct usb_interface *intf, +const struct usb_device_id *id) +{[...]
Quoted 26 lines
Quoted 17 lines
Quoted 17 lines
drivers/input/mouse/synaptics_usb.c new file +7
-0+ synusb->urb =usb_alloc_urb (0 ,GFP_KERNEL ); +if (!synusb->urb) { + error = -ENOMEM ; + goto err_free_mem; + } + + synusb->data = usb_alloc_coherent(udev,
SYNUSB_RECV_SIZE,GFP_KERNEL,
+&synusb->urb->transfer_dma);drivers/input/mouse/synaptics_usb.c new file +7
-0+if (!synusb->data) { + error = -ENOMEM ; + goto err_free_urb; + } + + usb_fill_int_urb(synusb->urb, udev, + usb_rcvintpipe(udev,Quoted 8 lines
drivers/input/mouse/synaptics_usb.c line 357
Quoted 5 lines
ep->bEndpointAddress),drivers/input/mouse/synaptics_usb.c new file +4
-0+ synusb->data,SYNUSB_RECV_SIZE , + synusb_irq, synusb, + ep->bInterval); + synusb->urb->transfer_flags |=URB_NO_TRANSFER_DMA_MAP ;According to the comment in the original driver you must submit the URB. Are you sure not doing so is save?
Quoted 1 line
Seems to work here...
Let me comment on this issue in more detail, so you can better judge what is the "right" solution here.
Quoted 8 lines
Both ways actually work. But, at least for the cPad, the device will pretend it got reconnected if the int urb is not fetched (while not suspended, of course). This means it would disconnect if no input device is opened (AFAIK most X.org input drivers close the devices when one switches to console) and one touches the pad (actually, a slight breeze of air might be enough, it is very sensitive!). Another annoying side effect in the case of the cPad is that the background light flashes on every reconnect.
OK, then maybe we should have cPad start transfers immediately and let other devices do it in open. There is no reason to do work if nobody interested in it...
Another option is to move configuring interface to open()?
Thanks.
Signature · 1 line
Dmitry
Quoted 50 lines · On Wed, 4 Jan 2012 22:36:11 -0800 Dmitry Torokhov <[email protected]>…
Quoted 40 lines · Jan Steinhoff
Quoted 5 lines · On Wed, 4 Jan 2012 02:05:53 -0800 Dmitry Torokhov <[email protected]>…
Quoted 4 lines · Oliver Neukum
Am Mittwoch, 4. Januar 2012, 10:41:03 schrieb Dmitry Torokhov:
Quoted 3 lines
drivers/input/mouse/synaptics_usb.c new file +3
-0+static int synusb_probe (struct usb_interface *intf, +const struct usb_device_id *id) +{[...]
Quoted 27 lines
Quoted 18 lines
Quoted 18 lines
drivers/input/mouse/synaptics_usb.c new file +7
-0+ synusb->urb =usb_alloc_urb (0 ,GFP_KERNEL ); +if (!synusb->urb) { + error = -ENOMEM ; + goto err_free_mem; + } + + synusb->data = usb_alloc_coherent(udev,
SYNUSB_RECV_SIZE,GFP_KERNEL,
+&synusb->urb->transfer_dma);drivers/input/mouse/synaptics_usb.c new file +7
-0+if (!synusb->data) { + error = -ENOMEM ; + goto err_free_urb; + } + + usb_fill_int_urb(synusb->urb, udev, + usb_rcvintpipe(udev,
ep->bEndpointAddress),Quoted 8 lines
Quoted 5 lines
drivers/input/mouse/synaptics_usb.c new file +4
-0+ synusb->data,SYNUSB_RECV_SIZE , + synusb_irq, synusb, + ep->bInterval); + synusb->urb->transfer_flags |=
URB_NO_TRANSFER_DMA_MAP;According to the comment in the original driver you must submit the URB. Are you sure not doing so is save?
Quoted 1 line
Seems to work here...
Let me comment on this issue in more detail, so you can better judge what is the "right" solution here.
Quoted 8 lines
Both ways actually work. But, at least for the cPad, the device will pretend it got reconnected if the int urb is not fetched (while not suspended, of course). This means it would disconnect if no input device is opened (AFAIK most X.org input drivers close the devices when one switches to console) and one touches the pad (actually, a slight breeze of air might be enough, it is very sensitive!). Another annoying side effect in the case of the cPad is that the background light flashes on every reconnect.
Quoted 3 lines
OK, then maybe we should have cPad start transfers immediately and let other devices do it in open. There is no reason to do work if nobody interested in it...
Given that the devices are very similar, I expect that also the other devices may behave like this. So this is maybe not only a cPad related problem. But I have never investigated this for other devices, so I am not sure.
Regarding the extra amount of work, I thought it is finally less expensive to run through the callback and drop the 8 bytes of data, than to let the device trigger a disconnect, probe, and associated device deletion/creation by udev every 60 sec or so (it maybe even produces more USB traffic).
Quoted 1 line
Another option is to move configuring interface to
open()?
The reconnect will always happen if the int endpoint is not "in use". So configuring the device not in probe will not help, will it?
I think the best way would be to immediately suspend the device, if there is an easy way to trigger this using the driver or usb core. (And if there is no easy way, it is still possible to do it via sysfs if someone is annoyed by this minor "reconnect bug".) Further, this should be fine for all devices, whether they have this "reconnect bug" or not.
Dmitry Torokhov
maintainerQuoted 61 lines · Jan Steinhoff
Quoted 50 lines · On Wed, 4 Jan 2012 22:36:11 -0800 Dmitry Torokhov <[email protected]>…
Quoted 40 lines · Jan Steinhoff
Quoted 5 lines · On Wed, 4 Jan 2012 02:05:53 -0800 Dmitry Torokhov <[email protected]>…
Quoted 4 lines · Oliver Neukum
Am Mittwoch, 4. Januar 2012, 10:41:03 schrieb Dmitry Torokhov:
Quoted 3 lines
(quoted diff) +3
-0+static int synusb_probe(struct usb_interface *intf, + const struct usb_device_id *id) +{[...]
Quoted 27 lines
Quoted 18 lines
Quoted 18 lines
(quoted diff) +7
-0+ synusb->urb = usb_alloc_urb(0, GFP_KERNEL); + if (!synusb->urb) { + error = -ENOMEM; + goto err_free_mem; + } + + synusb->data = usb_alloc_coherent(udev,
SYNUSB_RECV_SIZE,GFP_KERNEL,
+&synusb->urb->transfer_dma);(quoted diff) +7
-0+ if (!synusb->data) { + error = -ENOMEM; + goto err_free_urb; + } + + usb_fill_int_urb(synusb->urb, udev, + usb_rcvintpipe(udev,
ep->bEndpointAddress),Quoted 8 lines
Quoted 5 lines
(quoted diff) +4
-0+ synusb->data, SYNUSB_RECV_SIZE, + synusb_irq, synusb, + ep->bInterval); + synusb->urb->transfer_flags |=
URB_NO_TRANSFER_DMA_MAP;According to the comment in the original driver you must submit the URB. Are you sure not doing so is save?
Quoted 1 line
Seems to work here...
Let me comment on this issue in more detail, so you can better judge what is the "right" solution here.
Quoted 8 lines
Both ways actually work. But, at least for the cPad, the device will pretend it got reconnected if the int urb is not fetched (while not suspended, of course). This means it would disconnect if no input device is opened (AFAIK most X.org input drivers close the devices when one switches to console) and one touches the pad (actually, a slight breeze of air might be enough, it is very sensitive!). Another annoying side effect in the case of the cPad is that the background light flashes on every reconnect.
Quoted 3 lines
OK, then maybe we should have cPad start transfers immediately and let other devices do it in open. There is no reason to do work if nobody interested in it...
Given that the devices are very similar, I expect that also the other devices may behave like this. So this is maybe not only a cPad related problem. But I have never investigated this for other devices, so I am not sure.
Quoted 5 lines
Regarding the extra amount of work, I thought it is finally less expensive to run through the callback and drop the 8 bytes of data, than to let the device trigger a disconnect, probe, and associated device deletion/creation by udev every 60 sec or so (it maybe even produces more USB traffic).
Right, we do not want to go through connect/disconnect cycle if we can help it.
Quoted 11 lines
Quoted 2 lines
Another option is to move configuring interface to
open()?The reconnect will always happen if the int endpoint is not "in use". So configuring the device not in probe will not help, will it?
I think the best way would be to immediately suspend the device, if there is an easy way to trigger this using the driver or usb core. (And if there is no easy way, it is still possible to do it via sysfs if someone is annoyed by this minor "reconnect bug".) Further, this should be fine for all devices, whether they have this "reconnect bug" or not.
Unfortunately forcing suspend requires kernel to be compiled with suspend/autosuspend support which we can't guarantee. So I still decided to go with a device flag instructing driver to start IO immediately.
The patch below seems to work well on my combo device (Lenovo keyboard with integrated touchpad/trackpoint). Could you please tell me if it works with your devices as well?
Thanks.
Signature · 27 lines
Dmitry Input: add Synaptics USB device driver From: Jan Steinhoff <[email protected]> This patch adds a driver for Synaptics USB touchpad or pointing stick devices. These USB devices emulate an USB mouse by default, so one can also use the usbhid driver. However, in combination with special user space drivers this kernel driver allows one to customize the behaviour of the device. An extended version of this driver with support for the cPad background display can be found at <http://jan-steinhoff.de/linux/synaptics-usb.html>. Signed-off-by: Jan Steinhoff <[email protected]> Signed-off-by: Dmitry Torokhov <[email protected]> --- drivers/hid/hid-core.c | 10 + drivers/hid/hid-ids.h | 11 + drivers/input/mouse/Kconfig | 16 + drivers/input/mouse/Makefile | 1 drivers/input/mouse/synaptics_usb.c | 589 +++++++++++++++++++++++++++++++++++ 5 files changed, 627 insertions(+), 0 deletions(-) create mode 100644 drivers/input/mouse/synaptics_usb.c
drivers/hid/hid-core.c +10 -0
@@ -1892,6 +1892,16 @@ static const struct hid_device_id hid_ignore_list[] = { { HID_USB_DEVICE(USB_VENDOR_ID_PANJIT, 0x0004) }, { HID_USB_DEVICE(USB_VENDOR_ID_PHILIPS, USB_DEVICE_ID_PHILIPS_IEEE802154_DONGLE) }, { HID_USB_DEVICE(USB_VENDOR_ID_POWERCOM, USB_DEVICE_ID_POWERCOM_UPS) }, +#if defined(CONFIG_MOUSE_SYNAPTICS_USB) || defined(CONFIG_MOUSE_SYNAPTICS_USB_MODULE) + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_TP) }, + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_INT_TP) }, + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_CPAD) }, + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_STICK) }, + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_WP) }, + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_COMP_TP) }, + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_WTP) }, + { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, USB_DEVICE_ID_SYNAPTICS_DPAD) }, +#endif { HID_USB_DEVICE(USB_VENDOR_ID_VERNIER, USB_DEVICE_ID_VERNIER_LABPRO) }, { HID_USB_DEVICE(USB_VENDOR_ID_VERNIER, USB_DEVICE_ID_VERNIER_GOTEMP) }, { HID_USB_DEVICE(USB_VENDOR_ID_VERNIER, USB_DEVICE_ID_VERNIER_SKIP) },
drivers/hid/hid-ids.h +11 -0
@@ -633,6 +633,17 @@#define USB_DEVICE_ID_SYMBOL_SCANNER_1 0x0800#define USB_DEVICE_ID_SYMBOL_SCANNER_2 0x1300 +#define USB_VENDOR_ID_SYNAPTICS 0x06cb +#define USB_DEVICE_ID_SYNAPTICS_TP 0x0001 +#define USB_DEVICE_ID_SYNAPTICS_INT_TP 0x0002 +#define USB_DEVICE_ID_SYNAPTICS_CPAD 0x0003 +#define USB_DEVICE_ID_SYNAPTICS_TS 0x0006 +#define USB_DEVICE_ID_SYNAPTICS_STICK 0x0007 +#define USB_DEVICE_ID_SYNAPTICS_WP 0x0008 +#define USB_DEVICE_ID_SYNAPTICS_COMP_TP 0x0009 +#define USB_DEVICE_ID_SYNAPTICS_WTP 0x0010 +#define USB_DEVICE_ID_SYNAPTICS_DPAD 0x0013 +#define USB_VENDOR_ID_THRUSTMASTER 0x044f#define USB_VENDOR_ID_TOPSEED 0x0766
drivers/input/mouse/Kconfig +16 -0
@@ -322,4 +322,20 @@ config MOUSE_SYNAPTICS_I2C To compile this driver as a module, choose M here: the module will be called synaptics_i2c. +config MOUSE_SYNAPTICS_USB + tristate "Synaptics USB device support" + depends on USB + help + Say Y here if you want to use a Synaptics USB touchpad or pointing + stick. + + While these devices emulate an USB mouse by default and can be used + with standard usbhid driver, this driver, together with its X.Org + counterpart, allows you to fully utilize capabilities of the device. + More information can be found at: + <http://jan-steinhoff.de/linux/synaptics-usb.html> + + To compile this driver as a module, choose M here: the + module will be called synaptics_usb. + endif
drivers/input/mouse/Makefile +1 -0
@@ -18,6 +18,7 @@ obj-$(CONFIG_MOUSE_PXA930_TRKBALL) += pxa930_trkball.o obj-$(CONFIG_MOUSE_RISCPC) += rpcmouse.o obj-$(CONFIG_MOUSE_SERIAL) += sermouse.o obj-$(CONFIG_MOUSE_SYNAPTICS_I2C) += synaptics_i2c.o +obj-$(CONFIG_MOUSE_SYNAPTICS_USB) += synaptics_usb.o obj-$(CONFIG_MOUSE_VSXXXAA) += vsxxxaa.o psmouse-objs := psmouse-base.o synaptics.o
drivers/input/mouse/synaptics_usb.c new file +589 -0
589 lines not on this page · raw message
Quoted 3 lines · On Tue, 10 Jan 2012 01:43:34 -0800 Dmitry Torokhov <[email protected]>…
The patch below seems to work well on my combo device (Lenovo keyboard with integrated touchpad/trackpoint). Could you please tell me if it works with your devices as well?
Yes, it works. Thanks a lot for the cleanup and improvement!
Just a last remark: The reconnects do not appear while the cPad is suspended, so it is save to allow it to autosuspend. Can the autopm calls be removed from probe and disconnect, and needs_remote_wakeup be set in probe instead?
Dmitry Torokhov
maintainerQuoted 6 lines · Jan Steinhoff
Quoted 3 lines · On Tue, 10 Jan 2012 01:43:34 -0800 Dmitry Torokhov <[email protected]>…
The patch below seems to work well on my combo device (Lenovo keyboard with integrated touchpad/trackpoint). Could you please tell me if it works with your devices as well?
Quoted 6 lines
Yes, it works. Thanks a lot for the cleanup and improvement!
Just a last remark: The reconnects do not appear while the cPad is suspended, so it is save to allow it to autosuspend. Can the autopm calls be removed from probe and disconnect, and
needs_remote_wakeupbe set in probe instead?
OK, how about thie patch below then (on top of the previous one)?
Thanks.
Signature · 12 lines
Dmitry Input: synaptics-use - rework starting of cPad-like devices From: Dmitry Torokhov <[email protected]> Signed-off-by: Dmitry Torokhov <[email protected]> --- drivers/input/mouse/synaptics_usb.c | 65 ++++++++++++----------------------- 1 files changed, 22 insertions(+), 43 deletions(-)
drivers/input/mouse/synaptics_usb.c +22 -43
@@ -245,11 +245,6 @@ static int synusb_open(struct input_dev *dev)struct synusb *synusb =input_get_drvdata (dev);int retval; -if (synusb->flags &SYNUSB_IO_ALWAYS ) { -/* We already started IO in synusb_probe() */ -return 0 ; - } - retval =usb_autopm_get_interface (synusb->intf);if (retval) { dev_err(&synusb->intf->dev,
@@ -277,16 +272,15 @@ out:static void synusb_close (struct input_dev *dev) {struct synusb *synusb =input_get_drvdata (dev); +int autopm_error; -if (!(synusb->flags &SYNUSB_IO_ALWAYS )) { -int autopm_error =usb_autopm_get_interface (synusb->intf); + autopm_error =usb_autopm_get_interface (synusb->intf); -usb_kill_urb (synusb->urb); - synusb->intf->needs_remote_wakeup =0 ; +usb_kill_urb (synusb->urb); + synusb->intf->needs_remote_wakeup =0 ; -if (!autopm_error) -usb_autopm_put_interface (synusb->intf); - } +if (!autopm_error) +usb_autopm_put_interface (synusb->intf); } static int synusb_probe(struct usb_interface *intf,
@@ -380,8 +374,10 @@ static int synusb_probe(struct usb_interface *intf,usb_to_input_id (udev, &input_dev->id); input_dev->dev.parent = &synusb->intf->dev; - input_dev->open = synusb_open; - input_dev->close = synusb_close; +if (!(synusb->flags &SYNUSB_IO_ALWAYS )) { + input_dev->open = synusb_open; + input_dev->close = synusb_close; + }input_set_drvdata (input_dev, synusb);
@@ -412,40 +408,25 @@ static int synusb_probe(struct usb_interface *intf,usb_set_intfdata (intf, synusb); +if (synusb->flags &SYNUSB_IO_ALWAYS ) { + error =synusb_open (input_dev); +if (error) + goto err_free_dma; + } + error =input_register_device (input_dev);if (error) {dev_err (&udev->dev,"Failed to register input device, error %d\n" , error); - goto err_free_dma; - } - -if (synusb->flags &SYNUSB_IO_ALWAYS ) { - error =usb_autopm_get_interface (synusb->intf); -if (error) { -dev_err (&udev->dev, -"%s - usb_autopm_get_interface failed, error: %d\n" , - __func__, error); - goto err_unregister_input; - } - - error =usb_submit_urb (synusb->urb,GFP_KERNEL ); -if (error) { -dev_err (&synusb->intf->dev, -"%s - usb_submit_urb failed, error: %d\n" , - __func__, error); - error = -EIO ; - goto err_put_intf; - } + goto err_stop_io; }return 0 ; -err_put_intf: -usb_autopm_put_interface (synusb->intf); -err_unregister_input: -input_unregister_device (input_dev); - input_dev =NULL ; +err_stop_io: +if (synusb->flags &SYNUSB_IO_ALWAYS ) +synusb_close (synusb->input); err_free_dma:usb_free_coherent (udev,SYNUSB_RECV_SIZE , synusb->data, synusb->urb->transfer_dma);
@@ -464,10 +445,8 @@ static void synusb_disconnect(struct usb_interface *intf)struct synusb *synusb =usb_get_intfdata (intf);struct usb_device *udev =interface_to_usbdev (intf); -if (synusb->flags &SYNUSB_IO_ALWAYS ) { -usb_kill_urb (synusb->urb); -usb_autopm_put_interface (synusb->intf); - } +if (synusb->flags &SYNUSB_IO_ALWAYS ) +synusb_close (synusb->input);input_unregister_device (synusb->input);
Quoted 10 lines · On Wed, 4 Jan 2012 01:41:03 -0800 Dmitry Torokhov <[email protected]>…
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.)
Quoted 1 line
- 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.
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
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.
Quoted 3 lines
- 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?
Quoted 4 lines
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.
Thanks!
Jan
Dmitry Torokhov
maintainerQuoted 11 lines · Jan Steinhoff
Quoted 9 lines · On Wed, 4 Jan 2012 01:41:03 -0800 Dmitry Torokhov <[email protected]>…
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.
Quoted 7 lines
Quoted 1 line
- got rid of BTN_MISC/BTN_MIDDLE option (at least for now);
Please make
BTN_MIDDLEthe 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.
Quoted 10 lines
Quoted 2 lines
- 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).
Quoted 6 lines
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
#ifdefCONFIG_MOUSE_SYNAPTICS_USB... #endif
This is a good idea, I'll add ifdefs.
Quoted 4 lines
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.
Quoted 8 lines
Quoted 4 lines
- runtime PM (hopefully I got it right);
- open/close.
I do not understand why
needs_remote_wakeupis flipped in open/close. As far as I know, the device can not autosuspend afterusb_autopm_get_interfacewas called anyway?
We do usb_autopm_put_interface as last action in open() so the device will be suspended and needs wakeups.
Quoted 6 lines
Quoted 5 lines
Still TODO:
- query device for supported rages instead of having module parameters.
Quoted 6 lines
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.
Signature · 1 line
Dmitry