Hi, Alan Stern <st...@rowland.harvard.edu> 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
signature.asc
Description: PGP signature
