Beside usb8452_spi_set_io_voltage() and some nits, this looks
quite good :)
But indentation seems to be broken in many places now (more
than I commented on). It seems your editor assumes 1 tab ==
4 character spaces? it should be 8.
10 comments:
Patch Set #21, Line 150: "USB%u::0x%04X::0x%04X::%08lX::RAW",
Nit, odd indentation (it seems you replaced 8 spaces with 2 tabs? should be 1).
Patch Set #21, Line 151: &usb_bus, &vid, &pid, &serial_as_number) != 4) {
here too
Patch Set #21, Line 154: resource_name);
and here
Patch Set #21, Line 222: // usb8452_io_voltages_in_100mV is a decreasing list of supported voltages
comment seems already outdated :-(
Patch Set #21, Line 227: i + 1
This condition is far too complex, and, if I read this correctly,
it may access beyond the array if the requested voltage is above
all array entries. i.e. for `i == ARRAY_SIZE() - 1` and
`IO_voltage_100mV > usb8452_io_voltages_in_100mV[i]` this
would be:
if ((true && false) || (true && undefined))
// usb8452_io_voltages_in_100mV is a decreasing list of supported voltages
for (i = 0; i < ARRAY_SIZE(usb8452_io_voltages_in_100mV); i++) {
if ((i == (ARRAY_SIZE(usb8452_io_voltages_in_100mV) - 1) &&
usb8452_io_voltages_in_100mV[i] == IO_voltage_100mV) ||
(usb8452_io_voltages_in_100mV[i] <= IO_voltage_100mV &&
IO_voltage_100mV < usb8452_io_voltages_in_100mV[i + 1])) {
selected_voltage_100mV = usb8452_io_voltages_in_100mV[i];
if (IO_voltage_100mV != usb8452_io_voltages_in_100mV[i]) {
msg_pwarn("USB-8452 IO Voltage forced to: %.1f V\n",
(float)selected_voltage_100mV / 10.0f);
} else {
msg_pinfo("USB-8452 IO Voltage set to: %.1f V\n",
(float)selected_voltage_100mV / 10.0f);
}
break;
}
}
if (selected_voltage_100mV == 0) {
msg_pwarn("The USB-8452 does not supports the %.1f V IO voltage\n",
IOVoltage_mV / 10.0f);
selected_voltage_100mV = kNi845x12Volts;
msg_pwarn("The output voltage is set to 1.2V (this is the lowest voltage)\n");
msg_pwarn("Supported IO voltages: ");
for (i = 0; i < ARRAY_SIZE(usb8452_io_voltages_in_100mV); i--) {
msg_pwarn("%.1fV", (float)usb8452_io_voltages_in_100mV[i] / 10.0f);
if (i != ARRAY_SIZE(usb8452_io_voltages_in_100mV) - 1)
msg_pwarn(", ");
}
msg_pwarn("\n");
return -1;
}
I think this whole block could benefit from a better separation of
selecting the array index and status reporting, e.g. for an ascending
list, this should be all that's needed to select the index (it should
be that easy, because we know the list is ordered):
for (i = ARRAY_SIZE(usb8452_io_voltages_in_100mV); i > 0; --i) {
if (IO_voltage_100mV >= usb8452_io_voltages_in_100mV[i])
break;
}
selected_voltage_100mV = usb8452_io_voltages_in_100mV[i];Then, we could decide what message to show:
if (IO_voltage_100mV < usb8452_io_voltages_in_100mV[0]) {
/* unsupported / would have to round up */
unsupported voltage message
voltages list
return -1;
} else if (selected_voltage_100mV != IO_voltage_100mV) {
/* we rounded down */
msg_pwarn("USB-8452 IO Voltage forced to: %.1f V\n", ...);
} else {
/* exact match */
msg_pinfo("USB-8452 IO Voltage set to: %.1f V\n", ...);
}Or maybe even move `if (IO_voltage_100mV < usb8452_io_voltages_in_100mV[0])`
above the loop? Now that I looked up it, would actually match well with the
`if (IOVoltage_mV > 3300)`. And why would we print the whole list of sup-
ported voltages only for values below but not above the possible?
Patch Set #21, Line 270: int32 i = ni845xSpiConfigurationSetClockRate(configuration_handle, SCK_freq_in_KHz);
broken indentation
if (clock_freq_read_KHz != SCK_freq_in_KHz) {
msg_pinfo("SPI clock frequency forced to: %d KHz (requested: %d KHz)\n",
(int)clock_freq_read_KHz, (int)SCK_freq_in_KHz);
} else {
msg_pinfo("SPI clock frequency set to: %d KHz\n", (int)SCK_freq_in_KHz);
}
return 0;
indentation
Patch Set #21, Line 365: if (CS_number < 0 || 7 < CS_number) {
Missing check of length of `CS_str`, e.g. what happens if somebody passes "10"?
Patch Set #21, Line 386: free(speed_str);
move above the `if`?
To view, visit change 25683. To unsubscribe, or for help writing mail filters, visit settings.