Attention is currently required from: Felix Singer, Richard Hughes, Edward O'Callaghan, Anastasia Klimchuk.
3 comments:
Patchset:
have you tested if the current implementation works?
There is a testing information in commit message, I asked to add earlier.
I meant if it works correctly in fwupd.
What would be a better way to wire update_progress into spi infra, what do you think?
I'd start with a general rule, SPI is special though because more things
go through the master. Maybe: "The chip read/write functions are generally
resonsible for updating the progress state. If they simply wrap the functions
of a master, the responsibility is delegated to those master functions." So
for SPI this would mean whatever we set in `struct spi_master` should update
the progress.
However, I'm not convinced that this is even a good strategy for an overall
progress indication. Counting individual interations inside the chip func-
tions makes sense for a smooth advance but the only place where we actually
know the grand total is at the core of flashrom.c, e.g. in walk_by_layout(). I
couldn't test this because of the bugs in the current implementation (write
progress just stayed at 0% in dummyflasher), but I imagine for a write it
would just jump through a lot of 0..100% for erase/read/write multiple times.
I'm not sure if this is intended. It would actually not provide more infor-
mation to the user than a spinning wheel would.
File include/libflashrom.h:
Patch Set #2, Line 85: typedef void(flashrom_progress_callback)(struct flashrom_flashctx *flashctx);
I think I can answer this one. […]
But struct flashrom_flashctx is opaque, so there's only one state effectively.
I wonder why is there a flashctx argument at all? Any additional information
the callee needs can be conveyed in `user_data`.
So why not make things simple and pass `(progress_state, user_data)`?
File libflashrom.c:
Patch Set #2, Line 70: return flashctx->progress_state;
We should create a patch that consistently introduces guards for parameter sanity.
So far we guard when it's allowed to pass NULL. Everything else should probably
be an assert()?
To view, visit change 64663. To unsubscribe, or for help writing mail filters, visit settings.