Attention is currently required from: ZhiYuanNJ.
Anastasia Klimchuk 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 7:
(5 comments)
Patchset:
PS7: Thanks for adding new feature! I gave some comments.
File ch347_spi.c:
https://review.coreboot.org/c/flashrom/+/82776/comment/52b187d8_ca5593e1?usp... : PS7, Line 355: arg = extract_programmer_param_str(cfg, "spispeed"); Since you are adding a param, this needs to be explained on manpage for the programmer. Manpage source code is located here: doc/classic_cli_manpage.rst (on the website it looks like this https://flashrom.org/classic_cli_manpage.html#ch347-spi-programmer) You need to update the section for ch347_spi
You can test your changes locally, the information is here: https://flashrom.org/how_to_add_docs.html (or you can view the same doc in the tree in your repo)
https://review.coreboot.org/c/flashrom/+/82776/comment/448ec752_e54e847e?usp... : PS7, Line 357: index = 0 Let's use different variable name, index is already used for indexing the device table. `speed_index` or something like that, but not the same variable name.
As a concrete example how things will go wrong: if spispeed param is not provided, you won't even reach this line of code (because arg is NULL). So your index will be whatever is initialized from device table - which is completely different table.
You need its own variable for spispeed index, give it default value, and use the same default value on line 366 for the case when spispeed param is unknown value.
https://review.coreboot.org/c/flashrom/+/82776/comment/7421829d_502fefb5?usp... : PS7, Line 365: Invalid SPI speed It would be invalid value if you would throw an error and exit, however you do not, but instead continue with default value. So I think the better name is "unknown" instead of "invalid". I suggest
Unknown value of spispeed parameter, using default XXX clock spi
https://review.coreboot.org/c/flashrom/+/82776/comment/5537ffa6_defad3df?usp... : PS7, Line 365: 30MHz It seems you are changing the default, why? Previously the divisor was fixed to 2, which corresponds to 15M with index=2 Now it will be 1, when spispeed value is not recognized?
-
Anastasia Klimchuk (Code Review)