Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36596 )
Change subject: include: introduce update* for mmio operations ......................................................................
Patch Set 5:
(Sorry, I must have missed this going by when it was committed...)
We already have clrsetbits_le32() (and _le16(), etc.) in <endian.h> that have been used for the same purpose all across the code base for years. Why are we adding a new API that covers exactly the same thing? If we don't like that name anymore we can discuss changing it, but I don't think we should have a bunch of different ways to write exactly the same access.
Also, I thought <device/mmio.h> was now supposed to be the new catchall header you should include for MMIO stuff, and that's what most code is including. We've been starting to add more generic MMIO helper functions there already, but now you're introducing this confusingly similarly named file but neither of them includes the other. Shouldn't we keep all the MMIO stuff under one common include point? (Whether that's <mmio.h> or <device/mmio.h> I don't care, but considering that most files already include the latter it would probably be easier to keep it that way.)
(Also, slightly tangential but if you want to write new code doing read-modify-write bitfield accesses in registers you should check out Hung-Te's cool new SET32_BITFIELD() API which can make this sort of stuff look a lot nicer and avoid errors: https://review.coreboot.org/cgit/coreboot.git/tree/src/include/device/mmio.h...)
-
Julius Werner (Code Review)