David Hendricks has posted comments on this change. ( https://review.coreboot.org/17946 )
Change subject: Add a convenient libflashrom interface ......................................................................
Patch Set 6: Code-Review+1
(6 comments)
The code looks good overall, I just have a few general comments. I'd be fine with committing this as-is.
https://review.coreboot.org/#/c/17946/5/flashrom.c File flashrom.c:
PS5, Line 2557: t too
https://review.coreboot.org/#/c/17946/6/flashrom.c File flashrom.c:
Line 2507: return 1; Would it make sense to add a wrapper function that calls unmap_flash() and maybe other functions (e.g. to re-lock the chip) to undo prepare_flash_access()? Maybe unprepare_flash_access() or restore_original_flash_access() or something?
PS6, Line 2610: that : * lap "containing" might be a better word for this
Line 2696: } else Style nit: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu...
https://review.coreboot.org/#/c/17946/6/libflashrom.c File libflashrom.c:
PS6, Line 42: fl_ I had originally thought fl_ was used when describing flash layout stuff that's not necessarily tied with the library (internal function args and variables). Can you clarify when/how this prefix is to be used so it doesn't get copy+pasted inappropriately? Maybe use "fl_" for flash layout and "lf_" for libflashrom?
PS6, Line 217: << 10 Probably should be '* KiB', but that's another matter.