Re: [PATCH 1/5]:hidbus prototype 070409:core headers

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