Daniel Thompson has posted comments on this change. ( https://review.coreboot.org/27444 )
Change subject: usbdev: Refactor device discovery code ......................................................................
Patch Set 2:
(5 comments)
Thanks for the review.
https://review.coreboot.org/#/c/27444/2/usbdev.c File usbdev.c:
https://review.coreboot.org/#/c/27444/2/usbdev.c@25 PS2, Line 25: _t
_t is for standard libraries, _func maybe? Actually, as it's only […]
It is only used once but the prototype of get_by_vid_pid_filter() becomes tough to read without the typedef. I'll switch this to _func.
https://review.coreboot.org/#/c/27444/2/usbdev.c@84 PS2, Line 84: void *
`const void *` to make that clear (unless the compiler won't let us)
Ack
https://review.coreboot.org/#/c/27444/2/usbdev.c@105 PS2, Line 105: /* no need to NULL check handle; the conditional decrement makes it unnecessary */
No idea what this is supposed to mean. If you need to read the code […]
Fair point. I'll also use the typedef to describe better how the filter works (specifically the way we call it with handle == NULL as a pre-check and handle set as a post-check).
As it happens this comments is saying that if handle is non-null then it is guaranteed that *nump is false. However its probably clearer just to add a check of the handle than to explain why no check is needed.
https://review.coreboot.org/#/c/27444/2/usbdev.c@118 PS2, Line 118:
no space after closing parenthesis
Ack
https://review.coreboot.org/#/c/27444/2/usbdev.c@115 PS2, Line 115: struct libusb_device_handle *usb_dev_get_by_vid_pid_serial( : struct libusb_context *usb_ctx, uint16_t vid, uint16_t pid, const char *serialno) : { : return get_by_vid_pid_filter(usb_ctx, vid, pid, filter_by_serial, (void *) serialno); : }
please group with the used filter function
Ack