Attention is currently required from: Miklós Márton, Alexander Goncharov.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72154 )
Change subject: ni845x_spi: pass global state through func params ......................................................................
Patch Set 2:
(4 comments)
Patchset:
PS2:
This programmer requires a special environment to run, and unfortunately, it only works on Windows. […]
Just a small note from me: maybe we can do the code review first (for all the patches in the chain) and then when all comments resolved, do the testing? Also as it's typically happens, testing is done at the end of the chain (not for each individual patch).
Let's maybe keep this comment unresolved, and do the rest of code reviews. Miklós thank you so much for help!
File ni845x_spi.c:
https://review.coreboot.org/c/flashrom/+/72154/comment/d3c279e4_dc4bf5cf PS2, Line 131: static int32 ni845x_spi_open_resource(char *resource_handle, uInt32 *opened_handle, : enum USB845x_type device_pid) I am thinking, this could fit in one line: the hard limit for line length is 112 chars. Of course not all the lines need to be that long, but for the case like this (function arguments list) we can use one line, mainly because it makes it easier to grep.
https://review.coreboot.org/c/flashrom/+/72154/comment/396a202a_216c01c0 PS2, Line 152: static int ni845x_spi_open(const char *serial, uInt32 *return_handle, : enum USB845x_type *device_pid) this can be one line too
https://review.coreboot.org/c/flashrom/+/72154/comment/d9cbca5d_cd118ba6 PS2, Line 233: set_io_voltage_mV Not sure you meant it: there is `set_io_voltage_mV` which is an existing second argument in the function, and there is also `io_voltage_in_mV` which is global. Why do you replace one with the other? To follow what the patch is doing, you would need to add one more argument to this function, `io_voltage_in_mV` and use it instead of a global var?
-
Anastasia Klimchuk (Code Review)