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 25:
> Patch Set 25: Code-Review+2
>
> (4 comments)
>
> I'm willing to merge this as is. The automatic voltage adaption
> seems a little rough on the edges, though. Generally, such a
> feature would belong into a higher level of flashrom. And then
> there is the possibility that the user didn't connect the chip
> directly to the programmer...
>
> Anyway, this already adds a lot to flashrom's traits. I'll try
> to rebase it in Gerrit. If that works and there are no further
> comments, I'll try to merge it this weekend.
Good timing, I will fix the outstanding issues today/tomorrow. I agree with the adding option for the IO voltage override.
--
To view, visit https://review.coreboot.org/c/flashrom/+/25683
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9477b6f0193bfdf20bbe63421a7fb97b597ec549
Gerrit-Change-Number: 25683
Gerrit-PatchSet: 25
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Miklós Márton <martonmiklosqdev(a)gmail.com>
Gerrit-CC: Stefan T <stefan.tauner(a)gmx.at>
Gerrit-Comment-Date: Fri, 04 Oct 2019 13:32:04 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
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 25: Code-Review+2
(4 comments)
I'm willing to merge this as is. The automatic voltage adaption
seems a little rough on the edges, though. Generally, such a
feature would belong into a higher level of flashrom. And then
there is the possibility that the user didn't connect the chip
directly to the programmer...
Anyway, this already adds a lot to flashrom's traits. I'll try
to rebase it in Gerrit. If that works and there are no further
comments, I'll try to merge it this weekend.
https://review.coreboot.org/c/flashrom/+/25683/25/ni845x_spi.c
File ni845x_spi.c:
https://review.coreboot.org/c/flashrom/+/25683/25/ni845x_spi.c@253
PS25, Line 253: if (io_voltage_100mV < usb8452_io_voltages_in_100mV[0]) {
Nit, if called with `use_smaller = false` we might not want to take this path?
Currently, that is not possible with the chips in our database anyway.
https://review.coreboot.org/c/flashrom/+/25683/25/ni845x_spi.c@443
PS25, Line 443: if (device_pid == USB8451) {
Nit, taking this path will result in no warning printed, even if
the user specified a `voltage`, AFAICS? usb8452_spi_set_io_voltage()
should show the warning and return 0, so I guess we just don't need
this `if` around it.
https://review.coreboot.org/c/flashrom/+/25683/25/ni845x_spi.c@504
PS25, Line 504: return -1;
This path is a little troubling. What if the user has a voltage translator
hooked-up? They wouldn't have a way to force operation...
https://review.coreboot.org/c/flashrom/+/25683/25/ni845x_spi.c@542
PS25, Line 542: }
Might look nicer with the whole `if` block in a separate fucntion.
--
To view, visit https://review.coreboot.org/c/flashrom/+/25683
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9477b6f0193bfdf20bbe63421a7fb97b597ec549
Gerrit-Change-Number: 25683
Gerrit-PatchSet: 25
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Márton Miklós <martonmiklosqdev(a)gmail.com>
Gerrit-CC: Stefan T <stefan.tauner(a)gmx.at>
Gerrit-Comment-Date: Fri, 04 Oct 2019 12:45:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment