[coreboot-gerrit] Change in coreboot[master]: intelblocks/pci_dev: Create header for pci devices

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/18576 )

Change subject: intelblocks/pci_dev: Create header for pci devices
......................................................................


Patch Set 13:

> > > (1 comment)
 > > >
 > > > So we've saved 12 lines of duplicated code in doing all of
 > this?
 > > Is
 > > > that worth the trouble?
 > >
 > > its not only about 12 line, it would rather see this as one step
 > to
 > > use common macro name for PCI device between small and big core
 > and
 > > use the one standard, if you see earlier they both never look
 > same,
 > > although internally does the same PCI_DEV refer.
 > 
 > How's that going to be done when the pci device numbers, functions,
 > and names are SoC dependent? Using a common naming strategy is
 > orthogonal to using a shared a header file. They are 2 separate
 > things. 

 > Common naming can be a requirement for the common block
 > implementations. 
Yes, common naming between big core and small core was requirement hence we have tried to streamline those soc header. if your only concern to have common header to hold those "12" line of code? then i will move those below lines into respective soc/pci_devs.h. is that okay?

#include <device/pci_def.h>
#include <rules.h>

#define _SA_DEVFN(slot)		PCI_DEVFN(SA_DEV_SLOT_ ## slot, 0)
#define _PCH_DEVFN(slot, func)	PCI_DEVFN(PCH_DEV_SLOT_ ## slot, func)

#if !defined(__SIMPLE_DEVICE__)
#include <device/device.h>
#include <device/pci_def.h>
#define _SA_DEV(slot)		dev_find_slot(0, _SA_DEVFN(slot))
#define _PCH_DEV(slot, func)	dev_find_slot(0, _PCH_DEVFN(slot, func))
#else
#include <arch/io.h>
#define _SA_DEV(slot)		PCI_DEV(0, SA_DEV_SLOT_ ## slot, 0)
#define _PCH_DEV(slot, func)	PCI_DEV(0, PCH_DEV_SLOT_ ## slot, func)
#endif


It does not require sharing a header file for 12
 > lines of boiler plate.
 > 
 > If you look at the exposed API you are expecting its some like
 > PCH_PMC_DEV, e.g. That's just a name the code is expecting.

-- 
To view, visit https://review.coreboot.org/18576
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5e4c7502e9678c0a367e9c7a96cf848d5b24f68e
Gerrit-PatchSet: 13
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan at intel.com>
Gerrit-Reviewer: Barnali Sarkar <barnali.sarkar at intel.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams at intel.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Pratikkumar Prajapati <pratikkumar.v.prajapati at intel.corp-partner.google.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-Reviewer: dhaval v sharma <dhaval.v.sharma at intel.com>
Gerrit-HasComments: No