Thank you very much for your review. I will try to explain why this
patch is so big (sorry for the inconvenience if I speak too much, and if
I make too much mistakes, English is not my native speaking).
I first have to tell that I'm not a kernel hacker, but since December I
started working on my free time (and some time on my job time when my
PhD tutor allows me) on including multitouch in Xorg.
I recently bought a Magic Mouse in order to have a light weight
multitouch device to test.
The point is that when I used Michael's first version of hid-magicmouse
I faced some troubles:
- The first point was the kernel oops when disconnecting and closing the
Xserver. It was really embarrassing as I had to restart often my Xserver
in order to take the changes into account.
- The second point was that with input hotplugging activated, the
xserver saw two input device, one of them was not sending any events.
So I had a look at the source of the driver.
I first found the explanation of the second input device (not very
difficult to find though), and I did not understand why it was necessary
as the driver already was creating one.
Then I also saw that contrary to the driver I saw before (hid-stantum
and hid-ntrig) the hid-magicmouse uses raw event instead of higher level
events. I thought it was not very clear (at least for me as I do not
speak very well bit fields) and I started to write a work-around.
The last point that was worrying me was the line (after the call of
hid_register_report):
'report->size = 6;'
The driver registered a new report, tells it has an arbitrary size (or
maybe not?) whereas it has no field initialized.
So I wrote first a big patch to avoid all the 'problems' listed above.
I did not send it immediately and I was a little bit overload at work.
However, when I saw that the driver had been included in the next branch
of the kernel, I took time to work on the patch again. But, I only tried
to tackle the problem of the second input device.
To sum up, the advantages of my patch are, I think, the following
(though I do not want to seem to be competing with Micheal):
- It's documenting the data the magic mouse sends as the mouse does not
send the descriptor.
- It will allow another patch to avoid the use of the raw events instead
of high level events (don't know if it is really a benefit)
As Micheal said there are huge pitfalls:
- It heavy regards to its aim
- I insert fields into the descriptor that I use them later with a
slightly change in the meaning (some keywords are not well choose and
the whole meaning is split between the registering of the fields and the
mapping)
- coding style (not really a problem though)
- raw events have the benefits of giving the number of touches into the
data size, something we don't have with high level events only.
Maybe a solution would just be to add a comment documenting the bit
fields in the data? (as now, most of the problems had been solved by the
last patch of Michael)
Cheers,
Benjamin
Le 09/03/2010 01:33, Michael Poole a écrit :
Benjamin Tissoires writes:
[snip]
---
drivers/hid/hid-magicmouse.c | 198 ++++++++++++++++++++++++++++++++++++------
1 files changed, 172 insertions(+), 26 deletions(-)
This diffstat, compared to the functionality being added, says an awful
lot about the approach (IMO). What is the ongoing maintenance benefit
of writing fake report pseudo-descriptors only to translate them
immediately?
In a moment I'll send a much smaller patch that is similar to one of Ed
Tomlinson's patches (unfortunately never provided with his
Signed-Off-By), but which also avoids creating two input devices.
There are numerous coding style violations in this version:
[snip]
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
