Daniel Gröber (dxld) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42115 )
Change subject: spi/winbond: Pull out winbond_region_to_bpbits function ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42115/2/src/drivers/spi/winbond.c File src/drivers/spi/winbond.c:
https://review.coreboot.org/c/coreboot/+/42115/2/src/drivers/spi/winbond.c@2... PS2, Line 214: static int winbond_region_to_bpbits(
Same here.
Not sure what you mean here? The bit about forward-declared static fns? Same answer then :)
https://review.coreboot.org/c/coreboot/+/42115/2/src/drivers/spi/winbond.c@4... PS2, Line 467: ret = winbond_get_bpbits(flash, &bpbits);
Why the get_bpbits() here? Aren't you going to overwrite the whole thing anyway?
We're doing a read-modify-write cycle on the bpbits because the region_to_bpbits function doesn't necessarily fill-in everything (in the SPI_WRITE_PROTECTION_PRESERVE case).
https://review.coreboot.org/c/coreboot/+/42115/2/src/drivers/spi/winbond.c@4... PS2, Line 497: val.reg1.bp3.srp0 = bpbits.winbond.srp0;
Why is this down here and not with the others above?
If you look at the removed lines just below, just after the `switch (mode) { block you'll see that it's simply the order it was in before :)
I suppose there's no technical reason to keep it this way, we could move the {val,mask}.reg2 init above the `if` and merge the two `if` blocks. On the other hand there's already a lot of numbers flying around here which is confusing enough so I kind of prefer doing it in this "first reg1, then reg2" sort of way.