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.