Miklós Márton has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/25683 )
Change subject: Add support for National Instruments USB-845x devices ......................................................................
Patch Set 19:
(18 comments)
Patch Set 17:
(19 comments)
First time I looked through all the code. I know it's already been lying around for more than a year, sorry for that.
It looks nearly ready, although I added some serious comments. Also, there is still a lot of CamelCase. And generally, we try to follow the coding style of the Linux kernel. There is a nice tool to help with that (checkpatch.pl [1]). That is, if you have perl installed. I don't use it myself, but let me try...
./checkpath.pl --no-tree --max-line-length=112 --file ni845x_spi.cThe first warning is spurious, but the errors should be checked. And the other warnings might give some nice hints.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/scr...
Thanks for the review Nico, no worries for the timing, it is better later than never ;).
Also thanks for the checkpatch hint, it revealed a lot of things!
https://review.coreboot.org/c/flashrom/+/25683/17/flashrom.8.tmpl File flashrom.8.tmpl:
https://review.coreboot.org/c/flashrom/+/25683/17/flashrom.8.tmpl@1144 PS17, Line 1144: 1230A2
Nit, example has only 6 digits.
Nice catch! Actually NI used 6 hexa digits for the products manufactured in the US and 7 digits for the products from Hungary and Malaysia. However if I remember correctly all USB-845x products were manufactured outside the US.
https://review.coreboot.org/c/flashrom/+/25683/17/ni845x_spi.c File ni845x_spi.c:
https://review.coreboot.org/c/flashrom/+/25683/17/ni845x_spi.c@43 PS17, Line 43: static char error_string_buffer[1024];
Writing a function around this would probably save a lot lines below […]
Done
https://review.coreboot.org/c/flashrom/+/25683/17/ni845x_spi.c@107 PS17, Line 107: * @brief ni845x_spi_open
Nit, adds no information.
Done
https://review.coreboot.org/c/flashrom/+/25683/17/ni845x_spi.c@109 PS17, Line 109: competition
completion?
Done
https://review.coreboot.org/c/flashrom/+/25683/17/ni845x_spi.c@134 PS17, Line 134: "USB%d::0x%04X::0x%04X::%08X::RAW",
%d expects an `int`, %X an `unsigned int`. If the pointers you pass […]
Done
https://review.coreboot.org/c/flashrom/+/25683/17/ni845x_spi.c@207 PS17, Line 207: IO_voltage_100mV = ((float)IOVoltage_mV / 100.0f);
Doing the calculation as `float` shouldn't make a difference.
Done
https://review.coreboot.org/c/flashrom/+/25683/17/ni845x_spi.c@219 PS17, Line 219: msg_pwarn
Shouldn't be a warning, msg_pinfo() maybe?
Done
https://review.coreboot.org/c/flashrom/+/25683/17/ni845x_spi.c@226 PS17, Line 226: if (selected_voltage_100mV == 0) {
This looks odd. I would have expect it to select 1.2V automatically here. […]
Done
https://review.coreboot.org/c/flashrom/+/25683/17/ni845x_spi.c@266 PS17, Line 266: Set
Get
Done
https://review.coreboot.org/c/flashrom/+/25683/17/ni845x_spi.c@301 PS17, Line 301: &usb_bus, &vid, &pid, &serial_as_number);
Check return code?
Done
https://review.coreboot.org/c/flashrom/+/25683/17/ni845x_spi.c@317 PS17, Line 317: ni845xFindDeviceNext(device_find_handle, resource_handle);
Check return code?
Done
https://review.coreboot.org/c/flashrom/+/25683/17/ni845x_spi.c@342 PS17, Line 342: CS_number = atoi(CS_str);
atoi() isn't a good choice, it would return a valid `0` on any error. […]
Done
https://review.coreboot.org/c/flashrom/+/25683/17/ni845x_spi.c@345 PS17, Line 345: return 1;
leaking `CS_str`
Done
https://review.coreboot.org/c/flashrom/+/25683/17/ni845x_spi.c@382 PS17, Line 382: if (serial_number)
Not needed, as `free(NULL);` is valid.
Done
https://review.coreboot.org/c/flashrom/+/25683/17/ni845x_spi.c@391 PS17, Line 391: atoi
atoi(), see above.
Done
https://review.coreboot.org/c/flashrom/+/25683/17/ni845x_spi.c@399 PS17, Line 399:
None of the error paths (early `return`s) after opening the device […]
Done
https://review.coreboot.org/c/flashrom/+/25683/17/ni845x_spi.c@441 PS17, Line 441: msg_cinfo("FAILED.\n");
Why the second message?
Done
https://review.coreboot.org/c/flashrom/+/25683/17/ni845x_spi.c@449 PS17, Line 449: (uInt32)
the cast seems unnecessary
Done
-
Miklós Márton (Code Review)