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 8: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h File src/include/device/mmio.h:
https://review.coreboot.org/c/coreboot/+/35463/5/src/include/device/mmio.h@1... PS5, Line 137: #define GET_BITFIELD(value, name) \
Done with WRITE32_FIELDS and READ32_FIELD. […]
Hmm... the SET32/GET32/EXTRACT also makes a lot of sense to me and is shorter. But sounds like you changed your mind now?
One more thing I thought of now is that sometimes you know you don't need to preserve any data (e.g. because you're initializing a controller you just reset), so you don't need to do a full read-modify-write... but you're still writing fields in the end, so using this API would still be nice. Do we maybe want to cover that case as well? Maybe use SET32 for the read-modify-write and WRITE32 for a simple write to the whole register (then the macro names are also closer to the MMIO primitives they're based on)?
I'm okay if you want to merge it like this, but I think going to the SET32/GET32 model (with potentially also a WRITE32 option) would also be a good option. I'll leave it up to you.