Attention is currently required from: ZhiYuanNJ.
Nicholas Chin has posted comments on this change by ZhiYuanNJ. ( https://review.coreboot.org/c/flashrom/+/82776?usp=email )
Change subject: ch347_spi: Add spi clock frequency selection ......................................................................
Patch Set 4:
(8 comments)
Patchset:
PS4: Sorry for not reviewing this earlier; I was busy with other things and missed this patch.
File ch347_spi.c:
https://review.coreboot.org/c/flashrom/+/82776/comment/8a5c9cb7_6dc6e609?usp... : PS4, Line 56: speed I think `divisor` might be a better term here, as the values passed to the CH347 divide the reference clock by 2^divisor_value. These values are passed as the second argument for the `ch347_spi_config`, and the function signature for it already names it as the divisor: `ch347_spi_config(struct ch347_spi_data *ch347_data, uint8_t divisor)`
https://review.coreboot.org/c/flashrom/+/82776/comment/08517c1e_5dbee440?usp... : PS4, Line 72: 0x0 There isn't really a need to write the speeds in hexadecimal, so just drop the `0x` prefix
https://review.coreboot.org/c/flashrom/+/82776/comment/cca817da_929a6b8a?usp... : PS4, Line 287: 0x0 Just use decimal ```suggestion int spispeed = 0; /* defaulet 60M SPI */ ``` Also, in my experience it's quite difficult to get SPI flash chips working at 60 MHz, which is why I had hard coded a divisor of 2 (15 MHz) in the original code. It seems like flashrom only uses the standard JEDEC read (0x03) command, and many flash chips only support a maximum of 50 MHz or lower for standard reads.
https://review.coreboot.org/c/flashrom/+/82776/comment/1f1059ee_9931a252?usp... : PS4, Line 287: defaulet default
https://review.coreboot.org/c/flashrom/+/82776/comment/a7f95cef_2355c2f9?usp... : PS4, Line 365: defaul default
https://review.coreboot.org/c/flashrom/+/82776/comment/e8e6089d_c45d23b9?usp... : PS4, Line 365: M I would use MHz for more clarity instead of just M
https://review.coreboot.org/c/flashrom/+/82776/comment/57551249_47459730?usp... : PS4, Line 365: (60M clock spi) I don't think the parenthesis really add any value here; I would just drop them.
-
Nicholas Chin (Code Review)