From: Evan Lloyd <Evan.Lloyd@arm.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
Matteo Carlini <Matteo.Carlini@arm.com>,
"eif.lindholm@linaro.org" <eif.lindholm@linaro.org>,
nd <nd@arm.com>
Subject: Re: [PATCH v2 01/13] ArmPlatformPkg: Tidy Lcd code: Coding standard
Date: Tue, 2 Jan 2018 15:11:24 +0000 [thread overview]
Message-ID: <HE1PR08MB26848142B9D42CE24CD04ACB8B190@HE1PR08MB2684.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <CAKv+Gu_SO9NdF=TNZwypji+3MC-an8wOs20hRfi58HVuPUYz=Q@mail.gmail.com>
Hi Ard.
One aim of these changes is to get those files we have to play with into a state where a beautifier like indent, astyle, or clang-format can be used to help tidy our changes. (NOTE, we do not have that fully working yet, but they do help.) In a world where we have to play with several contradictory formatting standards (not just EDK2) then anything that can help is welcome.
Of the changes made:
Fixing the include guards: is a small improvement. (Ideally patchcheck should reject these.)
Reducing lines to 80 columns: makes Leif (at least) happy, and aligns with formatter behaviour.
Correcting Doxygen format comments: prevents Doxygen generating gibberish.
Spaces before '(': Maintains consistency, and aligns with desired formatter behaviour.
More below:
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: 23 December 2017 13:19
> To: Evan Lloyd <Evan.Lloyd@arm.com>
> Cc: edk2-devel@lists.01.org; "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 v2 01/13] ArmPlatformPkg: Tidy Lcd code: Coding
> standard
>
> On 22 December 2017 at 18:34, <evan.lloyd@arm.com> wrote:
> > From: Girish Pathak <girish.pathak at arm.com>
> >
> > There is no functional modification in this change As preparation for
> > further work, the formatting is corrected to meet the EDKII coding
> > standard.
> > Of specific note, some invalid include guards were fixed.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> > Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
>
> Hi Girish, Evan,
>
> I am sorry, but I really don't see the point of this patch. Given that the
> coding standard is not in line with common practice in Tianocore, changing
> comments to remove empty lines after // or changing one style to the
> other is just pointless churn. Also, changes like
>
> > VOID
> > -LcdShutdown (
> > - VOID
> > - )
> > +LcdShutdown (VOID)
> > {
>
> look backward to me, and so if the coding standard mandates that, we
> should changes the coding standard, not the code.
>
> --
> Ard.
>
[[Evan Lloyd]] Hi Ard.
The coding standard doesn't mandate this format, but permits it (5.7.1.5).
Our case is that whilst either format is acceptable, consistency is desirable, so we aimed (however imperfectly) to use a consistent style.
In this instance, though, this would be reverted by the formatting tools, so I agree that it is pointless.
>
>
> > ---
> >
> ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.h |
> 10 +-
> > ArmPlatformPkg/Include/Library/LcdPlatformLib.h | 14 +-
> > ArmPlatformPkg/Library/HdLcd/HdLcd.h | 21 ++-
> >
> ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.c |
> 187 +++++++++++---------
> > ArmPlatformPkg/Library/HdLcd/HdLcd.c | 96 +++++----
> -
> > ArmPlatformPkg/Library/PL111Lcd/PL111Lcd.c | 72 ++++--
> --
> > 6 files changed, 212 insertions(+), 188 deletions(-)
> >
> > diff --git
> a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.
> h
> b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.
> h
> > index
> b66efd34561f655b74a5ecfad8a97281cdd5929d..2b001b107927fc75317ce
> 39d370049d7740953a8 100644
> > ---
> a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.
> h
> > +++
> b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.
> h
> > @@ -1,6 +1,6 @@
> > /** @file
> >
> > - Copyright (c) 2011, ARM Ltd. All rights reserved.<BR>
> > + Copyright (c) 2011-2017, ARM Ltd. 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
> > @@ -11,9 +11,8 @@
> >
> > **/
> >
> > -#ifndef __ARM_VE_GRAPHICS_DXE_H__
> > -#define __ARM_VE_GRAPHICS_DXE_H__
> > -
> > +#ifndef LCD_GRAPHICS_OUTPUT_DXE_H_
> > +#define LCD_GRAPHICS_OUTPUT_DXE_H_
> >
> > #include <Base.h>
> >
> > @@ -25,7 +24,6 @@
> >
> > #include <Protocol/DevicePath.h>
> >
> > -
> > //
> > // Device structures
> > //
> > @@ -106,4 +104,4 @@ InitializeDisplay (
> > IN LCD_INSTANCE* Instance
> > );
> >
> > -#endif /* __ARM_VE_GRAPHICS_DXE_H__ */
> > +#endif /* LCD_GRAPHICS_OUTPUT_DXE_H_ */
> > diff --git a/ArmPlatformPkg/Include/Library/LcdPlatformLib.h
> b/ArmPlatformPkg/Include/Library/LcdPlatformLib.h
> > index
> b9bdf471e2d65dba7a0fcb0f7ecc352bd576b46b..b9316ec8de8425a83e2f6
> 27f5c24821ff9a2f750 100644
> > --- a/ArmPlatformPkg/Include/Library/LcdPlatformLib.h
> > +++ b/ArmPlatformPkg/Include/Library/LcdPlatformLib.h
> > @@ -1,6 +1,6 @@
> > /** @file
> >
> > - Copyright (c) 2011, ARM Ltd. All rights reserved.<BR>
> > + Copyright (c) 2011-2017, ARM Ltd. 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
> > @@ -11,8 +11,8 @@
> >
> > **/
> >
> > -#ifndef __LCDPLATFORMLIB_H
> > -#define __LCDPLATFORMLIB_H
> > +#ifndef LCD_PLATFORM_LIB_H_
> > +#define LCD_PLATFORM_LIB_H_
> >
> > #include <Protocol/GraphicsOutput.h>
> >
> > @@ -158,8 +158,9 @@
> > #define LCD_12BPP_444_BLUE_MASK 0x0000000F
> > #define LCD_12BPP_444_RESERVED_MASK 0x0000F000
> >
> > -
> > -// The enumeration indexes maps the PL111 LcdBpp values used in the
> LCD Control Register
> > +/** The enumeration indexes maps the PL111 LcdBpp values used in the
> LCD Control
> > + Register
> > +**/
> > typedef enum {
> > LCD_BITS_PER_PIXEL_1 = 0,
> > LCD_BITS_PER_PIXEL_2,
> > @@ -171,7 +172,6 @@ typedef enum {
> > LCD_BITS_PER_PIXEL_12_444
> > } LCD_BPP;
> >
> > -
> > EFI_STATUS
> > LcdPlatformInitializeDisplay (
> > IN EFI_HANDLE Handle
> > @@ -218,4 +218,4 @@ LcdPlatformGetBpp (
> > OUT LCD_BPP* Bpp
> > );
> >
> > -#endif
> > +#endif /* LCD_PLATFORM_LIB_H_ */
> > diff --git a/ArmPlatformPkg/Library/HdLcd/HdLcd.h
> b/ArmPlatformPkg/Library/HdLcd/HdLcd.h
> > index
> 6df97a9dfee60e9fda615cf3bea1b6a164a42333..861d3c398f7d6b9a171b4
> d8718c2816419d8e20a 100644
> > --- a/ArmPlatformPkg/Library/HdLcd/HdLcd.h
> > +++ b/ArmPlatformPkg/Library/HdLcd/HdLcd.h
> > @@ -1,6 +1,6 @@
> > -/** @file HDLcd.h
> > +/** @file
> >
> > - Copyright (c) 2011-2012, ARM Ltd. All rights reserved.<BR>
> > + Copyright (c) 2011-2017, ARM Ltd. All rights reserved.<BR>
> >
> > This program and the accompanying materials
> > are licensed and made available under the terms and conditions of the
> BSD License
> > @@ -12,13 +12,10 @@
> >
> > **/
> >
> > -#ifndef _HDLCD_H_
> > -#define _HDLCD_H_
> > +#ifndef HDLCD_H_
> > +#define HDLCD_H_
> >
> > -//
> > // HDLCD Controller Register Offsets
> > -//
> > -
> > #define HDLCD_REG_VERSION ((UINTN)PcdGet32
> (PcdArmHdLcdBase) + 0x000)
> > #define HDLCD_REG_INT_RAWSTAT ((UINTN)PcdGet32
> (PcdArmHdLcdBase) + 0x010)
> > #define HDLCD_REG_INT_CLEAR ((UINTN)PcdGet32
> (PcdArmHdLcdBase) + 0x014)
> > @@ -44,10 +41,7 @@
> > #define HDLCD_REG_GREEN_SELECT ((UINTN)PcdGet32
> (PcdArmHdLcdBase) + 0x248)
> > #define HDLCD_REG_BLUE_SELECT ((UINTN)PcdGet32
> (PcdArmHdLcdBase) + 0x24C)
> >
> > -
> > -//
> > // HDLCD Values of registers
> > -//
> >
> > // HDLCD Interrupt mask, clear and status register
> > #define HDLCD_DMA_END BIT0 /* DMA has finished
> reading a frame */
> > @@ -79,6 +73,11 @@
> > #define HDLCD_DATA_LOW 0
> > #define HDLCD_PXCLK_LOW 0
> >
> > +// Default polarities
> > +#define HDLCD_DEFAULT_POLARITIES HDLCD_PXCLK_LOW |
> HDLCD_DATA_HIGH \
> > + | HDLCD_DATEN_HIGH | HDLCD_HSYNC_LOW \
> > + | HDLCD_VSYNC_HIGH
> > +
> > // Pixel Format
> > #define HDLCD_LITTLE_ENDIAN (0 << 31)
> > #define HDLCD_BIG_ENDIAN (1 << 31)
> > @@ -86,4 +85,4 @@
> > // Number of bytes per pixel
> > #define HDLCD_4BYTES_PER_PIXEL ((4 - 1) << 3)
> >
> > -#endif /* _HDLCD_H_ */
> > +#endif /* HDLCD_H_ */
> > diff --git
> a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.
> c
> b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.
> c
> > index
> b721061fc1df5695092e8c71da97ae0b9af46b3f..948e5692c943cdf62a10f4
> e6290470d3ccd334f1 100644
> > ---
> a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.
> c
> > +++
> b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.
> c
> > @@ -1,6 +1,6 @@
> > /** @file
> >
> > - Copyright (c) 2011-2014, ARM Ltd. All rights reserved.<BR>
> > + Copyright (c) 2011-2017, ARM Ltd. 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
> > @@ -9,7 +9,7 @@
> > THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS,
> > WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> >
> > - **/
> > +**/
> >
> > #include <PiDxe.h>
> > #include <Library/BaseMemoryLib.h>
> > @@ -22,12 +22,10 @@
> >
> > #include "LcdGraphicsOutputDxe.h"
> >
> > -
> /**********************************************************
> ************
> > - *
> > - * This file implements the Graphics Output protocol on
> ArmVersatileExpress
> > - * using the Lcd controller
> > - *
> > -
> **********************************************************
> ************/
> > +/** This file implements the Graphics Output protocol on
> ArmVersatileExpress
> > + using the Lcd controller
> > +
> > +**/
> >
> > //
> > // Global variables
> > @@ -64,7 +62,10 @@ LCD_INSTANCE mLcdTemplate = {
> > {
> > {
> > HARDWARE_DEVICE_PATH, HW_VENDOR_DP,
> > - { (UINT8) (sizeof(VENDOR_DEVICE_PATH)), (UINT8)
> ((sizeof(VENDOR_DEVICE_PATH)) >> 8) },
> > + {
> > + (UINT8)(sizeof (VENDOR_DEVICE_PATH)),
> > + (UINT8)((sizeof (VENDOR_DEVICE_PATH)) >> 8)
> > + },
> > },
> > // Hardware Device Path for Lcd
> > EFI_CALLER_ID_GUID // Use the driver's GUID
> > @@ -73,10 +74,13 @@ LCD_INSTANCE mLcdTemplate = {
> > {
> > END_DEVICE_PATH_TYPE,
> > END_ENTIRE_DEVICE_PATH_SUBTYPE,
> > - { sizeof(EFI_DEVICE_PATH_PROTOCOL), 0 }
> > + {
> > + sizeof (EFI_DEVICE_PATH_PROTOCOL),
> > + 0
> > + }
> > }
> > },
> > - (EFI_EVENT) NULL // ExitBootServicesEvent
> > + (EFI_EVENT)NULL // ExitBootServicesEvent
> > };
> >
> > EFI_STATUS
> > @@ -86,7 +90,7 @@ LcdInstanceContructor (
> > {
> > LCD_INSTANCE* Instance;
> >
> > - Instance = AllocateCopyPool (sizeof(LCD_INSTANCE), &mLcdTemplate);
> > + Instance = AllocateCopyPool (sizeof (LCD_INSTANCE), &mLcdTemplate);
> > if (Instance == NULL) {
> > return EFI_OUT_OF_RESOURCES;
> > }
> > @@ -113,23 +117,23 @@ InitializeDisplay (
> > UINTN VramSize;
> >
> > Status = LcdPlatformGetVram (&VramBaseAddress, &VramSize);
> > - if (EFI_ERROR(Status)) {
> > + if (EFI_ERROR (Status)) {
> > return Status;
> > }
> >
> > // Setup the LCD
> > Status = LcdInitialize (VramBaseAddress);
> > - if (EFI_ERROR(Status)) {
> > + if (EFI_ERROR (Status)) {
> > goto EXIT_ERROR_LCD_SHUTDOWN;
> > }
> >
> > Status = LcdPlatformInitializeDisplay (Instance->Handle);
> > - if (EFI_ERROR(Status)) {
> > + if (EFI_ERROR (Status)) {
> > goto EXIT_ERROR_LCD_SHUTDOWN;
> > }
> >
> > // Setup all the relevant mode information
> > - Instance->Gop.Mode->SizeOfInfo =
> sizeof(EFI_GRAPHICS_OUTPUT_MODE_INFORMATION);
> > + Instance->Gop.Mode->SizeOfInfo = sizeof
> (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION);
> > Instance->Gop.Mode->FrameBufferBase = VramBaseAddress;
> >
> > // Set the flag before changing the mode, to avoid infinite loops
> > @@ -139,7 +143,8 @@ InitializeDisplay (
> > goto EXIT;
> >
> > EXIT_ERROR_LCD_SHUTDOWN:
> > - DEBUG((DEBUG_ERROR, "InitializeDisplay: ERROR - Can not initialise the
> display. Exit Status=%r\n", Status));
> > + DEBUG ((DEBUG_ERROR, "InitializeDisplay: ERROR - Can not initialise
> the display. Exit Status=%r\n", Status));
> > +
> > LcdShutdown ();
> >
> > EXIT:
> > @@ -157,40 +162,44 @@ LcdGraphicsOutputDxeInitialize (
> > LCD_INSTANCE* Instance;
> >
> > Status = LcdIdentify ();
> > - if (EFI_ERROR(Status)) {
> > + if (EFI_ERROR (Status)) {
> > goto EXIT;
> > }
> >
> > Status = LcdInstanceContructor (&Instance);
> > - if (EFI_ERROR(Status)) {
> > + if (EFI_ERROR (Status)) {
> > goto EXIT;
> > }
> >
> > // Install the Graphics Output Protocol and the Device Path
> > - Status = gBS->InstallMultipleProtocolInterfaces(
> > - &Instance->Handle,
> > - &gEfiGraphicsOutputProtocolGuid, &Instance->Gop,
> > - &gEfiDevicePathProtocolGuid, &Instance->DevicePath,
> > - NULL
> > - );
> > + Status = gBS->InstallMultipleProtocolInterfaces (
> > + &Instance->Handle,
> > + &gEfiGraphicsOutputProtocolGuid,
> > + &Instance->Gop,
> > + &gEfiDevicePathProtocolGuid,
> > + &Instance->DevicePath,
> > + NULL
> > + );
> >
> > - if (EFI_ERROR(Status)) {
> > - DEBUG((DEBUG_ERROR, "GraphicsOutputDxeInitialize: Can not install
> the protocol. Exit Status=%r\n", Status));
> > + if (EFI_ERROR (Status)) {
> > + DEBUG ((DEBUG_ERROR, "LcdGraphicsOutputDxeInitialize: Can not
> install the protocol. Exit Status=%r\n", Status));
> > goto EXIT;
> > }
> >
> > // Register for an ExitBootServicesEvent
> > - // When ExitBootServices starts, this function here will make sure that
> the graphics driver will shut down properly,
> > - // i.e. it will free up all allocated memory and perform any necessary
> hardware re-configuration.
> > + // When ExitBootServices starts, this function will make sure that the
> > + // graphics driver shuts down properly, i.e. it will free up all
> > + // allocated memory and perform any necessary hardware re-
> configuration.
> > Status = gBS->CreateEvent (
> > - EVT_SIGNAL_EXIT_BOOT_SERVICES,
> > - TPL_NOTIFY,
> > - LcdGraphicsExitBootServicesEvent, NULL,
> > - &Instance->ExitBootServicesEvent
> > - );
> > + EVT_SIGNAL_EXIT_BOOT_SERVICES,
> > + TPL_NOTIFY,
> > + LcdGraphicsExitBootServicesEvent,
> > + NULL,
> > + &Instance->ExitBootServicesEvent
> > + );
> >
> > - if (EFI_ERROR(Status)) {
> > - DEBUG((DEBUG_ERROR, "GraphicsOutputDxeInitialize: Can not install
> the ExitBootServicesEvent handler. Exit Status=%r\n", Status));
> > + if (EFI_ERROR (Status)) {
> > + DEBUG ((DEBUG_ERROR, "LcdGraphicsOutputDxeInitialize: Can not
> install the ExitBootServicesEvent handler. Exit Status=%r\n", Status));
> > goto EXIT_ERROR_UNINSTALL_PROTOCOL;
> > }
> >
> > @@ -198,48 +207,46 @@ LcdGraphicsOutputDxeInitialize (
> > goto EXIT;
> >
> > EXIT_ERROR_UNINSTALL_PROTOCOL:
> > - /* The following function could return an error message,
> > - * however, to get here something must have gone wrong already,
> > - * so preserve the original error, i.e. don't change
> > - * the Status variable, even it fails to uninstall the protocol.
> > - */
> > + // The following function could return an error message,
> > + // however, to get here something must have gone wrong already,
> > + // so preserve the original error, i.e. don't change
> > + // the Status variable, even it fails to uninstall the protocol.
> > gBS->UninstallMultipleProtocolInterfaces (
> > - Instance->Handle,
> > - &gEfiGraphicsOutputProtocolGuid, &Instance->Gop, // Uninstall
> Graphics Output protocol
> > - &gEfiDevicePathProtocolGuid, &Instance->DevicePath, // Uninstall
> device path
> > - NULL
> > - );
> > + Instance->Handle,
> > + &gEfiGraphicsOutputProtocolGuid,
> > + &Instance->Gop, // Uninstall Graphics Output protocol
> > + &gEfiDevicePathProtocolGuid,
> > + &Instance->DevicePath, // Uninstall device path
> > + NULL
> > + );
> >
> > EXIT:
> > return Status;
> > -
> > }
> >
> > -/***************************************
> > - * This function should be called
> > - * on Event: ExitBootServices
> > - * to free up memory, stop the driver
> > - * and uninstall the protocols
> > - ***************************************/
> > +/** This function should be called
> > + on Event: ExitBootServices
> > + to free up memory, stop the driver
> > + and uninstall the protocols
> > +**/
> > VOID
> > LcdGraphicsExitBootServicesEvent (
> > IN EFI_EVENT Event,
> > IN VOID *Context
> > )
> > {
> > - // By default, this PCD is FALSE. But if a platform starts a predefined OS
> that
> > - // does not use a framebuffer then we might want to disable the display
> controller
> > - // to avoid to display corrupted information on the screen.
> > + // By default, this PCD is FALSE. But if a platform starts a predefined OS
> > + // that does not use a framebuffer then we might want to disable the
> display
> > + // controller to avoid to display corrupted information on the screen.
> > if (FeaturePcdGet (PcdGopDisableOnExitBootServices)) {
> > // Turn-off the Display controller
> > LcdShutdown ();
> > }
> > }
> >
> > -/***************************************
> > - * GraphicsOutput Protocol function, mapping to
> > - * EFI_GRAPHICS_OUTPUT_PROTOCOL.QueryMode
> > - ***************************************/
> > +/** GraphicsOutput Protocol function, mapping to
> > + EFI_GRAPHICS_OUTPUT_PROTOCOL.QueryMode
> > +**/
> > EFI_STATUS
> > EFIAPI
> > LcdGraphicsQueryMode (
> > @@ -252,19 +259,22 @@ LcdGraphicsQueryMode (
> > EFI_STATUS Status = EFI_SUCCESS;
> > LCD_INSTANCE *Instance;
> >
> > - Instance = LCD_INSTANCE_FROM_GOP_THIS(This);
> > + Instance = LCD_INSTANCE_FROM_GOP_THIS (This);
> >
> > // Setup the hardware if not already done
> > - if( !mDisplayInitialized ) {
> > - Status = InitializeDisplay(Instance);
> > - if (EFI_ERROR(Status)) {
> > + if (!mDisplayInitialized) {
> > + Status = InitializeDisplay (Instance);
> > + if (EFI_ERROR (Status)) {
> > goto EXIT;
> > }
> > }
> >
> > // Error checking
> > - if ( (This == NULL) || (Info == NULL) || (SizeOfInfo == NULL) ||
> (ModeNumber >= This->Mode->MaxMode) ) {
> > - DEBUG((DEBUG_ERROR, "LcdGraphicsQueryMode: ERROR - For mode
> number %d : Invalid Parameter.\n", ModeNumber ));
> > + if ((This == NULL)
> > + || (Info == NULL)
> > + || (SizeOfInfo == NULL)
> > + || (ModeNumber >= This->Mode->MaxMode)) {
> > + DEBUG ((DEBUG_ERROR, "LcdGraphicsQueryMode: ERROR - For mode
> number %d : Invalid Parameter.\n", ModeNumber));
> > Status = EFI_INVALID_PARAMETER;
> > goto EXIT;
> > }
> > @@ -275,21 +285,20 @@ LcdGraphicsQueryMode (
> > goto EXIT;
> > }
> >
> > - *SizeOfInfo = sizeof( EFI_GRAPHICS_OUTPUT_MODE_INFORMATION);
> > + *SizeOfInfo = sizeof (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION);
> >
> > - Status = LcdPlatformQueryMode (ModeNumber,*Info);
> > - if (EFI_ERROR(Status)) {
> > - FreePool(*Info);
> > + Status = LcdPlatformQueryMode (ModeNumber, *Info);
> > + if (EFI_ERROR (Status)) {
> > + FreePool (*Info);
> > }
> >
> > EXIT:
> > return Status;
> > }
> >
> > -/***************************************
> > - * GraphicsOutput Protocol function, mapping to
> > - * EFI_GRAPHICS_OUTPUT_PROTOCOL.SetMode
> > - ***************************************/
> > +/** GraphicsOutput Protocol function, mapping to
> > + EFI_GRAPHICS_OUTPUT_PROTOCOL.SetMode
> > +**/
> > EFI_STATUS
> > EFIAPI
> > LcdGraphicsSetMode (
> > @@ -305,47 +314,48 @@ LcdGraphicsSetMode (
> > Instance = LCD_INSTANCE_FROM_GOP_THIS (This);
> >
> > // Setup the hardware if not already done
> > - if(!mDisplayInitialized) {
> > + if (!mDisplayInitialized) {
> > Status = InitializeDisplay (Instance);
> > - if (EFI_ERROR(Status)) {
> > + if (EFI_ERROR (Status)) {
> > goto EXIT;
> > }
> > }
> >
> > // Check if this mode is supported
> > - if( ModeNumber >= This->Mode->MaxMode ) {
> > - DEBUG((DEBUG_ERROR, "LcdGraphicsSetMode: ERROR - Unsupported
> mode number %d .\n", ModeNumber ));
> > + if (ModeNumber >= This->Mode->MaxMode) {
> > + DEBUG ((DEBUG_ERROR, "LcdGraphicsSetMode: ERROR - Unsupported
> mode number %d .\n", ModeNumber));
> > Status = EFI_UNSUPPORTED;
> > goto EXIT;
> > }
> >
> > // Set the oscillator frequency to support the new mode
> > Status = LcdPlatformSetMode (ModeNumber);
> > - if (EFI_ERROR(Status)) {
> > + if (EFI_ERROR (Status)) {
> > Status = EFI_DEVICE_ERROR;
> > goto EXIT;
> > }
> >
> > // Update the UEFI mode information
> > This->Mode->Mode = ModeNumber;
> > - LcdPlatformQueryMode (ModeNumber,&Instance->ModeInfo);
> > - Status = LcdPlatformGetBpp(ModeNumber, &Bpp);
> > - if (EFI_ERROR(Status)) {
> > + LcdPlatformQueryMode (ModeNumber, &Instance->ModeInfo);
> > + Status = LcdPlatformGetBpp (ModeNumber, &Bpp);
> > + if (EFI_ERROR (Status)) {
> > DEBUG ((DEBUG_ERROR, "LcdGraphicsSetMode: ERROR - Couldn't get
> bytes per pixel, status: %r\n", Status));
> > goto EXIT;
> > }
> > This->Mode->FrameBufferSize = Instance->ModeInfo.VerticalResolution
> > - * Instance->ModeInfo.PixelsPerScanLine
> > - * GetBytesPerPixel(Bpp);
> > + * Instance->ModeInfo.PixelsPerScanLine
> > + * GetBytesPerPixel (Bpp);
> >
> > // Set the hardware to the new mode
> > Status = LcdSetMode (ModeNumber);
> > - if (EFI_ERROR(Status)) {
> > + if (EFI_ERROR (Status)) {
> > Status = EFI_DEVICE_ERROR;
> > goto EXIT;
> > }
> >
> > - // The UEFI spec requires that we now clear the visible portions of the
> output display to black.
> > + // The UEFI spec requires that we now clear the visible portions of the
> > + // output display to black.
> >
> > // Set the fill colour to black
> > SetMem (&FillColour, sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL), 0x0);
> > @@ -361,7 +371,8 @@ LcdGraphicsSetMode (
> > 0,
> > This->Mode->Info->HorizontalResolution,
> > This->Mode->Info->VerticalResolution,
> > - 0);
> > + 0
> > + );
> >
> > EXIT:
> > return Status;
> > @@ -372,7 +383,7 @@ GetBytesPerPixel (
> > IN LCD_BPP Bpp
> > )
> > {
> > - switch(Bpp) {
> > + switch (Bpp) {
> > case LCD_BITS_PER_PIXEL_24:
> > return 4;
> >
> > diff --git a/ArmPlatformPkg/Library/HdLcd/HdLcd.c
> b/ArmPlatformPkg/Library/HdLcd/HdLcd.c
> > index
> 24efb68f23e3393a96fc760732d978b6346a2807..f1fffb1e83173f03f044fc0
> f4a5ad5c0b864695b 100644
> > --- a/ArmPlatformPkg/Library/HdLcd/HdLcd.c
> > +++ b/ArmPlatformPkg/Library/HdLcd/HdLcd.c
> > @@ -1,6 +1,6 @@
> > -/** @file Lcd.c
> > +/** @file
> >
> > - Copyright (c) 2011-2012, ARM Ltd. All rights reserved.<BR>
> > + Copyright (c) 2011-2017, ARM Ltd. All rights reserved.<BR>
> >
> > This program and the accompanying materials
> > are licensed and made available under the terms and conditions of the
> BSD License
> > @@ -21,12 +21,9 @@
> >
> > #include "HdLcd.h"
> >
> > -
> /**********************************************************
> ************
> > - *
> > - * This file contains all the bits of the Lcd that are
> > - * platform independent.
> > - *
> > -
> **********************************************************
> ************/
> > +/** This file contains all the bits of the Lcd that are
> > + platform independent.
> > +**/
> >
> > STATIC
> > UINTN
> > @@ -34,7 +31,7 @@ GetBytesPerPixel (
> > IN LCD_BPP Bpp
> > )
> > {
> > - switch(Bpp) {
> > + switch (Bpp) {
> > case LCD_BITS_PER_PIXEL_24:
> > return 4;
> >
> > @@ -60,21 +57,27 @@ LcdInitialize (
> > )
> > {
> > // Disable the controller
> > - MmioWrite32(HDLCD_REG_COMMAND, HDLCD_DISABLE);
> > + MmioWrite32 (HDLCD_REG_COMMAND, HDLCD_DISABLE);
> >
> > // Disable all interrupts
> > - MmioWrite32(HDLCD_REG_INT_MASK, 0);
> > + MmioWrite32 (HDLCD_REG_INT_MASK, 0);
> >
> > // Define start of the VRAM. This never changes for any graphics mode
> > - MmioWrite32(HDLCD_REG_FB_BASE, (UINT32) VramBaseAddress);
> > + MmioWrite32 (HDLCD_REG_FB_BASE, (UINT32)VramBaseAddress);
> >
> > // Setup various registers that never change
> > - MmioWrite32(HDLCD_REG_BUS_OPTIONS, (4 << 8) | HDLCD_BURST_8);
> > - MmioWrite32(HDLCD_REG_POLARITIES, HDLCD_PXCLK_LOW |
> HDLCD_DATA_HIGH | HDLCD_DATEN_HIGH | HDLCD_HSYNC_LOW |
> HDLCD_VSYNC_HIGH);
> > - MmioWrite32(HDLCD_REG_PIXEL_FORMAT, HDLCD_LITTLE_ENDIAN |
> HDLCD_4BYTES_PER_PIXEL);
> > - MmioWrite32(HDLCD_REG_RED_SELECT, (0 << 16 | 8 << 8 | 0));
> > - MmioWrite32(HDLCD_REG_GREEN_SELECT, (0 << 16 | 8 << 8 | 8));
> > - MmioWrite32(HDLCD_REG_BLUE_SELECT, (0 << 16 | 8 << 8 | 16));
> > + MmioWrite32 (HDLCD_REG_BUS_OPTIONS, (4 << 8) |
> HDLCD_BURST_8);
> > +
> > + MmioWrite32 (HDLCD_REG_POLARITIES, HDLCD_DEFAULT_POLARITIES);
> > +
> > + MmioWrite32 (
> > + HDLCD_REG_PIXEL_FORMAT,
> > + HDLCD_LITTLE_ENDIAN | HDLCD_4BYTES_PER_PIXEL
> > + );
> > +
> > + MmioWrite32 (HDLCD_REG_RED_SELECT, (0 << 16 | 8 << 8 | 0));
> > + MmioWrite32 (HDLCD_REG_GREEN_SELECT, (0 << 16 | 8 << 8 | 8));
> > + MmioWrite32 (HDLCD_REG_BLUE_SELECT, (0 << 16 | 8 << 8 | 16));
> >
> > return EFI_SUCCESS;
> > }
> > @@ -96,63 +99,66 @@ LcdSetMode (
> > UINT32 BytesPerPixel;
> > LCD_BPP LcdBpp;
> >
> > -
> > // Set the video mode timings and other relevant information
> > - Status = LcdPlatformGetTimings (ModeNumber,
> > - &HRes,&HSync,&HBackPorch,&HFrontPorch,
> > - &VRes,&VSync,&VBackPorch,&VFrontPorch);
> > + Status = LcdPlatformGetTimings (
> > + ModeNumber,
> > + &HRes,
> > + &HSync,
> > + &HBackPorch,
> > + &HFrontPorch,
> > + &VRes,
> > + &VSync,
> > + &VBackPorch,
> > + &VFrontPorch
> > + );
> > ASSERT_EFI_ERROR (Status);
> > - if (EFI_ERROR( Status )) {
> > + if (EFI_ERROR (Status)) {
> > return EFI_DEVICE_ERROR;
> > }
> >
> > - Status = LcdPlatformGetBpp (ModeNumber,&LcdBpp);
> > + Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
> > ASSERT_EFI_ERROR (Status);
> > - if (EFI_ERROR( Status )) {
> > + if (EFI_ERROR (Status)) {
> > return EFI_DEVICE_ERROR;
> > }
> >
> > - BytesPerPixel = GetBytesPerPixel(LcdBpp);
> > + BytesPerPixel = GetBytesPerPixel (LcdBpp);
> >
> > // Disable the controller
> > - MmioWrite32(HDLCD_REG_COMMAND, HDLCD_DISABLE);
> > + MmioWrite32 (HDLCD_REG_COMMAND, HDLCD_DISABLE);
> >
> > // Update the frame buffer information with the new settings
> > - MmioWrite32(HDLCD_REG_FB_LINE_LENGTH, HRes * BytesPerPixel);
> > - MmioWrite32(HDLCD_REG_FB_LINE_PITCH, HRes * BytesPerPixel);
> > - MmioWrite32(HDLCD_REG_FB_LINE_COUNT, VRes - 1);
> > + MmioWrite32 (HDLCD_REG_FB_LINE_LENGTH, HRes * BytesPerPixel);
> > + MmioWrite32 (HDLCD_REG_FB_LINE_PITCH, HRes * BytesPerPixel);
> > + MmioWrite32 (HDLCD_REG_FB_LINE_COUNT, VRes - 1);
> >
> > // Set the vertical timing information
> > - MmioWrite32(HDLCD_REG_V_SYNC, VSync);
> > - MmioWrite32(HDLCD_REG_V_BACK_PORCH, VBackPorch);
> > - MmioWrite32(HDLCD_REG_V_DATA, VRes - 1);
> > - MmioWrite32(HDLCD_REG_V_FRONT_PORCH, VFrontPorch);
> > + MmioWrite32 (HDLCD_REG_V_SYNC, VSync);
> > + MmioWrite32 (HDLCD_REG_V_BACK_PORCH, VBackPorch);
> > + MmioWrite32 (HDLCD_REG_V_DATA, VRes - 1);
> > + MmioWrite32 (HDLCD_REG_V_FRONT_PORCH, VFrontPorch);
> >
> > // Set the horizontal timing information
> > - MmioWrite32(HDLCD_REG_H_SYNC, HSync);
> > - MmioWrite32(HDLCD_REG_H_BACK_PORCH, HBackPorch);
> > - MmioWrite32(HDLCD_REG_H_DATA, HRes - 1);
> > - MmioWrite32(HDLCD_REG_H_FRONT_PORCH, HFrontPorch);
> > + MmioWrite32 (HDLCD_REG_H_SYNC, HSync);
> > + MmioWrite32 (HDLCD_REG_H_BACK_PORCH, HBackPorch);
> > + MmioWrite32 (HDLCD_REG_H_DATA, HRes - 1);
> > + MmioWrite32 (HDLCD_REG_H_FRONT_PORCH, HFrontPorch);
> >
> > // Enable the controller
> > - MmioWrite32(HDLCD_REG_COMMAND, HDLCD_ENABLE);
> > + MmioWrite32 (HDLCD_REG_COMMAND, HDLCD_ENABLE);
> >
> > return EFI_SUCCESS;
> > }
> >
> > VOID
> > -LcdShutdown (
> > - VOID
> > - )
> > +LcdShutdown (VOID)
> > {
> > // Disable the controller
> > MmioWrite32 (HDLCD_REG_COMMAND, HDLCD_DISABLE);
> > }
> >
> > EFI_STATUS
> > -LcdIdentify (
> > - VOID
> > - )
> > +LcdIdentify (VOID)
> > {
> > return EFI_SUCCESS;
> > }
> > diff --git a/ArmPlatformPkg/Library/PL111Lcd/PL111Lcd.c
> b/ArmPlatformPkg/Library/PL111Lcd/PL111Lcd.c
> > index
> 9b4a02045ab7ca170e0d4362ee0e2bcf1d275bdb..19a5f3c58c63173f6528d
> 7c1eedf51053dbeaf7e 100644
> > --- a/ArmPlatformPkg/Library/PL111Lcd/PL111Lcd.c
> > +++ b/ArmPlatformPkg/Library/PL111Lcd/PL111Lcd.c
> > @@ -1,6 +1,6 @@
> > -/** @file PL111Lcd.c
> > +/** @file
> >
> > - Copyright (c) 2011-2012, ARM Ltd. All rights reserved.<BR>
> > + Copyright (c) 2011-2017, ARM Ltd. All rights reserved.<BR>
> >
> > This program and the accompanying materials
> > are licensed and made available under the terms and conditions of the
> BSD License
> > @@ -19,17 +19,12 @@
> >
> > #include "PL111Lcd.h"
> >
> > -
> /**********************************************************
> ************
> > - *
> > - * This file contains all the bits of the PL111 that are
> > - * platform independent.
> > - *
> > -
> **********************************************************
> ************/
> > +/** This file contains all the bits of the PL111 that are
> > + platform independent.
> >
> > +**/
> > EFI_STATUS
> > -LcdIdentify (
> > - VOID
> > - )
> > +LcdIdentify (VOID)
> > {
> > DEBUG ((EFI_D_WARN, "Probing ID registers at 0x%lx for a PL111\n",
> > PL111_REG_CLCD_PERIPH_ID_0));
> > @@ -54,11 +49,11 @@ LcdInitialize (
> > )
> > {
> > // Define start of the VRAM. This never changes for any graphics mode
> > - MmioWrite32(PL111_REG_LCD_UP_BASE, (UINT32) VramBaseAddress);
> > - MmioWrite32(PL111_REG_LCD_LP_BASE, 0); // We are not using a
> double buffer
> > + MmioWrite32 (PL111_REG_LCD_UP_BASE, (UINT32)VramBaseAddress);
> > + MmioWrite32 (PL111_REG_LCD_LP_BASE, 0); // We are not using a
> double buffer
> >
> > // Disable all interrupts from the PL111
> > - MmioWrite32(PL111_REG_LCD_IMSC, 0);
> > + MmioWrite32 (PL111_REG_LCD_IMSC, 0);
> >
> > return EFI_SUCCESS;
> > }
> > @@ -81,45 +76,60 @@ LcdSetMode (
> > LCD_BPP LcdBpp;
> >
> > // Set the video mode timings and other relevant information
> > - Status = LcdPlatformGetTimings (ModeNumber,
> > - &HRes,&HSync,&HBackPorch,&HFrontPorch,
> > - &VRes,&VSync,&VBackPorch,&VFrontPorch);
> > + Status = LcdPlatformGetTimings (
> > + ModeNumber,
> > + &HRes,
> > + &HSync,
> > + &HBackPorch,
> > + &HFrontPorch,
> > + &VRes,
> > + &VSync,
> > + &VBackPorch,
> > + &VFrontPorch
> > + );
> > ASSERT_EFI_ERROR (Status);
> > - if (EFI_ERROR( Status )) {
> > + if (EFI_ERROR (Status)) {
> > return EFI_DEVICE_ERROR;
> > }
> >
> > - Status = LcdPlatformGetBpp (ModeNumber,&LcdBpp);
> > + Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
> > ASSERT_EFI_ERROR (Status);
> > - if (EFI_ERROR( Status )) {
> > + if (EFI_ERROR (Status)) {
> > return EFI_DEVICE_ERROR;
> > }
> >
> > // Disable the CLCD_LcdEn bit
> > - LcdControl = MmioRead32( PL111_REG_LCD_CONTROL);
> > - MmioWrite32(PL111_REG_LCD_CONTROL, LcdControl & ~1);
> > + LcdControl = MmioRead32 (PL111_REG_LCD_CONTROL);
> > + MmioWrite32 (PL111_REG_LCD_CONTROL, LcdControl & ~1);
> >
> > // Set Timings
> > - MmioWrite32 (PL111_REG_LCD_TIMING_0,
> HOR_AXIS_PANEL(HBackPorch, HFrontPorch, HSync, HRes));
> > - MmioWrite32 (PL111_REG_LCD_TIMING_1,
> VER_AXIS_PANEL(VBackPorch, VFrontPorch, VSync, VRes));
> > - MmioWrite32 (PL111_REG_LCD_TIMING_2, CLK_SIG_POLARITY(HRes));
> > + MmioWrite32 (
> > + PL111_REG_LCD_TIMING_0,
> > + HOR_AXIS_PANEL (HBackPorch, HFrontPorch, HSync, HRes)
> > + );
> > +
> > + MmioWrite32 (
> > + PL111_REG_LCD_TIMING_1,
> > + VER_AXIS_PANEL (VBackPorch, VFrontPorch, VSync, VRes)
> > + );
> > +
> > + MmioWrite32 (PL111_REG_LCD_TIMING_2, CLK_SIG_POLARITY (HRes));
> > MmioWrite32 (PL111_REG_LCD_TIMING_3, 0);
> >
> > // PL111_REG_LCD_CONTROL
> > - LcdControl = PL111_CTRL_LCD_EN | PL111_CTRL_LCD_BPP(LcdBpp) |
> PL111_CTRL_LCD_TFT | PL111_CTRL_BGR;
> > - MmioWrite32(PL111_REG_LCD_CONTROL, LcdControl);
> > + LcdControl = PL111_CTRL_LCD_EN | PL111_CTRL_LCD_BPP (LcdBpp)
> > + | PL111_CTRL_LCD_TFT | PL111_CTRL_BGR;
> > + MmioWrite32 (PL111_REG_LCD_CONTROL, LcdControl);
> >
> > // Turn on power to the LCD Panel
> > LcdControl |= PL111_CTRL_LCD_PWR;
> > - MmioWrite32(PL111_REG_LCD_CONTROL, LcdControl);
> > + MmioWrite32 (PL111_REG_LCD_CONTROL, LcdControl);
> >
> > return EFI_SUCCESS;
> > }
> >
> > VOID
> > -LcdShutdown (
> > - VOID
> > - )
> > +LcdShutdown (VOID)
> > {
> > // Disable the controller
> > MmioAnd32 (PL111_REG_LCD_CONTROL, ~PL111_CTRL_LCD_EN);
> > --
> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> >
next prev parent reply other threads:[~2018-01-02 15:06 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-22 18:34 [PATCH v2 00/13] ArmPlatformPkg: Update GOP evan.lloyd
2017-12-22 18:34 ` [PATCH v2 01/13] ArmPlatformPkg: Tidy Lcd code: Coding standard evan.lloyd
2017-12-23 13:19 ` Ard Biesheuvel
2018-01-02 15:11 ` Evan Lloyd [this message]
2018-01-02 15:21 ` Ard Biesheuvel
2018-01-02 15:21 ` Ard Biesheuvel
2018-01-03 16:06 ` Evan Lloyd
2017-12-22 18:34 ` [PATCH v2 02/13] ArmPlatformPkg: Tidy Lcd code: Updated comments evan.lloyd
2017-12-22 18:34 ` [PATCH v2 03/13] ArmPlatformPkg: PL111 and HDLCD: add const qualifier evan.lloyd
2017-12-23 13:21 ` Ard Biesheuvel
2017-12-22 18:34 ` [PATCH v2 04/13] ArmPlatformPkg: HDLCD and PL111: Update debug ASSERTS evan.lloyd
2017-12-23 13:22 ` Ard Biesheuvel
2017-12-22 18:34 ` [PATCH v2 05/13] ArmPlatformPkg: PL111Lcd: Replace magic number with macro evan.lloyd
2017-12-23 13:24 ` Ard Biesheuvel
2017-12-22 18:34 ` [PATCH v2 06/13] ArmPlatformPkg: Implement LcdIdentify function for HDLCD GOP evan.lloyd
2017-12-23 13:24 ` Ard Biesheuvel
2017-12-22 18:34 ` [PATCH v2 07/13] ArmPlatformPkg: Redefine LcdPlatformGetTimings function evan.lloyd
2017-12-23 13:27 ` Ard Biesheuvel
2017-12-22 18:34 ` [PATCH v2 08/13] ArmPlatformPkg: Add PCD to select pixel format evan.lloyd
2017-12-23 13:29 ` Ard Biesheuvel
2017-12-22 18:34 ` [PATCH v2 09/13] ArmPlatformPkg: PCD to swap red/blue format for HDLCD evan.lloyd
2017-12-23 13:34 ` Ard Biesheuvel
2017-12-22 18:34 ` [PATCH v2 10/13] ArmPlatformPkg: Additional display modes evan.lloyd
2017-12-23 13:35 ` Ard Biesheuvel
2017-12-22 18:34 ` [PATCH v2 11/13] ArmPlatformPkg: Reserving framebuffer at build evan.lloyd
2017-12-23 13:36 ` Ard Biesheuvel
2017-12-22 18:34 ` [PATCH v2 12/13] ArmPlatformPkg: New DP500/DP550/DP650 GOP driver evan.lloyd
2017-12-23 13:44 ` Ard Biesheuvel
2017-12-22 18:34 ` [PATCH v2 13/13] ArmPlatformPkg: Introduce SCMI protocol evan.lloyd
2017-12-23 14:05 ` Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=HE1PR08MB26848142B9D42CE24CD04ACB8B190@HE1PR08MB2684.eurprd08.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox