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 21:
(10 comments)
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.
https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c File ni845x_spi.c:
https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@150 PS21, 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).
https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@151 PS21, Line 151: &usb_bus, &vid, &pid, &serial_as_number) != 4) { here too
https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@154 PS21, Line 154: resource_name); and here
https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@222 PS21, Line 222: // usb8452_io_voltages_in_100mV is a decreasing list of supported voltages comment seems already outdated :-(
https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@227 PS21, 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))
https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@222 PS21, Line 222: // 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?
https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@270 PS21, Line 270: int32 i = ni845xSpiConfigurationSetClockRate(configuration_handle, SCK_freq_in_KHz); broken indentation
https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@285 PS21, Line 285: 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
https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@365 PS21, Line 365: if (CS_number < 0 || 7 < CS_number) { Missing check of length of `CS_str`, e.g. what happens if somebody passes "10"?
https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@386 PS21, Line 386: free(speed_str); move above the `if`?
-
Nico Huber (Code Review)