From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c09::232; helo=mail-wm0-x232.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x232.google.com (mail-wm0-x232.google.com [IPv6:2a00:1450:400c:c09::232]) (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 1052721A1099A for ; Tue, 12 Dec 2017 09:07:38 -0800 (PST) Received: by mail-wm0-x232.google.com with SMTP id g75so134856wme.0 for ; Tue, 12 Dec 2017 09:12:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=8zDeNk+IXqgFqJvJeqYlE+iUqcgP3RXwlm1mB2/bfzc=; b=ks95RmTsXezt39Eck6j77zonufdgGgIL0EtlDesNgnb0Gx5L+iPrSDAiArrM7wPJTu yqSJyu52dH7EKjqZPpUreZgCuVjst49un498eIDuQDKFpQe8FmMrR4mUIVTZtQ6jgpRc z4wgPxlXabBJtGmB/UVeWnCMKds3zBFOk9L3k= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=8zDeNk+IXqgFqJvJeqYlE+iUqcgP3RXwlm1mB2/bfzc=; b=lvbgOzs1x0RfBQ9yauWqmZ3HRiPOIlQ2hlVQl7/0wq3GvAj5Y+vev88OCE/wg8odrS za9GwFlnxHsObqWDRibOnSh/APRRJR5NlL8ehqD1hHQCzwcOgw8AdAXYK9a/5R1UaX7o bCOG79c1n0DpmMtc9fOkg1/XDW6Wz6coWDbfvPBBrlkzKOPzEW83wpEVZgfsHabzt1CD vmoxeK8ElEC0Hlu8jjYafJ2BunviuNWpgSS9a57aPiYeV6Srz6YtUE4CIg1F842K+rlS GMMP0GyE5XgT+u2J9pXo5yusOhSsPHm/SeF/n+2FWL6pTgBvGdEeKEx7jzgiDtL9WgeF 4VsA== X-Gm-Message-State: AKGB3mIpQIMD8ButXLU95LUKr1bNfIji+a7QmFOMCDFBTlVcf6lW+Hhx Edebea+ib1T1qRMyT8H0VB3a6Q== X-Google-Smtp-Source: ACJfBovweCe1g+dmaOQ2vEkQmzoGOMefsNM3GvVKjK68VNKOa3TKwczucxyOacDpr76VSQ85kCZ1HQ== X-Received: by 10.28.9.195 with SMTP id 186mr2109610wmj.122.1513098736010; Tue, 12 Dec 2017 09:12:16 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id v16sm19927003wrb.11.2017.12.12.09.12.14 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 12 Dec 2017 09:12:14 -0800 (PST) Date: Tue, 12 Dec 2017 17:12:13 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , Girish Pathak , Evan Lloyd Message-ID: <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> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) 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:07:39 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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. > 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