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 21:
(10 comments)
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).
Done
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
Done
https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@154 PS21, Line 154: resource_name);
and here
Done
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 :-(
Done
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, […]
Done
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 […]
Done
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
Done
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
Done
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. […]
Done
https://review.coreboot.org/c/flashrom/+/25683/21/ni845x_spi.c@386 PS21, Line 386: free(speed_str);
move above the `if`?
Done
-
Miklós Márton (Code Review)