Attention is currently required from: Arthur Heymans, Christian Walter, Dinesh Gehlot, Elyes Haouas, Eran Mitrani, Jeff Daly, Johnny Lin, Kapil Porwal, Subrata Banik, Tarun, Tim Chu, Vanessa Eusebio.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80193?usp=email )
Change subject: soc/intel: Use write{64,32,16,8}p and read{64,32,16,8}p ......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1: Is this really a good idea? I think we intentionally made the `write32()` function take a `void *` (and not just be a macro that includes the cast automatically, as I believe it actually was at some point in the early days until it was eventually cleaned up). Allowing raw integers to be thrown around as addresses increases the potential for errors. If we now introduce a new macro that does the cast, even if it has a different name, I think we are weakening that type safety by giving authors who don't like to separate types cleanly an easy and convenient crutch to lean on.
Basically, I think when you're trying to call write32() with an integer address then in 99% of the cases that means you already made a mistake beforehand, and should've been keeping that address in a pointer variable instead. Obviously not all our code is written perfectly (especially some of the older stuff... e.g. Braswell isn't exactly recent code), and we aren't always there to insist on perfect type hygiene in every review, but forcing people to write a cumbersome type cast on every call if they use integers to hold addresses acts as a sort of forcing function that helps automatically push them towards cleaner ways to write things. That's why I don't believe we should be proliferating macros that make the "bad" way to write things easier. (In fact, I'm not sure how that `write32p()` function of families came into the code base, I'd suggest we get rid of it again.)
-
Julius Werner (Code Review)