Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 9:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h@6... PS9, Line 60: DEFINE_BITFIELD nit: why does this and EXTRACT say BITFIELD(S) when all the others just say FIELD(s)? I'd standardize on FIELD(S) for all of them.
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h@6... PS9, Line 68: * SET32_FIELDS(®, name, value, [name, value, ...]) nit: might be worth adding an explicit sentence explaining what it does to each of these (especially for the SET32/WRITE32 difference).
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h@7... PS9, Line 78: * SET32_FIELDS(&disp_regs.ctrl, DISP_TYPE, 2); nit: weird indentation?
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h@9... PS9, Line 99: * setting up to 8 fields at one invocation. nit: technically, SET32 is LE32 and WRITE32 uses CPU byte order (which is a bit of an artifact of the fact that we only have clrsetbits with explicit byte orders, I think it would make more sense if we just used a clrsetbits32() that used CPU byte order most of the time... but as long as we don't have BE CPUs, and we likely never will, none of this really matters).
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h@1... PS9, Line 106: name##_FIELD_SIZE = (high_bit) - (low_bit) + 1 } nit: for existing macros that you're supposed to call completely out of normal C code context (e.g. the memlayout stuff, or DECLARE_REGION), you're not supposed to call them with semicolons at the end. I think that would probably make sense here as well, so I'd consider putting the semicolon in the macro (although you normally don't do that for stuff that's meant to be used like a C statement... but this isn't).
Also, for optical balancing I'd put the closing brace on a new line.
https://review.coreboot.org/c/coreboot/+/35463/9/src/include/device/mmio.h@1... PS9, Line 158: 4, INVALID, 3, INVALID, 2, INVALID, 1, INVALID) nit: could deduplicate a few more intermediary macros with something like
#define _BF_IMPL2(op, addr, n1, v1, <...etc...>) op(addr, _BF_APPLY##NARGS(_BF_MASK, n1, v1, <...etc...>), _BF_APPLY##NARGS(_BF_VALUE, n1, v1, <...etc...>)) #define _BF_IMPL(op, addr, ...) _BF_IMPL2(op, addr, __VA_ARGS__, 8, INVALID, <...etc...>) #define _WRITE32_FIELDS_IMPL(addr, masks, values) write32(addr, values) #define WRITE32_FIELDS(addr, ...) _BF_IMPL(_WRITE32_FIELDS_IMPL, addr, __VA_ARGS__)