From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::241; helo=mail-it0-x241.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x241.google.com (mail-it0-x241.google.com [IPv6:2607:f8b0:4001:c0b::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B91062035B2C5 for ; Sat, 23 Dec 2017 06:11:15 -0800 (PST) Received: by mail-it0-x241.google.com with SMTP id m11so20045521iti.1 for ; Sat, 23 Dec 2017 06:16:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=3AmIJh8vXdwQDNZo3Wmel+2R0k+bVuIo15mW+6U564o=; b=kJBfmADjkXE/MQwUAJtp396LKOeepIrQ4ELZYygy3Icu60MfoI86Y5ax9/F5rCfWFS 4LyF37iwT4L1j28csuL3BIAkL/ZeztczmdM/27zcJ4GlUZCGRBR5lmU4gYXCdomw/ESo cSXGTJalaF2FLpyyfbgaazBxZJV7fvUB2oZTI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=3AmIJh8vXdwQDNZo3Wmel+2R0k+bVuIo15mW+6U564o=; b=A7hV3yWEaGcPfA985dKvCuAoOYpx2nZc1c39K+oqT2VRMRs0nGWWGvcaEVYVluyjO4 ILCG0zkcnOJxsMXom8kYnUZx8brBvqi01sQHnfuMbPXphu0BevtJbfii9oLcxnzqGcdy lP1h8PZrM1DzbD0msv5ljpw0tucHpmD3RlIol6PljTswVS8rnMpGnfHH+7Qv1OgPY2xw PSywMWTwk79LJ/6z4Oy03RcbTRDZlpJ+6zEZ/tCwBpFaKgFtRd4JCXQrF3kHbzw3B7fV NpqHkn0Zl75e/XokBPLW3QIOIfgLZCUzDpPCPCJQnxLTWmc16tKczFHajeG+2SzJCZba 7Svg== X-Gm-Message-State: AKGB3mLE1zWr9Heh6XoV+ID7rzRKwOKSAV3UoHIlgNW+/jKLKjyYjRDD siqfYMnO7HLbLEqNTHjZbyEjMmHnM26wrEPLQHXXZM5x X-Google-Smtp-Source: ACJfBosYUKoyq9NqCyU0USBxGFoWifJF0k6HK2a6zCPDAxOZARrs+3ziFsQGKEg0DAG0nrZjFzgwH3fBEFCRYyM3y2c= X-Received: by 10.36.78.212 with SMTP id r203mr21788307ita.58.1514038566057; Sat, 23 Dec 2017 06:16:06 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.52.14 with HTTP; Sat, 23 Dec 2017 06:16:05 -0800 (PST) In-Reply-To: <20171222190821.12440-10-evan.lloyd@arm.com> References: <20171222190821.12440-1-evan.lloyd@arm.com> <20171222190821.12440-10-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Sat, 23 Dec 2017 14:16:05 +0000 Message-ID: To: Evan Lloyd Cc: "edk2-devel@lists.01.org" , Arvind Chauhan , Daniil Egranov , Thomas Panakamattam Abraham , <"ard.biesheuvel@linaro.org"@arm.com>, <"leif.lindholm@linaro.org"@arm.com>, <"Matteo.Carlini@arm.com"@arm.com>, <"nd@arm.com"@arm.com> Subject: Re: [PATCH edk2-platforms v2 09/18] ARM/VExpressPkg: PL11LcdArmVExpressLib: Improvement conditional X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 23 Dec 2017 14:11:16 -0000 Content-Type: text/plain; charset="UTF-8" On 22 December 2017 at 19:08, wrote: > From: Girish Pathak > > PL111_CLCD_SITE and ARM_VE_MOTHERBOARD_SITE are both constants and > available at build time. Use conditional compilation to process the code > based on the value of PL111_CLCD_SITE, instead of selecting code in a > switch statement at runtime. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Girish Pathak > Signed-off-by: Evan Lloyd Please drop this patch. Replacing C conditionals with CPP conditionals makes the code more difficult to read, and reduces build time coverage. The compiler is perfectly capable of removing the unreachable code (and these are not hot paths anyway) > --- > Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c | 40 +++++++------------- > 1 file changed, 14 insertions(+), 26 deletions(-) > > diff --git a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c > index 92918bb4ee6811db47791a435bc06a6dc77ae9a3..cf50b20fd9b1b44a81963655c2f88305ce6bdc67 100644 > --- a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c > +++ b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c > @@ -206,16 +206,11 @@ LcdPlatformGetVram ( > ASSERT (VramBaseAddress != NULL); > ASSERT (VramSize != NULL); > > - // Is it on the motherboard or on the daughterboard? > - switch (PL111_CLCD_SITE) { > - > - case ARM_VE_MOTHERBOARD_SITE: > +#if (PL111_CLCD_SITE == ARM_VE_MOTHERBOARD_SITE) > *VramBaseAddress = (EFI_PHYSICAL_ADDRESS)PL111_CLCD_VRAM_MOTHERBOARD_BASE; > *VramSize = LCD_VRAM_SIZE; > Status = EFI_SUCCESS; > - break; > - > - case ARM_VE_DAUGHTERBOARD_1_SITE: > +#elif (PL111_CLCD_SITE == ARM_VE_DAUGHTERBOARD_1_SITE) > *VramBaseAddress = (EFI_PHYSICAL_ADDRESS)LCD_VRAM_CORE_TILE_BASE; > *VramSize = LCD_VRAM_SIZE; > > @@ -242,13 +237,9 @@ LcdPlatformGetVram ( > ASSERT (FALSE); > gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize)); > } > - break; > - > - default: > - // Unsupported site > - Status = EFI_UNSUPPORTED; > - break; > - } > +#else > +#error PL111LcdVExpressLib: Unsupported PL111_CLCD_SITE > +#endif // (PL111_CLCD_SITE == ARM_VE_MOTHERBOARD_SITE) > > return Status; > } > @@ -299,18 +290,15 @@ LcdPlatformSetMode ( > return EFI_INVALID_PARAMETER; > } > > - switch (PL111_CLCD_SITE) { > - case ARM_VE_MOTHERBOARD_SITE: > - Function = SYS_CFG_OSC; > - OscillatorId = PL111_CLCD_MOTHERBOARD_VIDEO_MODE_OSC_ID; > - break; > - case ARM_VE_DAUGHTERBOARD_1_SITE: > - Function = SYS_CFG_OSC_SITE1; > - OscillatorId = FixedPcdGet32 (PcdPL111LcdVideoModeOscId); > - break; > - default: > - return EFI_UNSUPPORTED; > - } > +#if (PL111_CLCD_SITE == ARM_VE_MOTHERBOARD_SITE) > + Function = SYS_CFG_OSC; > + OscillatorId = PL111_CLCD_MOTHERBOARD_VIDEO_MODE_OSC_ID; > +#elif (PL111_CLCD_SITE == ARM_VE_DAUGHTERBOARD_1_SITE) > + Function = SYS_CFG_OSC_SITE1; > + OscillatorId = FixedPcdGet32 (PcdPL111LcdVideoModeOscId); > +#else > +#error PL111LcdVExpressLib: Unsupported PL111_CLCD_SITE > +#endif // (PL111_CLCD_SITE == ARM_VE_MOTHERBOARD_SITE) > > // Set the video mode oscillator > Status = ArmPlatformSysConfigSetDevice ( > -- > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759") >