OK.OK.:) I will generate V3. A improper patch can never escape from your eyes.
Regards, Ray From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo Ersek Sent: Friday, February 19, 2016 6:41 PM To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-de...@ml01.01.org Cc: Tian, Feng <feng.t...@intel.com>; Fan, Jeff <jeff....@intel.com> Subject: Re: [edk2] [Patch V2 1/2] MdeModulePkg/PciBus: Refine to reduce code lines. On 02/19/16 03:18, Ruiyu Ni wrote: > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ruiyu Ni <ruiyu...@intel.com<mailto:ruiyu...@intel.com>> > Cc: Feng Tian <feng.t...@intel.com<mailto:feng.t...@intel.com>> > Cc: Jeff Fan <jeff....@intel.com<mailto:jeff....@intel.com>> > --- > MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c | 70 > ++++++++++++---------------------- > 1 file changed, 24 insertions(+), 46 deletions(-) I think this patch is correct. However, it is very hard to read. I'd like to propose the following two improvements: (1) Please split this patch in two. In the first step, please do not reorganize the control flow, *only* rename the variable. In the second step (a separate patch), the control flow can be reorganized. Both of the patches should point out that they incur no observable change. (2) In the reorganized version, please add // // fall through // comments. Although they are not required by the coding style, I've seen them elsewhere in edk2, and they are quite helpful. Thanks Laszlo > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c > index 4160632..50ed866 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c > @@ -1,7 +1,7 @@ > /** @file > EFI PCI IO protocol functions implementation for PCI Bus module. > > -Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR> > +Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD > License > which accompanies this distribution. The full text of the license may be > found at > @@ -1774,9 +1774,8 @@ PciIoGetBarAttributes ( > OUT VOID **Resources OPTIONAL > ) > { > - UINT8 *Configuration; > PCI_IO_DEVICE *PciIoDevice; > - EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *AddressSpace; > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor; > EFI_ACPI_END_TAG_DESCRIPTOR *End; > > PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This); > @@ -1798,19 +1797,18 @@ PciIoGetBarAttributes ( > } > > if (Resources != NULL) { > - Configuration = AllocateZeroPool (sizeof > (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) + sizeof (EFI_ACPI_END_TAG_DESCRIPTOR)); > - if (Configuration == NULL) { > + Descriptor = AllocateZeroPool (sizeof > (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) + sizeof (EFI_ACPI_END_TAG_DESCRIPTOR)); > + if (Descriptor == NULL) { > return EFI_OUT_OF_RESOURCES; > } > > - AddressSpace = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Configuration; > + *Resources = Descriptor; > > - AddressSpace->Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR; > - AddressSpace->Len = (UINT16) (sizeof > (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3); > - > - AddressSpace->AddrRangeMin = PciIoDevice->PciBar[BarIndex].BaseAddress; > - AddressSpace->AddrLen = PciIoDevice->PciBar[BarIndex].Length; > - AddressSpace->AddrRangeMax = PciIoDevice->PciBar[BarIndex].Alignment; > + Descriptor->Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR; > + Descriptor->Len = (UINT16) (sizeof > (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3); > + Descriptor->AddrRangeMin = PciIoDevice->PciBar[BarIndex].BaseAddress; > + Descriptor->AddrLen = PciIoDevice->PciBar[BarIndex].Length; > + Descriptor->AddrRangeMax = PciIoDevice->PciBar[BarIndex].Alignment; > > switch (PciIoDevice->PciBar[BarIndex].BarType) { > case PciBarTypeIo16: > @@ -1818,59 +1816,41 @@ PciIoGetBarAttributes ( > // > // Io > // > - AddressSpace->ResType = ACPI_ADDRESS_SPACE_TYPE_IO; > - break; > - > - case PciBarTypeMem32: > - // > - // Mem > - // > - AddressSpace->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; > - // > - // 32 bit > - // > - AddressSpace->AddrSpaceGranularity = 32; > + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO; > break; > > case PciBarTypePMem32: > // > - // Mem > - // > - AddressSpace->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; > - // > // prefechable > // > - AddressSpace->SpecificFlag = 0x6; > - // > - // 32 bit > - // > - AddressSpace->AddrSpaceGranularity = 32; > - break; > + Descriptor->SpecificFlag = > EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE; > > - case PciBarTypeMem64: > + case PciBarTypeMem32: > // > // Mem > // > - AddressSpace->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; > + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; > // > - // 64 bit > + // 32 bit > // > - AddressSpace->AddrSpaceGranularity = 64; > + Descriptor->AddrSpaceGranularity = 32; > break; > > case PciBarTypePMem64: > // > - // Mem > + // prefechable > // > - AddressSpace->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; > + Descriptor->SpecificFlag = > EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE; > + > + case PciBarTypeMem64: > // > - // prefechable > + // Mem > // > - AddressSpace->SpecificFlag = 0x6; > + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; > // > // 64 bit > // > - AddressSpace->AddrSpaceGranularity = 64; > + Descriptor->AddrSpaceGranularity = 64; > break; > > default: > @@ -1880,11 +1860,9 @@ PciIoGetBarAttributes ( > // > // put the checksum > // > - End = (EFI_ACPI_END_TAG_DESCRIPTOR *) (AddressSpace + 1); > + End = (EFI_ACPI_END_TAG_DESCRIPTOR *) (Descriptor + 1); > End->Desc = ACPI_END_TAG_DESCRIPTOR; > End->Checksum = 0; > - > - *Resources = Configuration; > } > > return EFI_SUCCESS; > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
