Nico Huber 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 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.c
The 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...
https://review.coreboot.org/#/c/25683/17/flashrom.8.tmpl File flashrom.8.tmpl:
https://review.coreboot.org/#/c/25683/17/flashrom.8.tmpl@1144 PS17, Line 1144: 1230A2 Nit, example has only 6 digits.
https://review.coreboot.org/#/c/25683/10/ni845x_spi.c File ni845x_spi.c:
https://review.coreboot.org/#/c/25683/10/ni845x_spi.c@123 PS10, Line 123: %ld
int32 is coming from the NI's header where it is defined as a long: […]
Stefan is correct. You shouldn't know what the header file defines. The header file could change, for instance if NI would issue a 64-bit version of the library. Please either cast `tmp` to the expected type or use the `inttypes.h` definitions as Stefan suggested.
https://review.coreboot.org/#/c/25683/17/ni845x_spi.c File ni845x_spi.c:
https://review.coreboot.org/#/c/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 and the buffer could be local. e.g.
void ni845x_report_error(const char *const func, const int32 err) { static char buf[1024]; ni845xStatusToString(err, sizeof(buf), buf); msg_perr("%s failed with: %s (%d)\n", func, buf, (int)err); }
Callers could use `__func__` to reduce copy-paste errors:
ni845x_report_error(__func__, tmp);
https://review.coreboot.org/#/c/25683/17/ni845x_spi.c@107 PS17, Line 107: * @brief ni845x_spi_open Nit, adds no information.
https://review.coreboot.org/#/c/25683/17/ni845x_spi.c@109 PS17, Line 109: competition completion?
https://review.coreboot.org/#/c/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 are compatible, is implementation specific.
A generally portable version would be (assuming bus numbers are >= 0):
unsigned int usb_bus, vid, device_pid; unsigned long int serial_as_number; sccanf(resource_name, "USB%u::0x%04X::0x%04X::%08lX::RAW", &usb_bus, &vid, &device_pid, &serial_as_number);
https://review.coreboot.org/#/c/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.
https://review.coreboot.org/#/c/25683/17/ni845x_spi.c@219 PS17, Line 219: msg_pwarn Shouldn't be a warning, msg_pinfo() maybe?
https://review.coreboot.org/#/c/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. The loop above doesn't look like it's possible to select 1.2V.
https://review.coreboot.org/#/c/25683/17/ni845x_spi.c@266 PS17, Line 266: Set Get
https://review.coreboot.org/#/c/25683/17/ni845x_spi.c@301 PS17, Line 301: &usb_bus, &vid, &pid, &serial_as_number); Check return code?
https://review.coreboot.org/#/c/25683/17/ni845x_spi.c@317 PS17, Line 317: ni845xFindDeviceNext(device_find_handle, resource_handle); Check return code?
https://review.coreboot.org/#/c/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.
One good pattern is to use strtoul() and check `endptr`, e.g.:
cs_number = strtoul(cs_str, &endptr, 0); if (!*cs_str || *endptr || cs_number < 0 || 7 < cs_number) { return 1; }
In this case, however, as there is only a single digit to parse, this should be safe too:
cs_number = cs_str[0] - '0'; if (cs_str[1] || cs_number < 0 || 7 < cs_number) { return 1; }
https://review.coreboot.org/#/c/25683/17/ni845x_spi.c@345 PS17, Line 345: return 1; leaking `CS_str`
https://review.coreboot.org/#/c/25683/17/ni845x_spi.c@382 PS17, Line 382: if (serial_number) Not needed, as `free(NULL);` is valid.
https://review.coreboot.org/#/c/25683/17/ni845x_spi.c@391 PS17, Line 391: atoi atoi(), see above.
https://review.coreboot.org/#/c/25683/17/ni845x_spi.c@399 PS17, Line 399: None of the error paths (early `return`s) after opening the device closes the device. Maybe just call ni845x_spi_shutdown() directly, in case.
It would be nice to sort the code blocks here: 1. do all the parameter parsing, 2. open the device and apply the settings. This should also ease the error path handling.
https://review.coreboot.org/#/c/25683/17/ni845x_spi.c@441 PS17, Line 441: msg_cinfo("FAILED.\n"); Why the second message?
https://review.coreboot.org/#/c/25683/17/ni845x_spi.c@449 PS17, Line 449: (uInt32) the cast seems unnecessary
-
Nico Huber (Code Review)