From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Leif Lindholm <leif.lindholm@linaro.org>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
Girish Pathak <girish.pathak@arm.com>,
Evan Lloyd <evan.lloyd@arm.com>
Subject: Re: [PATCH v2 1/5] ArmPlatformPkg: introduce LcdHwLib library class
Date: Tue, 12 Dec 2017 17:14:30 +0000 [thread overview]
Message-ID: <CAKv+Gu-rAt-gdC44hWEVcGqY97q1AWXpsmR-JYzRX-AaPKRU5w@mail.gmail.com> (raw)
In-Reply-To: <20171212171212.lhuhr37m3puddz77@bivouac.eciton.net>
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.
next prev parent reply other threads:[~2017-12-12 17:09 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-08 17:31 [PATCH v2 0/5] ArmPlatformPkg: refactor LcdGraphicsOutputDxe driver Ard Biesheuvel
2017-12-08 17:31 ` [PATCH v2 1/5] ArmPlatformPkg: introduce LcdHwLib library class Ard Biesheuvel
2017-12-11 17:32 ` Leif Lindholm
2017-12-11 17:56 ` Ard Biesheuvel
2017-12-12 17:12 ` Leif Lindholm
2017-12-12 17:14 ` Ard Biesheuvel [this message]
2017-12-08 17:31 ` [PATCH v2 2/5] ArmPlatformPkg: implement LcdHwLib for PL111 Ard Biesheuvel
2017-12-11 17:39 ` Leif Lindholm
2017-12-11 17:57 ` Ard Biesheuvel
2017-12-08 17:31 ` [PATCH v2 3/5] ArmPlatformPkg: implement LcdHwLib for HdLcd Ard Biesheuvel
2017-12-11 17:41 ` Leif Lindholm
2017-12-08 17:31 ` [PATCH v2 4/5] ArmPlatformPkg: create hw-agnostic LcdGraphicsOutputDxe driver Ard Biesheuvel
2017-12-11 17:42 ` Leif Lindholm
2017-12-08 17:31 ` [PATCH v2 5/5] ArmPlatformPkg: remove old PL111/HdLcd driver code Ard Biesheuvel
2017-12-11 17:43 ` Leif Lindholm
2017-12-12 17:44 ` [PATCH v2 0/5] ArmPlatformPkg: refactor LcdGraphicsOutputDxe driver Ard Biesheuvel
2017-12-12 19:10 ` Ard Biesheuvel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKv+Gu-rAt-gdC44hWEVcGqY97q1AWXpsmR-JYzRX-AaPKRU5w@mail.gmail.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox