[S] Change in flashrom[main]: ch347_spi: Add spi clock frequency selection

[Anastasia Klimchuk (Code Review)]

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?



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/82776?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: If2be48929db540a6598ac0b60b37e64597156db7
Gerrit-Change-Number: 82776
Gerrit-PatchSet: 7
Gerrit-Owner: ZhiYuanNJ
Gerrit-Reviewer: Nicholas Chin <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Anastasia Klimchuk <[e-mail]>
Gerrit-Attention: ZhiYuanNJ
Gerrit-Comment-Date: Sat, 29 Jun 2024 08:56:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No