2008-07-19 11:14:26

by Henrik Rydberg

[permalink] [raw]
Subject: [PATCH] linux-input: bcm5974-0.57: mode-switch to atp_open, cleanup bug fixed

From: Henrik Rydberg <[email protected]>

BCM5974: This driver adds support for the multitouch trackpad on the new
Apple Macbook Air and Macbook Pro Penryn laptops. It replaces the
appletouch driver on those computers, and integrates well with the
synaptics driver of the Xorg system.

Signed-off-by: Henrik Rydberg <[email protected]>
---

This is version bcm5974-0.57. Changes since 0.55 are in short:

* Leave device initialization to hid; use mode-switch only
* Mode switch moved to atp_open and simplified
* Corrected cleanup action bug in atp_open
* Simplified control package detection
* Removed pre_reset, post_reset; not needed

diff --git a/drivers/input/mouse/Kconfig b/drivers/input/mouse/Kconfig
index 7bbea09..42eb49a 100644
--- a/drivers/input/mouse/Kconfig
+++ b/drivers/input/mouse/Kconfig
@@ -130,6 +130,29 @@ config MOUSE_APPLETOUCH
To compile this driver as a module, choose M here: the
module will be called appletouch.

+config MOUSE_BCM5974
+ tristate "Apple USB BCM5974 Multitouch trackpad support"
+ depends on USB_ARCH_HAS_HCD
+ depends on USB
+ help
+ Say Y here if you have an Apple USB BCM5974 Multitouch
+ trackpad.
+
+ The BCM5974 is the multitouch trackpad found in the Macbook
+ Air (JAN2008) and Macbook Pro Penryn (FEB2008) laptops.
+
+ It is also found in the IPhone (2007) and Ipod Touch (2008).
+
+ This driver provides multitouch functionality together with
+ the synaptics X11 driver.
+
+ The interface is currently identical to the appletouch interface,
+ for further information, see
+ <file:Documentation/input/appletouch.txt>.
+
+ To compile this driver as a module, choose M here: the
+ module will be called bcm5974.
+
config MOUSE_INPORT
tristate "InPort/MS/ATIXL busmouse"
depends on ISA
diff --git a/drivers/input/mouse/Makefile b/drivers/input/mouse/Makefile
index 9e6e363..d4d2025 100644
--- a/drivers/input/mouse/Makefile
+++ b/drivers/input/mouse/Makefile
@@ -6,6 +6,7 @@

obj-$(CONFIG_MOUSE_AMIGA) += amimouse.o
obj-$(CONFIG_MOUSE_APPLETOUCH) += appletouch.o
+obj-$(CONFIG_MOUSE_BCM5974) += bcm5974.o
obj-$(CONFIG_MOUSE_ATARI) += atarimouse.o
obj-$(CONFIG_MOUSE_RISCPC) += rpcmouse.o
obj-$(CONFIG_MOUSE_INPORT) += inport.o
diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
new file mode 100644
index 0000000..1e1cf04
--- /dev/null
+++ b/drivers/input/mouse/bcm5974.c
@@ -0,0 +1,647 @@
+/*
+ * Apple USB BCM5974 (Macbook Air and Penryn Macbook Pro) multitouch driver
+ *
+ * Copyright (C) 2008 Henrik Rydberg ([email protected])
+ *
+ * The USB initialization and package decoding was made by
+ * Scott Shawcroft as part of the touchd user-space driver project:
+ * Copyright (C) 2008 Scott Shawcroft ([email protected])
+ *
+ * The BCM5974 driver is based on the appletouch driver:
+ * Copyright (C) 2001-2004 Greg Kroah-Hartman ([email protected])
+ * Copyright (C) 2005 Johannes Berg ([email protected])
+ * Copyright (C) 2005 Stelian Pop ([email protected])
+ * Copyright (C) 2005 Frank Arnold ([email protected])
+ * Copyright (C) 2005 Peter Osterlund ([email protected])
+ * Copyright (C) 2005 Michael Hanselmann ([email protected])
+ * Copyright (C) 2006 Nicolas Boichat ([email protected])
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/usb/input.h>
+#include <linux/hid.h>
+
+#define APPLE_VENDOR_ID 0x05AC
+
+/* MacbookAir, aka wellspring */
+#define ATP_WELLSPRING_ANSI 0x0223
+#define ATP_WELLSPRING_ISO 0x0224
+#define ATP_WELLSPRING_JIS 0x0225
+/* MacbookProPenryn, aka wellspring2 */
+#define ATP_WELLSPRING2_ANSI 0x0230
+#define ATP_WELLSPRING2_ISO 0x0231
+#define ATP_WELLSPRING2_JIS 0x0232
+
+#define ATP_DEVICE(prod) { \
+ .match_flags = (USB_DEVICE_ID_MATCH_DEVICE | \
+ USB_DEVICE_ID_MATCH_INT_CLASS | \
+ USB_DEVICE_ID_MATCH_INT_PROTOCOL), \
+ .idVendor = APPLE_VENDOR_ID, \
+ .idProduct = (prod), \
+ .bInterfaceClass = USB_INTERFACE_CLASS_HID, \
+ .bInterfaceProtocol = USB_INTERFACE_PROTOCOL_MOUSE \
+}
+
+/* table of devices that work with this driver */
+static const struct usb_device_id atp_table [] = {
+ /* MacbookAir1.1 */
+ ATP_DEVICE(ATP_WELLSPRING_ANSI),
+ ATP_DEVICE(ATP_WELLSPRING_ISO),
+ ATP_DEVICE(ATP_WELLSPRING_JIS),
+ /* MacbookProPenryn */
+ ATP_DEVICE(ATP_WELLSPRING2_ANSI),
+ ATP_DEVICE(ATP_WELLSPRING2_ISO),
+ ATP_DEVICE(ATP_WELLSPRING2_JIS),
+ /* Terminating entry */
+ {}
+};
+MODULE_DEVICE_TABLE(usb, atp_table);
+
+MODULE_AUTHOR("Henrik Rydberg");
+MODULE_DESCRIPTION("Apple USB BCM5974 multitouch driver");
+MODULE_LICENSE("GPL");
+
+#define dprintk(level, format, a...)\
+ { if (debug >= level) printk(KERN_DEBUG format, ##a); }
+
+static int debug = 1;
+module_param(debug, int, 0644);
+MODULE_PARM_DESC(debug, "Activate debugging output");
+
+/* button data structure */
+struct bt_data {
+ u8 unknown1; /* constant */
+ u8 button; /* left button */
+ u8 rel_x; /* relative x coordinate */
+ u8 rel_y; /* relative y coordinate */
+};
+
+/* trackpad header structure */
+struct tp_header {
+ u8 unknown1[16]; /* constants, timers, etc */
+ u8 fingers; /* number of fingers on trackpad */
+ u8 unknown2[9]; /* constants, timers, etc */
+};
+
+/* trackpad finger structure */
+struct tp_finger {
+ __le16 origin; /* left/right origin? */
+ __le16 abs_x; /* absolute x coodinate */
+ __le16 abs_y; /* absolute y coodinate */
+ __le16 rel_x; /* relative x coodinate */
+ __le16 rel_y; /* relative y coodinate */
+ __le16 size_major; /* finger size, major axis? */
+ __le16 size_minor; /* finger size, minor axis? */
+ __le16 orientation; /* 16384 when point, else 15 bit angle */
+ __le16 force_major; /* trackpad force, major axis? */
+ __le16 force_minor; /* trackpad force, minor axis? */
+ __le16 unused[3]; /* zeros */
+ __le16 multi; /* one finger: varies, more fingers: constant */
+};
+
+/* trackpad data structure, empirically at least ten fingers */
+struct tp_data {
+ struct tp_header header;
+ struct tp_finger finger[16];
+};
+
+/* device-specific parameters */
+struct atp_params {
+ int dim; /* logical dimension */
+ int fuzz; /* logical noise value */
+ int devmin; /* device minimum reading */
+ int devmax; /* device maximum reading */
+};
+
+/* device-specific configuration */
+struct atp_config {
+ int ansi, iso, jis; /* the product id of this device */
+ int bt_ep; /* the endpoint of the button interface */
+ int bt_datalen; /* data length of the button interface */
+ int tp_ep; /* the endpoint of the trackpad interface */
+ int tp_datalen; /* data length of the trackpad interface */
+ struct atp_params p; /* finger pressure limits */
+ struct atp_params w; /* finger width limits */
+ struct atp_params x; /* horizontal limits */
+ struct atp_params y; /* vertical limits */
+};
+
+/* logical device structure */
+struct atp {
+ char phys[64];
+ struct usb_device *udev; /* usb device */
+ struct input_dev *input; /* input dev */
+ struct atp_config cfg; /* device configuration */
+ int open; /* >0: open, else closed */
+ int suspended; /* >0: suspended, else open */
+ struct urb *bt_urb; /* button usb request block */
+ struct bt_data *bt_data; /* button transferred data */
+ struct urb *tp_urb; /* trackpad usb request block */
+ struct tp_data *tp_data; /* trackpad transferred data */
+};
+
+/* logical dimensions */
+#define DIM_PRESSURE 256 /* maximum finger pressure */
+#define DIM_WIDTH 16 /* maximum finger width */
+#define DIM_X 1280 /* maximum trackpad x value */
+#define DIM_Y 800 /* maximum trackpad y value */
+
+/* logical signal quality */
+#define SN_PRESSURE 45 /* pressure signal-to-noise ratio */
+#define SN_WIDTH 100 /* width signal-to-noise ratio */
+#define SN_COORD 250 /* coordinate signal-to-noise ratio */
+
+/* device constants */
+static const struct atp_config atp_config_table[] = {
+ {
+ ATP_WELLSPRING_ANSI,
+ ATP_WELLSPRING_ISO,
+ ATP_WELLSPRING_JIS,
+ 0x84, sizeof(struct bt_data),
+ 0x81, sizeof(struct tp_data),
+ { DIM_PRESSURE, DIM_PRESSURE / SN_PRESSURE, 0, 256 },
+ { DIM_WIDTH, DIM_WIDTH / SN_WIDTH, 0, 2048 },
+ { DIM_X, DIM_X / SN_COORD, -4824, 5342 },
+ { DIM_Y, DIM_Y / SN_COORD, -172, 5820 }
+ },
+ {
+ ATP_WELLSPRING2_ANSI,
+ ATP_WELLSPRING2_ISO,
+ ATP_WELLSPRING2_JIS,
+ 0x84, sizeof(struct bt_data),
+ 0x81, sizeof(struct tp_data),
+ { DIM_PRESSURE, DIM_PRESSURE / SN_PRESSURE, 0, 256 },
+ { DIM_WIDTH, DIM_WIDTH / SN_WIDTH, 0, 2048 },
+ { DIM_X, DIM_X / SN_COORD, -4824, 4824 },
+ { DIM_Y, DIM_Y / SN_COORD, -172, 4290 }
+ },
+ {}
+};
+
+/* return the device-specific configuration by device */
+static const struct atp_config *atp_product_config(struct usb_device *udev)
+{
+ u16 id = le16_to_cpu(udev->descriptor.idProduct);
+ const struct atp_config *config;
+ for (config = atp_config_table; config->ansi; ++config)
+ if (config->ansi == id || config->iso == id ||
+ config->jis == id)
+ return config;
+ return atp_config_table;
+}
+
+/* convert 16-bit little endian to signed integer */
+static inline int raw2int(__le16 x)
+{
+ return (signed short)le16_to_cpu(x);
+}
+
+/* scale device data to logical dimensions (asserts devmin < devmax) */
+static inline int int2scale(const struct atp_params *p, int x)
+{
+ return x * p->dim / (p->devmax - p->devmin);
+}
+
+/* all logical value ranges are [0,dim). */
+static inline int int2bound(const struct atp_params *p, int x)
+{
+ int s = int2scale(p, x);
+ return s < 0 ? 0 : s >= p->dim ? p->dim - 1 : s;
+}
+
+/* report button data as logical button state */
+static int report_bt_state(struct atp *dev, int size)
+{
+ if (size != sizeof(struct bt_data))
+ return -EIO;
+
+ input_report_key(dev->input, BTN_LEFT, dev->bt_data->button);
+
+ return 0;
+}
+
+/* report trackpad data as logical trackpad state */
+static int report_tp_state(struct atp *dev, int size)
+{
+ const struct atp_config *c = &dev->cfg;
+ const struct tp_finger *f = dev->tp_data->finger;
+ const int fingers = (size - 26) / 28;
+ int p, w, x, y, n;
+
+ if (size < 26 || (size - 26) % 28 != 0)
+ return -EIO;
+
+ if (!fingers) {
+ input_report_abs(dev->input, ABS_PRESSURE, 0);
+ input_report_key(dev->input, BTN_TOOL_FINGER, false);
+ input_report_key(dev->input, BTN_TOOL_DOUBLETAP, false);
+ input_report_key(dev->input, BTN_TOOL_TRIPLETAP, false);
+ return 0;
+ }
+
+ p = raw2int(f->force_major);
+ w = raw2int(f->size_major);
+ x = raw2int(f->abs_x);
+ y = raw2int(f->abs_y);
+ n = p > 0 ? fingers : 0;
+
+ dprintk(9, "bcm5974: p: %+05d w: %+05d x: %+05d y: %+05d n: %d\n",
+ p, w, x, y, n);
+
+ input_report_abs(dev->input, ABS_PRESSURE, int2bound(&c->p, p));
+ input_report_abs(dev->input, ABS_TOOL_WIDTH, int2bound(&c->w, w));
+ input_report_abs(dev->input, ABS_X, int2bound(&c->x, x - c->x.devmin));
+ input_report_abs(dev->input, ABS_Y, int2bound(&c->y, c->y.devmax - y));
+ input_report_key(dev->input, BTN_TOOL_FINGER, n == 1);
+ input_report_key(dev->input, BTN_TOOL_DOUBLETAP, n == 2);
+ input_report_key(dev->input, BTN_TOOL_TRIPLETAP, n > 2);
+ return 0;
+}
+
+/* Wellspring initialization constants */
+#define ATP_WELLSPRING_MODE_READ_REQUEST_ID 1
+#define ATP_WELLSPRING_MODE_WRITE_REQUEST_ID 9
+#define ATP_WELLSPRING_MODE_REQUEST_VALUE 0x300
+#define ATP_WELLSPRING_MODE_REQUEST_INDEX 0
+#define ATP_WELLSPRING_MODE_VENDOR_VALUE 0x01
+
+static int atp_wellspring_mode(struct atp *dev)
+{
+ char *data = kmalloc(8, GFP_KERNEL);
+ int error = 0, size;
+
+ if (!data) {
+ err("bcm5974: out of memory");
+ error = -ENOMEM;
+ goto error;
+ }
+
+ /* read configuration */
+ size = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
+ ATP_WELLSPRING_MODE_READ_REQUEST_ID,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ ATP_WELLSPRING_MODE_REQUEST_VALUE,
+ ATP_WELLSPRING_MODE_REQUEST_INDEX, data, 8, 5000);
+
+ if (size != 8) {
+ err("bcm5974: could not read from device");
+ error = -EIO;
+ goto error;
+ }
+
+ /* apply the mode switch */
+ data[0] = ATP_WELLSPRING_MODE_VENDOR_VALUE;
+
+ /* write configuration */
+ size = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
+ ATP_WELLSPRING_MODE_WRITE_REQUEST_ID,
+ USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ ATP_WELLSPRING_MODE_REQUEST_VALUE,
+ ATP_WELLSPRING_MODE_REQUEST_INDEX, data, 8, 5000);
+
+ if (size != 8) {
+ err("bcm5974: could not write to device");
+ error = -EIO;
+ goto error;
+ }
+
+ dprintk(2, "bcm5974: switched to wellspring mode.\n");
+
+ kfree(data);
+ return 0;
+
+error:
+ kfree(data);
+ return error;
+}
+
+static void irq_button(struct urb *urb)
+{
+ struct atp *dev = urb->context;
+ int error;
+
+ switch (urb->status) {
+ case 0:
+ break;
+ case -EOVERFLOW:
+ case -ECONNRESET:
+ case -ENOENT:
+ case -ESHUTDOWN:
+ dbg("bcm5974: button urb shutting down: %d", urb->status);
+ return;
+ default:
+ dbg("bcm5974: button urb status: %d", urb->status);
+ goto exit;
+ }
+
+ if (report_bt_state(dev, dev->bt_urb->actual_length)) {
+ dprintk(1, "bcm5974: bad button package, length: %d\n",
+ dev->bt_urb->actual_length);
+ goto exit;
+ }
+
+ input_sync(dev->input);
+
+exit:
+ error = usb_submit_urb(dev->bt_urb, GFP_ATOMIC);
+ if (error)
+ err("bcm5974: button urb failed: %d", error);
+}
+
+static void irq_trackpad(struct urb *urb)
+{
+ struct atp *dev = urb->context;
+ int error;
+
+ switch (urb->status) {
+ case 0:
+ break;
+ case -EOVERFLOW:
+ case -ECONNRESET:
+ case -ENOENT:
+ case -ESHUTDOWN:
+ dbg("bcm5974: trackpad urb shutting down: %d", urb->status);
+ return;
+ default:
+ dbg("bcm5974: trackpad urb status: %d", urb->status);
+ goto exit;
+ }
+
+ /* control response ignored */
+ if (dev->tp_urb->actual_length == 2)
+ goto exit;
+
+ if (report_tp_state(dev, dev->tp_urb->actual_length)) {
+ dprintk(1, "bcm5974: bad trackpad package, length: %d\n",
+ dev->tp_urb->actual_length);
+ goto exit;
+ }
+
+ input_sync(dev->input);
+
+exit:
+ error = usb_submit_urb(dev->tp_urb, GFP_ATOMIC);
+ if (error)
+ err("bcm5974: trackpad urb failed: %d", error);
+}
+
+/*
+ * The Wellspring trackpad, like many recent Apple trackpads, share
+ * the usb device with the keyboard. Since keyboards are usually
+ * handled by the HID system, the device ends up being handled by two
+ * modules. Setting up the device therefore becomes slightly
+ * complicated. To enable multitouch features, a mode switch is
+ * required, which is usually applied via the control interface of the
+ * device. It can be argued where this switch should take place. In
+ * some drivers, like appletouch, the switch is made during
+ * probe. However, the hid module may also alter the state of the
+ * device, resulting in trackpad malfunction under certain
+ * circumstances. To get around this problem, there is at least one
+ * example that utilizes the USB_QUIRK_RESET_RESUME quirk in order to
+ * recieve a reset_resume request rather than the normal resume. Since
+ * the implementation of reset_resume is equal to mode switch plus
+ * open, it seems easier to always do the switch while opening the
+ * device.
+ */
+static int atp_open(struct input_dev *input)
+{
+ struct atp *dev = input_get_drvdata(input);
+
+ if (!dev->open) {
+ if (atp_wellspring_mode(dev)) {
+ dprintk(1, "bcm5974: mode switch failed\n");
+ goto error;
+ }
+ if (usb_submit_urb(dev->bt_urb, GFP_KERNEL))
+ goto error;
+ if (usb_submit_urb(dev->tp_urb, GFP_KERNEL))
+ goto err_kill_bt;
+ }
+
+ dev->open = 1;
+ dev->suspended = 0;
+ return 0;
+
+err_kill_bt:
+ usb_kill_urb(dev->bt_urb);
+error:
+ return -EIO;
+}
+
+static void atp_close(struct input_dev *input)
+{
+ struct atp *dev = input_get_drvdata(input);
+
+ usb_kill_urb(dev->tp_urb);
+ usb_kill_urb(dev->bt_urb);
+
+ dev->open = 0;
+ dev->suspended = 0;
+}
+
+static int atp_probe(struct usb_interface *iface,
+ const struct usb_device_id *id)
+{
+ struct usb_device *udev = interface_to_usbdev(iface);
+ const struct atp_config *cfg;
+ struct atp *dev;
+ struct input_dev *input_dev;
+ int error = 0;
+
+ /* find the product index */
+ cfg = atp_product_config(udev);
+
+ /* allocate memory for our device state and initialize it */
+ dev = kzalloc(sizeof(struct atp), GFP_KERNEL);
+ input_dev = input_allocate_device();
+ if (!dev || !input_dev) {
+ err("bcm5974: out of memory");
+ error = -ENOMEM;
+ goto err_free_devs;
+ }
+
+ dev->udev = udev;
+ dev->input = input_dev;
+ dev->cfg = *cfg;
+
+ dev->bt_urb = usb_alloc_urb(0, GFP_KERNEL);
+ if (!dev->bt_urb) {
+ error = -ENOMEM;
+ goto err_free_devs;
+ }
+
+ dev->tp_urb = usb_alloc_urb(0, GFP_KERNEL);
+ if (!dev->tp_urb) {
+ error = -ENOMEM;
+ goto err_free_bt_urb;
+ }
+
+ dev->bt_data = usb_buffer_alloc(dev->udev,
+ dev->cfg.bt_datalen, GFP_KERNEL,
+ &dev->bt_urb->transfer_dma);
+ if (!dev->bt_data) {
+ error = -ENOMEM;
+ goto err_free_urb;
+ }
+
+ dev->tp_data = usb_buffer_alloc(dev->udev,
+ dev->cfg.tp_datalen, GFP_KERNEL,
+ &dev->tp_urb->transfer_dma);
+ if (!dev->tp_data) {
+ error = -ENOMEM;
+ goto err_free_bt_buffer;
+ }
+
+ usb_fill_int_urb(dev->bt_urb, udev,
+ usb_rcvintpipe(udev, cfg->bt_ep),
+ dev->bt_data, dev->cfg.bt_datalen,
+ irq_button, dev, 1);
+
+ usb_fill_int_urb(dev->tp_urb, udev,
+ usb_rcvintpipe(udev, cfg->tp_ep),
+ dev->tp_data, dev->cfg.tp_datalen,
+ irq_trackpad, dev, 1);
+
+ usb_make_path(udev, dev->phys, sizeof(dev->phys));
+ strlcat(dev->phys, "/input0", sizeof(dev->phys));
+
+ input_dev->name = "bcm5974";
+ input_dev->phys = dev->phys;
+ usb_to_input_id(dev->udev, &input_dev->id);
+ input_dev->dev.parent = &iface->dev;
+
+ input_set_drvdata(input_dev, dev);
+
+ input_dev->open = atp_open;
+ input_dev->close = atp_close;
+
+ set_bit(EV_ABS, input_dev->evbit);
+ input_set_abs_params(input_dev, ABS_PRESSURE,
+ 0, cfg->p.dim, cfg->p.fuzz, 0);
+ input_set_abs_params(input_dev, ABS_TOOL_WIDTH,
+ 0, cfg->w.dim, cfg->w.fuzz, 0);
+ input_set_abs_params(input_dev, ABS_X,
+ 0, cfg->x.dim, cfg->x.fuzz, 0);
+ input_set_abs_params(input_dev, ABS_Y,
+ 0, cfg->y.dim, cfg->y.fuzz, 0);
+
+ set_bit(EV_KEY, input_dev->evbit);
+ 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);
+
+ error = input_register_device(dev->input);
+ if (error)
+ goto err_free_buffer;
+
+ /* save our data pointer in this interface device */
+ usb_set_intfdata(iface, dev);
+
+ return 0;
+
+err_free_buffer:
+ usb_buffer_free(dev->udev, dev->cfg.tp_datalen,
+ dev->tp_data, dev->tp_urb->transfer_dma);
+err_free_bt_buffer:
+ usb_buffer_free(dev->udev, dev->cfg.bt_datalen,
+ dev->bt_data, dev->bt_urb->transfer_dma);
+err_free_urb:
+ usb_free_urb(dev->tp_urb);
+err_free_bt_urb:
+ usb_free_urb(dev->bt_urb);
+err_free_devs:
+ usb_set_intfdata(iface, NULL);
+ kfree(dev);
+ input_free_device(input_dev);
+ return error;
+}
+
+static void atp_disconnect(struct usb_interface *iface)
+{
+ struct atp *dev = usb_get_intfdata(iface);
+
+ usb_set_intfdata(iface, NULL);
+
+ if (dev) {
+ input_unregister_device(dev->input);
+ usb_buffer_free(dev->udev, dev->cfg.tp_datalen,
+ dev->tp_data, dev->tp_urb->transfer_dma);
+ usb_buffer_free(dev->udev, dev->cfg.bt_datalen,
+ dev->bt_data, dev->bt_urb->transfer_dma);
+ usb_free_urb(dev->tp_urb);
+ usb_free_urb(dev->bt_urb);
+ kfree(dev);
+ }
+
+ printk(KERN_INFO "bcm5974: disconnected\n");
+}
+
+static int atp_suspend(struct usb_interface *iface, pm_message_t message)
+{
+ struct atp *dev = usb_get_intfdata(iface);
+
+ if (dev) {
+ if (!dev->suspended)
+ atp_close(dev->input);
+ dev->suspended++;
+ }
+
+ return 0;
+}
+
+static int atp_resume(struct usb_interface *iface)
+{
+ struct atp *dev = usb_get_intfdata(iface);
+ int error = 0;
+
+ if (dev && dev->suspended) {
+ if (!--dev->suspended)
+ error = atp_open(dev->input);
+ }
+
+ return error;
+}
+
+static struct usb_driver atp_driver = {
+ .name = "bcm5974",
+ .probe = atp_probe,
+ .disconnect = atp_disconnect,
+ .suspend = atp_suspend,
+ .resume = atp_resume,
+ .reset_resume = atp_resume,
+ .id_table = atp_table,
+};
+
+static int __init atp_init(void)
+{
+ return usb_register(&atp_driver);
+}
+
+static void __exit atp_exit(void)
+{
+ usb_deregister(&atp_driver);
+}
+
+module_init(atp_init);
+module_exit(atp_exit);
+


2008-07-20 05:10:44

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH] linux-input: bcm5974-0.57: mode-switch to atp_open, cleanup bug fixed

Hi Henrik,

On Sat, Jul 19, 2008 at 01:03:28PM +0200, Henrik Rydberg wrote:
> From: Henrik Rydberg <[email protected]>
>
> BCM5974: This driver adds support for the multitouch trackpad on the new
> Apple Macbook Air and Macbook Pro Penryn laptops. It replaces the
> appletouch driver on those computers, and integrates well with the
> synaptics driver of the Xorg system.
>

The driver looks pretty good by now, just a few comments from me...

> +
> + if (report_tp_state(dev, dev->tp_urb->actual_length)) {
> + dprintk(1, "bcm5974: bad trackpad package, length: %d\n",
> + dev->tp_urb->actual_length);
> + goto exit;
> + }
> +
> + input_sync(dev->input);

I'd rather had input_sync right in the report_tp_state, where the event
set is generated.

> +
> +exit:
> + error = usb_submit_urb(dev->tp_urb, GFP_ATOMIC);
> + if (error)
> + err("bcm5974: trackpad urb failed: %d", error);
> +}
> +
> +/*
> + * The Wellspring trackpad, like many recent Apple trackpads, share
> + * the usb device with the keyboard. Since keyboards are usually
> + * handled by the HID system, the device ends up being handled by two
> + * modules. Setting up the device therefore becomes slightly
> + * complicated. To enable multitouch features, a mode switch is
> + * required, which is usually applied via the control interface of the
> + * device. It can be argued where this switch should take place. In
> + * some drivers, like appletouch, the switch is made during
> + * probe. However, the hid module may also alter the state of the
> + * device, resulting in trackpad malfunction under certain
> + * circumstances. To get around this problem, there is at least one
> + * example that utilizes the USB_QUIRK_RESET_RESUME quirk in order to
> + * recieve a reset_resume request rather than the normal resume. Since
> + * the implementation of reset_resume is equal to mode switch plus
> + * open, it seems easier to always do the switch while opening the
> + * device.
> + */

What will happen if bcm driver is attached first, before HID had a
chance to do its part?

> +static int atp_open(struct input_dev *input)
> +{
> + struct atp *dev = input_get_drvdata(input);
> +
> + if (!dev->open) {
> + if (atp_wellspring_mode(dev)) {
> + dprintk(1, "bcm5974: mode switch failed\n");
> + goto error;
> + }
> + if (usb_submit_urb(dev->bt_urb, GFP_KERNEL))
> + goto error;
> + if (usb_submit_urb(dev->tp_urb, GFP_KERNEL))
> + goto err_kill_bt;
> + }
> +
> + dev->open = 1;
> + dev->suspended = 0;
> + return 0;
> +
> +err_kill_bt:
> + usb_kill_urb(dev->bt_urb);
> +error:
> + return -EIO;
> +}
> +
> +static void atp_close(struct input_dev *input)
> +{
> + struct atp *dev = input_get_drvdata(input);
> +
> + usb_kill_urb(dev->tp_urb);
> + usb_kill_urb(dev->bt_urb);
> +
> + dev->open = 0;
> + dev->suspended = 0;
> +}
> +

[...skipped...]

> +
> +static void atp_disconnect(struct usb_interface *iface)
> +{
> + struct atp *dev = usb_get_intfdata(iface);
> +
> + usb_set_intfdata(iface, NULL);
> +
> + if (dev) {

Is this check needed? How can intfdata be NULL and we still get into
this function?

> + input_unregister_device(dev->input);
> + usb_buffer_free(dev->udev, dev->cfg.tp_datalen,
> + dev->tp_data, dev->tp_urb->transfer_dma);
> + usb_buffer_free(dev->udev, dev->cfg.bt_datalen,
> + dev->bt_data, dev->bt_urb->transfer_dma);
> + usb_free_urb(dev->tp_urb);
> + usb_free_urb(dev->bt_urb);
> + kfree(dev);
> + }
> +
> + printk(KERN_INFO "bcm5974: disconnected\n");
> +}
> +
> +static int atp_suspend(struct usb_interface *iface, pm_message_t message)
> +{
> + struct atp *dev = usb_get_intfdata(iface);
> +
> + if (dev) {
> + if (!dev->suspended)
> + atp_close(dev->input);
> + dev->suspended++;
> + }
> +
> + return 0;
> +}
> +
> +static int atp_resume(struct usb_interface *iface)
> +{
> + struct atp *dev = usb_get_intfdata(iface);
> + int error = 0;
> +
> + if (dev && dev->suspended) {
> + if (!--dev->suspended)
> + error = atp_open(dev->input);

This will cause us to sumit URBs and start reporting events even though
there may be no users of the device (if it was not opened by anyone)
which defeats the purpose of implementing open and close methods of the
input device. You may want to implent start/pause_traffic function and
use them in open/close and suspend/resume.

Also, you need to have a locking that would serialize suspend/resume
with regard to open/close.

And the last request - could we do s/atp_/bcm5974/g so we don't get
confused with the original atp driver?

--
Dmitry

2008-07-21 12:25:01

by Henrik Rydberg

[permalink] [raw]
Subject: Re: [PATCH] linux-input: bcm5974-0.57: mode-switch to atp_open, cleanup bug fixed

Hello Dmitry,

thank you so much for the review. I will get back tonight with an
updated version.

> What will happen if bcm driver is attached first, before HID had a
> chance to do its part?

It seems it will still work, because the bcm5974 gets reopened after HID
is initialized.

Cheers,
Henrik

2008-07-22 00:56:11

by Henrik Rydberg

[permalink] [raw]
Subject: Re: [PATCH] linux-input: bcm5974-0.57: mode-switch to atp_open, cleanup bug fixed

Hi Dmitri,

Regarding open/close and suspend/resume, I have difficulties finding out
what the proper behavior is for the various error states that can
appear. I also wonder whether open should commute with suspend or not.
It would be of great help to get answers to the scenarios below.

1. From the state (!opened,!suspended), calling open followed by
suspend, or suspend followed by open, are we in the same state?

2. From the state (!opened,!suspended), calling open fails. What state
are we in?

3. From the state (!opened,suspended), calling open gets us to what
state?

4. From the state (!opened,suspended), calling open fails. Where are we?

5. From the state (opened,suspended), calling resume fails. What state
are we in?

Best regards,
Henrik Rydberg

2008-07-22 13:53:18

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH] linux-input: bcm5974-0.57: mode-switch to atp_open, cleanup bug fixed

On Tue, Jul 22, 2008 at 02:55:42AM +0200, Henrik Rydberg wrote:
> Hi Dmitri,
>
> Regarding open/close and suspend/resume, I have difficulties finding out
> what the proper behavior is for the various error states that can
> appear. I also wonder whether open should commute with suspend or not.
> It would be of great help to get answers to the scenarios below.
>

Hm, hard questions. I will try answering them but I am also CCing Alan
Stern and Oliver Neikum who are experts in USB suspend/resume.

> 1. From the state (!opened,!suspended), calling open followed by
> suspend, or suspend followed by open, are we in the same state?
>

No, we are not... if we first open then suspend then device stays open
but we are not getting reports from it. Opening suspended device will
fail. Autosuspended device should be waken up by open() though.

> 2. From the state (!opened,!suspended), calling open fails. What state
> are we in?

Device is closed, the driver will have to make sure that it cleans up
steps that succeded while opening device (like if you submitted 1 URB
successfully and the 2nd URB failed open should kill the first one
before returning).

>
> 3. From the state (!opened,suspended), calling open gets us to what
> state?
>

Depens on the kind of suspend - manual suspend will cause open to
fail. Autosuspend (if driver implements it) should resume the device.

> 4. From the state (!opened,suspended), calling open fails. Where are we?
>

Manual suspend will surely cause open to fail. Autosuspend - if
autoresume fails then (closed, unknown) otherwise (closed,
autosuspended - autosuspension may not kick in immediately I think).

> 5. From the state (opened,suspended), calling resume fails. What state
> are we in?
>

Screwed up ;) From the driver POV still (opened, suspended) I think.

Thanks!

--
Dmitry

2008-07-22 22:19:40

by Henrik Rydberg

[permalink] [raw]
Subject: Re: [PATCH] linux-input: bcm5974-0.57: mode-switch to atp_open, cleanup bug fixed

Dmitry,

thank you so much for your answers! I would like to send you
a non-patch of the updated driver shortly if I may; it will
require more testing before submission, but having your blessing
first should make things turn around faster.

> >
> > 3. From the state (!opened,suspended), calling open gets us to what
> > state?
> >

Depens on the kind of suspend - manual suspend will cause open to
fail. Autosuspend (if driver implements it) should resume the device.

[...]

>> 5. From the state (opened,suspended), calling resume fails. What state
>> are we in?
>>
>
> Screwed up ;) From the driver POV still (opened, suspended) I think.
>

I feel a bit reluctant to implement this particular behavior, since it
will leave the device in a bad state; if resume fails and stays suspended,
the device cannot later be opened according to 3). What if a failed
resume leaves the device in state (!opened,!suspended)? It could then
later be reopened.

Cheers,
Henrik Rydberg

2008-07-23 11:38:19

by Henrik Rydberg

[permalink] [raw]
Subject: Re: [PATCH] linux-input: bcm5974-0.57: mode-switch to atp_open, cleanup bug fixed

Hi Dmitry,

here are the tentative changes, addressing your comments.
The diff is against version 0.57.

Thanks,
Henrik
--

diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
index 1e1cf04..55c1f60 100644
--- a/drivers/input/mouse/bcm5974.c
+++ b/drivers/input/mouse/bcm5974.c
@@ -39,42 +39,43 @@
#include <linux/module.h>
#include <linux/usb/input.h>
#include <linux/hid.h>
+#include <linux/mutex.h>

-#define APPLE_VENDOR_ID 0x05AC
+#define USB_VENDOR_ID_APPLE 0x05ac

/* MacbookAir, aka wellspring */
-#define ATP_WELLSPRING_ANSI 0x0223
-#define ATP_WELLSPRING_ISO 0x0224
-#define ATP_WELLSPRING_JIS 0x0225
+#define USB_DEVICE_ID_APPLE_WELLSPRING_ANSI 0x0223
+#define USB_DEVICE_ID_APPLE_WELLSPRING_ISO 0x0224
+#define USB_DEVICE_ID_APPLE_WELLSPRING_JIS 0x0225
/* MacbookProPenryn, aka wellspring2 */
-#define ATP_WELLSPRING2_ANSI 0x0230
-#define ATP_WELLSPRING2_ISO 0x0231
-#define ATP_WELLSPRING2_JIS 0x0232
+#define USB_DEVICE_ID_APPLE_WELLSPRING2_ANSI 0x0230
+#define USB_DEVICE_ID_APPLE_WELLSPRING2_ISO 0x0231
+#define USB_DEVICE_ID_APPLE_WELLSPRING2_JIS 0x0232

-#define ATP_DEVICE(prod) { \
+#define BCM5974_DEVICE(prod) { \
.match_flags = (USB_DEVICE_ID_MATCH_DEVICE | \
USB_DEVICE_ID_MATCH_INT_CLASS | \
USB_DEVICE_ID_MATCH_INT_PROTOCOL), \
- .idVendor = APPLE_VENDOR_ID, \
+ .idVendor = USB_VENDOR_ID_APPLE, \
.idProduct = (prod), \
.bInterfaceClass = USB_INTERFACE_CLASS_HID, \
.bInterfaceProtocol = USB_INTERFACE_PROTOCOL_MOUSE \
}

/* table of devices that work with this driver */
-static const struct usb_device_id atp_table [] = {
+static const struct usb_device_id bcm5974_table [] = {
/* MacbookAir1.1 */
- ATP_DEVICE(ATP_WELLSPRING_ANSI),
- ATP_DEVICE(ATP_WELLSPRING_ISO),
- ATP_DEVICE(ATP_WELLSPRING_JIS),
+ BCM5974_DEVICE(USB_DEVICE_ID_APPLE_WELLSPRING_ANSI),
+ BCM5974_DEVICE(USB_DEVICE_ID_APPLE_WELLSPRING_ISO),
+ BCM5974_DEVICE(USB_DEVICE_ID_APPLE_WELLSPRING_JIS),
/* MacbookProPenryn */
- ATP_DEVICE(ATP_WELLSPRING2_ANSI),
- ATP_DEVICE(ATP_WELLSPRING2_ISO),
- ATP_DEVICE(ATP_WELLSPRING2_JIS),
+ BCM5974_DEVICE(USB_DEVICE_ID_APPLE_WELLSPRING2_ANSI),
+ BCM5974_DEVICE(USB_DEVICE_ID_APPLE_WELLSPRING2_ISO),
+ BCM5974_DEVICE(USB_DEVICE_ID_APPLE_WELLSPRING2_JIS),
/* Terminating entry */
{}
};
-MODULE_DEVICE_TABLE(usb, atp_table);
+MODULE_DEVICE_TABLE(usb, bcm5974_table);

MODULE_AUTHOR("Henrik Rydberg");
MODULE_DESCRIPTION("Apple USB BCM5974 multitouch driver");
@@ -125,7 +126,7 @@ struct tp_data {
};

/* device-specific parameters */
-struct atp_params {
+struct bcm5974_param {
int dim; /* logical dimension */
int fuzz; /* logical noise value */
int devmin; /* device minimum reading */
@@ -133,26 +134,27 @@ struct atp_params {
};

/* device-specific configuration */
-struct atp_config {
+struct bcm5974_config {
int ansi, iso, jis; /* the product id of this device */
int bt_ep; /* the endpoint of the button interface */
int bt_datalen; /* data length of the button interface */
int tp_ep; /* the endpoint of the trackpad interface */
int tp_datalen; /* data length of the trackpad interface */
- struct atp_params p; /* finger pressure limits */
- struct atp_params w; /* finger width limits */
- struct atp_params x; /* horizontal limits */
- struct atp_params y; /* vertical limits */
+ struct bcm5974_param p; /* finger pressure limits */
+ struct bcm5974_param w; /* finger width limits */
+ struct bcm5974_param x; /* horizontal limits */
+ struct bcm5974_param y; /* vertical limits */
};

/* logical device structure */
-struct atp {
+struct bcm5974 {
char phys[64];
struct usb_device *udev; /* usb device */
struct input_dev *input; /* input dev */
- struct atp_config cfg; /* device configuration */
- int open; /* >0: open, else closed */
- int suspended; /* >0: suspended, else open */
+ struct bcm5974_config cfg; /* device configuration */
+ struct mutex mutex; /* serialize access to open/suspend */
+ int opened; /* >0: opened, else closed */
+ int manually_suspended; /* >0: manually suspended */
struct urb *bt_urb; /* button usb request block */
struct bt_data *bt_data; /* button transferred data */
struct urb *tp_urb; /* trackpad usb request block */
@@ -171,11 +173,11 @@ struct atp {
#define SN_COORD 250 /* coordinate signal-to-noise ratio */

/* device constants */
-static const struct atp_config atp_config_table[] = {
+static const struct bcm5974_config bcm5974_config_table[] = {
{
- ATP_WELLSPRING_ANSI,
- ATP_WELLSPRING_ISO,
- ATP_WELLSPRING_JIS,
+ USB_DEVICE_ID_APPLE_WELLSPRING_ANSI,
+ USB_DEVICE_ID_APPLE_WELLSPRING_ISO,
+ USB_DEVICE_ID_APPLE_WELLSPRING_JIS,
0x84, sizeof(struct bt_data),
0x81, sizeof(struct tp_data),
{ DIM_PRESSURE, DIM_PRESSURE / SN_PRESSURE, 0, 256 },
@@ -184,9 +186,9 @@ static const struct atp_config atp_config_table[] = {
{ DIM_Y, DIM_Y / SN_COORD, -172, 5820 }
},
{
- ATP_WELLSPRING2_ANSI,
- ATP_WELLSPRING2_ISO,
- ATP_WELLSPRING2_JIS,
+ USB_DEVICE_ID_APPLE_WELLSPRING2_ANSI,
+ USB_DEVICE_ID_APPLE_WELLSPRING2_ISO,
+ USB_DEVICE_ID_APPLE_WELLSPRING2_JIS,
0x84, sizeof(struct bt_data),
0x81, sizeof(struct tp_data),
{ DIM_PRESSURE, DIM_PRESSURE / SN_PRESSURE, 0, 256 },
@@ -198,15 +200,14 @@ static const struct atp_config atp_config_table[] = {
};

/* return the device-specific configuration by device */
-static const struct atp_config *atp_product_config(struct usb_device *udev)
+static const struct bcm5974_config *bcm5974_get_config(struct usb_device *udev)
{
u16 id = le16_to_cpu(udev->descriptor.idProduct);
- const struct atp_config *config;
- for (config = atp_config_table; config->ansi; ++config)
- if (config->ansi == id || config->iso == id ||
- config->jis == id)
- return config;
- return atp_config_table;
+ const struct bcm5974_config *cfg;
+ for (cfg = bcm5974_config_table; cfg->ansi; ++cfg)
+ if (cfg->ansi == id || cfg->iso == id || cfg->jis == id)
+ return cfg;
+ return bcm5974_config_table;
}

/* convert 16-bit little endian to signed integer */
@@ -216,33 +217,55 @@ static inline int raw2int(__le16 x)
}

/* scale device data to logical dimensions (asserts devmin < devmax) */
-static inline int int2scale(const struct atp_params *p, int x)
+static inline int int2scale(const struct bcm5974_param *p, int x)
{
return x * p->dim / (p->devmax - p->devmin);
}

/* all logical value ranges are [0,dim). */
-static inline int int2bound(const struct atp_params *p, int x)
+static inline int int2bound(const struct bcm5974_param *p, int x)
{
int s = int2scale(p, x);
return s < 0 ? 0 : s >= p->dim ? p->dim - 1 : s;
}

+/* setup which logical events to report */
+static void setup_events_to_report(struct input_dev *input_dev,
+ const struct bcm5974_config *cfg)
+{
+ set_bit(EV_ABS, input_dev->evbit);
+ input_set_abs_params(input_dev, ABS_PRESSURE,
+ 0, cfg->p.dim, cfg->p.fuzz, 0);
+ input_set_abs_params(input_dev, ABS_TOOL_WIDTH,
+ 0, cfg->w.dim, cfg->w.fuzz, 0);
+ input_set_abs_params(input_dev, ABS_X,
+ 0, cfg->x.dim, cfg->x.fuzz, 0);
+ input_set_abs_params(input_dev, ABS_Y,
+ 0, cfg->y.dim, cfg->y.fuzz, 0);
+
+ set_bit(EV_KEY, input_dev->evbit);
+ 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);
+}
+
/* report button data as logical button state */
-static int report_bt_state(struct atp *dev, int size)
+static int report_bt_state(struct bcm5974 *dev, int size)
{
if (size != sizeof(struct bt_data))
return -EIO;

input_report_key(dev->input, BTN_LEFT, dev->bt_data->button);
+ input_sync(dev->input);

return 0;
}

/* report trackpad data as logical trackpad state */
-static int report_tp_state(struct atp *dev, int size)
+static int report_tp_state(struct bcm5974 *dev, int size)
{
- const struct atp_config *c = &dev->cfg;
+ const struct bcm5974_config *c = &dev->cfg;
const struct tp_finger *f = dev->tp_data->finger;
const int fingers = (size - 26) / 28;
int p, w, x, y, n;
@@ -255,6 +278,7 @@ static int report_tp_state(struct atp *dev, int size)
input_report_key(dev->input, BTN_TOOL_FINGER, false);
input_report_key(dev->input, BTN_TOOL_DOUBLETAP, false);
input_report_key(dev->input, BTN_TOOL_TRIPLETAP, false);
+ input_sync(dev->input);
return 0;
}

@@ -274,17 +298,18 @@ static int report_tp_state(struct atp *dev, int size)
input_report_key(dev->input, BTN_TOOL_FINGER, n == 1);
input_report_key(dev->input, BTN_TOOL_DOUBLETAP, n == 2);
input_report_key(dev->input, BTN_TOOL_TRIPLETAP, n > 2);
+ input_sync(dev->input);
return 0;
}

/* Wellspring initialization constants */
-#define ATP_WELLSPRING_MODE_READ_REQUEST_ID 1
-#define ATP_WELLSPRING_MODE_WRITE_REQUEST_ID 9
-#define ATP_WELLSPRING_MODE_REQUEST_VALUE 0x300
-#define ATP_WELLSPRING_MODE_REQUEST_INDEX 0
-#define ATP_WELLSPRING_MODE_VENDOR_VALUE 0x01
+#define BCM5974_WELLSPRING_MODE_READ_REQUEST_ID 1
+#define BCM5974_WELLSPRING_MODE_WRITE_REQUEST_ID 9
+#define BCM5974_WELLSPRING_MODE_REQUEST_VALUE 0x300
+#define BCM5974_WELLSPRING_MODE_REQUEST_INDEX 0
+#define BCM5974_WELLSPRING_MODE_VENDOR_VALUE 0x01

-static int atp_wellspring_mode(struct atp *dev)
+static int bcm5974_wellspring_mode(struct bcm5974 *dev)
{
char *data = kmalloc(8, GFP_KERNEL);
int error = 0, size;
@@ -297,10 +322,10 @@ static int atp_wellspring_mode(struct atp *dev)

/* read configuration */
size = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
- ATP_WELLSPRING_MODE_READ_REQUEST_ID,
+ BCM5974_WELLSPRING_MODE_READ_REQUEST_ID,
USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
- ATP_WELLSPRING_MODE_REQUEST_VALUE,
- ATP_WELLSPRING_MODE_REQUEST_INDEX, data, 8, 5000);
+ BCM5974_WELLSPRING_MODE_REQUEST_VALUE,
+ BCM5974_WELLSPRING_MODE_REQUEST_INDEX, data, 8, 5000);

if (size != 8) {
err("bcm5974: could not read from device");
@@ -309,14 +334,14 @@ static int atp_wellspring_mode(struct atp *dev)
}

/* apply the mode switch */
- data[0] = ATP_WELLSPRING_MODE_VENDOR_VALUE;
+ data[0] = BCM5974_WELLSPRING_MODE_VENDOR_VALUE;

/* write configuration */
size = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
- ATP_WELLSPRING_MODE_WRITE_REQUEST_ID,
+ BCM5974_WELLSPRING_MODE_WRITE_REQUEST_ID,
USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
- ATP_WELLSPRING_MODE_REQUEST_VALUE,
- ATP_WELLSPRING_MODE_REQUEST_INDEX, data, 8, 5000);
+ BCM5974_WELLSPRING_MODE_REQUEST_VALUE,
+ BCM5974_WELLSPRING_MODE_REQUEST_INDEX, data, 8, 5000);

if (size != 8) {
err("bcm5974: could not write to device");
@@ -336,7 +361,7 @@ error:

static void irq_button(struct urb *urb)
{
- struct atp *dev = urb->context;
+ struct bcm5974 *dev = urb->context;
int error;

switch (urb->status) {
@@ -353,13 +378,9 @@ static void irq_button(struct urb *urb)
goto exit;
}

- if (report_bt_state(dev, dev->bt_urb->actual_length)) {
+ if (report_bt_state(dev, dev->bt_urb->actual_length))
dprintk(1, "bcm5974: bad button package, length: %d\n",
dev->bt_urb->actual_length);
- goto exit;
- }
-
- input_sync(dev->input);

exit:
error = usb_submit_urb(dev->bt_urb, GFP_ATOMIC);
@@ -369,7 +390,7 @@ exit:

static void irq_trackpad(struct urb *urb)
{
- struct atp *dev = urb->context;
+ struct bcm5974 *dev = urb->context;
int error;

switch (urb->status) {
@@ -390,13 +411,9 @@ static void irq_trackpad(struct urb *urb)
if (dev->tp_urb->actual_length == 2)
goto exit;

- if (report_tp_state(dev, dev->tp_urb->actual_length)) {
+ if (report_tp_state(dev, dev->tp_urb->actual_length))
dprintk(1, "bcm5974: bad trackpad package, length: %d\n",
dev->tp_urb->actual_length);
- goto exit;
- }
-
- input_sync(dev->input);

exit:
error = usb_submit_urb(dev->tp_urb, GFP_ATOMIC);
@@ -417,99 +434,149 @@ exit:
* device, resulting in trackpad malfunction under certain
* circumstances. To get around this problem, there is at least one
* example that utilizes the USB_QUIRK_RESET_RESUME quirk in order to
- * recieve a reset_resume request rather than the normal resume. Since
- * the implementation of reset_resume is equal to mode switch plus
- * open, it seems easier to always do the switch while opening the
- * device.
+ * recieve a reset_resume request rather than the normal resume.
+ * Since the implementation of reset_resume is equal to mode switch
+ * plus start_traffic, it seems easier to always do the switch when
+ * starting traffic on the device.
*/
-static int atp_open(struct input_dev *input)
+static int start_traffic(struct bcm5974 *dev)
{
- struct atp *dev = input_get_drvdata(input);
-
- if (!dev->open) {
- if (atp_wellspring_mode(dev)) {
- dprintk(1, "bcm5974: mode switch failed\n");
- goto error;
- }
- if (usb_submit_urb(dev->bt_urb, GFP_KERNEL))
- goto error;
- if (usb_submit_urb(dev->tp_urb, GFP_KERNEL))
- goto err_kill_bt;
+ if (bcm5974_wellspring_mode(dev)) {
+ dprintk(1, "bcm5974: mode switch failed\n");
+ goto error;
}
+ if (usb_submit_urb(dev->bt_urb, GFP_KERNEL))
+ goto error;
+ if (usb_submit_urb(dev->tp_urb, GFP_KERNEL))
+ goto err_kill_bt;

- dev->open = 1;
- dev->suspended = 0;
return 0;
-
err_kill_bt:
usb_kill_urb(dev->bt_urb);
error:
return -EIO;
}

-static void atp_close(struct input_dev *input)
+static void pause_traffic(struct bcm5974 *dev)
{
- struct atp *dev = input_get_drvdata(input);
-
usb_kill_urb(dev->tp_urb);
usb_kill_urb(dev->bt_urb);
+}

- dev->open = 0;
- dev->suspended = 0;
+/*
+ * The code below implements open/close and manual suspend/resume.
+ * All functions may be called in random order.
+ *
+ * Opening a suspended device fails with EACCES - permission denied.
+ *
+ * Failing a resume leaves the device resumed but closed.
+ */
+static int bcm5974_open(struct input_dev *input)
+{
+ struct bcm5974 *dev = input_get_drvdata(input);
+ int error = 0;
+
+ mutex_lock(&dev->mutex);
+ if (dev->manually_suspended)
+ error = -EACCES;
+ else if (!dev->opened)
+ error = start_traffic(dev);
+ dev->opened = !error;
+ mutex_unlock(&dev->mutex);
+
+ return error;
}

-static int atp_probe(struct usb_interface *iface,
+static void bcm5974_close(struct input_dev *input)
+{
+ struct bcm5974 *dev = input_get_drvdata(input);
+
+ mutex_lock(&dev->mutex);
+ if (!dev->manually_suspended)
+ pause_traffic(dev);
+ dev->opened = 0;
+ mutex_unlock(&dev->mutex);
+}
+
+static int bcm5974_suspend(struct usb_interface *iface, pm_message_t message)
+{
+ struct bcm5974 *dev = usb_get_intfdata(iface);
+
+ if (dev) {
+ mutex_lock(&dev->mutex);
+ if (dev->opened && !dev->manually_suspended)
+ pause_traffic(dev);
+ dev->manually_suspended++;
+ mutex_unlock(&dev->mutex);
+ }
+
+ return 0;
+}
+
+static int bcm5974_resume(struct usb_interface *iface)
+{
+ struct bcm5974 *dev = usb_get_intfdata(iface);
+ int error = 0;
+
+ if (dev) {
+ mutex_lock(&dev->mutex);
+ if (dev->manually_suspended)
+ dev->manually_suspended--;
+ if (dev->opened && !dev->manually_suspended)
+ error = start_traffic(dev);
+ if (error)
+ dev->opened = 0;
+ mutex_unlock(&dev->mutex);
+ }
+
+ return error;
+}
+
+static int bcm5974_probe(struct usb_interface *iface,
const struct usb_device_id *id)
{
struct usb_device *udev = interface_to_usbdev(iface);
- const struct atp_config *cfg;
- struct atp *dev;
+ const struct bcm5974_config *cfg;
+ struct bcm5974 *dev;
struct input_dev *input_dev;
- int error = 0;
+ int error = -ENOMEM;

/* find the product index */
- cfg = atp_product_config(udev);
+ cfg = bcm5974_get_config(udev);

/* allocate memory for our device state and initialize it */
- dev = kzalloc(sizeof(struct atp), GFP_KERNEL);
+ dev = kzalloc(sizeof(struct bcm5974), GFP_KERNEL);
input_dev = input_allocate_device();
if (!dev || !input_dev) {
err("bcm5974: out of memory");
- error = -ENOMEM;
goto err_free_devs;
}

dev->udev = udev;
dev->input = input_dev;
dev->cfg = *cfg;
+ mutex_init(&dev->mutex);

+ /* setup urbs */
dev->bt_urb = usb_alloc_urb(0, GFP_KERNEL);
- if (!dev->bt_urb) {
- error = -ENOMEM;
+ if (!dev->bt_urb)
goto err_free_devs;
- }

dev->tp_urb = usb_alloc_urb(0, GFP_KERNEL);
- if (!dev->tp_urb) {
- error = -ENOMEM;
+ if (!dev->tp_urb)
goto err_free_bt_urb;
- }

dev->bt_data = usb_buffer_alloc(dev->udev,
dev->cfg.bt_datalen, GFP_KERNEL,
&dev->bt_urb->transfer_dma);
- if (!dev->bt_data) {
- error = -ENOMEM;
+ if (!dev->bt_data)
goto err_free_urb;
- }

dev->tp_data = usb_buffer_alloc(dev->udev,
dev->cfg.tp_datalen, GFP_KERNEL,
&dev->tp_urb->transfer_dma);
- if (!dev->tp_data) {
- error = -ENOMEM;
+ if (!dev->tp_data)
goto err_free_bt_buffer;
- }

usb_fill_int_urb(dev->bt_urb, udev,
usb_rcvintpipe(udev, cfg->bt_ep),
@@ -521,6 +588,7 @@ static int atp_probe(struct usb_interface *iface,
dev->tp_data, dev->cfg.tp_datalen,
irq_trackpad, dev, 1);

+ /* create bcm5974 device */
usb_make_path(udev, dev->phys, sizeof(dev->phys));
strlcat(dev->phys, "/input0", sizeof(dev->phys));

@@ -531,24 +599,10 @@ static int atp_probe(struct usb_interface *iface,

input_set_drvdata(input_dev, dev);

- input_dev->open = atp_open;
- input_dev->close = atp_close;
+ input_dev->open = bcm5974_open;
+ input_dev->close = bcm5974_close;

- set_bit(EV_ABS, input_dev->evbit);
- input_set_abs_params(input_dev, ABS_PRESSURE,
- 0, cfg->p.dim, cfg->p.fuzz, 0);
- input_set_abs_params(input_dev, ABS_TOOL_WIDTH,
- 0, cfg->w.dim, cfg->w.fuzz, 0);
- input_set_abs_params(input_dev, ABS_X,
- 0, cfg->x.dim, cfg->x.fuzz, 0);
- input_set_abs_params(input_dev, ABS_Y,
- 0, cfg->y.dim, cfg->y.fuzz, 0);
-
- set_bit(EV_KEY, input_dev->evbit);
- 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);
+ setup_events_to_report(input_dev, cfg);

error = input_register_device(dev->input);
if (error)
@@ -571,77 +625,49 @@ err_free_bt_urb:
usb_free_urb(dev->bt_urb);
err_free_devs:
usb_set_intfdata(iface, NULL);
- kfree(dev);
input_free_device(input_dev);
+ kfree(dev);
return error;
}

-static void atp_disconnect(struct usb_interface *iface)
+static void bcm5974_disconnect(struct usb_interface *iface)
{
- struct atp *dev = usb_get_intfdata(iface);
+ struct bcm5974 *dev = usb_get_intfdata(iface);

usb_set_intfdata(iface, NULL);

- if (dev) {
- input_unregister_device(dev->input);
- usb_buffer_free(dev->udev, dev->cfg.tp_datalen,
- dev->tp_data, dev->tp_urb->transfer_dma);
- usb_buffer_free(dev->udev, dev->cfg.bt_datalen,
- dev->bt_data, dev->bt_urb->transfer_dma);
- usb_free_urb(dev->tp_urb);
- usb_free_urb(dev->bt_urb);
- kfree(dev);
- }
+ input_unregister_device(dev->input);
+ usb_buffer_free(dev->udev, dev->cfg.tp_datalen,
+ dev->tp_data, dev->tp_urb->transfer_dma);
+ usb_buffer_free(dev->udev, dev->cfg.bt_datalen,
+ dev->bt_data, dev->bt_urb->transfer_dma);
+ usb_free_urb(dev->tp_urb);
+ usb_free_urb(dev->bt_urb);
+ kfree(dev);

printk(KERN_INFO "bcm5974: disconnected\n");
}

-static int atp_suspend(struct usb_interface *iface, pm_message_t message)
-{
- struct atp *dev = usb_get_intfdata(iface);
-
- if (dev) {
- if (!dev->suspended)
- atp_close(dev->input);
- dev->suspended++;
- }
-
- return 0;
-}
-
-static int atp_resume(struct usb_interface *iface)
-{
- struct atp *dev = usb_get_intfdata(iface);
- int error = 0;
-
- if (dev && dev->suspended) {
- if (!--dev->suspended)
- error = atp_open(dev->input);
- }
-
- return error;
-}
-
-static struct usb_driver atp_driver = {
+static struct usb_driver bcm5974_driver = {
.name = "bcm5974",
- .probe = atp_probe,
- .disconnect = atp_disconnect,
- .suspend = atp_suspend,
- .resume = atp_resume,
- .reset_resume = atp_resume,
- .id_table = atp_table,
+ .probe = bcm5974_probe,
+ .disconnect = bcm5974_disconnect,
+ .suspend = bcm5974_suspend,
+ .resume = bcm5974_resume,
+ .reset_resume = bcm5974_resume,
+ .id_table = bcm5974_table,
};

-static int __init atp_init(void)
+static int __init bcm5974_init(void)
{
- return usb_register(&atp_driver);
+ return usb_register(&bcm5974_driver);
}

-static void __exit atp_exit(void)
+static void __exit bcm5974_exit(void)
{
- usb_deregister(&atp_driver);
+ usb_deregister(&bcm5974_driver);
}

-module_init(atp_init);
-module_exit(atp_exit);
+module_init(bcm5974_init);
+module_exit(bcm5974_exit);

2008-07-23 13:38:31

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH] linux-input: bcm5974-0.57: mode-switch to atp_open, cleanup bug fixed

On Tue, 22 Jul 2008, Dmitry Torokhov wrote:

> > 4. From the state (!opened,suspended), calling open fails. Where are we?
> >
>
> Manual suspend will surely cause open to fail. Autosuspend - if
> autoresume fails then (closed, unknown) otherwise (closed,
> autosuspended - autosuspension may not kick in immediately I think).
>
> > 5. From the state (opened,suspended), calling resume fails. What state
> > are we in?
> >
>
> Screwed up ;) From the driver POV still (opened, suspended) I think.

In both of these cases, the final result depends on the reason why the
resume failed. If it failed for administrative reasons (such as it was
an autoresume and the device had been manually suspended), then the
final state is still suspended. But if it fails because the kernel
actually tried to resume the device and couldn't do so, then the final
state is that the device is gone, logically disconnected.

Alan Stern