Attention is currently required from: Felix Singer, Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk.
Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66373 )
Change subject: tree: provide flashrom context into programmer_delay() ......................................................................
Patch Set 8:
(14 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/66373/comment/994f54ff_d57a7a01 PS7, Line 7: tree: provide flashrom context into programmer's delay
It's not passed to the programmers anymore
Done
https://review.coreboot.org/c/flashrom/+/66373/comment/26595fc9_3253bd69 PS7, Line 9: Add the flashrom context to the delay function declaration, which : located in `struct programmer_entry`.
Doesn't apply anymore. Also, there is not *that* delay function. […]
Done
https://review.coreboot.org/c/flashrom/+/66373/comment/06fefbef_79b360a8 PS7, Line 14: TOPIC=register_master_api
Also, if you don't have a huge patch series then a topic doesn't make much sense. […]
That topic was actual as long as this patch changed programmer delay API.
As I see this patch will be a small chain in the series of patches by Edward. He uses `programmer_handle_global` topic in gerrit. I hope he will add `TOPIC=` in his next patches.
https://review.coreboot.org/c/flashrom/+/66373/comment/585b99f4_386b3b32 PS7, Line 19: Ticket: https://ticket.coreboot.org/issues/391
Same here. […]
Not directly. That task is blocked and I can't refactor programmer code to use own structure.
I think I should delete it anyway.
File amd_imc.c:
https://review.coreboot.org/c/flashrom/+/66373/comment/34cc27e8_edbe6770 PS7, Line 63: /* flashctx is not completely ready during init, so it's not : * needed (NULL). */
Same here
Done
File atavia.c:
https://review.coreboot.org/c/flashrom/+/66373/comment/d330225e_2836bd40 PS7, Line 93: /* flashctx is not needed (NULL) because atavia does : * not use it in its delay function. */
Same here
Done
File dediprog.c:
https://review.coreboot.org/c/flashrom/+/66373/comment/014bd7af_eeacfe89 PS7, Line 319: /* flashctx not needed (NULL) because dediprog does not use it in its : * delay function. */
Same here
Done
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/66373/comment/f3258508_fbeee20b PS1, Line 258: /* return or exit? */
If flashrom continues to execute code, than it will be unexpected behavior. So for now, use exit(). […]
Ack
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/66373/comment/028c31da_41e2cc94 PS5, Line 220: void programmer_delay(const struct flashctx *flash, unsigned int usecs)
Currently the commit message doesn't give any hint why this is necessary. […]
Abandoned.
https://review.coreboot.org/c/flashrom/+/66373/comment/f60d28da_8f57acda PS5, Line 222: if (!usecs) : return; : : if (!programmer->delay) : return internal_delay(flash, usecs); // maybe don't pass context into internal? or pass NULL? : : if (!flash) { : msg_perr("%s called but flashctx is not valid!\n" : "Please report a bug at flashrom@flashrom.org\n", __func__); : exit(1); : } : programmer->delay(flash, usecs); : } :
leave all this diff out this patch and have it purely just about plumbing flashctx though the contro […]
I got you! Done.
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/66373/comment/6e8fafb4_f8ba90a7 PS6, Line 226: return internal_delay(flash, usecs);
If we put CB:67393 on top of this patch then `programmer_delay()` has been largely dealt with from t […]
This patch originally did a bit more, so yeah. Thanks for the clarification!
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/66373/comment/b8f2b905_387086ce PS7, Line 878: flashctx is not needed (NULL) because ichspi does not use : * it in its delay function. */
Same here
Done
File pony_spi.c:
https://review.coreboot.org/c/flashrom/+/66373/comment/26007511_72fe1242 PS7, Line 248: Also flashctx is not : * completely ready during init anyway.
This doesn't add much value. So please remove.
Done
File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/66373/comment/f200c8e1_bf745acb PS7, Line 1015: * flashctx not needed (NULL) because raiden_debug_spi does not : * use it in its delay function. Also flashctx is not : * completely ready during init anyway. */
Same here
Done
-
Alexander Goncharov (Code Review)