| Message ID | 20171208173128.28485-2-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 99cfb43aa6bef761d588b3192f8402a484bb5354 |
| Headers | show |
| Series | ArmPlatformPkg: refactor LcdGraphicsOutputDxe driver | expand |
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? / Leif > + return EFI_SUCCESS; > +} > + > +/** > + 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 > + ) > +{ > + return EFI_SUCCESS; > +} > + > +/** > + 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 > + ) > +{ > + return EFI_SUCCESS; > +} > + > +/** > + De-initializes the display. > +**/ > +VOID > +LcdShutdown ( > + VOID > + ) > +{ > +} > diff --git a/ArmPlatformPkg/Library/LcdHwNullLib/LcdHwNullLib.inf b/ArmPlatformPkg/Library/LcdHwNullLib/LcdHwNullLib.inf > new file mode 100644 > index 000000000000..d556bed65548 > --- /dev/null > +++ b/ArmPlatformPkg/Library/LcdHwNullLib/LcdHwNullLib.inf > @@ -0,0 +1,28 @@ > +#/** @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. > +# > +#**/ > + > +[Defines] > + INF_VERSION = 0x0001001A > + BASE_NAME = LcdHwNullLib > + FILE_GUID = bb1fde98-1de2-410e-8850-fdcb8e67ebc0 > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = LcdHwLib > + > +[Sources] > + LcdHwNullLib.c > + > +[Packages] > + ArmPlatformPkg/ArmPlatformPkg.dec > + MdePkg/MdePkg.dec > -- > 2.11.0 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Mon, Dec 11, 2017 at 05:56:37PM +0000, Ard Biesheuvel wrote: > 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. I don't necessarily agree, but I also don't consider this very important. I guess my internal model of LibNulls is that if they can be paractically used as do-nothing variants, they return OK on anything, whereas if practical use requires further implementation, they ASSERT and/or return error. This is not necessarily a correct world view. For the ArmPlatformPkg resolution, you don't care about runtime behaviour - it will never be executed. > 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. Sure - but in this case you would only use this library if you actually had a display driver to link against. Anyway: Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> / Leif _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 12 December 2017 at 17:12, Leif Lindholm <leif.lindholm@linaro.org> wrote: > On Mon, Dec 11, 2017 at 05:56:37PM +0000, Ard Biesheuvel wrote: >> 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. > > I don't necessarily agree, but I also don't consider this very > important. > > I guess my internal model of LibNulls is that if they can be > paractically used as do-nothing variants, they return OK on anything, > whereas if practical use requires further implementation, they ASSERT > and/or return error. This is not necessarily a correct world view. > > For the ArmPlatformPkg resolution, you don't care about > runtime behaviour - it will never be executed. > Indeed. Which is essentially why it doesn't matter if we ASSERT () or not. >> 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. > > Sure - but in this case you would only use this library if you > actually had a display driver to link against. > Yes. > Anyway: > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> > Thanks. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 + ) +{ + return EFI_SUCCESS; +} + +/** + 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 + ) +{ + return EFI_SUCCESS; +} + +/** + 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 + ) +{ + return EFI_SUCCESS; +} + +/** + De-initializes the display. +**/ +VOID +LcdShutdown ( + VOID + ) +{ +} diff --git a/ArmPlatformPkg/Library/LcdHwNullLib/LcdHwNullLib.inf b/ArmPlatformPkg/Library/LcdHwNullLib/LcdHwNullLib.inf new file mode 100644 index 000000000000..d556bed65548 --- /dev/null +++ b/ArmPlatformPkg/Library/LcdHwNullLib/LcdHwNullLib.inf @@ -0,0 +1,28 @@ +#/** @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. +# +#**/ + +[Defines] + INF_VERSION = 0x0001001A + BASE_NAME = LcdHwNullLib + FILE_GUID = bb1fde98-1de2-410e-8850-fdcb8e67ebc0 + MODULE_TYPE = BASE + VERSION_STRING = 1.0 + LIBRARY_CLASS = LcdHwLib + +[Sources] + LcdHwNullLib.c + +[Packages] + ArmPlatformPkg/ArmPlatformPkg.dec + MdePkg/MdePkg.dec