Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35463 )
Change subject: device/mmio.h: Add bit field helpers ......................................................................
Patch Set 10:
(5 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 standardiz […]
I feel for definition, it was more clear to call it BITFIELD; and for set/write/read, it's clear enough we're dealing with fields in regs.
But if you think FIELD is still clear enough, then yes we may unify them.
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?
Done
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 th […]
well there's currently no clrsetbits32.
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. […]
Done
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 […]
Done