Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 ) Change subject: raiden_debug_spi.c: Refactor to support multiple protocols ...................................................................... Patch Set 3: (10 comments) The general direction looks good however these changes are *way* way too big to review and definitely can be broken down into sizable chunks that each stand on their own merit. https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c File raiden_debug_spi.c: https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@140 PS3, Line 140: GOOGLE_RAIDEN_SPI_PROTOCOL_V1 probably a enum https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@266 PS3, Line 266: int16_t max_spi_write_count; : uint16_t max_spi_read_count; the spi master already has these fields, you should fill in the data there. https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@438 PS3, Line 438: ; \n https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@448 PS3, Line 448: if (write_count > ctx_data->max_spi_write_count) { validation should happen before packing structs https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@529 PS3, Line 529: .max_data_read = 0, : .max_data_write = 0, not sure why you are not using these fields and putting them into the local state data context? https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@568 PS3, Line 568: mixing tabs / spaces https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@584 PS3, Line 584: : ctx_data->protocol_version = : ctx_data->dev->interface_descriptor->bInterfaceProtocol; : switch(ctx_data->protocol_version) { this is its own function, `enum protocol_rev_t raiden_debug_detect_protocol_rev(struct usb_device *device)` https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@592 PS3, Line 592: : ctx_data->max_spi_write_count= SPI_TRANSFER_V1_MAX; : ctx_data->max_spi_read_count = SPI_TRANSFER_V1_MAX; have two spi_master_raiden_debug spi_master struct's for each protocol and register the one with the correctly packed data. https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@620 PS3, Line 620: more tabs and spaces mixed throughout this file. https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@813 PS3, Line 813: configure_protoco no, just declare the state you want (read/write max_size in this case) for each path and choose the correct state to register as the spi master after a simple detection function. -- To view, visit https://review.coreboot.org/c/flashrom/+/41532 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id404af14e55fa0884e29f28880206aaad4deba66 Gerrit-Change-Number: 41532 Gerrit-PatchSet: 3 Gerrit-Owner: Brian Nemec <bnemec@google.com> Gerrit-Reviewer: Brian Nemec <bnemec@chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Tue, 19 May 2020 15:40:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment