From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::442; helo=mail-wr1-x442.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr1-x442.google.com (mail-wr1-x442.google.com [IPv6:2a00:1450:4864:20::442]) (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 E3DF42119680B for ; Tue, 4 Dec 2018 08:00:55 -0800 (PST) Received: by mail-wr1-x442.google.com with SMTP id l9so16512827wrt.13 for ; Tue, 04 Dec 2018 08:00:55 -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=qdifpOFc1wUWFLhjXaWPx/3HL5XzTH65UBboeaDnQms=; b=Ifq2rfHZ1hsMpONt0pKEvVtTo6jBaQXBEDy+BmHol551PDR9/VBFudhXRq4OvFKHYl +Q0P2lm/3Pw4Di3W4lGcyWQVvmKozV7MbspSjELM998eBJ7U/ZmxT+j1S9Ohv23YUbCw G9rGq7aRBxpVjSCe/+vIiLdeRU4qKutH+TBkM= 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=qdifpOFc1wUWFLhjXaWPx/3HL5XzTH65UBboeaDnQms=; b=pVCB/4G3YMXUWkrBxiqqBxHZ7r7jrN4fzlsgzlyXDk59loj6mrEqKwAtu5hN1+XkyD Boq5SIj4xDcqppTL8oorR83IDvkXiBk9+DDGe3NlaHcXclaGrD6CLXCkL81zRKmyZSp/ O4tE9PvryJHFRQFshmSk7HiBvhBmK51bquWGkOe/JAfP1tizFl+szsjsRG/YKae/kr/r kP71ItopL4tJofVvRVDPgaZf1m/ogLmw2U8dJfbT7raVSgaoyHGKOGX90AS7gtyuO1nK tDUn+dH/MWzh6J6X1jyeH4TbifBnWCBaS6kAyA3XWWvXvAwcaGGmxGPThhtGAJTYugXw 9WJA== X-Gm-Message-State: AA+aEWYE9l0CzVT6T8mEsNcXb4S127S1H9RgICevmzsvutjkBlBsi+M+ SqUvY5ChLEeCBuYsbRt/2jSdSQ== X-Google-Smtp-Source: AFSGD/W1xuq6a8UwYHXhPRAMSR04qX0QV+oIYfSxO5YAH2Pm4bAMp3BjaWT717a24iGf/mi5R8VCkw== X-Received: by 2002:adf:fbc8:: with SMTP id d8mr15295694wrs.318.1543939254303; Tue, 04 Dec 2018 08:00:54 -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 k7sm18863151wrl.51.2018.12.04.08.00.53 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 04 Dec 2018 08:00:53 -0800 (PST) Date: Tue, 4 Dec 2018 16:00:51 +0000 From: Leif Lindholm To: Marcin Wojtas Cc: edk2-devel@lists.01.org, ard.biesheuvel@linaro.org, nadavh@marvell.com, jsd@semihalf.com, jaz@semihalf.com, kostap@marvell.com, jinghua Message-ID: <20181204160051.cju4hj56v5iymoj3@bivouac.eciton.net> References: <1540000661-1956-1-git-send-email-mw@semihalf.com> <1540000661-1956-8-git-send-email-mw@semihalf.com> MIME-Version: 1.0 In-Reply-To: <1540000661-1956-8-git-send-email-mw@semihalf.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [platforms: PATCH 07/12] Marvell/Protocol: Introduce MARVELL_GPIO_PROTOCOL X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 04 Dec 2018 16:00:56 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sat, Oct 20, 2018 at 03:57:36AM +0200, Marcin Wojtas wrote: > From: jinghua > > This patch introduces protocol that can be used by multiple > producers (e.g. platform driver or I2C I/O expander). > It exposes generic api for controlling GPIO pins state. > Drives are differentiated by MARVELL_GPIO_DRIVER_TYPE > field of driver's MV_GPIO_DEVICE_PATH. In order to ease > selection of the desired GPIO controller a helper > function was added - MarvellGpioGetHandle(). > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Marcin Wojtas > --- > Silicon/Marvell/Marvell.dec | 1 + > Silicon/Marvell/Include/Protocol/MvGpio.h | 199 ++++++++++++++++++++ > 2 files changed, 200 insertions(+) > create mode 100644 Silicon/Marvell/Include/Protocol/MvGpio.h > > diff --git a/Silicon/Marvell/Marvell.dec b/Silicon/Marvell/Marvell.dec > index 616624e..7e1c37a 100644 > --- a/Silicon/Marvell/Marvell.dec > +++ b/Silicon/Marvell/Marvell.dec > @@ -215,6 +215,7 @@ > [Protocols] > gMarvellBoardDescProtocolGuid = { 0xebed8738, 0xd4a6, 0x4001, { 0xa9, 0xc9, 0x52, 0xb0, 0xcb, 0x7d, 0xdb, 0xf9 }} > gMarvellEepromProtocolGuid = { 0x71954bda, 0x60d3, 0x4ef8, { 0x8e, 0x3c, 0x0e, 0x33, 0x9f, 0x3b, 0xc2, 0x2b }} > + gMarvellGpioProtocolGuid = { 0x8b01a5b7, 0xc570, 0x4e97, { 0x80, 0x95, 0x4f, 0x74, 0x2a, 0x8d, 0x7d, 0x43 }} > gMarvellMdioProtocolGuid = { 0x40010b03, 0x5f08, 0x496a, { 0xa2, 0x64, 0x10, 0x5e, 0x72, 0xd3, 0x71, 0xaa }} > gMarvellPhyProtocolGuid = { 0x32f48a43, 0x37e3, 0x4acf, { 0x93, 0xc4, 0x3e, 0x57, 0xa7, 0xb0, 0xfb, 0xdc }} > gMarvellSpiMasterProtocolGuid = { 0x23de66a3, 0xf666, 0x4b3e, { 0xaa, 0xa2, 0x68, 0x9b, 0x18, 0xae, 0x2e, 0x19 }} > diff --git a/Silicon/Marvell/Include/Protocol/MvGpio.h b/Silicon/Marvell/Include/Protocol/MvGpio.h > new file mode 100644 > index 0000000..a335cab > --- /dev/null > +++ b/Silicon/Marvell/Include/Protocol/MvGpio.h > @@ -0,0 +1,199 @@ > +/** > +* > +* Copyright (C) 2018, Marvell International Ltd. and its affiliates. > +* > +* 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 __MARVELL_GPIO_PROTOCOL_H__ > +#define __MARVELL_GPIO_PROTOCOL_H__ > + > +#include > + > +#include > +#include > +#include > +#include > + > +extern EFI_GUID gMarvellGpioProtocolGuid; > + > +typedef struct _MARVELL_GPIO_PROTOCOL MARVELL_GPIO_PROTOCOL; > + > +typedef enum { > + GPIO_MODE_INPUT = 0x0, > + GPIO_MODE_OUTPUT = 0x1 > +} MARVELL_GPIO_MODE; GPIO_ is a bit too specific a prefix (even if only used in Marvell-specific code, it makes it look not Marvell-specific). MVGPIO? > + > +typedef enum { > + GPIO_DRIVER_TYPE_SOC_CONTROLLER = 0x0, > + GPIO_DRIVER_TYPE_PCA95XX = 0x1 > +} MARVELL_GPIO_DRIVER_TYPE; Here is sort of what I was getting at before. If you ever end up adding another type to this enum (say on a derivative board), that means the hard (semantic) dependence on I2c for the expander will make things really confusing. And may require substantial code refactoring at that point. > + > +typedef enum { > + PCA9505_ID, Are these referring to software-addressable hardware IDs, or just used for software-internal references? > + PCA9534_ID, > + PCA9535_ID, > + PCA9536_ID, > + PCA9537_ID, > + PCA9538_ID, > + PCA9539_ID, > + PCA9554_ID, > + PCA9555_ID, > + PCA9556_ID, > + PCA9557_ID, > + PCA95XX_MAX_ID, > +} MARVELL_IO_EXPANDER_TYPE_PCA95XX; MARVELL_PCA95XX_VARIANT? If you're checking for which version of it you're dealing with, you already know what it is. Either way, it would be easier to understand its use if it was introduced with the patch that adds code that makes use of it. > + > +typedef > +EFI_STATUS > +(EFIAPI *MV_GPIO_DIRECTION_OUTPUT) ( > + IN MARVELL_GPIO_PROTOCOL *This, > + IN UINTN ControllerIndex, > + IN UINTN GpioPin, > + IN BOOLEAN Value > + ); > + > +typedef > +EFI_STATUS > +(EFIAPI *MV_GPIO_DIRECTION_INPUT) ( > + IN MARVELL_GPIO_PROTOCOL *This, > + IN UINTN ControllerIndex, > + IN UINTN GpioPin > + ); > + > +typedef > +EFI_STATUS > +(EFIAPI *MV_GPIO_GET_FUNCTION) ( > + IN MARVELL_GPIO_PROTOCOL *This, > + IN UINTN ControllerIndex, > + IN UINTN GpioPin, > + OUT MARVELL_GPIO_MODE *Mode > + ); > + > +typedef > +EFI_STATUS > +(EFIAPI *MV_GPIO_GET_VALUE) ( > + IN MARVELL_GPIO_PROTOCOL *This, > + IN UINTN ControllerIndex, > + IN UINTN GpioPin, > + OUT BOOLEAN *Value > + ); > + > +typedef > +EFI_STATUS > +(EFIAPI *MV_GPIO_SET_VALUE) ( > + IN MARVELL_GPIO_PROTOCOL *This, > + IN UINTN ControllerIndex, > + IN UINTN GpioPin, > + IN BOOLEAN Value > + ); > + > +struct _MARVELL_GPIO_PROTOCOL { > + MV_GPIO_DIRECTION_INPUT DirectionInput; > + MV_GPIO_DIRECTION_OUTPUT DirectionOutput; > + MV_GPIO_GET_FUNCTION GetFunction; > + MV_GPIO_GET_VALUE GetValue; > + MV_GPIO_SET_VALUE SetValue; > +}; > + > +typedef struct { > + VENDOR_DEVICE_PATH Header; > + MARVELL_GPIO_DRIVER_TYPE GpioDriverType; > + EFI_DEVICE_PATH_PROTOCOL End; > +} MV_GPIO_DEVICE_PATH; > + > +typedef struct { > + UINTN ControllerId; > + UINTN PinNumber; > + BOOLEAN ActiveHigh; > +} GPIO_PIN_DESC; > + > +/* > + * Select desired protocol producer upon MARVELL_GPIO_DRIVER_TYPE > + * field of driver's MV_GPIO_DEVICE_PATH. > + */ > +STATIC > +inline > +EFI_STATUS > +EFIAPI > +MvGpioGetProtocol ( > + IN MARVELL_GPIO_DRIVER_TYPE GpioDriverType, > + IN OUT MARVELL_GPIO_PROTOCOL **GpioProtocol > + ) > +{ > + MV_GPIO_DEVICE_PATH *GpioDevicePath; > + EFI_DEVICE_PATH *DevicePath; > + EFI_HANDLE *HandleBuffer; > + EFI_STATUS Status; > + UINTN HandleCount; > + UINTN Index; > + > + /* Locate Handles of all MARVELL_GPIO_PROTOCOL producers */ > + Status = gBS->LocateHandleBuffer (ByProtocol, > + &gMarvellGpioProtocolGuid, > + NULL, > + &HandleCount, > + &HandleBuffer); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a: Unable to locate handles\n", __FUNCTION__)); > + return Status; > + } > + > + /* Iterate over all protocol producers */ > + for (Index = 0; Index < HandleCount; Index++) { > + /* Open device path protocol installed on each handle */ > + Status = gBS->OpenProtocol (HandleBuffer[Index], > + &gEfiDevicePathProtocolGuid, > + (VOID **)&DevicePath, > + gImageHandle, > + NULL, > + EFI_OPEN_PROTOCOL_GET_PROTOCOL); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a: Unable to find DevicePath\n", __FUNCTION__)); > + continue; > + } > + > + while (!IsDevicePathEndType (DevicePath)) { > + /* Check if GpioDriverType matches one found in the device path */ > + GpioDevicePath = (MV_GPIO_DEVICE_PATH *)DevicePath; > + if (GpioDevicePath->GpioDriverType != GpioDriverType) { > + DevicePath = NextDevicePathNode (DevicePath); > + continue; > + } > + > + /* > + * Open GpioProtocol. With EFI_OPEN_PROTOCOL_GET_PROTOCOL attribute > + * the consumer is not obliged to call CloseProtocol. > + */ > + Status = gBS->OpenProtocol (HandleBuffer[Index], > + &gMarvellGpioProtocolGuid, > + (VOID **)GpioProtocol, > + gImageHandle, > + NULL, > + EFI_OPEN_PROTOCOL_GET_PROTOCOL); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, > + "%a: Unable to open GPIO protocol\n", > + __FUNCTION__)); > + gBS->FreePool (HandleBuffer); > + return Status; > + } > + > + gBS->FreePool (HandleBuffer); > + > + return EFI_SUCCESS; > + } > + } Could this while loop be broken out into a separate helper function? The loop and function exit conditions become a but foggy with the current layout. / Leif > + > + gBS->FreePool (HandleBuffer); > + > + return EFI_UNSUPPORTED; > +} > + > +#endif // __MARVELL_GPIO_PROTOCOL_H__ > -- > 2.7.4 >