Patch Set 3:
(10 comments)
The general direction looks good however these changes are *way* way too big to review and definitely can be broken down into sizable chunks that each stand on their own merit.
Totally fair, I can break this down further if we need.
I'd also like to have just a single quick discussion about the tabs/spaces usage here so it's not scattered in 20 messages and multiple CL's. My goal with the indentation has been to use tabs to represent indentation levels and to use spaces whenever we are aligning text, like to the function declarations or as parameters, especially in the multi-line strings. This does result in more characters but it's an indentation / alignment style that is robust to different tab widths.
Same idea as https://dmitryfrank.com/articles/indent_with_tabs_align_with_spaces
11 comments:
Patch Set #3, Line 140: GOOGLE_RAIDEN_SPI_PROTOCOL_V1
probably a enum
Ack
int16_t max_spi_write_count;
uint16_t max_spi_read_count;
the spi master already has these fields, you should fill in the data there.
These are directly replacing 2 constants that we've been using:
We need to make sure that flashrom will not attempt a SPI transfer larger than the platform will support. It looks like some parts of flashrom will treat the max_data_read/write values as the maximum size of the SPI transfer and construct a message smaller than it, other parts interpret that line to mean that's the maximum number of bytes in a message excluding the header meaning you end up with slightly larger packets, I've seen up to 61 bytes.
To get around this issue, we have been telling the rest of flashrom that it's maximum capacity is equal to the maximum SPI packet - JEDEC_BYTE_PROGRAM_OUTSIZE. We've been using 2 values to keep track of these fields before in the form of 'max_data_read = (PAYLOAD_SIZE - JEDEC_BYTE_PROGRAM_OUTSIZE)' but we've been comparing the 'read_count' against PAYLOAD_SIZE.
Can use tabs here
My goal has been to indent with tabs and align with spaces. That style is the most robust to different editor / preferences since changing tab width keeps text in alignment with the functions above that they are using.
This comment applies to this line and many of the other similar lines, although I do have a few lines where I accidentally used spaces for indentation too so those need to be fixed.
\n
Ack
Patch Set #3, Line 448: if (write_count > ctx_data->max_spi_write_count) {
validation should happen before packing structs
Ack
/* We were successful at performing the SPI transfer. */
return status;
This can be moved at line 498: […]
Ack
.max_data_read = 0,
.max_data_write = 0,
not sure why you are not using these fields and putting them into the local state data context?
See comment above near the 'struct raiden_debug_spi_data'
ctx_data->protocol_version =
ctx_data->dev->interface_descriptor->bInterfaceProtocol;
switch(ctx_data->protocol_version) {
this is its own function, `enum protocol_rev_t raiden_debug_detect_protocol_rev(struct usb_device *d […]
Ack
ctx_data->max_spi_write_count= SPI_TRANSFER_V1_MAX;
ctx_data->max_spi_read_count = SPI_TRANSFER_V1_MAX;
have two spi_master_raiden_debug spi_master struct's for each protocol and register the one with the […]
See comment about configure_protocol() from below: Short answer protocol 2 allows SPI transfers much larger than what platforms support and some of our chips have different maximum SPI transfer sizes.
more tabs and spaces mixed throughout this file.
Ack
Patch Set #3, Line 813: configure_protoco
no, just declare the state you want (read/write max_size in this case) for each path and choose the […]
We need to communicate with the device in order to identify how large of a SPI transfer is supported. Some devices like the STM32's used in Servo Micros and C2D2 work for SPI transactions over 256 bytes, others like the Cr50's support 128 bytes. We could do a mapping of the protocol version number at this point to resolve that as a way around this issue:
For instance 1 is the existing path, 2 could be a value over 256 that the STM32F0's support, 3 could be 128 bytes.
It'd be a bit less flexible since changing the packet limit burns another version number, but there's some pros to having a simpler init process.
To view, visit change 41532. To unsubscribe, or for help writing mail filters, visit settings.