Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44308 )
Change subject: flashrom: Added Support for BoyaMicro BY25Q128AS ......................................................................
Patch Set 1: Code-Review+1
(7 comments)
Welcome!
Overall, looks good, but with some minor nits about formatting.
https://review.coreboot.org/c/flashrom/+/44308/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/44308/1//COMMIT_MSG@7 PS1, Line 7: flashrom: Added Support for BoyaMicro BY25Q128AS A few nits about the commit summary:
- The `prefix` often refers to the touched files. Here, `flashchips` would be good (both flashchips.c and flashchips.h are touched). - Commit summaries are written in imperative tense: `Add support for ...`. - `Support` shouldn't be capitalized. - The vendor's name seems to be `Boya Microelectronics`.
Instead, I would use:
flashchips: Add support for Boya Microelectronics BY25Q128AS
Note that commit summaries never end with a period.
https://review.coreboot.org/c/flashrom/+/44308/1//COMMIT_MSG@9 PS1, Line 9: Tested on Buspirate missing period `.` at the end.
https://review.coreboot.org/c/flashrom/+/44308/1/flashchips.h File flashchips.h:
https://review.coreboot.org/c/flashrom/+/44308/1/flashchips.h@211 PS1, Line 211: These spaces should be a tab (tabs are 8 characters wide in flashrom)
https://review.coreboot.org/c/flashrom/+/44308/1/flashchips.h@212 PS1, Line 212: Same here, spaces
https://review.coreboot.org/c/flashrom/+/44308/1/flashchips.c File flashchips.c:
https://review.coreboot.org/c/flashrom/+/44308/1/flashchips.c@3445 PS1, Line 3445: more spaces
https://review.coreboot.org/c/flashrom/+/44308/1/flashchips.c@3446 PS1, Line 3446: Boya nit: Boya Microelectronics
https://review.coreboot.org/c/flashrom/+/44308/1/flashchips.c@3449 PS1, Line 3449: BOYA_ID the `BOYA_ID` macro name is fine, no need to change it.