Patch set 3:Code-Review +1
7 comments:
/*
*
*/
kill these off?
static int
flashrom_pci_device_shutdown(void *data)
one line
/* Filter by bb:dd.f (if supplied by the user). */
pcidev_bdf = extract_programmer_param("pci");
if (pcidev_bdf) {
char *msg;
if ((msg = pci_filter_parse_slot(&filter, pcidev_bdf))) {
msg_perr("Error: %s\n", msg);
return NULL;
}
free(pcidev_bdf);
}
put this into its own function that just extracts the param and validates the string is the correct format?
/* Only continue if exactly one supported PCI dev has been found. */
if (!found) {
msg_perr("Error: No supported PCI device found.\n");
return NULL;
} else if (found > 1) {
msg_perr("Error: Multiple supported PCI devices found. Use 'flashrom -p xxxx:pci=bb:dd.f'\n"
"to explicitly select the card with the given BDF (PCI bus, device, function).\n");
return NULL;
block seems to need to be indented to match the rest of the for-loop. I think this iterator logic can be sliced into its own static function because it would be nice to unit-test it.
validate the allocation before using it. also 'pcidev_data' seems more appropriate
Patch Set #3, Line 835: /* programmer specific */
current comment has minimal value, could do with a little bit more explanation for users of the API.
Patch Set #3, Line 852: struct flashrom_pci_device *flashrom_pci_init(const struct flashrom_pci_match *matches);
Add documentation comment in the header.
To view, visit change 29078. To unsubscribe, or for help writing mail filters, visit settings.