Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30209 )
Change subject: soc/intel/common/block/lpc: create LPC_GET_DEV macro ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/30209/2/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/lpc_lib.c:
https://review.coreboot.org/c/coreboot/+/30209/2/src/soc/intel/common/block/... PS2, Line 29: #if !defined(__SIMPLE_DEVICE__) : #include <device/device.h> : #define LPC_GET_DEV pcidev_on_root(PCH_DEV_SLOT_LPC, 0x0) : #else : #define LPC_GET_DEV PCH_DEV_LPC : #endif
Compiles fine as by Aamir's suggestion, btw.
yes compilation will work as long as #define __SIMPLE_DEVICE__ is there.
No, I meant this patch but with PCH_DEV_LPC from soc/pci_devs.h instead of your new macro works fine.
Do you mean PCH_DEV_LPC from soc/pci_devs.h will work without having #define __SIMPLE_DEVICE__ at line 18?
Yes, if you do the same, related modifications below, i.e. remove the pci_devfn_t declarations.
But i don't know why we have this guard at first place and want to compile this entire file as part of simple device. is that saving anything ? general query.
It provides a stable API even if the file is compiled for ramstage and pre-ram stages.
Intention is not to break the stability here for sure, just wondering if you have real reason why #define __SIMPLE_DEVICE__ been added at first place ?
Let me rephrase:
If we #define __SIMPLE_DEVICE__ on top of a .c file. All included headers will behave the same, no matter if we compile for ramstage or not. That's what I called `stable API`. The same API for ramstage and others.
If we don't #define __SIMPLE_DEVICE__ explicitly, it's still defined by the build system for pre-ram stages. Then the #if in header files apply in one way or the other depending on if we build for ramstage or not. This was originally intended for the headers files in src/ include/device/.
So, the explicit define (as it was done here) makes it easier to compile the same .c file for both ramstage and pre-ram stages _without_ further tricks (like #if in include/soc/pci_def.h). It makes, however, usage of `struct device` harder.
The way include/soc/pci_def.h does it, with the #if, has led to many errors in the past (mostly due to missing null-checks). Hence, I call it discouraged.
Hence, avoids the need for any #if in platform code.
FYI, this is soc code not the platform code. hatch, poppy are the platform code in my understanding. this is soc library. i was adhering old review comments from Arthur who has suggested to have a macro rather function names.
Sorry, I don't know how you call things. To me, a platform in this context is chipset + CPU. What I actually meant is #if outside of src/include/device/.
About Arthur's comments, they were for patch set 1 which looks much differently. Also, the original problem patch set 1 tried to solve was fixed in the meantime. There was
#ifdef __SIMPLE_DEVICE__ pci_devfn_t dev; #else struct device *dev; #endif
earlier, but isn't anymore. Which explains the commit message, maybe.
Maybe let's back up and start with the problem you are trying to solve? Is there anything you want to add to / change in this file that isn't possible because of the `#define __SIMPLE_DEVICE__`?
because you can't use them anymore to get a `struct device *`. That's why there are some explicit calls to pcidev_on_root() below.
yes, that's the point. this could have been avoided but it has some other assumptions :) about "struct device *"
Why avoid it? The code is more explicit this way. It's clear to the reader that a pointer is involved, which makes review and maintenance easier.
-
Nico Huber (Code Review)