Attention is currently required from: Thomas Heijligen, Angel Pons, qianfan, Nicholas Chin.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70573 )
Change subject: ch347_spi.c: Add initial support for the WCH CH347 ......................................................................
Patch Set 3:
(5 comments)
Patchset:
PS3:
Nicholas, just to check: is this patch still needed? It seems like CB:70529 + CB:71066 add new progr […]
After a number of discussions in several irc channels, the conclusion is: this patch is going ahead and anything extra from CB:70529 can be added on the top.
Few things that are missing to add a new programmer:
There a few things need to be added to this patch: 1) add new programmer to meson build system (currently only added to make) 2) add man page entry for new programmer 3) add to test_build.sh 4) add yourself to MAINTAINERS file (if you agree!)
This is a coincidence, but: there is one more new programmer being under review at the moment in CB:71801 . The programmer is independent of this one, but it can be a useful example as of all the places that need to be updated when introducing new programmer.
Thank you for your work!
File ch347_spi.c:
https://review.coreboot.org/c/flashrom/+/70573/comment/4f38919c_e448ce7b PS3, Line 62: /* TODO: Set this depending on the mode */ new line before
https://review.coreboot.org/c/flashrom/+/70573/comment/95361b93_0ce0253b PS3, Line 68: ch347_data->handle = NULL; new line before
https://review.coreboot.org/c/flashrom/+/70573/comment/4fba5271_da300058 PS3, Line 69: free(data);
Both my code and `ft2232_spi` allocate memory for the data passed around in flashctx, so I would thi […]
Yes please, free the data :)
Nicholas, you did the right thing, thank you for not introducing the global state in a new programmer. The reason `ch341a_spi` still has global state (and therefore does not have data to free) is because removing global state was blocked by refactoring of `programmer_delay`, which took a long time in review. However, that refactoring merged, and now `ch341a_spi` is unblocked and global state will be removed soon.
Another relevant point is that: if programmer has a lifecycle unit test, the test can check that there are no memory leaks at shutdown. `ch341a_spi` already has unit test. Maybe, at some point later, we can add a unit test for `ch347_spi`. Not now of course, but this would be a good idea for future, especially that existing test for ch341 can be taken as a starting point.
https://review.coreboot.org/c/flashrom/+/70573/comment/14f4aa3f_05ffdf34 PS3, Line 123: Could not read write response It is confusing, given that both "read" and "write" are the operations, what does it mean "Could not read write response" ? is it read response or write response? If you looks at the source code, you can see this is in the context of `ch347_write` so probably about "write response" but imagine a user looking into the logs.
TLDR: Lets make it clear this is about write operation. Could not parse response from write operation? Maybe something else, this is just my suggestion.
-
Anastasia Klimchuk (Code Review)