[v2] ArmPlatformPkg: Update GOP

[edk2] [PATCH v2 01/13] ArmPlatformPkg: Tidy Lcd code: Coding standard

Posted by evan.lloyd@arm.com 7 years ago

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>
---
 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..2b001b107927fc75317ce39d370049d7740953a8 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..b9316ec8de8425a83e2f627f5c24821ff9a2f750 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..861d3c398f7d6b9a171b4d8718c2816419d8e20a 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..948e5692c943cdf62a10f4e6290470d3ccd334f1 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..f1fffb1e83173f03f044fc0f4a5ad5c0b864695b 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..19a5f3c58c63173f6528d7c1eedf51053dbeaf7e 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")

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 01/13] ArmPlatformPkg: Tidy Lcd code: Coding standard

Posted by Ard Biesheuvel 7 years ago

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.



> ---
>  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..2b001b107927fc75317ce39d370049d7740953a8 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..b9316ec8de8425a83e2f627f5c24821ff9a2f750 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..861d3c398f7d6b9a171b4d8718c2816419d8e20a 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..948e5692c943cdf62a10f4e6290470d3ccd334f1 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..f1fffb1e83173f03f044fc0f4a5ad5c0b864695b 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..19a5f3c58c63173f6528d7c1eedf51053dbeaf7e 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")
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 01/13] ArmPlatformPkg: Tidy Lcd code: Coding standard

Posted by Evan Lloyd 6 years, 11 months ago

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")
> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 01/13] ArmPlatformPkg: Tidy Lcd code: Coding standard

Posted by Ard Biesheuvel 6 years, 11 months ago

On 2 January 2018 at 15:11, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
> 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.
>

To be honest, this is an aspect I hadn't considered at all. It would
be excellent if we could use tooling to fix our code wrt to coding
style, and if changes such as these bring us closer to that goal, I am
all for it.

Would it be feasible to run that on entire packages, i.e., ArmPkg and
ArmPlatformPkg?

> 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")
>> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 01/13] ArmPlatformPkg: Tidy Lcd code: Coding standard

Posted by Ard Biesheuvel 6 years, 11 months ago

(fix Leif's email address)

On 2 January 2018 at 15:21, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 2 January 2018 at 15:11, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
>> 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.
>>
>
> To be honest, this is an aspect I hadn't considered at all. It would
> be excellent if we could use tooling to fix our code wrt to coding
> style, and if changes such as these bring us closer to that goal, I am
> all for it.
>
> Would it be feasible to run that on entire packages, i.e., ArmPkg and
> ArmPlatformPkg?
>
>> 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")
>>> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 01/13] ArmPlatformPkg: Tidy Lcd code: Coding standard

Posted by Evan Lloyd 6 years, 11 months ago

Hi Ard

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: 02 January 2018 15:21
> To: Evan Lloyd <Evan.Lloyd@arm.com>
> Cc: edk2-devel@lists.01.org; Matteo Carlini <Matteo.Carlini@arm.com>;
> eif.lindholm@linaro.org; nd <nd@arm.com>
> Subject: Re: [PATCH v2 01/13] ArmPlatformPkg: Tidy Lcd code: Coding
> standard
> 
> On 2 January 2018 at 15:11, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
> > 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.
> >
> 
> To be honest, this is an aspect I hadn't considered at all. It would be
> excellent if we could use tooling to fix our code wrt to coding style, and if
> changes such as these bring us closer to that goal, I am all for it.

 [[Evan Lloyd]] Excellent news - now you know why we were doing all those inexplicable things.  I really should have explained this more clearly before.

> 
> Would it be feasible to run that on entire packages, i.e., ArmPkg and
> ArmPlatformPkg?

 [[Evan Lloyd]]  As it stands, none of the tools is very happy with the EDK2 style, and they tend to miss meeting it in different ways.
(As an aside - one common problem is that I have found no means of coercing them to force the ");" at the end of a function call onto its own line.  Your recent CCS change to make that optional helps a lot with that.)  However, I have so far failed to get any of the above tools to do exactly what we want.  All that I can achieve is to generate a formatted view that can be compared against the original with a visual program like kdiff3 - highlighting some formatting improvements that can then be copied across, whilst the mismatches can be ignored.  This is not much, but it does help, especially with spacing and line length errors.

If you are interested, I am very happy to make those parameter files that I have available for consideration, as long as everyone understands how far from fully compliant they are, and that their limitations are manifold.  I suspect the path to a fully automated reformat will be a long and winding road.  The best bet might be to actually change one of the Open Source tools to suit us, but I certainly don't know of anyone with the leisure time available to take that on.

Regards,
Evan

> 
...
> >> > --
> >> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> >> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel