Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35018 )
Change subject: soc/amd/common/block: Create new SPI code ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_flash.c:
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 40: cmd_len--; Seems like a complex way to just write?
`return min((SPI_FIFO_DEPTH - cmd_len - 1), buf_len);`
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 184: if (spi_data_ptr->non_standard != NON_STANDARD_SPI_NONE) isn't this essentially a duplicate check to the branch in non_standard_spi_write() ?
Although, as an aside, I still believe that fch_spi_special.c could ultimately be dedup into this function however I wont block on that.
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... File src/soc/amd/common/block/spi/fch_spi_special.c:
https://review.coreboot.org/c/coreboot/+/35018/7/src/soc/amd/common/block/sp... PS7, Line 117: int ret = 0; not sure if you missed this one but you can inline `non_standard_sst_write_aai()` here and pass `spi_data_ptr` as an argument so you can get rid of these two `get_ctrl_spi_data()` call sites.
``` int non_standard_spi_write(const struct spi_data *spi_data_ptr, uint32_t offset, size_t len, const void *buf) { if (spi_data_ptr->non_standard != NON_STANDARD_SPI_SST) return 0;
[...non_standard_sst_write_aai()...] } ```