Nico Huber has posted comments on this change. ( https://review.coreboot.org/25683 )
Change subject: Add support for National Instruments USB-845x devices ......................................................................
Patch Set 8:
(18 comments)
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'C:\Program Files\National Instruments\NI-845x\MS Visual C' Please try the following:
NI845X_LIBS += -L'${ProgramFiles}\National Instruments\NI-845x\MS Visual C' NI845X_LIBS += -L'${ProgramFiles(x86)}\National Instruments\NI-845x\MS Visual C'
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
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c File ni845x_spi.c:
PS8: I'm only through half the file yet, will have to continue another time... hopefully soon.
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@15 PS8, Line 15: * : * You should have received a copy of the GNU General Public License : * along with this program; if not, write to the Free Software : * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA Please remove this paragraph.
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@20 PS8, Line 20: #if CONFIG_NI845X_SPI == 1 remove, please. Makefile already takes care of that.
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@34 PS8, Line 34: USB845xType No CamelCase please, same for all other new identifiers.
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@43 PS8, Line 43: static char *serial_number = NULL; // by default open the first connected device This is not supposed to be global, as you added it as an argument to ni845x_spi_open().
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@50 PS8, Line 50: // forward declaration obviously
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@52 PS8, Line 52: no space after asterisk
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@55 PS8, Line 55: static uint8_t usb8452_io_voltages_in_100mV[5] = { should be `static const`
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@63 PS8, Line 63: /* Copied from dediprog.c */ If it's really a verbatim copy, please place it into its own file (e.g. `parameters.c`).
If not, the original code is GPL v2 only licensed...
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@117 PS8, Line 117: no space after asterisk
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@117 PS8, Line 117: char *serial should be `const char`
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@119 PS8, Line 119: resource_handle Why is it called `handle`? isn't it rather `resource_value`?
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@136 PS8, Line 136: serial_number should use the `serial` function parameter instead
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@160 PS8, Line 160: } else (if != 0)?
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@166 PS8, Line 166: ARRAY_SIZE You can use `sizeof()` instead, as we know it's a char array.
https://review.coreboot.org/#/c/25683/8/ni845x_spi.c@169 PS8, Line 169: } This whole block is very confusing. I don't see bigger problems, but think we can save many indentation levels and redundant code if we use a control flow as follows:
tmp = ni845xFindDevice(...); if (tmp != 0) { ... return -1; }
for (; serial && found_devices_count; --found_devices_count) { sscanf(resource_handle, ...); if (strtol(serial, ...) == serial_as_number) break; if (found_devices_count > 1) { if (ni845xFindDeviceNext(...) != 0) goto _close_ret; } }
if (found_devices_count) ret = ni845x_spi_open_resource(...);
_close_ret:
Just a sketch, I guess we should check the return value of sscanf() too and (as I didn't look at the library API) I don't know if we should call ni845xCloseFindDeviceHandle() at all if ni845xFindDeviceNext() failed?
-
Nico Huber (Code Review)