Attention is currently required from: Miklós Márton, Anastasia Klimchuk.
Alexander Goncharov 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 3:
(4 comments)
File ni845x_spi.c:
https://review.coreboot.org/c/flashrom/+/72154/comment/8e228805_709e6be8 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. […]
OK, It takes 113 chars - it's no big deal.
https://review.coreboot.org/c/flashrom/+/72154/comment/0fdc1a02_ba9ad6a4 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
Done
https://review.coreboot.org/c/flashrom/+/72154/comment/bf52b72b_21093f01 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 func […]
This second argument (`set_io_voltage_mV`) contains a pointer to `io_voltage_in_mV` global var. If we add one more argument to this function, it will be pointer to `io_voltage_in_mV` too! So, let's just use the second argument.
Code snippets that prove my words are below.
ni845x_spi.c:447:
``` if (usb8452_spi_set_io_voltage(flash->chip->voltage.max, &io_voltage_in_mV, USE_LOWER)) { ... } ```
ni845x_spi.c:470:
``` if (usb8452_spi_set_io_voltage(flash->chip->voltage.min, &io_voltage_in_mV, USE_HIGHER)) { ... } ```
ni845x_spi.c:619:
``` if (usb8452_spi_set_io_voltage(requested_io_voltage_mV, &io_voltage_in_mV, USE_LOWER) < 0) { ... } ```
https://review.coreboot.org/c/flashrom/+/72154/comment/134fdc2c_2be02433 PS2, Line 290: if (set_io_voltage_mV) Miklós, how about dropping this condition? Maybe not in this patch, but in the one of the next. This argument should contain a valid pointer when you call the function to get the IO voltage that was set. Or am I missing something?
-
Alexander Goncharov (Code Review)