Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/41532 )
Change subject: raiden_debug_spi.c: Refactor to support multiple protocols ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@266 PS3, Line 266: int16_t max_spi_write_count; : uint16_t max_spi_read_count;
I'm describing a bug in flashrom, the one that this block comment talks about: […]
By flashrom drivers I believe you are talking about spi masters? In that case, it is up to them how they interpret as they are implementation specific so whatever they happen to do is basically irrelevant here. Only what the core logic in spi[25].c really applies here.
This sounds like the maximum USB packet size with maximum spi transaction size concepts are getting muddled together.
https://review.coreboot.org/c/flashrom/+/41532/3/raiden_debug_spi.c@813 PS3, Line 813: configure_protoco
Are you suggesting copying the data from 'spi_master_raiden_debug' into a malloc'd buffer. […]
No. Here is a two things to consider;
First, whatever happens in other spi masters isn't relevant to here as they are specific to their use-case and only the core flashrom logic matters. The values here are just bounds for sanity checking, I don't think you need too much complexity.
The first step is making the raiden_debug_detect_protocol_rev() helper function that isolates the rev detection, where and how bounds is determined is a separate problem.
The other thing to consider is to perhaps think about dispatch in this way,
``` static struct spi_master spi_master_raiden_debug_v1 = { .max_data_read = XXX, .max_data_write = XXX, ... };
static struct spi_master spi_master_raiden_debug_v2 = { .max_data_read = YYY, .max_data_write = YYY, ... };
init() { ... switch (raiden_debug_detect_protocol_rev()) { case PROTOCOL_V1: register_spi_master(&spi_master_raiden_debug_v1); } ```
Then either _v1 or _v2 struct members like `.command = send_command` have wrapper functions around the canonical path that deal with the specialization, if say v2 has somehow dynamic bounds.