2005-12-13 22:37:10

by Michael Hanselmann

[permalink] [raw]
Subject: [PATCH 2.6 1/2] usb/input: Add relayfs support to appletouch driver

This patch adds support for relayfs to the appletouch driver to make
debugging easier.

Signed-off-by: Michael Hanselmann <[email protected]>
Acked-by: Rene Nussbaumer <[email protected]>
Acked-by: Johannes Berg <[email protected]>
---
diff -Npur linux-2.6.15-rc5.orig/Documentation/input/appletouch.txt linux-2.6.15-rc5/Documentation/input/appletouch.txt
--- linux-2.6.15-rc5.orig/Documentation/input/appletouch.txt 2005-12-13 00:09:24.000000000 +0100
+++ linux-2.6.15-rc5/Documentation/input/appletouch.txt 2005-12-13 21:28:26.000000000 +0100
@@ -77,6 +78,8 @@ full tracing (each sample is being trace
or
echo "1" > /sys/module/appletouch/parameters/debug

+To make debugging easier, the driver also supports the relayfs filesystem.
+
Links:
------

diff -Npur linux-2.6.15-rc5.orig/drivers/usb/input/appletouch.txt linux-2.6.15-rc5/drivers/usb/input/appletouch.txt
--- linux-2.6.15-rc5.orig/drivers/usb/input/appletouch.c 2005-12-13 00:09:24.000000000 +0100
+++ linux-2.6.15-rc5/drivers/usb/input/appletouch.c 2005-12-13 22:41:28.000000000 +0100
@@ -6,9 +6,18 @@
* Copyright (C) 2005 Stelian Pop ([email protected])
* Copyright (C) 2005 Frank Arnold ([email protected])
* Copyright (C) 2005 Peter Osterlund ([email protected])
+ * Copyright (C) 2005 Parag Warudkar ([email protected])
+ * Copyright (C) 2005 Michael Hanselmann ([email protected])
*
* Thanks to Alex Harper <[email protected]> for his inputs.
*
+ * Nov 2005 - Parag Warudkar
+ * o Added ability to export data via relayfs
+ *
+ * Nov/Dec 2005 - Michael Hanselmann
+ * o Compile relayfs support only if enabled in the kernel
+ * o Enable relayfs only if requested by the user
+ *
* 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
@@ -35,6 +44,10 @@
#include <linux/input.h>
#include <linux/usb_input.h>

+#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
+#include <linux/relayfs_fs.h>
+#endif
+
/* Apple has powerbooks which have the keyboard with different Product IDs */
#define APPLE_VENDOR_ID 0x05AC

@@ -57,6 +70,11 @@ static struct usb_device_id atp_table []
};
MODULE_DEVICE_TABLE (usb, atp_table);

+#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
+struct rchan* rch = NULL;
+struct rchan_callbacks* rcb = NULL;
+#endif
+
/* size of a USB urb transfer */
#define ATP_DATASIZE 81

@@ -73,6 +91,7 @@ MODULE_DEVICE_TABLE (usb, atp_table);

/* maximum pressure this driver will report */
#define ATP_PRESSURE 300
+
/*
* multiplication factor for the X and Y coordinates.
* We try to keep the touchpad aspect ratio while still doing only simple
@@ -124,7 +143,7 @@ struct atp {
if (debug) printk(format, ##a); \
} while (0)

-MODULE_AUTHOR("Johannes Berg, Stelian Pop, Frank Arnold");
+MODULE_AUTHOR("Johannes Berg, Stelian Pop, Frank Arnold, Parag Warudkar, Michael Hanselmann");
MODULE_DESCRIPTION("Apple PowerBooks USB touchpad driver");
MODULE_LICENSE("GPL");

@@ -132,6 +151,12 @@ static int debug = 1;
module_param(debug, int, 0644);
MODULE_PARM_DESC(debug, "Activate debugging output");

+#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
+static int relayfs = 0;
+module_param(relayfs, int, 0644);
+MODULE_PARM_DESC(relayfs, "Activate relayfs support");
+#endif
+
static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
int *z, int *fingers)
{
@@ -194,6 +219,13 @@ static void atp_complete(struct urb* urb
goto exit;
}

+#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
+ /* "rch" is NULL if relayfs is not enabled */
+ if (rch && dev->data) {
+ relay_write(rch, dev->data, dev->urb->actual_length);
+ }
+#endif
+
/* reorder the sensors values */
for (i = 0; i < 8; i++) {
/* X values */
@@ -463,11 +495,43 @@ static struct usb_driver atp_driver = {

static int __init atp_init(void)
{
+#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
+ rcb = NULL;
+ rch = NULL;
+
+ if (relayfs) {
+ rcb = kmalloc(sizeof(struct rchan_callbacks), GFP_KERNEL);
+ if (!rcb)
+ return -ENOMEM;
+
+ rcb->subbuf_start = NULL;
+ rcb->buf_mapped = NULL;
+ rcb->buf_unmapped = NULL;
+
+ rch = relay_open("atpdata", NULL, 256, 256, NULL);
+ if (!rch) {
+ kfree(rcb);
+ return -ENOMEM;
+ }
+
+ printk("appletouch: Relayfs enabled.\n");
+ } else {
+ printk("appletouch: Relayfs disabled.\n");
+ }
+#endif
+
return usb_register(&atp_driver);
}

static void __exit atp_exit(void)
{
+#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
+ if(rch)
+ relay_close(rch);
+ if(rcb)
+ kfree(rcb);
+#endif
+
usb_deregister(&atp_driver);
}


2005-12-13 22:40:25

by Michael Hanselmann

[permalink] [raw]
Subject: [PATCH 2.6 2/2] usb/input: Add Geyser 2 support to appletouch driver

This patch adds support for the Geyser 2 touchpads used on Post-Oct 2005
Apple PowerBooks to the appletouch driver.

Signed-off-by: Michael Hanselmann <[email protected]>
Acked-by: Rene Nussbaumer <[email protected]>
Acked-by: Johannes Berg <[email protected]>
---
diff -Npur linux-2.6.15-rc5.orig/Documentation/input/appletouch.txt linux-2.6.15-rc5/Documentation/input/appletouch.txt
--- linux-2.6.15-rc5.orig/Documentation/input/appletouch.txt 2005-12-13 00:09:24.000000000 +0100
+++ linux-2.6.15-rc5/Documentation/input/appletouch.txt 2005-12-13 21:28:26.000000000 +0100
@@ -3,7 +3,7 @@ Apple Touchpad Driver (appletouch)
Copyright (C) 2005 Stelian Pop <[email protected]>

appletouch is a Linux kernel driver for the USB touchpad found on post
-February 2005 Apple Alu Powerbooks.
+February 2005 and October 2005 Apple Aluminium Powerbooks.

This driver is derived from Johannes Berg's appletrackpad driver[1], but it has
been improved in some areas:
@@ -13,7 +13,8 @@ been improved in some areas:

Credits go to Johannes Berg for reverse-engineering the touchpad protocol,
Frank Arnold for further improvements, and Alex Harper for some additional
-information about the inner workings of the touchpad sensors.
+information about the inner workings of the touchpad sensors. Michael
+Hanselmann added support for the October 2005 models.

Usage:
------

diff -Npur linux-2.6.15-rc5.orig/drivers/usb/input/appletouch.c linux-2.6.15-rc5/drivers/usb/input/appletouch.c
--- linux-2.6.15-rc5.orig/drivers/usb/input/appletouch.c 2005-12-13 22:44:36.000000000 +0100
+++ linux-2.6.15-rc5/drivers/usb/input/appletouch.c 2005-12-13 22:55:19.000000000 +0100
@@ -17,6 +17,7 @@
* Nov/Dec 2005 - Michael Hanselmann
* o Compile relayfs support only if enabled in the kernel
* o Enable relayfs only if requested by the user
+ * o Added support for new October 2005 PowerBooks (Geyser 2)
*
* 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
@@ -51,6 +52,11 @@
/* Apple has powerbooks which have the keyboard with different Product IDs */
#define APPLE_VENDOR_ID 0x05AC

+/* These names come from Info.plist in AppleUSBTrackpad.kext */
+#define GEYSER_ANSI_PRODUCT_ID 0x0214
+#define GEYSER_ISO_PRODUCT_ID 0x0215
+#define GEYSER_JIS_PRODUCT_ID 0x0216
+
#define ATP_DEVICE(prod) \
.match_flags = USB_DEVICE_ID_MATCH_DEVICE | \
USB_DEVICE_ID_MATCH_INT_CLASS | \
@@ -66,7 +72,14 @@ static struct usb_device_id atp_table []
{ ATP_DEVICE(0x020F) },
{ ATP_DEVICE(0x030A) },
{ ATP_DEVICE(0x030B) },
- { } /* Terminating entry */
+
+ /* PowerBooks Oct 2005 */
+ { ATP_DEVICE(GEYSER_ANSI_PRODUCT_ID) },
+ { ATP_DEVICE(GEYSER_ISO_PRODUCT_ID) },
+ { ATP_DEVICE(GEYSER_JIS_PRODUCT_ID) },
+
+ /* Terminating entry */
+ { }
};
MODULE_DEVICE_TABLE (usb, atp_table);

@@ -75,9 +88,6 @@ struct rchan* rch = NULL;
struct rchan_callbacks* rcb = NULL;
#endif

-/* size of a USB urb transfer */
-#define ATP_DATASIZE 81
-
/*
* number of sensors. Note that only 16 instead of 26 X (horizontal)
* sensors exist on 12" and 15" PowerBooks. All models have 16 Y
@@ -127,6 +137,8 @@ struct atp {
signed char xy_old[ATP_XSENSORS + ATP_YSENSORS];
/* accumulated sensors */
int xy_acc[ATP_XSENSORS + ATP_YSENSORS];
+ int overflowwarn; /* overflow warning printed? */
+ int datalen; /* size of an USB urb transfer */
};

#define dbg_dump(msg, tab) \
@@ -157,6 +169,16 @@ module_param(relayfs, int, 0644);
MODULE_PARM_DESC(relayfs, "Activate relayfs support");
#endif

+/* Checks if the device a Geyser 2 (ANSI, ISO, JIS) */
+static inline int atp_is_geyser_2(struct atp *dev)
+{
+ int16_t productId = le16_to_cpu(dev->udev->descriptor.idProduct);
+
+ return (productId == GEYSER_ANSI_PRODUCT_ID) ||
+ (productId == GEYSER_ISO_PRODUCT_ID) ||
+ (productId == GEYSER_JIS_PRODUCT_ID);
+}
+
static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
int *z, int *fingers)
{
@@ -200,6 +222,13 @@ static void atp_complete(struct urb* urb
case 0:
/* success */
break;
+ case -EOVERFLOW:
+ if(!dev->overflowwarn) {
+ printk("appletouch: OVERFLOW with data "
+ "length %d, actual length is %d\n",
+ dev->datalen, dev->urb->actual_length);
+ dev->overflowwarn = 1;
+ }
case -ECONNRESET:
case -ENOENT:
case -ESHUTDOWN:
@@ -214,7 +243,7 @@ static void atp_complete(struct urb* urb
}

/* drop incomplete datasets */
- if (dev->urb->actual_length != ATP_DATASIZE) {
+ if (dev->urb->actual_length != dev->datalen) {
dprintk("appletouch: incomplete data package.\n");
goto exit;
}
@@ -227,17 +256,72 @@ static void atp_complete(struct urb* urb
#endif

/* reorder the sensors values */
- for (i = 0; i < 8; i++) {
- /* X values */
- dev->xy_cur[i ] = dev->data[5 * i + 2];
- dev->xy_cur[i + 8] = dev->data[5 * i + 4];
- dev->xy_cur[i + 16] = dev->data[5 * i + 42];
- if (i < 2)
- dev->xy_cur[i + 24] = dev->data[5 * i + 44];
-
- /* Y values */
- dev->xy_cur[i + 26] = dev->data[5 * i + 1];
- dev->xy_cur[i + 34] = dev->data[5 * i + 3];
+ if (atp_is_geyser_2(dev)) {
+ memset(dev->xy_cur, 0, sizeof(dev->xy_cur));
+
+ /*
+ * The values are laid out like this:
+ * Y1, Y2, -, Y3, Y4, -, ...
+ * '-' is an unused value.
+ *
+ * The logic in a loop:
+ * for (i = 0, j = 19; i < 20; i += 2, j += 3) {
+ * dev->xy_cur[i] = dev->data[j];
+ * dev->xy_cur[i + 1] = dev->data[j + 1];
+ * }
+ *
+ * This code is called about 100 times per second because it is
+ * called for each interrupt transfer from the touchpad.
+ * Therefore it should be as fast as possible. Writing the
+ * following code in a loop would take about twice the time to
+ * run if not more.
+ */
+
+ /* read X values */
+ dev->xy_cur[0] = dev->data[19];
+ dev->xy_cur[1] = dev->data[20];
+ dev->xy_cur[2] = dev->data[22];
+ dev->xy_cur[3] = dev->data[23];
+ dev->xy_cur[4] = dev->data[25];
+ dev->xy_cur[5] = dev->data[26];
+ dev->xy_cur[6] = dev->data[28];
+ dev->xy_cur[7] = dev->data[29];
+ dev->xy_cur[8] = dev->data[31];
+ dev->xy_cur[9] = dev->data[32];
+ dev->xy_cur[10] = dev->data[34];
+ dev->xy_cur[11] = dev->data[35];
+ dev->xy_cur[12] = dev->data[37];
+ dev->xy_cur[13] = dev->data[38];
+ dev->xy_cur[14] = dev->data[40];
+ dev->xy_cur[15] = dev->data[41];
+ dev->xy_cur[16] = dev->data[43];
+ dev->xy_cur[17] = dev->data[44];
+ dev->xy_cur[18] = dev->data[46];
+ dev->xy_cur[19] = dev->data[47];
+
+ /* read Y values */
+ dev->xy_cur[ATP_XSENSORS + 0] = dev->data[1];
+ dev->xy_cur[ATP_XSENSORS + 1] = dev->data[2];
+ dev->xy_cur[ATP_XSENSORS + 2] = dev->data[4];
+ dev->xy_cur[ATP_XSENSORS + 3] = dev->data[5];
+ dev->xy_cur[ATP_XSENSORS + 4] = dev->data[7];
+ dev->xy_cur[ATP_XSENSORS + 5] = dev->data[8];
+ dev->xy_cur[ATP_XSENSORS + 6] = dev->data[10];
+ dev->xy_cur[ATP_XSENSORS + 7] = dev->data[11];
+ dev->xy_cur[ATP_XSENSORS + 8] = dev->data[13];
+ } else {
+ for (i = 0; i < 8; i++) {
+ /* X values */
+ dev->xy_cur[i ] = dev->data[5 * i + 2];
+ dev->xy_cur[i + 8] = dev->data[5 * i + 4];
+ dev->xy_cur[i + 16] = dev->data[5 * i + 42];
+ if (i < 2)
+ dev->xy_cur[i + 24] = dev->data[5 * i + 44];
+
+ /* Y values */
+ dev->xy_cur[i + 26] = dev->data[5 * i + 1];
+ dev->xy_cur[i + 34] = dev->data[5 * i + 3];
+ }
}

dbg_dump("sample", dev->xy_cur);
@@ -248,16 +332,24 @@ static void atp_complete(struct urb* urb
dev->x_old = dev->y_old = -1;
memcpy(dev->xy_old, dev->xy_cur, sizeof(dev->xy_old));

- /* 17" Powerbooks have 10 extra X sensors */
- for (i = 16; i < ATP_XSENSORS; i++)
- if (dev->xy_cur[i]) {
- printk("appletouch: 17\" model detected.\n");
+ /* 17" Powerbooks have extra X sensors */
+ for (i = (atp_is_geyser_2(dev)?15:16); i < ATP_XSENSORS; i++) {
+ if (!dev->xy_cur[i]) continue;
+
+ printk("appletouch: 17\" model detected.\n");
+ if(atp_is_geyser_2(dev))
+ input_set_abs_params(dev->input, ABS_X, 0,
+ (20 - 1) *
+ ATP_XFACT - 1,
+ ATP_FUZZ, 0);
+ else
input_set_abs_params(dev->input, ABS_X, 0,
(ATP_XSENSORS - 1) *
ATP_XFACT - 1,
ATP_FUZZ, 0);
- break;
- }
+
+ break;
+ }

goto exit;
}
@@ -314,7 +406,8 @@ static void atp_complete(struct urb* urb
memset(dev->xy_acc, 0, sizeof(dev->xy_acc));
}

- input_report_key(dev->input, BTN_LEFT, !!dev->data[80]);
+ input_report_key(dev->input, BTN_LEFT,
+ !!dev->data[dev->datalen - 1]);

input_sync(dev->input);

@@ -355,7 +448,6 @@ static int atp_probe(struct usb_interfac
int int_in_endpointAddr = 0;
int i, retval = -ENOMEM;

-
/* set up the endpoint information */
/* use only the first interrupt-in endpoint */
iface_desc = iface->cur_altsetting;
@@ -385,6 +477,8 @@ static int atp_probe(struct usb_interfac

dev->udev = udev;
dev->input = input_dev;
+ dev->overflowwarn = 0;
+ dev->datalen = (atp_is_geyser_2(dev)?64:81);

dev->urb = usb_alloc_urb(0, GFP_KERNEL);
if (!dev->urb) {
@@ -392,7 +486,7 @@ static int atp_probe(struct usb_interfac
goto err_free_devs;
}

- dev->data = usb_buffer_alloc(dev->udev, ATP_DATASIZE, GFP_KERNEL,
+ dev->data = usb_buffer_alloc(dev->udev, dev->datalen, GFP_KERNEL,
&dev->urb->transfer_dma);
if (!dev->data) {
retval = -ENOMEM;
@@ -401,7 +495,7 @@ static int atp_probe(struct usb_interfac

usb_fill_int_urb(dev->urb, udev,
usb_rcvintpipe(udev, int_in_endpointAddr),
- dev->data, ATP_DATASIZE, atp_complete, dev, 1);
+ dev->data, dev->datalen, atp_complete, dev, 1);

usb_make_path(udev, dev->phys, sizeof(dev->phys));
strlcat(dev->phys, "/input0", sizeof(dev->phys));
@@ -417,14 +511,25 @@ static int atp_probe(struct usb_interfac

set_bit(EV_ABS, input_dev->evbit);

- /*
- * 12" and 15" Powerbooks only have 16 x sensors,
- * 17" models are detected later.
- */
- input_set_abs_params(input_dev, ABS_X, 0,
- (16 - 1) * ATP_XFACT - 1, ATP_FUZZ, 0);
- input_set_abs_params(input_dev, ABS_Y, 0,
- (ATP_YSENSORS - 1) * ATP_YFACT - 1, ATP_FUZZ, 0);
+ if (atp_is_geyser_2(dev)) {
+ /*
+ * Oct 2005 15" PowerBooks have 15 X sensors, 17" are detected
+ * later.
+ */
+ input_set_abs_params(input_dev, ABS_X, 0,
+ ((15 - 1) * ATP_XFACT) - 1, ATP_FUZZ, 0);
+ input_set_abs_params(input_dev, ABS_Y, 0,
+ ((9 - 1) * ATP_YFACT) - 1, ATP_FUZZ, 0);
+ } else {
+ /*
+ * 12" and 15" Powerbooks only have 16 x sensors,
+ * 17" models are detected later.
+ */
+ input_set_abs_params(input_dev, ABS_X, 0,
+ (16 - 1) * ATP_XFACT - 1, ATP_FUZZ, 0);
+ input_set_abs_params(input_dev, ABS_Y, 0,
+ (ATP_YSENSORS - 1) * ATP_YFACT - 1, ATP_FUZZ, 0);
+ }
input_set_abs_params(input_dev, ABS_PRESSURE, 0, ATP_PRESSURE, 0, 0);

set_bit(EV_KEY, input_dev->evbit);
@@ -459,7 +564,7 @@ static void atp_disconnect(struct usb_in
usb_kill_urb(dev->urb);
input_unregister_device(dev->input);
usb_free_urb(dev->urb);
- usb_buffer_free(dev->udev, ATP_DATASIZE,
+ usb_buffer_free(dev->udev, dev->datalen,
dev->data, dev->urb->transfer_dma);
kfree(dev);
}

2005-12-14 13:57:24

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH 2.6 1/2] usb/input: Add relayfs support to appletouch driver

On Tue, 2005-12-13 at 23:36 +0100, Michael Hanselmann wrote:
> +module_param(relayfs, int, 0644);

Why do you make the parameter writable if writing it doesn't change
anything?

johannes


Attachments:
signature.asc (832.00 B)
This is a digitally signed message part

2005-12-14 21:39:27

by Michael Hanselmann

[permalink] [raw]
Subject: Re: [PATCH 2.6 1/2] usb/input: Add relayfs support to appletouch driver

This patch adds support for relayfs to the appletouch driver to make
debugging easier.

Signed-off-by: Michael Hanselmann <[email protected]>
Acked-by: Rene Nussbaumer <[email protected]>
Acked-by: Johannes Berg <[email protected]>
---
Updated to make the relayfs parameter readonly.

diff -Npur linux-2.6.15-rc5.orig/Documentation/input/appletouch.txt linux-2.6.15-rc5/Documentation/input/appletouch.txt
--- linux-2.6.15-rc5.orig/Documentation/input/appletouch.txt 2005-12-13 00:09:24.000000000 +0100
+++ linux-2.6.15-rc5/Documentation/input/appletouch.txt 2005-12-13 21:28:26.000000000 +0100
@@ -77,6 +78,8 @@ full tracing (each sample is being trace
or
echo "1" > /sys/module/appletouch/parameters/debug

+To make debugging easier, the driver also supports the relayfs filesystem.
+
Links:
------

diff -Npur linux-2.6.15-rc5.orig/drivers/usb/input/appletouch.txt linux-2.6.15-rc5/drivers/usb/input/appletouch.txt
--- linux-2.6.15-rc5.orig/drivers/usb/input/appletouch.c 2005-12-13 00:09:24.000000000 +0100
+++ linux-2.6.15-rc5/drivers/usb/input/appletouch.c 2005-12-13 22:41:28.000000000 +0100
@@ -6,9 +6,18 @@
* Copyright (C) 2005 Stelian Pop ([email protected])
* Copyright (C) 2005 Frank Arnold ([email protected])
* Copyright (C) 2005 Peter Osterlund ([email protected])
+ * Copyright (C) 2005 Parag Warudkar ([email protected])
+ * Copyright (C) 2005 Michael Hanselmann ([email protected])
*
* Thanks to Alex Harper <[email protected]> for his inputs.
*
+ * Nov 2005 - Parag Warudkar
+ * o Added ability to export data via relayfs
+ *
+ * Nov/Dec 2005 - Michael Hanselmann
+ * o Compile relayfs support only if enabled in the kernel
+ * o Enable relayfs only if requested by the user
+ *
* 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
@@ -35,6 +44,10 @@
#include <linux/input.h>
#include <linux/usb_input.h>

+#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
+#include <linux/relayfs_fs.h>
+#endif
+
/* Apple has powerbooks which have the keyboard with different Product IDs */
#define APPLE_VENDOR_ID 0x05AC

@@ -57,6 +70,11 @@ static struct usb_device_id atp_table []
};
MODULE_DEVICE_TABLE (usb, atp_table);

+#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
+struct rchan* rch = NULL;
+struct rchan_callbacks* rcb = NULL;
+#endif
+
/* size of a USB urb transfer */
#define ATP_DATASIZE 81

@@ -73,6 +91,7 @@ MODULE_DEVICE_TABLE (usb, atp_table);

/* maximum pressure this driver will report */
#define ATP_PRESSURE 300
+
/*
* multiplication factor for the X and Y coordinates.
* We try to keep the touchpad aspect ratio while still doing only simple
@@ -124,7 +143,7 @@ struct atp {
if (debug) printk(format, ##a); \
} while (0)

-MODULE_AUTHOR("Johannes Berg, Stelian Pop, Frank Arnold");
+MODULE_AUTHOR("Johannes Berg, Stelian Pop, Frank Arnold, Parag Warudkar, Michael Hanselmann");
MODULE_DESCRIPTION("Apple PowerBooks USB touchpad driver");
MODULE_LICENSE("GPL");

@@ -132,6 +151,12 @@ static int debug = 1;
module_param(debug, int, 0644);
MODULE_PARM_DESC(debug, "Activate debugging output");

+#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
+static int relayfs = 0;
+module_param(relayfs, int, 0444);
+MODULE_PARM_DESC(relayfs, "Activate relayfs support");
+#endif
+
static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
int *z, int *fingers)
{
@@ -194,6 +219,13 @@ static void atp_complete(struct urb* urb
goto exit;
}

+#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
+ /* "rch" is NULL if relayfs is not enabled */
+ if (rch && dev->data) {
+ relay_write(rch, dev->data, dev->urb->actual_length);
+ }
+#endif
+
/* reorder the sensors values */
for (i = 0; i < 8; i++) {
/* X values */
@@ -463,11 +495,43 @@ static struct usb_driver atp_driver = {

static int __init atp_init(void)
{
+#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
+ rcb = NULL;
+ rch = NULL;
+
+ if (relayfs) {
+ rcb = kmalloc(sizeof(struct rchan_callbacks), GFP_KERNEL);
+ if (!rcb)
+ return -ENOMEM;
+
+ rcb->subbuf_start = NULL;
+ rcb->buf_mapped = NULL;
+ rcb->buf_unmapped = NULL;
+
+ rch = relay_open("atpdata", NULL, 256, 256, NULL);
+ if (!rch) {
+ kfree(rcb);
+ return -ENOMEM;
+ }
+
+ printk("appletouch: Relayfs enabled.\n");
+ } else {
+ printk("appletouch: Relayfs disabled.\n");
+ }
+#endif
+
return usb_register(&atp_driver);
}

static void __exit atp_exit(void)
{
+#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
+ if(rch)
+ relay_close(rch);
+ if(rcb)
+ kfree(rcb);
+#endif
+
usb_deregister(&atp_driver);
}

2005-12-14 22:04:28

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH 2.6 1/2] usb/input: Add relayfs support to appletouch driver

Hi,

On 12/14/05, Michael Hanselmann <[email protected]> wrote:
> *
> + * Nov 2005 - Parag Warudkar
> + * o Added ability to export data via relayfs
> + *
> + * Nov/Dec 2005 - Michael Hanselmann
> + * o Compile relayfs support only if enabled in the kernel
> + * o Enable relayfs only if requested by the user
> + *

We have an SCM, not need to have a changelog in the driver.

>
> +#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
> +struct rchan* rch = NULL;
> +struct rchan_callbacks* rcb = NULL;

Initializing with 0/NULL adds to the size of the image for no reason.

>
> +#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
> +static int relayfs = 0;

Same here.

> +module_param(relayfs, int, 0444);
> +MODULE_PARM_DESC(relayfs, "Activate relayfs support");
> +#endif
> +
> static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
> int *z, int *fingers)
> {
> @@ -194,6 +219,13 @@ static void atp_complete(struct urb* urb
> goto exit;
> }
>
> +#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
> + /* "rch" is NULL if relayfs is not enabled */
> + if (rch && dev->data) {
> + relay_write(rch, dev->data, dev->urb->actual_length);
> + }
> +#endif
> +

Haveing ifdefs in the middle of the code is not too nice. can we
please have something like this:

#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
static inline void appletouch_relayfs_dump(...)
{
...
}
static int appletouch_relayfs_init(void)
{
...
}
static void appletouch_relayfs_exit()
{
...
}
#else
static inline void appletouch_relayfs_dump(...) { }
static int appletouch_relayfs_init(void) { return 0; }
static void appletouch_relayfs_exit() { }
#endif

Also, would not it be better to initialize relayfs only when device is
opened because there won't be any data otehrwise?

--
Dmitry

2005-12-14 23:31:16

by Michael Hanselmann

[permalink] [raw]
Subject: Re: [PATCH 2.6 1/2] usb/input: Add relayfs support to appletouch driver

This patch adds support for relayfs to the appletouch driver to make debugging
easier.

Signed-off-by: Michael Hanselmann <[email protected]>
Acked-by: Rene Nussbaumer <[email protected]>
Acked-by: Johannes Berg <[email protected]>
---
> Initializing with 0/NULL adds to the size of the image for no reason.

Does gcc automatically initialize them to that value?

> Haveing ifdefs in the middle of the code is not too nice. can we
> please have something like this:
> [...]

> Also, would not it be better to initialize relayfs only when device is
> opened because there won't be any data otehrwise?

Sure, both is done in this patch:

diff -rup linux-2.6.15-rc5.orig/Documentation/input/appletouch.txt b/Documentation/input/appletouch.txt
--- linux-2.6.15-rc5.orig/Documentation/input/appletouch.txt 2005-12-13 00:09:24.000000000 +0100
+++ b/Documentation/input/appletouch.txt 2005-12-15 00:09:33.000000000 +0100
@@ -77,6 +77,8 @@ full tracing (each sample is being trace
or
echo "1" > /sys/module/appletouch/parameters/debug

+To make debugging easier, the driver also supports the relayfs filesystem.
+
Links:
------

diff -rup linux-2.6.15-rc5.orig/drivers/usb/input/appletouch.c b/drivers/usb/input/appletouch.c
--- linux-2.6.15-rc5.orig/drivers/usb/input/appletouch.c 2005-12-13 22:44:24.000000000 +0100
+++ b/drivers/usb/input/appletouch.c 2005-12-15 00:25:09.000000000 +0100
@@ -6,6 +6,8 @@
* Copyright (C) 2005 Stelian Pop ([email protected])
* Copyright (C) 2005 Frank Arnold ([email protected])
* Copyright (C) 2005 Peter Osterlund ([email protected])
+ * Copyright (C) 2005 Parag Warudkar ([email protected])
+ * Copyright (C) 2005 Michael Hanselmann ([email protected])
*
* Thanks to Alex Harper <[email protected]> for his inputs.
*
@@ -35,6 +37,10 @@
#include <linux/input.h>
#include <linux/usb_input.h>

+#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
+#include <linux/relayfs_fs.h>
+#endif
+
/* Apple has powerbooks which have the keyboard with different Product IDs */
#define APPLE_VENDOR_ID 0x05AC

@@ -73,6 +79,7 @@ MODULE_DEVICE_TABLE (usb, atp_table);

/* maximum pressure this driver will report */
#define ATP_PRESSURE 300
+
/*
* multiplication factor for the X and Y coordinates.
* We try to keep the touchpad aspect ratio while still doing only simple
@@ -124,7 +131,7 @@ struct atp {
if (debug) printk(format, ##a); \
} while (0)

-MODULE_AUTHOR("Johannes Berg, Stelian Pop, Frank Arnold");
+MODULE_AUTHOR("Johannes Berg, Stelian Pop, Frank Arnold, Parag Warudkar, Michael Hanselmann");
MODULE_DESCRIPTION("Apple PowerBooks USB touchpad driver");
MODULE_LICENSE("GPL");

@@ -132,6 +139,68 @@ static int debug = 1;
module_param(debug, int, 0644);
MODULE_PARM_DESC(debug, "Activate debugging output");

+#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
+static int relayfs;
+module_param(relayfs, int, 0444);
+MODULE_PARM_DESC(relayfs, "Activate relayfs support");
+
+struct rchan *rch;
+struct rchan_callbacks *rcb;
+
+static inline void atp_relayfs_dump(struct atp *dev)
+{
+ /* "rch" is NULL if relayfs is disabled */
+ if (rch && dev->data) {
+ relay_write(rch, dev->data, dev->urb->actual_length);
+ }
+}
+
+static int appletouch_relayfs_init(void)
+{
+ // Make sure the variables aren't initialized to some bogus value when
+ // relayfs is disabled
+ rcb = NULL;
+ rch = NULL;
+
+ if (relayfs) {
+ rcb = kmalloc(sizeof(struct rchan_callbacks), GFP_KERNEL);
+ if (!rcb)
+ return -ENOMEM;
+
+ rcb->subbuf_start = NULL;
+ rcb->buf_mapped = NULL;
+ rcb->buf_unmapped = NULL;
+
+ rch = relay_open("atpdata", NULL, 256, 256, NULL);
+ if (!rch) {
+ kfree(rcb);
+ return -ENOMEM;
+ }
+
+ printk(KERN_INFO "appletouch: Relayfs enabled.\n");
+ }
+
+ return 0;
+}
+
+static void appletouch_relayfs_exit(void)
+{
+ if (rch) {
+ printk(KERN_INFO "appletouch: Relayfs disabled\n");
+ relay_close(rch);
+ rch = NULL;
+ }
+ if (rcb) {
+ kfree(rcb);
+ rcb = NULL;
+ }
+}
+#else
+static inline void atp_relayfs_dump(struct atp *dev) { }
+static int appletouch_relayfs_init(void) { return 0; }
+static void appletouch_relayfs_exit(void) { }
+#endif
+
static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
int *z, int *fingers)
{
@@ -194,6 +263,8 @@ static void atp_complete(struct urb* urb
goto exit;
}

+ atp_relayfs_dump(dev);
+
/* reorder the sensors values */
for (i = 0; i < 8; i++) {
/* X values */
@@ -297,6 +368,11 @@ exit:
static int atp_open(struct input_dev *input)
{
struct atp *dev = input->private;
+ int result;
+
+ result = appletouch_relayfs_init();
+ if (result < 0)
+ return result;

if (usb_submit_urb(dev->urb, GFP_ATOMIC))
return -EIO;
@@ -310,6 +386,8 @@ static void atp_close(struct input_dev *
struct atp *dev = input->private;

usb_kill_urb(dev->urb);
+ appletouch_relayfs_exit();
+
dev->open = 0;
}

2005-12-15 03:43:34

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH 2.6 1/2] usb/input: Add relayfs support to appletouch driver

On Wednesday 14 December 2005 18:31, Michael Hanselmann wrote:
> This patch adds support for relayfs to the appletouch driver to make debugging
> easier.
>
> Signed-off-by: Michael Hanselmann <[email protected]>
> Acked-by: Rene Nussbaumer <[email protected]>
> Acked-by: Johannes Berg <[email protected]>
> ---
> > Initializing with 0/NULL adds to the size of the image for no reason.
>
> Does gcc automatically initialize them to that value?

Yes, global and static variables are guaranteed to be initialized with 0.

Couple of more comments:

1. I don't think that relayfs parameter worth an entry in sysfs.
2. We should not signal error condition in appletouch_open if
appletouch_relayfs_init fails - the devce is still functioning fine.
Just emit a warning.
3. I must be missing something but I do not see rcb being used anywhere...

The adjusted patch is below. I am still not sure if this really should be
in mainline. Was it ever used?

--
Dmitry

From: Michael Hanselmann <[email protected]>

Input: appletouch - add relayfs support

This patch adds support for relayfs to the appletouch driver to make
debugging easier.

Signed-off-by: Michael Hanselmann <[email protected]>
Acked-by: Rene Nussbaumer <[email protected]>
Acked-by: Johannes Berg <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
---

Documentation/input/appletouch.txt | 2 +
drivers/usb/input/appletouch.c | 61 ++++++++++++++++++++++++++++++++++++-
2 files changed, 62 insertions(+), 1 deletion(-)

Index: work/Documentation/input/appletouch.txt
===================================================================
--- work.orig/Documentation/input/appletouch.txt
+++ work/Documentation/input/appletouch.txt
@@ -77,6 +77,8 @@ full tracing (each sample is being trace
or
echo "1" > /sys/module/appletouch/parameters/debug

+To make debugging easier, the driver also supports the relayfs filesystem.
+
Links:
------

Index: work/drivers/usb/input/appletouch.c
===================================================================
--- work.orig/drivers/usb/input/appletouch.c
+++ work/drivers/usb/input/appletouch.c
@@ -6,6 +6,8 @@
* Copyright (C) 2005 Stelian Pop ([email protected])
* Copyright (C) 2005 Frank Arnold ([email protected])
* Copyright (C) 2005 Peter Osterlund ([email protected])
+ * Copyright (C) 2005 Parag Warudkar ([email protected])
+ * Copyright (C) 2005 Michael Hanselmann ([email protected])
*
* Thanks to Alex Harper <[email protected]> for his inputs.
*
@@ -35,6 +37,10 @@
#include <linux/input.h>
#include <linux/usb_input.h>

+#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
+#include <linux/relayfs_fs.h>
+#endif
+
/* Apple has powerbooks which have the keyboard with different Product IDs */
#define APPLE_VENDOR_ID 0x05AC

@@ -73,6 +79,7 @@ MODULE_DEVICE_TABLE (usb, atp_table);

/* maximum pressure this driver will report */
#define ATP_PRESSURE 300
+
/*
* multiplication factor for the X and Y coordinates.
* We try to keep the touchpad aspect ratio while still doing only simple
@@ -124,7 +131,7 @@ struct atp {
if (debug) printk(format, ##a); \
} while (0)

-MODULE_AUTHOR("Johannes Berg, Stelian Pop, Frank Arnold");
+MODULE_AUTHOR("Johannes Berg, Stelian Pop, Frank Arnold, Parag Warudkar, Michael Hanselmann");
MODULE_DESCRIPTION("Apple PowerBooks USB touchpad driver");
MODULE_LICENSE("GPL");

@@ -132,6 +139,51 @@ static int debug = 1;
module_param(debug, int, 0644);
MODULE_PARM_DESC(debug, "Activate debugging output");

+#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
+static int relayfs;
+module_param(relayfs, int, 0);
+MODULE_PARM_DESC(relayfs, "Activate relayfs support");
+
+static struct rchan *rch;
+
+static inline void atp_relayfs_dump(struct atp *dev)
+{
+ /* "rch" is NULL if relayfs is disabled */
+ if (rch && dev->data)
+ relay_write(rch, dev->data, dev->urb->actual_length);
+}
+
+static int appletouch_relayfs_init(void)
+{
+ if (relayfs) {
+
+ rch = relay_open("atpdata", NULL, 256, 256, NULL);
+ if (!rch) {
+ printk(KERN_WARNING
+ "appletouch: Failed to enable Relayfs.\n");
+ return -ENOMEM;
+ }
+
+ printk(KERN_INFO "appletouch: Relayfs enabled.\n");
+ }
+
+ return 0;
+}
+
+static void appletouch_relayfs_exit(void)
+{
+ if (rch) {
+ printk(KERN_INFO "appletouch: Relayfs disabled\n");
+ relay_close(rch);
+ rch = NULL;
+ }
+}
+#else
+static inline void atp_relayfs_dump(struct atp *dev) { }
+static int appletouch_relayfs_init(void) { return 0; }
+static void appletouch_relayfs_exit(void) { }
+#endif
+
static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
int *z, int *fingers)
{
@@ -194,6 +246,8 @@ static void atp_complete(struct urb* urb
goto exit;
}

+ atp_relayfs_dump(dev);
+
/* reorder the sensors values */
for (i = 0; i < 8; i++) {
/* X values */
@@ -301,7 +355,10 @@ static int atp_open(struct input_dev *in
if (usb_submit_urb(dev->urb, GFP_ATOMIC))
return -EIO;

+ appletouch_relayfs_init();
+
dev->open = 1;
+
return 0;
}

@@ -310,6 +367,8 @@ static void atp_close(struct input_dev *
struct atp *dev = input->private;

usb_kill_urb(dev->urb);
+ appletouch_relayfs_exit();
+
dev->open = 0;
}

2005-12-15 08:37:15

by Michael Hanselmann

[permalink] [raw]
Subject: Re: [PATCH 2.6 1/2] usb/input: Add relayfs support to appletouch driver

On Wed, Dec 14, 2005 at 10:43:27PM -0500, Dmitry Torokhov wrote:
> The adjusted patch is below. I am still not sure if this really should be
> in mainline. Was it ever used?

That patch looks fine for me.

I would like to see it in mainline because it makes debugging and
figuring out new protocols much easier. For example, Apple changed the
protocol with the latest PowerBooks (see the other patch for that). Why
should everyone willing to implement a new protocol rewrite this code?

2005-12-15 19:50:46

by Olof Johansson

[permalink] [raw]
Subject: Re: [PATCH 2.6 1/2] usb/input: Add relayfs support to appletouch driver

On Thu, Dec 15, 2005 at 12:31:08AM +0100, Michael Hanselmann wrote:
> This patch adds support for relayfs to the appletouch driver to make debugging
> easier.

I think I agree with previous comments regarding debug code in the driver:
It's unlikely to ever be used by more than a couple of people at very
rare occasions (new hardware releases), and the barrier to using it is
still high; new users need to learn how to parse the data anyway. I don't
see a reason to include this in mainline.

That aside, comments on the patch below.

> diff -rup linux-2.6.15-rc5.orig/drivers/usb/input/appletouch.c b/drivers/usb/input/appletouch.c
> --- linux-2.6.15-rc5.orig/drivers/usb/input/appletouch.c 2005-12-13 22:44:24.000000000 +0100
> +++ b/drivers/usb/input/appletouch.c 2005-12-15 00:25:09.000000000 +0100
> @@ -6,6 +6,8 @@
> * Copyright (C) 2005 Stelian Pop ([email protected])
> * Copyright (C) 2005 Frank Arnold ([email protected])
> * Copyright (C) 2005 Peter Osterlund ([email protected])
> + * Copyright (C) 2005 Parag Warudkar ([email protected])
> + * Copyright (C) 2005 Michael Hanselmann ([email protected])
> *
> * Thanks to Alex Harper <[email protected]> for his inputs.
> *
> @@ -35,6 +37,10 @@
> #include <linux/input.h>
> #include <linux/usb_input.h>
>
> +#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
> +#include <linux/relayfs_fs.h>
> +#endif
> +
> /* Apple has powerbooks which have the keyboard with different Product IDs */
> #define APPLE_VENDOR_ID 0x05AC
>
> @@ -73,6 +79,7 @@ MODULE_DEVICE_TABLE (usb, atp_table);
>
> /* maximum pressure this driver will report */
> #define ATP_PRESSURE 300
> +

Whitespace change

> /*
>
> * multiplication factor for the X and Y coordinates.
> * We try to keep the touchpad aspect ratio while still doing only simple
> @@ -124,7 +131,7 @@ struct atp {
> if (debug) printk(format, ##a); \
> } while (0)
>
> -MODULE_AUTHOR("Johannes Berg, Stelian Pop, Frank Arnold");
> +MODULE_AUTHOR("Johannes Berg, Stelian Pop, Frank Arnold, Parag Warudkar, Michael Hanselmann");
> MODULE_DESCRIPTION("Apple PowerBooks USB touchpad driver");
> MODULE_LICENSE("GPL");
>
> @@ -132,6 +139,68 @@ static int debug = 1;
> module_param(debug, int, 0644);
> MODULE_PARM_DESC(debug, "Activate debugging output");
>
> +#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
> +static int relayfs;
> +module_param(relayfs, int, 0444);
> +MODULE_PARM_DESC(relayfs, "Activate relayfs support");
> +
> +struct rchan *rch;
> +struct rchan_callbacks *rcb;

static, please.

> +
> +static inline void atp_relayfs_dump(struct atp *dev)
> +{
> + /* "rch" is NULL if relayfs is disabled */
> + if (rch && dev->data) {
> + relay_write(rch, dev->data, dev->urb->actual_length);
> + }
> +}
> +
> +static int appletouch_relayfs_init(void)
> +{
> + // Make sure the variables aren't initialized to some bogus value when
> + // relayfs is disabled
> + rcb = NULL;
> + rch = NULL;

Huh? BSS is initialized to 0, this is unneccessary.
(Also, the comment is C++-style, please use /* */ comments.)

> +
> + if (relayfs) {

Please do:
if (!relayfs)
return 0;

To save indentation.

> + rcb = kmalloc(sizeof(struct rchan_callbacks), GFP_KERNEL);
> + if (!rcb)
> + return -ENOMEM;
> +
> + rcb->subbuf_start = NULL;
> + rcb->buf_mapped = NULL;
> + rcb->buf_unmapped = NULL;
> +
> + rch = relay_open("atpdata", NULL, 256, 256, NULL);
> + if (!rch) {
> + kfree(rcb);
> + return -ENOMEM;

ENOMEM for a failed open? Seems odd to me.
Should you also set rcb = NULL?

> + }
> +
> + printk(KERN_INFO "appletouch: Relayfs enabled.\n");
> + }
> +
> + return 0;
> +}
> +
> +static void appletouch_relayfs_exit(void)
> +{
> + if (rch) {
> + printk(KERN_INFO "appletouch: Relayfs disabled\n");
> + relay_close(rch);
> + rch = NULL;
> + }
> + if (rcb) {
> + kfree(rcb);
> + rcb = NULL;

No need to check arguments to kfree.

> + }
> +}
> +#else
> +static inline void atp_relayfs_dump(struct atp *dev) { }
> +static int appletouch_relayfs_init(void) { return 0; }
> +static void appletouch_relayfs_exit(void) { }
> +#endif
> +
> static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
> int *z, int *fingers)
> {
> @@ -194,6 +263,8 @@ static void atp_complete(struct urb* urb
> goto exit;
> }
>
> + atp_relayfs_dump(dev);
> +
> /* reorder the sensors values */
> for (i = 0; i < 8; i++) {
> /* X values */
> @@ -297,6 +368,11 @@ exit:
> static int atp_open(struct input_dev *input)
> {
> struct atp *dev = input->private;
> + int result;
> +
> + result = appletouch_relayfs_init();
> + if (result < 0)
> + return result;

Sounds harsh to deny USB open just because relayfs couldn't be setup.

Actually, this way you could just make the init function not return
anything; it doesn't matter if it fails or not -- life will go on.

>
> if (usb_submit_urb(dev->urb, GFP_ATOMIC))
> return -EIO;
> @@ -310,6 +386,8 @@ static void atp_close(struct input_dev *
> struct atp *dev = input->private;
>
> usb_kill_urb(dev->urb);
> + appletouch_relayfs_exit();
> +
> dev->open = 0;
> }
>
> _______________________________________________
> Linuxppc-dev mailing list
> [email protected]
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

2005-12-15 21:26:38

by Michael Hanselmann

[permalink] [raw]
Subject: Re: [PATCH 2.6 1/2] usb/input: Add relayfs support to appletouch driver

On Thu, Dec 15, 2005 at 11:50:17AM -0800, Olof Johansson wrote:
> I think I agree with previous comments regarding debug code in the driver:
> It's unlikely to ever be used by more than a couple of people at very
> rare occasions (new hardware releases), and the barrier to using it is
> still high; new users need to learn how to parse the data anyway. I don't
> see a reason to include this in mainline.

Okay, based on your comments, please drop that patch. How about the one
to support the Geyser 2 device? Should I do a rediff without relayfs
support?

Greets,
Michael

2005-12-15 21:45:12

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH 2.6 1/2] usb/input: Add relayfs support to appletouch driver

On 12/15/05, Michael Hanselmann <[email protected]> wrote:
> On Thu, Dec 15, 2005 at 11:50:17AM -0800, Olof Johansson wrote:
> > I think I agree with previous comments regarding debug code in the driver:
> > It's unlikely to ever be used by more than a couple of people at very
> > rare occasions (new hardware releases), and the barrier to using it is
> > still high; new users need to learn how to parse the data anyway. I don't
> > see a reason to include this in mainline.
>
> Okay, based on your comments, please drop that patch. How about the one
> to support the Geyser 2 device? Should I do a rediff without relayfs
> support?
>

If you could rediff it without relayfs I would add it to the input
tree. Altough I am not sure if manually unrolling that loop is such a
good idea. Maybe we should leave it to the compiler?

--
Dmitry

2005-12-16 17:31:50

by Horst H. von Brand

[permalink] [raw]
Subject: Re: [PATCH 2.6 1/2] usb/input: Add relayfs support to appletouch driver

Olof Johansson <[email protected]> wrote:

Just saw this.

> On Thu, Dec 15, 2005 at 12:31:08AM +0100, Michael Hanselmann wrote:

> > diff -rup linux-2.6.15-rc5.orig/drivers/usb/input/appletouch.c b/drivers/usb/input/appletouch.c
> > --- linux-2.6.15-rc5.orig/drivers/usb/input/appletouch.c 2005-12-13 22:44:24.000000000 +0100
> > +++ b/drivers/usb/input/appletouch.c 2005-12-15 00:25:09.000000000 +0100

[...]

> > +#if defined(CONFIG_RELAYFS_FS) || defined(CONFIG_RELAYFS_FS_MODULE)
> > +#include <linux/relayfs_fs.h>
> > +#endif

Why can't this be included regardless? If it does something that only makes
sense if relayfs is in use, better have that decision inside the header
file (least somebody just includes it and...).
--
Dr. Horst H. von Brand User #22616 counter.li.org
Departamento de Informatica Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria +56 32 654239
Casilla 110-V, Valparaiso, Chile Fax: +56 32 797513

2005-12-17 01:19:09

by Michael Hanselmann

[permalink] [raw]
Subject: Re: [PATCH 2.6 2/2] usb/input: Add Geyser 2 support to appletouch driver

This patch adds support for the Geyser 2 touchpads used on post Oct 2005
Apple PowerBooks to the appletouch driver.

Signed-off-by: Michael Hanselmann <[email protected]>
Acked-by: Rene Nussbaumer <[email protected]>
Acked-by: Johannes Berg <[email protected]>
Acked-by: Stelian Pop <[email protected]>
---
Rediffed without relayfs and made loops out of the manually unrolled
assignment code.

diff -rup linux-2.6.15-rc5.orig/Documentation/input/appletouch.txt b/Documentation/input/appletouch.txt
--- linux-2.6.15-rc5.orig/Documentation/input/appletouch.txt 2005-12-13 00:09:24.000000000 +0100
+++ b/Documentation/input/appletouch.txt 2005-12-17 02:06:27.000000000 +0100
@@ -3,7 +3,7 @@ Apple Touchpad Driver (appletouch)
Copyright (C) 2005 Stelian Pop <[email protected]>

appletouch is a Linux kernel driver for the USB touchpad found on post
-February 2005 Apple Alu Powerbooks.
+February 2005 and October 2005 Apple Aluminium Powerbooks.

This driver is derived from Johannes Berg's appletrackpad driver[1], but it has
been improved in some areas:
@@ -13,7 +13,8 @@ been improved in some areas:

Credits go to Johannes Berg for reverse-engineering the touchpad protocol,
Frank Arnold for further improvements, and Alex Harper for some additional
-information about the inner workings of the touchpad sensors.
+information about the inner workings of the touchpad sensors. Michael
+Hanselmann added support for the October 2005 models.

Usage:
------
diff -rup linux-2.6.15-rc5.orig/drivers/usb/input/appletouch.c b/drivers/usb/input/appletouch.c
--- linux-2.6.15-rc5.orig/drivers/usb/input/appletouch.c 2005-12-13 22:44:24.000000000 +0100
+++ b/drivers/usb/input/appletouch.c 2005-12-17 02:13:02.000000000 +0100
@@ -6,6 +6,7 @@
* 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])
*
* Thanks to Alex Harper <[email protected]> for his inputs.
*
@@ -38,6 +39,11 @@
/* Apple has powerbooks which have the keyboard with different Product IDs */
#define APPLE_VENDOR_ID 0x05AC

+/* These names come from Info.plist in AppleUSBTrackpad.kext */
+#define GEYSER_ANSI_PRODUCT_ID 0x0214
+#define GEYSER_ISO_PRODUCT_ID 0x0215
+#define GEYSER_JIS_PRODUCT_ID 0x0216
+
#define ATP_DEVICE(prod) \
.match_flags = USB_DEVICE_ID_MATCH_DEVICE | \
USB_DEVICE_ID_MATCH_INT_CLASS | \
@@ -53,13 +59,17 @@ static struct usb_device_id atp_table []
{ ATP_DEVICE(0x020F) },
{ ATP_DEVICE(0x030A) },
{ ATP_DEVICE(0x030B) },
- { } /* Terminating entry */
+
+ /* PowerBooks Oct 2005 */
+ { ATP_DEVICE(GEYSER_ANSI_PRODUCT_ID) },
+ { ATP_DEVICE(GEYSER_ISO_PRODUCT_ID) },
+ { ATP_DEVICE(GEYSER_JIS_PRODUCT_ID) },
+
+ /* Terminating entry */
+ { }
};
MODULE_DEVICE_TABLE (usb, atp_table);

-/* size of a USB urb transfer */
-#define ATP_DATASIZE 81
-
/*
* number of sensors. Note that only 16 instead of 26 X (horizontal)
* sensors exist on 12" and 15" PowerBooks. All models have 16 Y
@@ -108,6 +118,8 @@ struct atp {
signed char xy_old[ATP_XSENSORS + ATP_YSENSORS];
/* accumulated sensors */
int xy_acc[ATP_XSENSORS + ATP_YSENSORS];
+ int overflowwarn; /* overflow warning printed? */
+ int datalen; /* size of an USB urb transfer */
};

#define dbg_dump(msg, tab) \
@@ -124,7 +136,7 @@ struct atp {
if (debug) printk(format, ##a); \
} while (0)

-MODULE_AUTHOR("Johannes Berg, Stelian Pop, Frank Arnold");
+MODULE_AUTHOR("Johannes Berg, Stelian Pop, Frank Arnold, Michael Hanselmann");
MODULE_DESCRIPTION("Apple PowerBooks USB touchpad driver");
MODULE_LICENSE("GPL");

@@ -132,6 +144,16 @@ static int debug = 1;
module_param(debug, int, 0644);
MODULE_PARM_DESC(debug, "Activate debugging output");

+/* Checks if the device a Geyser 2 (ANSI, ISO, JIS) */
+static inline int atp_is_geyser_2(struct atp *dev)
+{
+ int16_t productId = le16_to_cpu(dev->udev->descriptor.idProduct);
+
+ return (productId == GEYSER_ANSI_PRODUCT_ID) ||
+ (productId == GEYSER_ISO_PRODUCT_ID) ||
+ (productId == GEYSER_JIS_PRODUCT_ID);
+}
+
static int atp_calculate_abs(int *xy_sensors, int nb_sensors, int fact,
int *z, int *fingers)
{
@@ -168,13 +190,20 @@ static inline void atp_report_fingers(st
static void atp_complete(struct urb* urb, struct pt_regs* regs)
{
int x, y, x_z, y_z, x_f, y_f;
- int retval, i;
+ int retval, i, j;
struct atp *dev = urb->context;

switch (urb->status) {
case 0:
/* success */
break;
+ case -EOVERFLOW:
+ if(!dev->overflowwarn) {
+ printk("appletouch: OVERFLOW with data "
+ "length %d, actual length is %d\n",
+ dev->datalen, dev->urb->actual_length);
+ dev->overflowwarn = 1;
+ }
case -ECONNRESET:
case -ENOENT:
case -ESHUTDOWN:
@@ -189,23 +218,45 @@ static void atp_complete(struct urb* urb
}

/* drop incomplete datasets */
- if (dev->urb->actual_length != ATP_DATASIZE) {
+ if (dev->urb->actual_length != dev->datalen) {
dprintk("appletouch: incomplete data package.\n");
goto exit;
}

/* reorder the sensors values */
- for (i = 0; i < 8; i++) {
- /* X values */
- dev->xy_cur[i ] = dev->data[5 * i + 2];
- dev->xy_cur[i + 8] = dev->data[5 * i + 4];
- dev->xy_cur[i + 16] = dev->data[5 * i + 42];
- if (i < 2)
- dev->xy_cur[i + 24] = dev->data[5 * i + 44];
-
- /* Y values */
- dev->xy_cur[i + 26] = dev->data[5 * i + 1];
- dev->xy_cur[i + 34] = dev->data[5 * i + 3];
+ if (atp_is_geyser_2(dev)) {
+ memset(dev->xy_cur, 0, sizeof(dev->xy_cur));
+
+ /*
+ * The values are laid out like this:
+ * Y1, Y2, -, Y3, Y4, -, ..., X1, X2, -, X3, X4, -, ...
+ * '-' is an unused value.
+ */
+
+ /* read X values */
+ for (i = 0, j = 19; i < 20; i += 2, j += 3) {
+ dev->xy_cur[i] = dev->data[j];
+ dev->xy_cur[i + 1] = dev->data[j + 1];
+ }
+
+ /* read Y values */
+ for (i = 0, j = 1; i < 9; i += 2, j += 3) {
+ dev->xy_cur[ATP_XSENSORS + i] = dev->data[j];
+ dev->xy_cur[ATP_XSENSORS + i + 1] = dev->data[j + 1];
+ }
+ } else {
+ for (i = 0; i < 8; i++) {
+ /* X values */
+ dev->xy_cur[i ] = dev->data[5 * i + 2];
+ dev->xy_cur[i + 8] = dev->data[5 * i + 4];
+ dev->xy_cur[i + 16] = dev->data[5 * i + 42];
+ if (i < 2)
+ dev->xy_cur[i + 24] = dev->data[5 * i + 44];
+
+ /* Y values */
+ dev->xy_cur[i + 26] = dev->data[5 * i + 1];
+ dev->xy_cur[i + 34] = dev->data[5 * i + 3];
+ }
}

dbg_dump("sample", dev->xy_cur);
@@ -216,16 +267,24 @@ static void atp_complete(struct urb* urb
dev->x_old = dev->y_old = -1;
memcpy(dev->xy_old, dev->xy_cur, sizeof(dev->xy_old));

- /* 17" Powerbooks have 10 extra X sensors */
- for (i = 16; i < ATP_XSENSORS; i++)
- if (dev->xy_cur[i]) {
- printk("appletouch: 17\" model detected.\n");
+ /* 17" Powerbooks have extra X sensors */
+ for (i = (atp_is_geyser_2(dev)?15:16); i < ATP_XSENSORS; i++) {
+ if (!dev->xy_cur[i]) continue;
+
+ printk("appletouch: 17\" model detected.\n");
+ if(atp_is_geyser_2(dev))
+ input_set_abs_params(dev->input, ABS_X, 0,
+ (20 - 1) *
+ ATP_XFACT - 1,
+ ATP_FUZZ, 0);
+ else
input_set_abs_params(dev->input, ABS_X, 0,
(ATP_XSENSORS - 1) *
ATP_XFACT - 1,
ATP_FUZZ, 0);
- break;
- }
+
+ break;
+ }

goto exit;
}
@@ -282,7 +341,8 @@ static void atp_complete(struct urb* urb
memset(dev->xy_acc, 0, sizeof(dev->xy_acc));
}

- input_report_key(dev->input, BTN_LEFT, !!dev->data[80]);
+ input_report_key(dev->input, BTN_LEFT,
+ !!dev->data[dev->datalen - 1]);

input_sync(dev->input);

@@ -353,6 +413,8 @@ static int atp_probe(struct usb_interfac

dev->udev = udev;
dev->input = input_dev;
+ dev->overflowwarn = 0;
+ dev->datalen = (atp_is_geyser_2(dev)?64:81);

dev->urb = usb_alloc_urb(0, GFP_KERNEL);
if (!dev->urb) {
@@ -360,7 +422,7 @@ static int atp_probe(struct usb_interfac
goto err_free_devs;
}

- dev->data = usb_buffer_alloc(dev->udev, ATP_DATASIZE, GFP_KERNEL,
+ dev->data = usb_buffer_alloc(dev->udev, dev->datalen, GFP_KERNEL,
&dev->urb->transfer_dma);
if (!dev->data) {
retval = -ENOMEM;
@@ -369,7 +431,7 @@ static int atp_probe(struct usb_interfac

usb_fill_int_urb(dev->urb, udev,
usb_rcvintpipe(udev, int_in_endpointAddr),
- dev->data, ATP_DATASIZE, atp_complete, dev, 1);
+ dev->data, dev->datalen, atp_complete, dev, 1);

usb_make_path(udev, dev->phys, sizeof(dev->phys));
strlcat(dev->phys, "/input0", sizeof(dev->phys));
@@ -385,14 +447,25 @@ static int atp_probe(struct usb_interfac

set_bit(EV_ABS, input_dev->evbit);

- /*
- * 12" and 15" Powerbooks only have 16 x sensors,
- * 17" models are detected later.
- */
- input_set_abs_params(input_dev, ABS_X, 0,
- (16 - 1) * ATP_XFACT - 1, ATP_FUZZ, 0);
- input_set_abs_params(input_dev, ABS_Y, 0,
- (ATP_YSENSORS - 1) * ATP_YFACT - 1, ATP_FUZZ, 0);
+ if (atp_is_geyser_2(dev)) {
+ /*
+ * Oct 2005 15" PowerBooks have 15 X sensors, 17" are detected
+ * later.
+ */
+ input_set_abs_params(input_dev, ABS_X, 0,
+ ((15 - 1) * ATP_XFACT) - 1, ATP_FUZZ, 0);
+ input_set_abs_params(input_dev, ABS_Y, 0,
+ ((9 - 1) * ATP_YFACT) - 1, ATP_FUZZ, 0);
+ } else {
+ /*
+ * 12" and 15" Powerbooks only have 16 x sensors,
+ * 17" models are detected later.
+ */
+ input_set_abs_params(input_dev, ABS_X, 0,
+ (16 - 1) * ATP_XFACT - 1, ATP_FUZZ, 0);
+ input_set_abs_params(input_dev, ABS_Y, 0,
+ (ATP_YSENSORS - 1) * ATP_YFACT - 1, ATP_FUZZ, 0);
+ }
input_set_abs_params(input_dev, ABS_PRESSURE, 0, ATP_PRESSURE, 0, 0);

set_bit(EV_KEY, input_dev->evbit);
@@ -427,7 +500,7 @@ static void atp_disconnect(struct usb_in
usb_kill_urb(dev->urb);
input_unregister_device(dev->input);
usb_free_urb(dev->urb);
- usb_buffer_free(dev->udev, ATP_DATASIZE,
+ usb_buffer_free(dev->udev, dev->datalen,
dev->data, dev->urb->transfer_dma);
kfree(dev);
}