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.

10 comments:

File raiden_debug_spi.c:
- Patch Set #3, Line 140: GOOGLE_RAIDEN_SPI_PROTOCOL_V1
  probably a enum
- Patch Set #3, Line 266:
```
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.
- Patch Set #3, Line 438: ;
  \n
- Patch Set #3, Line 448: if (write_count > ctx_data->max_spi_write_count) {
  validation should happen before packing structs
- Patch Set #3, Line 529:
```
.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?
- Patch Set #3, Line 568:
  mixing tabs / spaces
- Patch Set #3, Line 584:
```
	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 *device)`
- Patch Set #3, Line 592:
```
			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 correctly packed data.
- Patch Set #3, Line 620:
  more tabs and spaces mixed throughout this file.
- 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 correct state to register as the spi master after a simple detection function.

To view, visit change 41532. To unsubscribe, or for help writing mail filters, visit settings.