[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.
 > 
 > That's precisely my point. You wouldn't have to change anything but
 > provide consistent naming in soc/pci_devs.h.

will address in next patchset.

-- 
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