Márton Miklós has posted comments on this change. ( https://review.coreboot.org/25683 )
Change subject: Add support for National Instruments USB-845x devices ......................................................................
Patch Set 10:
(16 comments)
Review comments fixed, expect for the code copied from dediprog: I will wait for the authors feedback on the relicensing to GPL v2+.
https://review.coreboot.org/#/c/25683/8/Makefile File Makefile:
https://review.coreboot.org/#/c/25683/8/Makefile@967 PS8, Line 967: NI845X_LIBS += -L'${PROGRAMFILES(x86)}\National Instruments\NI-845x\MS Visual C'
Please try the following: […]
Done
https://review.coreboot.org/#/c/25683/8/flashrom.8.tmpl File flashrom.8.tmpl:
https://review.coreboot.org/#/c/25683/8/flashrom.8.tmpl@a1094 PS8, Line 1094: :
it seems you dropped this from the digilent_spi description
Done
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c File ni845x_spi.c:
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@15 PS8, Line 15: * : */ : : #include <ctype.h>
Please remove this paragraph.
Done
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@20 PS8, Line 20: #include <string.h>
remove, please. Makefile already takes care of that.
Done
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@34 PS8, Line 34:
No CamelCase please, same for all other new identifiers.
Done
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@43 PS8, Line 43: static char error_string_buffer[1024];
This is not supposed to be global, as you added it as an […]
Done
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@50 PS8, Line 50: kNi845x33Volts,
obviously
Done
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@52 PS8, Line 52:
no space after asterisk
Done
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@55 PS8, Line 55: };
should be `static const`
Done
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@117 PS8, Line 117: ial_as_numbe
should be `const char`
Done
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@117 PS8, Line 117:
no space after asterisk
Done
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@119 PS8, Line 119:
Why is it called `handle`? isn't it rather `resource_value`?
Done
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@136 PS8, Line 136: alformed reso
should use the `serial` function parameter instead
Done
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@160 PS8, Line 160: if (tmp) {
else (if != 0)?
Done
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@166 PS8, Line 166:
You can use `sizeof()` instead, as we know it's a char array.
Done
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@169 PS8, Line 169: * @brief ni845x_spi_open_resource
This whole block is very confusing. I don't see bigger problems, […]
Done