From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::233; helo=mail-it0-x233.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x233.google.com (mail-it0-x233.google.com [IPv6:2607:f8b0:4001:c0b::233]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id AB67A21B02825 for ; Tue, 12 Dec 2017 09:09:52 -0800 (PST) Received: by mail-it0-x233.google.com with SMTP id f143so81495itb.0 for ; Tue, 12 Dec 2017 09:14:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=zznuyLqwKx7JTVGnFFTSA2Y4/5LVOeuxJRE83qn//28=; b=bmY0WCzVC5fKTC46x0XbRrYnemLFMx4cMuN5AQsCcPP86KUmHX94OeIV5KUqmsq83d /+6FQnMMiHrbwoQRIpd6JlZB2Ke1hBa5n0HSmsLydjYz/JqTR5EhNdESFs0TAWVK4yGO Y4xxeTuk5eBZkxBlHR/+mWYHStgZlp2tihW/0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=zznuyLqwKx7JTVGnFFTSA2Y4/5LVOeuxJRE83qn//28=; b=kCWItRJLN1IdkdwuivcwIN6NP8/lJCLgBu9UCGLCcIBLNjxaOJjZDCtcRYlQ/ds+hO Nv+/p7Gm3oXkg5Thgms4cytFm1cmTfCejly69ems3gD4IC7BCJoQl8D8pLk1lVJI9m0Y Zi88EgdeZX+hioPrN5mO/M85oUJaFTb0i4fxn8ZJCetjjFxiUxL+WttABdEmC5LBMGDe rZt9oekigzTzIE0q5Vtg80zVkLBEhoqstg0DUXAx5tpzbN3bIOqDZKHMVOBJx8Kpez3R LeLatiiWUbHpkq6z+XmMkmyzuv+LncZRBYi9EradKFpdHOKK/pGrUbHBoXvlo2l3S1QO PQ0w== X-Gm-Message-State: AKGB3mJnMVrLWHLwxhDGEHUIFH8dD2d5TesUHHE8w+RJCKymIcM/eHJt /LEbNtulRYHWJULNNPgkrC7JfGNGCkK4g3kpYUacQQ== X-Google-Smtp-Source: ACJfBotEHPTRenO/0b0x1f1SOawru30P+KZ1Pq46NQ2MzpKfk4cfFL4SboyKjMiQCmeMezpU15wkoazUFnoFnvrsyLE= X-Received: by 10.107.170.90 with SMTP id t87mr5985434ioe.253.1513098870778; Tue, 12 Dec 2017 09:14:30 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.104.16 with HTTP; Tue, 12 Dec 2017 09:14:30 -0800 (PST) In-Reply-To: <20171212171212.lhuhr37m3puddz77@bivouac.eciton.net> References: <20171208173128.28485-1-ard.biesheuvel@linaro.org> <20171208173128.28485-2-ard.biesheuvel@linaro.org> <20171211173242.u5yol2egafnvciwx@bivouac.eciton.net> <20171212171212.lhuhr37m3puddz77@bivouac.eciton.net> From: Ard Biesheuvel Date: Tue, 12 Dec 2017 17:14:30 +0000 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" , Girish Pathak , Evan Lloyd Subject: Re: [PATCH v2 1/5] ArmPlatformPkg: introduce LcdHwLib library class X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Dec 2017 17:09:52 -0000 Content-Type: text/plain; charset="UTF-8" On 12 December 2017 at 17:12, Leif Lindholm wrote: > On Mon, Dec 11, 2017 at 05:56:37PM +0000, Ard Biesheuvel wrote: >> On 11 December 2017 at 17:32, Leif Lindholm 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 >> >> Signed-off-by: Evan Lloyd >> >> [ardb: add NULL implementation as well and add it to ArmPlatformPkg.dsc] >> >> Signed-off-by: Ard Biesheuvel >> >> --- >> >> 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.
>> >> + >> >> + 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 >> >> + >> >> +/** >> >> + 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 >> >> +#include >> >> +#include >> >> +#include >> >> + >> >> +/** >> >> + 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 > Thanks.