Jiri Kosina wrote:
> Hi Li,
>
> first, please apologize my delay in the reply, the patches are quite
> large and I have been busy with lots of different things lately.
>
>
hehe, you are welcome. I had not touch this patch since last month, OK,
let hid reload into the cache of my brain.
> There had been quite a long discussion some time ago, regarding the proper
> way to design hidbus. As far as I recall it, we came to the conclusion
> that the best way would be to allow defining of two different hooks - one
> for raw data and another for parsed reports and let drivers decice which
> one they want to use. I am missing the possibility to hook the parsed data
> in your implementation, or did I just overlook something?
>
For parsed data? they are in it ~_~
+struct hid_hook {
+ int (*hid_event)(struct hid_field*, struct hid_usage*, __s32, int);
+ int (*pre_report_event)(struct hid_report*, int);
+ int (*report_event)(struct hid_report*, int);
They works with parsed data, however with different call time, and
different parameters.
+ void (*setup_usage)(struct hid_field*, struct hid_usage*);
+ struct usage_page_block *usage_page_table;
+};
They are convenience API for parsed data, just like hid simple interface.
Here is the core process logic in hid_input_report():
+ if (!hid_report_pre_event(hid, report, interrupt))
+ return 0;
for (n = 0; n < report->maxfield; n++)
hid_input_field(hid, report->field[n], data, interrupt); /* hid_event()
and convenience API */
+ hid_report_event(hid, report, interrupt);
> Second, what is the exact point with hid_report_pre_event()? It is missing
> any explanatory comment (which applies to other code in your patches BTW)
> and I seem to be missing the point too.
>
>
Ah, this may have a bit of overdesign. but removing it is easy. I think
they can work before hid_input_field(), but take
hid_report as input parameter. they are can work as hid_report filter.
> Third, as far as my memory goes, I think we agreed that in order to design
> the layering properly and allow individual specialized drivers to work
> properly, the HID bus code should provide a way to register a driver for a
> specific VID/PID and also the report id (one or more). Then all reports
> for which there is no specialized driver are going to be handled by the
> default HID->input driver (or could be handled into hidraw if the
> in-kernel HID parser is unable to parse them). The reports for ids with a
> special driver are handed over to the driver. I unfortunately seem to be
> missing this functionality in the patch too ... ?
>
>
Yes, you are right!
May be, my memory is overflow? ;) I recall we agreed we need register
with VID/PID before. and I also think let driver itself match report is
better idea.
As I guess, needing to match report id is special and special and
special case. I do not like this make hid core complex. It seem that
hid_report_pre_event() is good chance.
> Last but not least, I have gone through this code several times, but I
> still don't fully understand why the 'hid_transport' abstraction is
> needed. I am still convinced that the current model works well, and there
> should be absolutely no need for hid_(un)register_transport() and the
> related things. Could some explanation please be put somewhere? (ideally
> in the patch's changelog entry).
>
>
OK, firse please see these two hid_transport instances:
+static struct hid_transport hid_bt_transport = {
+ .bus = BUS_BLUETOOTH,
+ .event = hidp_hidinput_event,
+ .open = hidp_open,
+ .close = hidp_close,
+ .update_busid = hidp_update_busid,
+ .module = THIS_MODULE,
+};
+static struct hid_transport hid_usb_transport = {
+ .bus = BUS_USB,
+ .module = THIS_MODULE,
+ .event = usb_hidinput_input_event,
+ .open = usbhid_open,
+ .close = usbhid_close,
+ .update_busid = usbhid_update_busid,
+ .raw_report = usbhid_raw_report,
+};
I think whatever bluetooth or USB behind, although all hid driver is
apparently equality, however, in fact, they are not so. the lightweight
driver need some information from "standard" driver, I think the
hid_transport is good place to find them. so
hidp_hidinput_event()/usb_hidinput_input_event() live here.
So, the author of driver need not take care of these commom code in
their sweet driver, the only thing they need do is just fill
hid_driver->bus with correct bus id.
Of course, we also can remove hid_transport abstraction, instead of,
just add these methods as EXPORT_GPL symbols, but I think this way more
flexible, it also enable the drivers above same bus share pure data
(hid_transport->private).
> And more generally - the patches seem a little bit bigger than they could
> be, I am not sure whether the layering and data structures you have are
> not a little bit overdesigned. But this will need some more review.
>
>
^_^
These patches is named prototype, so the modification is unavoidable.
Good luck.
- Li Yu
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
