Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47655 )
Change subject: it85spi.c: Refactor singleton states into reentrant pattern ......................................................................
Patch Set 6: Code-Review-1
(3 comments)
https://review.coreboot.org/c/flashrom/+/47655/6/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/47655/6/it85spi.c@308 PS6, Line 308: data->shm_io_base = 0; : data->ce_high = NULL; : data->ce_low = NULL; : data->it85xx_scratch_rom_reenter = 0; : Its a bit of a style thing, but no need to explicitly set these to 0 since: (a) you just calloc'ed the struct. (b) or, you followed my other suggestion and continue to use a static global for this struct, which, by definition, would be 0-initialized.
In this way, the code is simplified by removing boiler-plate, and we can focus the reader on the parts that do matter - instances where the variables are modified away from zero.
https://review.coreboot.org/c/flashrom/+/47655/6/it85spi.c@330 PS6, Line 330: /* These pointers are not used directly. They will be send to EC's : * register for indirect access. */ : base = 0xFFFFF000; : data->ce_high = ((unsigned char *)base) + 0xE00; /* 0xFFFFFE00 */ : data->ce_low = ((unsigned char *)base) + 0xD00; /* 0xFFFFFD00 */ : This part is very similar between the two LPC_* cases and is a good candidate for a small refactor in a different CL.
https://review.coreboot.org/c/flashrom/+/47655/6/it85spi.c@348 PS6, Line 348: return 1; This error path leaks data. I recommend using a goto error pattern in this function to avoid leaks.