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/scripts/checkpatch.pl
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!
18 comments:
Patch Set #17, 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.
Patch Set #17, Line 43: static char error_string_buffer[1024];
Writing a function around this would probably save a lot lines below […]
Done
Patch Set #17, Line 107: * @brief ni845x_spi_open
Nit, adds no information.
Done
Patch Set #17, Line 109: competition
completion?
Done
Patch Set #17, Line 134: "USB%d::0x%04X::0x%04X::%08X::RAW",
%d expects an `int`, %X an `unsigned int`. If the pointers you pass […]
Done
Patch Set #17, Line 207: IO_voltage_100mV = ((float)IOVoltage_mV / 100.0f);
Doing the calculation as `float` shouldn't make a difference.
Done
Patch Set #17, Line 219: msg_pwarn
Shouldn't be a warning, msg_pinfo() maybe?
Done
Patch Set #17, Line 226: if (selected_voltage_100mV == 0) {
This looks odd. I would have expect it to select 1.2V automatically here. […]
Done
Get
Done
Patch Set #17, Line 301: &usb_bus, &vid, &pid, &serial_as_number);
Check return code?
Done
Patch Set #17, Line 317: ni845xFindDeviceNext(device_find_handle, resource_handle);
Check return code?
Done
Patch Set #17, Line 342: CS_number = atoi(CS_str);
atoi() isn't a good choice, it would return a valid `0` on any error. […]
Done
Patch Set #17, Line 345: return 1;
leaking `CS_str`
Done
Patch Set #17, Line 382: if (serial_number)
Not needed, as `free(NULL);` is valid.
Done
atoi(), see above.
Done
None of the error paths (early `return`s) after opening the device […]
Done
Patch Set #17, Line 441: msg_cinfo("FAILED.\n");
Why the second message?
Done
Patch Set #17, Line 449: (uInt32)
the cast seems unnecessary
Done
To view, visit change 25683. To unsubscribe, or for help writing mail filters, visit settings.