Attention is currently required from: Subrata Banik, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62894 )
Change subject: ichspi: Rename HSFC_FDBC -> HSFC_FDBC_MASK ......................................................................
Patch Set 6:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/62894/comment/8b81f15d_a07c4761 PS6, Line 10: during the data portion of the SPI cycle.
What's the reasoning for this change? The commit message doesn't explain *why* this change would b […]
This sounds a bit like a personal preference to name macros. I'm usually in favor to allow authors some flexibility to give the code their own touch and to show respect to the original author(s) by not changing the style to ones own taste.
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/62894/comment/da5a71c4_9e4233df PS4, Line 508: FDBC
Hmmm, if the new macro name doesn't work nicely with the existing code, maybe it wasn't meant to […]
Subrata, as it's a macro, you can always simply run the file through a preprocessor to test if the output changes.
The original use of _pprint_reg() is flawed, though. Anastasia is right about the bit vs. reg parameter. As it's very error-prone, how about we get rid of _pprint_reg() again and add an optional argument (for the PCH100_ prefix) to pprint_reg() instead. It seems possible, didn't try.
-
Nico Huber (Code Review)