Re: [PATCH v2 1/5] ArmPlatformPkg: introduce LcdHwLib library class

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 11 December 2017 at 17:32, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, Dec 08, 2017 at 05:31:24PM +0000, Ard Biesheuvel wrote:
>> Add the declaration and include file for the new LcdHwLib library class,
>> which will allow us to abstract away from platform  variations in the
>> LCD graphics output driver.
>>
>> 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>
>> [ardb: add NULL implementation as well and add it to ArmPlatformPkg.dsc]
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmPlatformPkg/ArmPlatformPkg.dec                    |  1 +
>>  ArmPlatformPkg/ArmPlatformPkg.dsc                    |  1 +
>>  ArmPlatformPkg/Include/Library/LcdHwLib.h            | 68 ++++++++++++++++++
>>  ArmPlatformPkg/Library/LcdHwNullLib/LcdHwNullLib.c   | 75 ++++++++++++++++++++
>>  ArmPlatformPkg/Library/LcdHwNullLib/LcdHwNullLib.inf | 28 ++++++++
>>  5 files changed, 173 insertions(+)
>>
>> diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
>> index 6a7bbc02d011..b33b6e630d85 100644
>> --- a/ArmPlatformPkg/ArmPlatformPkg.dec
>> +++ b/ArmPlatformPkg/ArmPlatformPkg.dec
>> @@ -33,6 +33,7 @@ [Includes.common]
>>
>>  [LibraryClasses]
>>    ArmPlatformLib|Include/Library/ArmPlatformLib.h
>> +  LcdHwLib|Include/Library/LcdHwLib.h
>>    LcdPlatformLib|Include/Library/LcdPlatformLib.h
>>    NorFlashPlatformLib|Include/Library/NorFlashPlatformLib.h
>>    PL011UartLib|Include/Library/PL011UartLib.h
>> diff --git a/ArmPlatformPkg/ArmPlatformPkg.dsc b/ArmPlatformPkg/ArmPlatformPkg.dsc
>> index c3a8c257cb02..9dd64b472acf 100644
>> --- a/ArmPlatformPkg/ArmPlatformPkg.dsc
>> +++ b/ArmPlatformPkg/ArmPlatformPkg.dsc
>> @@ -101,6 +101,7 @@ [Components.common]
>>
>>    ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.inf
>>    ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf
>> +  ArmPlatformPkg/Library/LcdHwNullLib/LcdHwNullLib.inf
>>    ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.inf
>>    ArmPlatformPkg/Library/NorFlashPlatformNullLib/NorFlashPlatformNullLib.inf
>>    ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf
>> diff --git a/ArmPlatformPkg/Include/Library/LcdHwLib.h b/ArmPlatformPkg/Include/Library/LcdHwLib.h
>> new file mode 100644
>> index 000000000000..0c0480862aea
>> --- /dev/null
>> +++ b/ArmPlatformPkg/Include/Library/LcdHwLib.h
>> @@ -0,0 +1,68 @@
>> +/** @file LcdHwLib.h
>> +
>> +  This file contains interface functions for LcdHwLib of ArmPlatformPkg
>> +
>> +  Copyright (c) 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
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#ifndef LCD_HW_LIB_H_
>> +#define LCD_HW_LIB_H_
>> +
>> +#include <Uefi/UefiBaseType.h>
>> +
>> +/**
>> +  Check for presence of display
>> +
>> +  @retval EFI_SUCCESS            Platform implements display.
>> +  @retval EFI_NOT_FOUND          Display not found on the platform.
>> +
>> +**/
>> +EFI_STATUS
>> +LcdIdentify (
>> +  VOID
>> +  );
>> +
>> +/**
>> +  Initialize display.
>> +
>> +  @param  FrameBaseAddress       Address of the frame buffer.
>> +  @retval EFI_SUCCESS            Display initialization success.
>> +  @retval !(EFI_SUCCESS)         Display initialization failure.
>> +
>> +**/
>> +EFI_STATUS
>> +LcdInitialize (
>> +  EFI_PHYSICAL_ADDRESS  FrameBaseAddress
>> +  );
>> +
>> +/**
>> +  Set requested mode of the display.
>> +
>> +  @param  ModeNumber             Display mode number.
>> +  @retval EFI_SUCCESS            Display set mode success.
>> +  @retval EFI_DEVICE_ERROR       If mode not found/supported.
>> +
>> +**/
>> +EFI_STATUS
>> +LcdSetMode (
>> +  IN UINT32  ModeNumber
>> +  );
>> +
>> +/**
>> +  De-initializes the display.
>> +**/
>> +VOID
>> +LcdShutdown (
>> +  VOID
>> +  );
>> +
>> +#endif /* LCD_HW_LIB_H_ */
>> diff --git a/ArmPlatformPkg/Library/LcdHwNullLib/LcdHwNullLib.c b/ArmPlatformPkg/Library/LcdHwNullLib/LcdHwNullLib.c
>> new file mode 100644
>> index 000000000000..2d31b5183c95
>> --- /dev/null
>> +++ b/ArmPlatformPkg/Library/LcdHwNullLib/LcdHwNullLib.c
>> @@ -0,0 +1,75 @@
>> +/** @file
>> +
>> +  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
>> +
>> +  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
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  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 <Base.h>
>> +#include <Uefi/UefiBaseType.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/LcdPlatformLib.h>
>> +
>> +/**
>> +  Check for presence of display
>> +
>> +  @retval EFI_SUCCESS            Platform implements display.
>> +  @retval EFI_NOT_FOUND          Display not found on the platform.
>> +
>> +**/
>> +EFI_STATUS
>> +LcdIdentify (
>> +  VOID
>> +  )
>> +{
>
> Bikeshedding:
> Since this Null implementation can never fill any functionality other
> than letting build complete without platform-specific glue - would it
> not be more expected for it to ASSERT and/or return failure
> everywhere?
>

I think it makes sense to ASSERT on Null library member that take OUT
parameters, because you can never return anything meaningful. In this
case, we can just pretend that the Null LcdHwLib does not require
initialization, is always present and only supports a single mode, and
the driver could theoretically still work. Highly unlikely to be
useful for anything but still.

On ArmVirtQemu, we switched to the Null ArmPlatformLib implementation
because we need to fulfil the dependency to be able to use PrePeiCore,
but we no longer have any code to put in there.