Attention is currently required from: Felix Singer, Thomas Heijligen, Jean THOMAS, Alexander Goncharov.
Patch set 6:Code-Review +1
5 comments:
Patchset:
Hi Nico, thanks for your feedback. […]
Depends on the reader, I guess. I was a little confused by the double memcpy()
at first. Other flashrom developers might have avoided the big allocation. I
guess it doesn't matter. On systems with USB we probably have resources to spare.
Current implementation looks good.
One more style nit I've noticed: we also place a space before the asterisk
in pointer casts, also always spaces around operators with two or more operands.
File dirtyjtag_spi.c:
Patch Set #5, Line 121: size_t num_xfer = (len + max_xfer_size - 1 ) / max_xfer_size; // ceil(len/max_xfer_size)
1. Lift the `.max_data_*` limits so this function gets called for bigger
chunks. There should be less overhead as we wouldn't have to send the
TMS/STOP sequence as often. I assume this could speed up long reads.
Some of the memcpy() also seems unnecessary, but the USB transfers
probably limit speed so much that it doesn't matter.
I went with that option.
Did you test if it affects the read/write speed?
This could probably be written much simpler if we'd focus on the strcmp. […]
Minor misunderstanding, I think. I meant this `if` block is all that
is needed. The `if ((units > tmp) && (units < end))` is not needed
(or actually points to another problem, see below). `units < end`
checks if there is a unit given at all. I misinterpreted your original
intention but you fixed that already :) using only strcasecmp() implicitly
also checks that there is a string.
`units > tmp` could actually only be false if there was no number given.
In this case we shouldn't complain about the unit. Probably best to check
for `freq != 0` explicitly. Or for the final value `1 <= freq && freq <= UINT16_MAX`?
NB. The canonical name for this option in flashrom is `spispeed`. Your
code will probably be the most generic version so it might make sense
to turn it into an API. Effort to unify this setting was already discussed
a few times, but people seem to always get distracted ;)
Patch Set #5, Line 275: except TRST
Yes SRST and TMS are also high during init. […]
Looks good, thank you :)
To view, visit change 67878. To unsubscribe, or for help writing mail filters, visit settings.