Nico Huber 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.
Um, all across the code base == some ARM stuff? I think you are exaggerating a bit too much, it's the first time I hear about it. I agree, however, that we shouldn't have competing APIs.
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.)
Might be enough to point out that `device/mmio.h` existed already. I think it was just missed here.
-
Nico Huber (Code Review)