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
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.
i'm not very sure about that, Arthur's concern is still active herehttps://github.com/coreboot/coreboot/blob/master/src/soc/intel/common/block/...
Ok, that looks ugly, so? My shot at it: https://review.coreboot.org/c/coreboot/+/38946
and i was coping the same into this file, hence he has suggested to use macros rather function
Are you sure he meant adding macros or maybe using the existing ones?
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__`?
problem is having different macros for "dev". Inside pci_read_config16() function we are using PCH_DEV_LPC and https://review.coreboot.org/c/coreboot/+/30209/2/src/soc/intel/common/block/... here we are using struct device * hence user has to have lots of knowledge about different use case inside croeboot, which i was trying to avoid and provide more abstraction
I agree, but this has to be handled at a much lower level, in src/device/. The coreboot infrastructure wasn't ready for the amount of code sharing between stages as we have it today in soc/intel/. People added it never- theless, so now we sit on a pile of technical debt.