Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40207 )
Change subject: Replace DEVICE_NOOP with noop_(set|read)_resources ......................................................................
Patch Set 1:
Patch Set 1:
I believe that using uppercase macros would highlight the special status of these ops
But why highlight a special status that we don't understand? I know I probably should just push a patch that removes the whole topic (error printing for these particular two function pointers). But I know it's
It is hard to determine what your intent is without the complete story. So what you wish to do with these two inlines is to print errors in them and hence the two different symbol names so __func__ lets the error str know who dispatched. Am I understanding you correctly? That would change my view from the original without that extra context.
even less likely to find somebody who would feel confident enough to give a +2. Hence, taking baby steps here.
This sounds a bit ad hominem Nico, I am more than willing to +2 your patches as they are usually extremely well thought out, I am sure you know this! The only reason I didn't +2 the prior patch to this was because Angel just has a few unresolved comments otherwise that one looks good.
Yes, I am less keen on this change because that is precisely why I used an uppercased macro.
Ack. Please clear this up for us, why should these no-op pointers stick out? And what is the intended effect when they stick out?
In my experience, people see such MACRO_NAMES as boilerplate, something to copy-paste without thinking.
Yes, I agree coreboot does suffer from that infliction :/ However I guess that point can slip either way across the spectrum of style and thought process.
It is very easy and obvious to parse by eye or grep for, you certainly can't miss it.
Yes, obviously. But missing that there _is_ a no-op pointer is not a problem. Problems arise when we miss to set any pointer at all, and that is hard to see. Absence of upper-case looks the same as absence of lower case.
Should then the dispatch logic be checking this and not relying on anything particular struct members being packed (ones that print certain things)? That seems like a better solution and more inline with what your thoughts were above.
What are we actually fixing here, just style?
See commit message.
ACK