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: Code-Review+2
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?
No, absolutely not. `device.c` prints error messages for missing `.read_resources` and `.set_resources`. It makes some sense for PCI devices, for instance, but not always. Hence there are no-op implementations to avoid the error messages. That's AFAICT the one and only reason for the no-ops.
The idea to have individual names was to bring some explicitness back that was lost with the introduction of DEVICE_NOOP: When somebody uses noop_read_resources() that means they decided not to implement `.read_resources`. And it wouldn't spread to other fields anymore, e.g. a `.init = noop_read_resources` should smell weird even for a lazy author/reviewer.
I should have been clearer about that in the commit message (just didn't expect to document coreboot history for a cosmetic change).
gotcha Nico, thanks for taking the time to explain I appreciate it. gotcha Nico, thanks for taking the time to explain I appreciate it. As you noted below, code in `device/device.c` is barely documented in commit messages so adding more commits without intent in their messages continues that trend. As you highlighted above, the code is indeed old thus my questions.
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,
That was not my intention. Sorry if I gave you the impression. I meant it literally, there are very few people left who feel confi- dent about changes in `device/device.c`. The code is very old, barely documented by commit messages. It's especially hard to decide about cosmetic changes when one doesn't know the intentions of the original authors. So I avoided changes there because I have little time to waste. Actually, I've already wasted too much on this change, so this comment will be my last action on it (if somebody feels like it, they can take over, otherwise I will abandon it after a while).
Yes ok, let's get this done that's fair.