Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/29078 )
Change subject: add newer pci infrastructure ......................................................................
Patch Set 3: Code-Review+1
(7 comments)
https://review.coreboot.org/c/flashrom/+/29078/3/pcidev.c File pcidev.c:
https://review.coreboot.org/c/flashrom/+/29078/3/pcidev.c@351 PS3, Line 351: /* : * : */ kill these off?
https://review.coreboot.org/c/flashrom/+/29078/3/pcidev.c@354 PS3, Line 354: static int : flashrom_pci_device_shutdown(void *data) one line
https://review.coreboot.org/c/flashrom/+/29078/3/pcidev.c@396 PS3, Line 396: /* 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?
https://review.coreboot.org/c/flashrom/+/29078/3/pcidev.c@445 PS3, Line 445: /* 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.
https://review.coreboot.org/c/flashrom/+/29078/3/pcidev.c@455 PS3, Line 455: dev validate the allocation before using it. also 'pcidev_data' seems more appropriate
https://review.coreboot.org/c/flashrom/+/29078/3/programmer.h File programmer.h:
https://review.coreboot.org/c/flashrom/+/29078/3/programmer.h@835 PS3, Line 835: /* programmer specific */ current comment has minimal value, could do with a little bit more explanation for users of the API.
https://review.coreboot.org/c/flashrom/+/29078/3/programmer.h@852 PS3, Line 852: struct flashrom_pci_device *flashrom_pci_init(const struct flashrom_pci_match *matches); Add documentation comment in the header.
-
Edward O'Callaghan (Code Review)