Attention is currently required from: Yu-Ping Wu. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56543 )
Change subject: helpers: Add BIT_MASK macro ......................................................................
Patch Set 4:
(3 comments)
File src/commonlib/bsd/include/commonlib/bsd/helpers.h:
https://review.coreboot.org/c/coreboot/+/56543/comment/36a15694_e390b268 PS4, Line 59: * Separate the case of high >= 63 to avoid shift-count-overflow error. I don't understand the `high` >= 63... 63 is the highest bit (and this is inclusive), so `high` can't be higher? (If you want to prevent people from writing this wrong, use assert().)
https://review.coreboot.org/c/coreboot/+/56543/comment/6fbd92e4_a1e7c1ba PS4, Line 61: #define BIT_MASK(high, low) \ Don't we just want to call this GENMASK? While I don't think that name is great, Linux has already established that and we keep getting code ported over that uses it, so I think in that case just being consistent with the rest of the industry would be worth it. (If you do, please replace the existing definitions of GENMASK in src/soc/qualcomm/sc7180/include/soc/qcom_qup_se.h and src/soc/mediatek/common/pll.c with this global one.
File tests/commonlib/bsd/helpers-test.c:
https://review.coreboot.org/c/coreboot/+/56543/comment/e1342fd0_13ea36c7 PS4, Line 13: 4, 5 For invalid cases like this, I'd also prefer an assert() (unless you have practical examples why this would be desirable).
-
Julius Werner (Code Review)