Attention is currently required from: Richard Hughes, Edward O'Callaghan, Daniel Campello, Angel Pons, Patrick Rudolph. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/49643 )
Change subject: libflashrom: Return progress state to the library user ......................................................................
Patch Set 11:
(17 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/49643/comment/73dcf6ad_208784c9 PS11, Line 15: It would be realy good to test it on at least one real programmer, not dummy. For example linux_mtd?
Patchset:
PS11: Thank you Daniel, it would be cool to have this work completed!
File cli_output.c:
https://review.coreboot.org/c/flashrom/+/49643/comment/819acb68_405f80ee PS11, Line 87: Space between function name and arguments list not needed.
File flash.h:
https://review.coreboot.org/c/flashrom/+/49643/comment/a315f6ee_3501cc33 PS11, Line 481: void set_progress(struct flashctx *flash, enum flashrom_progress_stage stage, size_t current, size_t total); Lets move it into the /* cli_output.c */ section, together with other output stuff. Here is a bit of an unexpected place to find set_progress.
https://review.coreboot.org/c/flashrom/+/49643/comment/30d49fcc_0653831f PS11, Line 481: set_progress Looking into the usage of this function, maybe `update_progress` is better name? It is called repeatedly, and it actually updates the same piece of data, progress_userdata again and again.
File libflashrom.h:
https://review.coreboot.org/c/flashrom/+/49643/comment/eb0898a5_a6e861b0 PS11, Line 106: flashrom_set_progress_callback If set_progress becomes update_progress, this one becomes flashrom_update_progress_callback
File libflashrom.c:
https://review.coreboot.org/c/flashrom/+/49643/comment/46ed4436_29b18397 PS11, Line 102: chaned chaned -> changed (typo)
https://review.coreboot.org/c/flashrom/+/49643/comment/14839389_1bf1a8a4 PS11, Line 106: userdata This is called progress_userdata not userdata?
File spi.c:
https://review.coreboot.org/c/flashrom/+/49643/comment/3c7f38d5_f8c817b6 PS11, Line 104: size_t progress_start = start; : size_t progress_total = len - start; I think the variable naming can be improved, I spent a while trying to read this small piece. How about:
progress_start -> start_address progress_total -> end_address
And I would love to rename start into current_address , but this needs to be a separate patch. So start stays as is for now :\
File spi25.c:
https://review.coreboot.org/c/flashrom/+/49643/comment/1b7c0397_bfe64e4d PS11, Line 677: size_t progress_start = start; : size_t progress_total = len - start; This looks the same as previous, let do
progress_start -> start_address progress_total -> end_address
https://review.coreboot.org/c/flashrom/+/49643/comment/4c63de37_2da7410f PS11, Line 704: size_t progress_start = start; : size_t progress_total = len - start; And one more here :)
File tests/spi25.c:
https://review.coreboot.org/c/flashrom/+/49643/comment/81d1f549_8de460df PS11, Line 93: assert_true(0); : break; This probably can be `fail()` , if it's the intention
https://review.coreboot.org/c/flashrom/+/49643/comment/7c87a5b6_c30622d0 PS11, Line 106: new line
https://review.coreboot.org/c/flashrom/+/49643/comment/301d3faa_345d4bde PS11, Line 107: new line
https://review.coreboot.org/c/flashrom/+/49643/comment/e943397a_295be08e PS11, Line 110: new line
https://review.coreboot.org/c/flashrom/+/49643/comment/17563aef_dac349d3 PS11, Line 110: new line
https://review.coreboot.org/c/flashrom/+/49643/comment/6db90884_30c83205 PS11, Line 110: new line
-
Anastasia Klimchuk (Code Review)