Re: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 01/12] MdeModulePkg/PciBusDxe: Setup for PCI Express features

["Ni, Ray" <ray.ni@intel.com>]

> -----Original Message-----
> From: Javeed, Ashraf <ashraf.javeed@intel.com>
> Sent: Monday, February 10, 2020 4:26 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 01/12] MdeModulePkg/PciBusDxe: Setup for PCI
> Express features
> 
> My comments below.
> 
> Thanks
> Ashraf
> 
> > -----Original Message-----
> > From: Ni, Ray <ray.ni@intel.com>
> > Sent: Monday, February 10, 2020 12:50 PM
> > To: Javeed, Ashraf <ashraf.javeed@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> > Subject: RE: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 01/12]
> > MdeModulePkg/PciBusDxe: Setup for PCI Express features
> >
> > > > +  PCI_CAPABILITY_PCIEXP                     PciExpressCapabilityStructure;
> > 1. To Align with existing field "Pci", how about rename it to
> >    "PciExpressCapability" (no "Structure" suffix)?
> >
> As per PCI Express Base Specification, the feature name is "PCI Express Capability Structure" and I have used the same type
> although the data structure defined for the same has not used it. Is this really important that I have to remove the
> "Structure" from its name?
I prefer to use shorter name if possible and it also aligns to the existing code style.
Is it really that important that you keep the "Structure" to align to the PCIE spec? 😊

> >
> > 2. I see that only GetPciExpressProtocol() in PciPlatformSupport.c is used in this
> > patch.
> >     All other functions in PciFeatureSupport.c andPciPlatformSupport.c are not
> > used.
> >     It makes the reviewers confused about how those unused functions can be
> > used.
> >     You should remove these unused functions in this patch and add them in later
> > patches
> >     when the code logic calls them.
> >
> All the other main routines of the PciPlatformSupport.c are called from the code of PciFeatureSupport.c.
> All the routines of the PciFeatureSupport.c are called from within itself, and its main routine that defines the integration
> of PCI Express feature initialization is defined in the following second patch.
> If required, I can combine both the patches into one patch, which gives the basic structure of the PCI Express feature
> initialization in the PciBusDxe.
Are all the routines in the two C files called from the main routine in PciBus driver?
The answer with this patch is NO.
The guideline is one patch serves one purpose and it's better to be as small as possible so that the change can be easier to review.
With this guideline, combining both patches to one big one is not preferred.

> 
> > Thanks,
> > Ray