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 2:
(6 comments)
https://review.coreboot.org/#/c/25683/2/Makefile File Makefile:
https://review.coreboot.org/#/c/25683/2/Makefile@943 PS2, Line 943: NI845X_LIBS += -L'C:\Program Files (x86)\National Instruments\NI-845x\MS Visual C' : NI845X_LIBS += -L'C:\Program Files\National Instruments\NI-845x\MS Visual C' : NI845X_INCLUDES += -I'C:\Program Files (x86)\National Instruments\NI-845x\MS Visual C' : NI845X_INCLUDES += -I'C:\Program Files\National Instruments\NI-845x\MS Visual C' If there is no reasonable way to probe for this path, I would prefer to query the user for it.
Asking the user to set a variable in case the test below fails would be one way. I would do that anyway, even if we have a default here because a windows version and localization specific path is just too fragile.
https://review.coreboot.org/#/c/25683/2/Makefile@948 PS2, Line 948: LIBS += -lni845x How is it licensed? If it's not GPLv2 compatible, should we print a warning about shipping a binary linked against it?
https://review.coreboot.org/#/c/25683/2/Makefile@1349 PS2, Line 1349: spurious space
https://review.coreboot.org/#/c/25683/2/ni845x_spi.c File ni845x_spi.c:
PS2: Maybe run this file through `indent` first. And no camel-case, please.
https://review.coreboot.org/#/c/25683/2/ni845x_spi.c@8 PS2, Line 8: * the Free Software Foundation; version 2 of the License. Please do not restrict the code to version 2 unless you really want to. flashrom should be v2+, IMHO, the decision to make parts of it v2 only (to prevent a v3 fork) was rather stupid.
https://review.coreboot.org/#/c/25683/2/ni845x_spi.c@47 PS2, Line 47: uInt32 please use stdint.h types where applicable