Attention is currently required from: Anastasia Klimchuk, Peter Marheine.
Matti Finder has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84934?usp=email )
Change subject: rpmc: add rpmc commands feature ......................................................................
Patch Set 4:
(7 comments)
Patchset:
PS3:
Given that you agreed in the other comment, you can add yourself in MAINTAINERS file and include in […]
Done
File rpmc.c:
https://review.coreboot.org/c/flashrom/+/84934/comment/1d16add1_cfaf8bb3?usp... : PS3, Line 131: Unsupported
Maybe `Unknown` instead of `Unsupported`, because it's not even in our enum.
Done
https://review.coreboot.org/c/flashrom/+/84934/comment/1d4bee90_291806c2?usp... : PS3, Line 223: {
You don't need { } when the body of if conditional is only one line. […]
Done
https://review.coreboot.org/c/flashrom/+/84934/comment/d9dba094_29df7b76?usp... : PS3, Line 224: return -1;
I am adding comment here, as this is an example, but it applies to other code in this file. […]
My original of how to handle the return values wasn't that good, and just lead to a lot of questions. I have now switched it over to using a rpmc_result enum. Let me know if that makes the return values better to understand
File sfdp.c:
https://review.coreboot.org/c/flashrom/+/84934/comment/b066ede5_2b8a2ec1?usp... : PS3, Line 446: hdrs[i].id
Can these header IDs ever repeat from one header to the other? (probably not?) […]
No these ids are meant to be unique.
From how I understand the original code. We query the device which maps these tables somewhere in the flash memory. Since page 0 is mandatory it is always the first one that is read. The ones after that are device specific. So there is only a relation for the first one.
https://review.coreboot.org/c/flashrom/+/84934/comment/064aff5a_cd306c1a?usp... : PS3, Line 449: msg_cdbg("The chip contains an unknown " : "version of the JEDEC flash " : "parameters table, skipping it.\n");
I know it was like this before, but while you are here... maybe you can add `hdrs[i]. […]
Done
https://review.coreboot.org/c/flashrom/+/84934/comment/4aded69f_151b7de2?usp... : PS3, Line 461: 1
It's in hex few lines above (0x01 on line 448) and here is decimal. […]
Done