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 GENMASK and GENMASK_ULL macros ......................................................................
Patch Set 5:
(5 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/56543/comment/77446275_bb79f549 PS5, Line 12: helpers.h, for unsigned long and unsigned long long types, respectively. Do we really need two helpers? What happens if you just make it ULL to begin with, does it throw warnings or something?
Patchset:
PS5: Just to be clear I only meant to suggest standardizing on the general name (GENMASK), as long as we're generally compatible we don't have to copy the Linux code 1-to-1 (e.g. I don't think we need a separate _ULL version unless it's really necessary).
File src/commonlib/bsd/include/commonlib/bsd/helpers.h:
https://review.coreboot.org/c/coreboot/+/56543/comment/c8d7d7e3_42617fc6 PS5, Line 65: #define GENMASK_ULL(h, l) (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h)))) This file is BSD-licensed, so please don't copy Linux code wholesale (including the comment) in here (or did you also find this code under BSD somewhere else?). I mean there are only so many ways to write the same simple operation, but let's at least try to follow the rules as much as possible.
File src/soc/mediatek/common/pll.c:
https://review.coreboot.org/c/coreboot/+/56543/comment/5b6494f6_c273c414 PS5, Line 3: #include <commonlib/bsd/helpers.h> Prefer to just #include <types.h> instead of including this directly.
File tests/commonlib/bsd/helpers-test.c:
https://review.coreboot.org/c/coreboot/+/56543/comment/76104ba0_3cf6270b PS4, Line 13: 4, 5
Adding an assertion seems to make the macro unnecessarily complicated. […]
Yeah I don't think an assertion is needed, just wanted to bring up all options.