Message-ID: <4C80DB38.9040101@gmail.com>
Date:	Fri, 03 Sep 2010 13:25:44 +0200
From:	Jiri Slaby <jirislaby@...il.com>
To:	Michael Poole <mdpoole@...ilus.org>
CC:	Chase Douglas <chase.douglas@...onical.com>,
	Jiri Kosina <jkosina@...e.cz>,
	Henrik Rydberg <rydberg@...omail.se>,
	Tejun Heo <tj@...nel.org>, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/7 v3] HID: magicmouse: don't allow hidinput to initialize
 the device

On 09/02/2010 02:06 PM, Michael Poole wrote:
> On Thu, Sep 2, 2010 at 5:28 AM, Jiri Slaby wrote:
>> On 09/02/2010 01:57 AM, Michael Poole wrote:
>>> Jiri Slaby wrote:
>>>> This looks weird. Is there any past discussion about why you cannot use
>>>> hidinput and you have to do all the input bits yourself while cheating
>>>> this very weird way?
>>>
>>> The Magic Mouse and Magic Trackpad do not publish HID descriptors for
>>> the multitouch reports.  Given the variable-length report packets --
>>> and especially the Magic Trackpad's new, mutant DOUBLE_REPORT_ID
>>> packets -- it would be non-trivial to write accurate descriptors that
>>> the HID core can use.  (Someone wrote a patch to try that a few months
>>> ago.  It ended up adding significantly more lines to hid-magicmouse.c
>>> than it removed, and it was not obvious to me that it got the Report
>>> Count fields correct.)
>>
>> Ok, makes sense. The proper solution is to call a driver hook in
>> hidinput_connect to do the mapping instead of report parsing done there,
>> right? (And not setting [gs]etkeycode in that case.)
> 
> I do not know how this would work with the current HID core; could you
> elaborate?

No way. You would have to implement that.

>  For the Magic Mouse, there is a valid descriptor for the
> 0x10 (traditional mouse motion and clicks) report, and a pretty
> trivial descriptor in the vendor-defined usage region (which I assume
> Apple drivers use to identify the ensemble of multi-touch,
> mouse-up/down, laser status and battery level reports).  How would the
> driver's input_mapping() hook, or any other hook, map that single
> descriptor into fields for each of the parts of a multi-touch report,
> or support the other reports?  How would hid-magicmouse describe the
> report so that hid-core handles the variable-length multi-touch
> reports properly?  As far as I can tell, hid_report_raw_event() and
> its callees assume each report has fixed length.

If I understand correctly, you have 2 reports. One report is standard
HID and one is very specific and broken.
1) I don't understand where events from the standard report are going
now while no output is claimed. In other words, why you return from
raw_event in the default case 0, there is nobody to eat the data, right?
2) You handle events from the broken (or missing) report in
magicmouse_raw_event and it will remain as is.


So in the end there will be some .parse_reports hook called from inside
hidinput_connect or somewhere instead of parsing and you will do the job
you currently do in magicmouse_setup_input except the input structure
setup (this will be done generically in hid core).

The approach you have now is prone to errors, because somebody in the
future may add a new claimant without bearing in mind to update these
drivers.

regards,
-- 
js
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists