Attention is currently required from: Felix Singer, Nico Huber, Richard Hughes.
2 comments:
Patchset:
I think I can answer some of these, let me try! :)
have you tested if the current implementation works?
There is a testing information in commit message, I asked to add earlier.
I'm a bit confused about when update_progress() should get called.
Previous patch CB:49643 introduces update_progress() calls into many programmers. For me, it was easier to understand when looking at CB:49643
Does it do anything on a modern Intel system at all?
This question, maybe someone can confirm, but what I see is that CB:49643 is not changing ichspi.c at all, so I guess Intel systems are not touched?
There are odd paths like spi_chip_read() calls into the master, which may
call spi_read_chunked() which calls update_progress(). But spi_chip_read()
calls it as well? I wonder what was the reason to do it at the chip and
programmer levels.
This is interesting, I had a closer look into `spi_chip_read()` and `spi_read_chunked()` and I see now what you mean. It is possible that update_progress is called by both functions, however not always, but only in some cases.
I looked into flashrom_progress_cb in cli_output.c and it calculates percent progress, and then it produces output only of percent has changed. So it won't produce the same repeated lines like
READ 66% complete...
READ 66% complete...
It will be only line in such case.
What would be a better way to wire update_progress into spi infra, what do you think?
File include/libflashrom.h:
Patch Set #2, Line 85: typedef void(flashrom_progress_callback)(struct flashrom_flashctx *flashctx);
Why doesn't it get the progress passed directly?
I think I can answer this one. There is a progress_state in flashctx, so no need to pass another one.
The earlier versions in CB:49643 had two arguments: flashctx and progress_state and it was confusing on which exactly state is used. The patch owner was asked to remove the second argument during review. I can understand because there is a progress_state in flashctx already.
I hope I answered your question! At least, to the extent how I understand the question.
To view, visit change 64663. To unsubscribe, or for help writing mail filters, visit settings.