Attention is currently required from: ZhiYuanNJ.
5 comments:
Patchset:
Thanks for adding new feature! I gave some comments.
File ch347_spi.c:
Patch Set #7, 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)
Patch Set #7, 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.
Patch Set #7, 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
Patch Set #7, 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?
To view, visit change 82776. To unsubscribe, or for help writing mail filters, visit settings.