login
Search
From: Chase Douglas <chase.douglas@ubuntu.com>
The driver listens only for raw events from the device. If we allow
the hidinput layer to initialize, we can hit NULL pointer dereferences
in the hidinput layer because disconnecting only removes the hidinput
devices from the hid device while leaving the hid fields configured.
Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
Note that this mimics what the hid-picolcd module does.
drivers/hid/hid-magicmouse.c | 13 +++++++++----
1 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 319b0e5..d38b529 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -404,15 +404,20 @@ static int magicmouse_probe(struct hid_device *hdev,
goto err_free;
}
- ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+ /* When registering a hid device, one of hidinput, hidraw, or hiddev
+ * subsystems must claim the device. We are bypassing hidinput due to
+ * our raw event processing, and hidraw and hiddev may not claim the
+ * device. We get around this by telling hid_hw_start that input has
+ * claimed the device already, and then flipping the bit back.
+ */
+ hdev->claimed = HID_CLAIMED_INPUT;
+ ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDINPUT);
+ hdev->claimed &= ~HID_CLAIMED_INPUT;
if (ret) {
dev_err(&hdev->dev, "magicmouse hw start failed\n");
goto err_free;
}
- /* we are handling the input ourselves */
- hidinput_disconnect(hdev);
-
report = hid_register_report(hdev, HID_INPUT_REPORT, TOUCH_REPORT_ID);
if (!report) {
dev_err(&hdev->dev, "unable to register touch report\n");
--
1.7.1
--
From: Chase Douglas <chase.douglas@ubuntu.com>
By moving the feature reports to an array, we can easily support
more products with different initialization reports. This will be
useful for enabling the Apple Magic Trackpad.
Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
Acked-by: Michael Poole <mdpoole@troilus.org>
---
drivers/hid/hid-magicmouse.c | 35 ++++++++++++++++++++---------------
1 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index d38b529..3a2147d 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -377,14 +377,23 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
}
}
+struct feature {
+ __u8 data[3];
+ __u8 length;
+};
+
+static struct feature mouse_features[] = {
+ { { 0xd7, 0x01 }, 2 },
+ { { 0xf8, 0x01, 0x32 }, 3 }
+};
+
static int magicmouse_probe(struct hid_device *hdev,
const struct hid_device_id *id)
{
- __u8 feature_1[] = { 0xd7, 0x01 };
- __u8 feature_2[] = { 0xf8, 0x01, 0x32 };
struct input_dev *input;
struct magicmouse_sc *msc;
struct hid_report *report;
+ int i;
int ret;
msc = kzalloc(sizeof(*msc), GFP_KERNEL);
@@ -426,19 +435,15 @@ static int magicmouse_probe(struct hid_device *hdev,
}
report->size = 6;
- ret = hdev->hid_output_raw_report(hdev, feature_1, sizeof(feature_1),
- HID_FEATURE_REPORT);
- if (ret != sizeof(feature_1)) {
- dev_err(&hdev->dev, "unable to request touch data (1:%d)\n",
- ret);
- goto err_stop_hw;
- }
- ret = hdev->hid_output_raw_report(hdev, feature_2,
- sizeof(feature_2), HID_FEATURE_REPORT);
- if (ret != sizeof(feature_2)) {
- dev_err(&hdev->dev, "unable to request touch data (2:%d)\n",
- ret);
- goto err_stop_hw;
+ for (i = 0; i < ARRAY_SIZE(mouse_features); i++) {
+ ret = hdev->hid_output_raw_report(hdev, mouse_features[i].data,
+ mouse_features[i].length, HID_FEATURE_REPORT);
+ if (ret != ...The Magic Mouse device is very precise. No driver filtering of input
data needs to be performed.
Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
Acked-by: Michael Poole <mdpoole@troilus.org>
---
drivers/hid/hid-magicmouse.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 98bbc53..0fe2464 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -357,11 +357,11 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
__set_bit(EV_ABS, input->evbit);
input_set_abs_params(input, ABS_MT_TRACKING_ID, 0, 15, 0, 0);
- input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255, 4, 0);
- input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, 255, 4, 0);
- input_set_abs_params(input, ABS_MT_ORIENTATION, -32, 31, 1, 0);
+ input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
+ input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, 255, 0, 0);
+ input_set_abs_params(input, ABS_MT_ORIENTATION, -32, 31, 0, 0);
input_set_abs_params(input, ABS_MT_POSITION_X, -1100, 1358,
- 4, 0);
+ 0, 0);
/* Note: Touch Y position from the device is inverted relative
* to how pointer motion is reported (and relative to how USB
* HID recommends the coordinates work). This driver keeps
@@ -369,7 +369,7 @@ static void magicmouse_setup_input(struct input_dev *input, struct hid_device *h
* inverse of the reported Y.
*/
input_set_abs_params(input, ABS_MT_POSITION_Y, -1589, 2047,
- 4, 0);
+ 0, 0);
}
if (report_undeciphered) {
--
1.7.1
--
I am still not sure this is a good idea. Bandwidth from MT devices is a big deal. A statement roughly how much data comes out of mtdev (which does the filtering for type A devices) before and after this change would be reassuring. Cheers, --
As it is right now, hid-magicmouse doesn't support MT slots. I think all the fuzz code ends up comparing in the MT case is between one touch and another touch, not between one touch's current location and its previous location. If I'm correct, then it means a fuzz > 0 is incorrect for non-slotted MT devices. In fact, the code in drivers/input/input.c around line 194 looks like it discards defuzzing in this case, so one could say this patch is making things more correct :). Now a fuzz > 0 for the non-MT ABS axes may be useful, but this device exhibits no jitter, and we're not really worried about bandwidth in the single touch case. --
For type A devices, the filtering is performed in userspace, in mtdev, in the same manner as it would have been performed in the kernel in the MT slot case. Therefore, knowing the amount of messages coming out of mtdev is a direct The jitter is better measured by the actual amount of events. Henrik --
Yes, but we're only interested in the kernel driver when reviewing this patch. Leaving the fuzz in as it is has no effect right now on ABS_MT_* axes. On the other axes, such as the touch orientation, it's probably I would agree, if there was any jitter to measure. However, I can hold my fingers on the device and not see any extra events due to jitter. Simple inspection via evtest has convinced me. -- Chase --
"We" are interested in knowing if this patch makes any sense, given that filtering is in actuality performed in userspace, and thus modifying this code changes the stream rate into userspace applications, thank you. Henrik --
Because in-kernel defuzzing is turned off for ABS_MT_* axes on non-slotted MT devices, there will be no change in the number of events sent to the userspace due to this patch. Maybe I'm missing something more fundamental. In which case, I'll need more details to get it through my dense skull :). -- Chase --
All events passes through to mtdev, yes, but mtdev filters a considerable amount of events from passing through to X drivers like evdev. Thus, the fuzz values reported in the kernel driver impacts the performance in userspace, even if filtering is not done in the kernel. Henrik --
My disconnect was that I didn't understand that the fuzz value in the kernel is exported to userspace. I thought the defuzzing in mtdev was independent of the defuzzing in the kernel. Basically, I don't feel I have the time to do the analysis you want right now. If you really want, I can just remove this change. -- Chase --
On second thought, if there's no jitter from the device, the only reason to filter is to save bandwidth. The magicmouse devices are very verbose strictly because they have a higher resolution than most devices. If that's an issue for userspace, then the filtering should be based on the resolution. In fact, I see that's what you do in mtdev when the in-kernel fuzz is 0. This separates the kernel fuzz into a removal of jitter, and the mtdev fuzz into a dampening of the number of events passed to clients. Since magicmouse devices have no discernable jitter, shouldn't we set the in-kernel fuzz to 0 and let mtdev do its thing as appropriate? -- Chase --
Per-driver data is more precise than a generic value for all devices. This thread has documented the details of why there is an interest in the fuzz values. Skipping the patch until MT slots are implemented or measurements can be done might be the most sensible thing to do. Thanks, Henrik --
Yes, there's still per-driver fuzz data. If a device is jittery it makes sense to set the in-kernel fuzz factor to a reasonable value. If a device is not jittery, I don't see a need to do any defuzzing of the values in the driver itself. If we think it's necessary to reduce bandwidth, then the kernel should be defuzzing uniformly across devices to compensate. mtdev could do this defuzzing for bandwidth uniformly too. As it is, this driver just has an arbitrary value that is not used in the kernel and likely hasn't ever been used anywhere outside of mtdev. I think it makes sense to reset the fuzz to the baseline of 0 and fix it later if we find it's necessary. Otherwise we're just codifying an arbitrary value. -- Chase --
The trackpad speaks a similar, but different, protocol from the magic
mouse. However, only small code tweaks here and there are needed to make
basic multitouch work.
Extra logic is required for single-touch emulation of the touchpad. The
changes made here take the approach that only one finger may emulate the
single pointer when multiple fingers have touched the screen. Once that
finger is raised, all touches must be raised before any further single
touch events can be sent.
Sometimes the magic trackpad sends two distinct touch reports as one big
report. Simply splitting the packet in two and resending them through
magicmouse_raw_event ensures they are handled properly.
I also added myself to the copyright statement.
Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
drivers/hid/hid-core.c | 1 +
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-magicmouse.c | 229 ++++++++++++++++++++++++++++++++----------
3 files changed, 176 insertions(+), 55 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 616bddc..5226fd1 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1248,6 +1248,7 @@ static const struct hid_device_id hid_blacklist[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL4) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MIGHTYMOUSE) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICMOUSE) },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICTRACKPAD) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_FOUNTAIN_ANSI) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_FOUNTAIN_ISO) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_GEYSER_ANSI) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 7b3ca1d..9204cac 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -63,6 +63,7 @@
#define USB_VENDOR_ID_APPLE 0x05ac
#define ...Thanks for this driver. Comments inline. The logic using single_touch_id seems complicated. Any chance you could get by The tracking id reported by the macpad is not ideal; it works more like a slot that a MT protocol tracking id. Perhaps it is a slot? I would recommend looking at the recent submissions for bamboo touch, 3m-pct and w8001 to see how the If the pointer emulation was handled differently, and the above was replaced with something like if (!msc->ntouches) input_mt_sync(input); it would be short enough not to need to be broken out of the switch... Besides, BTN_TOUCH is emitted for the trackpad case, so the above code is not strictly As noted by Michael, this could likely be simplified. the "0x01" on the last line could be the same coding as wellspring mode for bcm5974. Thanks, Henrik --
The overall code for single touch handling is ~10 lines spread around the driver. I've tried to condense the code as much as possible. Perhaps someone could come up with something more elegant, but all this code does is keep track of the touch that is tied to single touch emulation. In my next submission I've added macros to make the -1 and -2 values Yes, I think it could be better handled. However, that handling probably belongs in another patch when slots are implemented in this driver. I I've rewritten the touch down logic, and this does get simple enough to I've simplified this for the next submission. -- Chase --
Acked-by: Michael Poole <mdpoole@troilus.org> One behavior that slightly surprised me -- which I believe is a quirk due to userspace not expecting touchpads to have button switches -- is that touches on the trackpad that do not close the switch can still be interpreted by X as clicks. Once the discussions about if/how to tweak this code settle down, I'll put together a patch to change the "down" and "last_up" logic as I suggested earlier. Michael Poole --
This is actually done by the X synaptics input module. It's the defacto X touchpad driver and enables niceties like two finger scrolling and whatnot. You can either use it as is, use xinput/xorg.conf to manipulate its configuration, or switch to the X evdev input module which is more "bare" support. FYI, our gesture support in Maverick is based on a modified version of the X evdev input module. However, because the whole stack of desktop libs and toolkits won't support gestures in Maverick, you lose things like two finger scroll. Thus, we're going to leave the default X input I actually decided to tackle this to make the patches I'm writing easier. I'll be sending a new batch shortly. Thanks, -- Chase --
From: Henrik Rydberg <rydberg@euromail.se>
By visual inspection, the reported touch_major and touch_minor axes
are roughly a factor of four too small. The factor is approximate,
since the protocol is not known and the HID report encodes touch size
with fewer bits than positions. This patch scales the reported values
by a factor of four.
Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
Acked-by: Michael Poole <mdpoole@troilus.org>
---
drivers/hid/hid-magicmouse.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 364556a..898d0b8 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -253,8 +253,8 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
/* Generate the input events for this touch. */
if (report_touches && down) {
input_report_abs(input, ABS_MT_TRACKING_ID, id);
- input_report_abs(input, ABS_MT_TOUCH_MAJOR, touch_major);
- input_report_abs(input, ABS_MT_TOUCH_MINOR, touch_minor);
+ input_report_abs(input, ABS_MT_TOUCH_MAJOR, touch_major << 2);
+ input_report_abs(input, ABS_MT_TOUCH_MINOR, touch_minor << 2);
input_report_abs(input, ABS_MT_ORIENTATION, orientation);
input_report_abs(input, ABS_MT_POSITION_X, x);
input_report_abs(input, ABS_MT_POSITION_Y, y);
--
1.7.1
--
The new format should be easier to read to determine which bits
correspond to which data. It also brings all the manipulation logic to
the top of the function. This makes size and orientation reading more
clear.
Note that the impetus for this change is the forthcoming support for the
Magic Trackpad, which has a different touch data protocol.
Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
Acked-by: Michael Poole <mdpoole@troilus.org>
---
drivers/hid/hid-magicmouse.c | 25 +++++++++++++------------
1 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 3a2147d..98bbc53 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -166,18 +166,21 @@ static void magicmouse_emit_buttons(struct magicmouse_sc *msc, int state)
static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tdata)
{
struct input_dev *input = msc->input;
- __s32 x_y = tdata[0] << 8 | tdata[1] << 16 | tdata[2] << 24;
- int misc = tdata[5] | tdata[6] << 8;
- int id = (misc >> 6) & 15;
- int x = x_y << 12 >> 20;
- int y = -(x_y >> 20);
- int down = (tdata[7] & TOUCH_STATE_MASK) != TOUCH_STATE_NONE;
+ int id = (tdata[6] << 2 | tdata[5] >> 6) & 0xf;
+ int x = (tdata[1] << 28 | tdata[0] << 20) >> 20;
+ int y = -((tdata[2] << 24 | tdata[1] << 16) >> 20);
+ int size = tdata[5] & 0x3f;
+ int orientation = (tdata[6] >> 2) - 32;
+ int touch_major = tdata[3];
+ int touch_minor = tdata[4];
+ int state = tdata[7] & TOUCH_STATE_MASK;
+ int down = state != TOUCH_STATE_NONE;
/* Store tracking ID and other fields. */
msc->tracking_ids[raw_id] = id;
msc->touches[id].x = x;
msc->touches[id].y = y;
- msc->touches[id].size = misc & 63;
+ msc->touches[id].size = size;
/* If requested, emulate a scroll wheel by detecting small
* vertical touch motions.
@@ -188,7 +191,7 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
int step_y = ...Thanks, this approach makes sense to me. --
