Hi,
Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes:
> On Wed, 25 Oct 2017, Felipe Balbi wrote:
>
>> Hi,
>>
>> The following series was compile-tested only (so far, at least). I
>> wanted to get some comments from folks to see what you guys think
>> about this before running tests.
>>
>> I don't have any device available which would support PTM_STATUS so I
>> guess I'd have to implement it on g_zero, if at all possible.
>>
>> Best Regards
>>
>> Felipe Balbi (4):
>> usb: core: add Status Type definitions
>> usb: core: rename usb_get_status() 'type' argument to 'recip'
>> usb: core: add a 'type' parameter to usb_get_status()
>> usb: core: add two usb get status helpers
>
> You should switch the order of patches 3 and 4. That is, replace
> usb_get_status() with usb_get_std_status() first, and then add the type
> parameter and the other helper routine. That way you won't have to
> update all the callers twice.
heh, good point :-)
> Also, I noticed an extra space in patch 3:
Will fix. There's another thing in patch 3:
+ switch (type) {
+ case USB_STATUS_TYPE_STANDARD:
+ if (recip != USB_RECIP_DEVICE)
+ return -EINVAL;
+
+ length = 2;
+ break;
+ case USB_STATUS_TYPE_PTM:
+ length = 4;
+ break;
+ default:
+ return -EINVAL;
+ }
The branch should be done on the PTM case, not Standard. I'll fix that
too.
>> + status = kmalloc(length, GFP_KERNEL);
>
> Otherwise this is unobjectionable. Have you considered making the
> helper routines static inline?
I can do that, but they are also so simple that GCC is likely to make
that decision for us. No? If you prefer static inlines in a header, I
can do that, no problem.
--
balbi
Attachment:
signature.asc
Description: PGP signature
