Nikolai Artemiev 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 2:
(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.
Done
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 i […]
Done
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 d […]
We should either store a callback in the flashchip structure or have an array of callbacks in the flashctx. I think the latter is better since this the chip restore function seems to be a driver implementation detail and not something that should be configured per chip.
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()
Done, but maybe we should chose a name that indicates that it will actually call the chip restore functions before deregistering them?
https://review.coreboot.org/c/flashrom/+/47276/1/flashrom.c@2327 PS1, Line 2327: chip_restore();
This seems like a reasonable place to me but maybe before the unmap?
Done