Re: [PATCH] hid: hid bus prototype 20070416

Re: [PATCH] hid: hid bus prototype 20070416

[Jiri Kosina]

On Mon, 16 Apr 2007, Li Yu wrote:

> HID bus prototype 20070416

Hi Li,

thanks for taking care. Well, the patch is quite huge, do you think you 
could split it into separate independent parts (use quilt or something 
similar for patch management) which could be reviewed independently?

As the code changes are often quite non-trivial, layering is changed, 
lots of files are touched, etc. it would help a lot.

Notes from a quick skim through the patch:

- it seems that you accidentaly deleted the newly added quirk for
   mightymouse in the bluetooth hid code?
- what is the point behind HID_QUIRK_SKIP? Why doesn't HID_QUIRK_IGNORE
   suffice? And why is it defined in so strange way:

@@ -270,6 +271,7 @@ struct hid_item {
  #define HID_QUIRK_LOGITECH_DESCRIPTOR          0x00100000
  #define HID_QUIRK_DUPLICATE_USAGES             0x00200000
  #define HID_QUIRK_RESET_LEDS                   0x00400000
+#define HID_QUIRK_SKIP                         0x80000000

- there are bunches of some easy codingstyle issues (spaces around '=',
   etc)

But doing really thorough review is quite hard, as the patch contains lots 
of unrelated changes. I'll look at it a little bit more, but when you send 
a broken-out version with separate changes, that'd be great.

Thanks,

-- 
Jiri Kosina

Re: [linux-usb-devel] [PATCH] hid: hid bus prototype 20070416

[Li Yu]

Jiri Kosina wrote:
> On Mon, 16 Apr 2007, Li Yu wrote:
>
>   
>> HID bus prototype 20070416
>>     
>
> Hi Li,
>
> thanks for taking care. Well, the patch is quite huge, do you think you 
> could split it into separate independent parts (use quilt or something 
> similar for patch management) which could be reviewed independently?
>
> As the code changes are often quite non-trivial, layering is changed, 
> lots of files are touched, etc. it would help a lot.
>
>   
OK, I must be next.
> Notes from a quick skim through the patch:
>
> - it seems that you accidentaly deleted the newly added quirk for
>    mightymouse in the bluetooth hid code?
>   

They should be lost while I play bluetooth.
> - what is the point behind HID_QUIRK_SKIP? Why doesn't HID_QUIRK_IGNORE
>    suffice? And why is it defined in so strange way:
>
> @@ -270,6 +271,7 @@ struct hid_item {
>   #define HID_QUIRK_LOGITECH_DESCRIPTOR          0x00100000
>   #define HID_QUIRK_DUPLICATE_USAGES             0x00200000
>   #define HID_QUIRK_RESET_LEDS                   0x00400000
> +#define HID_QUIRK_SKIP                         0x80000000
>
>   
I am sorry for missing some description here. In simple words, the
HID_QUIRK_IGNORE let usbhid do not register some hid devices at all,
however, the HID_QUIRK_SKIP just let usbhid skip matching with the
device which is marked that quirk, but still register this kind of hid
device. So another HID driver still handle have chances to handle it.
You can discover out there, the hid-core.c is not pure HID driver, it
also take the transports role here.

I think this quirk have such difference with others, so I define it so.
If we do like so, just change it. That is OK.

May be, we need another hid_skiplist[] ?

> - there are bunches of some easy codingstyle issues (spaces around '=',
>    etc)
>
>   
Yes, This is one of reasons for it is only for review.
> But doing really thorough review is quite hard, as the patch contains lots 
> of unrelated changes. I'll look at it a little bit more, but when you send 
> a broken-out version with separate changes, that'd be great.
>
>   
OK.
> Thanks,
>
>