Re: [PATCH 2/3] HID: suppress the second input for the Apple Magic Mouse.

[Rantanplan <rantanplanlechien@free.fr>]

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html