9 comments:
Patch Set #3, Line 10: The changes include:
These are essentially each their own commit to make this properly reviewable.
Patch Set #3, Line 12: Setting of USB SPI limits based on the device's USB protocol
commit 1.
* Creation of context states to keep track of bytes written to
and read from the device and individual USB packets.
commit 3?
* Creation of helper functions for handling transfer of data
between the USB packets and the data buffers.
commit 2?
* Removal of magic numbers on the USB return codes. Some renaming
of to make the protocol version clear.
commit 0.
int16_t max_spi_write_count;
uint16_t max_spi_read_count;
These are directly replacing 2 constants that we've been using: […]
When you say "other parts interpret" can you be a bit more concrete. I am unclear if you are actually describing a bug here or not. However this looks to me like you are working around one or two issues:
1.) Programmatically changing the rw max count depending on protocol detected instead of detecting and selecting from declarative data in struct's (the spi_master struct instances).
2.) Some issues around max not respecting the header being included or not, in which case that just sounds like a bug that should be addressed.
I don't think having more local state in the spi master that replicated what should be core logic to flashrom to work around an identified bug is the right maintainable path forwards unless I have miss-understood something here?
My goal has been to indent with tabs and align with spaces. […]
I suggest just sticking to the style already inherent in the file. Flashrom unfortunately doesn't have a clang-format file and it has been an area of discussion many times however the general style follows that of coreboot if in unsure.
Regardless Angel is correct in his suggestions here.
Patch Set #3, Line 495: status == 0
`if (!status)` is canonical.
Patch Set #3, Line 813: configure_protoco
We need to communicate with the device in order to identify how large of a SPI transfer is supported […]
I am not saying to not communicate with the device to interrogate what it supports however the results of that interrogation should be to select the correct declarative state rather than programmatically mutating localised singleton state.
This is precisely what lead to the cros_ec spi master becoming unmaintable and so un-upstreamable until it is rewritten. It had tried to shove all these different chip support paths via the one entry-point and initial state then later tries to fix things up at run-time programmatically with quarks.
To view, visit change 41532. To unsubscribe, or for help writing mail filters, visit settings.