Attention is currently required from: Felix Held.
Martin Roth has posted comments on this change by Felix Held. ( https://review.coreboot.org/c/coreboot/+/84786?usp=email )
Change subject: drivers/spi/spi_flash_sfdp: add basic SFDP support ......................................................................
Patch Set 10: Code-Review+2
(5 comments)
Patchset:
PS10: Just nits. Fix them here, in a follow-on, or don't fix them. Up to you.
Commit Message:
https://review.coreboot.org/c/coreboot/+/84786/comment/ddac3923_a24f19c4?usp... : PS10, Line 10: a : specific Nit: Maybe you only tested on one chip, but this should work with any chip that supports this mechanism (and meets the requirements), right? Maybe "any"?
Add the requirements listed in spi_flash_sfdp.c to the commit message?
File src/drivers/spi/spi_flash_sfdp.c:
https://review.coreboot.org/c/coreboot/+/84786/comment/868e51f9_5ce5ccc4?usp... : PS10, Line 46: 0x50444653 Nit: Use the ascii characters instead, or at least include them as a comment?
https://review.coreboot.org/c/coreboot/+/84786/comment/2a14015f_675c1183?usp... : PS10, Line 158: %#x %p?
https://review.coreboot.org/c/coreboot/+/84786/comment/93814d5e_847ebb83?usp... : PS10, Line 176: if (get_sfdp_header(flash, &sfdp_rev, ¶m_header_count, &access_protocol) != CB_SUCCESS) Does this line need to be wrapped?