Re: [platforms: PATCH 07/12] Marvell/Protocol: Introduce MARVELL_GPIO_PROTOCOL

[Leif Lindholm <leif.lindholm@linaro.org>]

On Sat, Oct 20, 2018 at 03:57:36AM +0200, Marcin Wojtas wrote:
> From: jinghua <jinghua@marvell.com>
> 
> 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 <mw@semihalf.com>
> ---
>  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 <Uefi.h>
> +
> +#include <Library/DebugLib.h>
> +#include <Library/DevicePathLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +
> +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
>