Change in coreboot[master]: soc/intel/common/block/lpc: create LPC_GET_DEV macro

[Nico Huber (Code Review)]

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.