Great, much better, here are some comments. On Thu, 2007-06-28 at 13:14 +0200, Soeren Sonnenburg wrote: > --- drivers/input/mouse/appletouch.c 2007-06-28 13:10:22.000000000 +0200 > +++ /home/sonne/Documents/open/src/mbp/appletouch/appletouch.c 2007-06-28 > 07:14:49.000000000 +0200 > @@ -1,5 +1,4 @@ > -/* > - * Apple USB Touchpad (for post-February 2005 PowerBooks and MacBooks) driver > +/* Apple USB Touchpad (for post-February 2005 PowerBooks and MacBooks) driver
kernel coding style is to have the /* on its own line, don't just change
things like that.
> +#include <linux/version.h>
Huh?
> +#include <asm/uaccess.h>
Hmm. I have a feeling that this shouldn't be needed. We'll see.
> /* Apple has powerbooks which have the keyboard with different Product IDs */
> -#define APPLE_VENDOR_ID 0x05AC
> +#define APPLE_VENDOR_ID 0x05AC
don't do useless whitespace changes
> -#define GEYSER4_ANSI_PRODUCT_ID 0x021A
> -#define GEYSER4_ISO_PRODUCT_ID 0x021B
> -#define GEYSER4_JIS_PRODUCT_ID 0x021C
> +#define GEYSER4_ANSI_PRODUCT_ID 0x021A
> +#define GEYSER4_ISO_PRODUCT_ID 0x021B
> +#define GEYSER4_JIS_PRODUCT_ID 0x021C
ditto.
> #define ATP_DEVICE(prod) \
> - .match_flags = USB_DEVICE_ID_MATCH_DEVICE | \
> - USB_DEVICE_ID_MATCH_INT_CLASS | \
> - USB_DEVICE_ID_MATCH_INT_PROTOCOL, \
> + .match_flags = USB_DEVICE_ID_MATCH_DEVICE | \
> + USB_DEVICE_ID_MATCH_INT_CLASS | \
> + USB_DEVICE_ID_MATCH_INT_PROTOCOL, \
again
> #define ATP_XFACT 64
> -#define ATP_YFACT 43
> +#define ATP_YFACT 43 // 86
?
> /*
> - * Threshold for the touchpad sensors. Any change less than ATP_THRESHOLD is
> - * ignored.
> + * Thresholds for the touchpad sensors.
> + *
> + * Any sensors less than ATP_THRESHOLD is ignored.
> + * APT_START_THRESHOLD defines the pressure needed to start the moving.
> + * We need at least APT_FINGER_THRESHOLD to get a finger recognized.
> + * If we have ATP_PALM_THRESHOLD, we recognize it as an palm.
> */
This sounds like you're doing a lot in the driver now that should be
done by synaptics in userspace...
> -/* MacBook Pro (Geyser 3 & 4) initialization constants */
> -#define ATP_GEYSER3_MODE_READ_REQUEST_ID 1
> -#define ATP_GEYSER3_MODE_WRITE_REQUEST_ID 9
> -#define ATP_GEYSER3_MODE_REQUEST_VALUE 0x300
> -#define ATP_GEYSER3_MODE_REQUEST_INDEX 0
> -#define ATP_GEYSER3_MODE_VENDOR_VALUE 0x04
> +/*
> + * MacBook Pro (Geyser 3) initialization constants
> + */
> +#define ATP_GEYSER3_MODE_READ_REQUEST_ID 1
> +#define ATP_GEYSER3_MODE_WRITE_REQUEST_ID 9
> +#define ATP_GEYSER3_MODE_REQUEST_VALUE 0x300
> +#define ATP_GEYSER3_MODE_REQUEST_INDEX 0
> +#define ATP_GEYSER3_MODE_VENDOR_VALUE 0x04
More whitespace changes that shouldn't be done.
> +/* (until we have a working automatic detection) */
> +static int seventeen = 0;
> +module_param(seventeen, int, 0644);
> +MODULE_PARM_DESC(seventeen, "Is this a 17\" Powerbook or MacBook?");
Hmm? We used to be able to recognise 17" powerbooks...
> +static int start_threshold = ATP_START_THRESHOLD;
> +module_param(start_threshold, int, 0644);
> +MODULE_PARM_DESC(start_threshold, "Pressure needed to start moving");
something you configure in synaptics
> +static int finger_threshold = ATP_FINGER_THRESHOLD;
> +module_param(finger_threshold, int, 0644);
> +MODULE_PARM_DESC(finger_threshold, "Least pressure that identifies a
> finger");
ditto.
> +static int palm_threshold = ATP_PALM_THRESHOLD;
> +module_param(palm_threshold, int, 0644);
> +MODULE_PARM_DESC(palm_threshold, "Pressure which triggers the palm
> detection");
> +static int palm_detect = 1;
> +module_param(palm_detect, int, 0644);
> +MODULE_PARM_DESC(palm_detect, "Detect and ignore palms on the touchpad");
ditto. Actually, I'm not sure how palm detection works in synaptics,
look into it and then support the minimal stuff you need in the driver.
> +static int report_touchsize = 0;
> +module_param(report_touchsize, int, 0644);
> +MODULE_PARM_DESC(report_touchsize, "Report touch's size (tool-width) for use
> by synaptics driver");
?? explain please
> +/* palm areas are defined as fractions of the pad in x direction */
> +/* Default: 4/13 * X-Size < X < 14/22 * X-Size */
> +
> +#define palm_left_nomi palm_region[0] // default: 4
> +#define palm_left_div palm_region[1] // default: 13
> +#define palm_right_nomi palm_region[2] // default: 14
> +#define palm_right_div palm_region[3] // default: 22
> +
> +static unsigned int palm_region[4] = { 4, 13, 14, 22 };
> +static unsigned int palm_region_count;
synaptics stuff.
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,22)
> + cancel_work_sync(&dev->reinit_thread);
> +#else
> + flush_scheduled_work();
> +#endif
No such stuff in submitted kernel code.
Ok more comments:
The modifier bit injection sucks. Remove it completely and figure out
how to use inputd.
Please first just submit a patch that adds basic support for macbook
touchpads that adds the data parsing, type differentiation, usb IDs,
etc.
Much of what you're doing here are userspace tasks and if you split the
patch it'll help us see what should be where.
johannes
signature.asc
Description: This is a digitally signed message part
