Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36596 )
Change subject: include: introduce update* for mmio operations ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36596/2/src/include/mmio.h File src/include/mmio.h:
https://review.coreboot.org/c/coreboot/+/36596/2/src/include/mmio.h@13 PS2, Line 13:
Should we allow users to expect the indirect inclusion of `arch/mmio.h`? e.g. only include `mmio.h` and then use read32() and update32().
Why wouldn't we expect that? The update functions wouldn't work if this header was included bare without the accompanying #include for readX/writeX().
If so, please leave a comment here that `mmio.h` provides read*() and write*() as well.
https://review.coreboot.org/c/coreboot/+/36596/1/src/include/mmio.h File src/include/mmio.h:
https://review.coreboot.org/c/coreboot/+/36596/1/src/include/mmio.h@19 PS1, Line 19: static __always_inline void update8(const volatile void *addr, uint8_t mask, uint8_t or)
For the record, I'm ok with the short names. It seems obvious what these […]
now I hate readX/writeX being bare w/o a namespace. :) But if we want the short form I can't argue against readX/writeX not having one for mmio but of course everything else needs a prefix because of the namespace collisions.
FWIW, I don't think about things an obviousness of what they do in their implementation. It's legibility at the call sites -- helps w/ reading code and reviewing patchsets, imo.