Attention is currently required from: Thomas Heijligen, Angel Pons, qianfan, Nicholas Chin.
5 comments:
Patchset:
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:
Patch Set #3, Line 62: /* TODO: Set this depending on the mode */
new line before
Patch Set #3, Line 68: ch347_data->handle = NULL;
new line before
Patch Set #3, 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.
Patch Set #3, 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.
To view, visit change 70573. To unsubscribe, or for help writing mail filters, visit settings.