Name of user not set #1002476 has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36184 )
Change subject: arch/acpi.h: Use of typedef for acpi_vfct_image_hdr ......................................................................
arch/acpi.h: Use of typedef for acpi_vfct_image_hdr
Use of typedef and modify the usage accordingly.
Change-Id: I65581702a60dbd286cb3910c6eeef5f9e1853cf1 Signed-off-by: Himanshu Sahdev himanshusah@hcl.com --- M src/arch/x86/include/arch/acpi.h M src/device/pci_rom.c 2 files changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/36184/1
diff --git a/src/arch/x86/include/arch/acpi.h b/src/arch/x86/include/arch/acpi.h index 6cd4e9f..c3420f8 100644 --- a/src/arch/x86/include/arch/acpi.h +++ b/src/arch/x86/include/arch/acpi.h @@ -251,7 +251,7 @@ } __packed acpi_madt_t;
/* VFCT image header */ -struct acpi_vfct_image_hdr { +typedef struct acpi_vfct_image_hdr { u32 PCIBus; u32 PCIDevice; u32 PCIFunction; @@ -262,7 +262,7 @@ u32 Revision; u32 ImageLength; u8 VbiosContent; // dummy - copy VBIOS here -} __packed; +} __packed acpi_vfct_image_hdr_t;
/* VFCT (VBIOS Fetch Table) */ struct acpi_vfct { @@ -271,7 +271,7 @@ u32 VBIOSImageOffset; u32 Lib1ImageOffset; u32 Reserved[4]; - struct acpi_vfct_image_hdr image_hdr; + acpi_vfct_image_hdr_t image_hdr; } __packed;
typedef struct acpi_ivrs_info { diff --git a/src/device/pci_rom.c b/src/device/pci_rom.c index 01c02e5..b2b2266 100644 --- a/src/device/pci_rom.c +++ b/src/device/pci_rom.c @@ -200,7 +200,7 @@ pci_rom_acpi_fill_vfct(struct device *device, struct acpi_vfct *vfct_struct, unsigned long current) { - struct acpi_vfct_image_hdr *header = &vfct_struct->image_hdr; + acpi_vfct_image_hdr_t *header = &vfct_struct->image_hdr; struct rom_header *rom;
rom = check_initialized(device);
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36184 )
Change subject: arch/acpi.h: Use of typedef for acpi_vfct_image_hdr ......................................................................
Patch Set 1: Code-Review-1
Why? Using struct directly (some times with const) is well established within coreboot. I can point you hundred of instances. I can't remember a single instance (except vendor code) where a structure is made a typedef.
Name of user not set #1002476 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36184 )
Change subject: arch/acpi.h: Use of typedef for acpi_vfct_image_hdr ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-1
Why? Using struct directly (some times with const) is well established within coreboot. I can point you hundred of instances. I can't remember a single instance (except vendor code) where a structure is made a typedef.
Thanks for your review and comments!!! I agree with you but after analyzing this file "/coreboot/src/arch/x86/include/arch/acpi.h", since almost all other struct defined in this file has used typedef that is the reason I have tried to provide the same kind of coding style as a base. Also, the typedef would be better for readibility purpose.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36184 )
Change subject: arch/acpi.h: Use of typedef for acpi_vfct_image_hdr ......................................................................
Patch Set 1: Code-Review+2
Patch Set 1:
Patch Set 1: Code-Review-1
Why? Using struct directly (some times with const) is well established within coreboot. I can point you hundred of instances. I can't remember a single instance (except vendor code) where a structure is made a typedef.
Thanks for your review and comments!!! I agree with you but after analyzing this file "/coreboot/src/arch/x86/include/arch/acpi.h", since almost all other struct defined in this file has used typedef that is the reason I have tried to provide the same kind of coding style as a base. Also, the typedef would be better for readibility purpose.
After reading the whole file, I have to agree with you, though I don't like the different style from other files. It is however a more recent change, so whoever added it followed standard style instead of following file specific style, thus validating your analysis.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36184 )
Change subject: arch/acpi.h: Use of typedef for acpi_vfct_image_hdr ......................................................................
arch/acpi.h: Use of typedef for acpi_vfct_image_hdr
Use of typedef and modify the usage accordingly.
Change-Id: I65581702a60dbd286cb3910c6eeef5f9e1853cf1 Signed-off-by: Himanshu Sahdev himanshusah@hcl.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/36184 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/arch/x86/include/arch/acpi.h M src/device/pci_rom.c 2 files changed, 4 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Richard Spiegel: Looks good to me, approved
diff --git a/src/arch/x86/include/arch/acpi.h b/src/arch/x86/include/arch/acpi.h index 6cd4e9f..c3420f8 100644 --- a/src/arch/x86/include/arch/acpi.h +++ b/src/arch/x86/include/arch/acpi.h @@ -251,7 +251,7 @@ } __packed acpi_madt_t;
/* VFCT image header */ -struct acpi_vfct_image_hdr { +typedef struct acpi_vfct_image_hdr { u32 PCIBus; u32 PCIDevice; u32 PCIFunction; @@ -262,7 +262,7 @@ u32 Revision; u32 ImageLength; u8 VbiosContent; // dummy - copy VBIOS here -} __packed; +} __packed acpi_vfct_image_hdr_t;
/* VFCT (VBIOS Fetch Table) */ struct acpi_vfct { @@ -271,7 +271,7 @@ u32 VBIOSImageOffset; u32 Lib1ImageOffset; u32 Reserved[4]; - struct acpi_vfct_image_hdr image_hdr; + acpi_vfct_image_hdr_t image_hdr; } __packed;
typedef struct acpi_ivrs_info { diff --git a/src/device/pci_rom.c b/src/device/pci_rom.c index 01c02e5..b2b2266 100644 --- a/src/device/pci_rom.c +++ b/src/device/pci_rom.c @@ -200,7 +200,7 @@ pci_rom_acpi_fill_vfct(struct device *device, struct acpi_vfct *vfct_struct, unsigned long current) { - struct acpi_vfct_image_hdr *header = &vfct_struct->image_hdr; + acpi_vfct_image_hdr_t *header = &vfct_struct->image_hdr; struct rom_header *rom;
rom = check_initialized(device);
-
Name of user not set (Code Review) -
Patrick Georgi (Code Review)
-
Richard Spiegel (Code Review)