Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47276 )
Change subject: WIP: flashrom: import chip restore handler from cros flashrom ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c File flashrom.c:
https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c@540 PS1, Line 540: hip_restore_fn_count this state should be contained within the flashctx structure.
https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c@541 PS1, Line 541: static struct chip_restore_func_data { : CHIP_RESTORE_CALLBACK; : struct flashctx *flash; : uint8_t status; : } chip_restore_fn[CHIP_RESTORE_MAXFN]; put the struct definition within the flashctx structure so that you don't need another copy (which is called flash here instead of flashctx - awkward). the flashctx would then carry the state over the required life-time
https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c@592 PS1, Line 592: register_chip_restore probably a good design is for the chip structure to contain a callback and if it exists then it is dispatch that does the registering. That way this function can be called in the generic core control flow when a chip that requires it is found?
https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c@653 PS1, Line 653: : static int chip_restore() move this under the register_chip_restore() function and also rename to deregister_chip_restore()
https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c@2327 PS1, Line 2327: chip_restore();
This seems like an ok place to call chip_restore(), but it's difficult to determine if it is equival […]
This seems like a reasonable place to me but maybe before the unmap?