Re: [platforms: PATCH v2 07/12] Marvell/Protocol: Introduce GPIO helper header

[Marcin Wojtas <mw@semihalf.com>]

Hi Leif,

wt., 15 sty 2019 o 00:12 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
>
> On Thu, Jan 10, 2019 at 02:44:34AM +0100, Marcin Wojtas wrote:
> > From: jinghua <jinghua@marvell.com>
> >
> > This patch introduces a helper header that can be used by
> > multiple EMBEDDED_GPIO protocol producers (e.g. platform
> > driver or GPIO expanders). The drives are differentiated by
> > MV_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 - MarvellGpioGetProtocol().
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > ---
> >  Silicon/Marvell/Include/Protocol/MvGpio.h | 156 ++++++++++++++++++++
> >  1 file changed, 156 insertions(+)
> >  create mode 100644 Silicon/Marvell/Include/Protocol/MvGpio.h
> >
> > diff --git a/Silicon/Marvell/Include/Protocol/MvGpio.h b/Silicon/Marvell/Include/Protocol/MvGpio.h
> > new file mode 100644
> > index 0000000..c9f1007
> > --- /dev/null
> > +++ b/Silicon/Marvell/Include/Protocol/MvGpio.h
> > @@ -0,0 +1,156 @@
> > +/**
> > +*
> > +*  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 __MV_GPIO_PROTOCOL_H__
> > +#define __MV_GPIO_PROTOCOL_H__
> > +
> > +#include <Uefi.h>
> > +
> > +#include <Library/DebugLib.h>
> > +#include <Library/DevicePathLib.h>
> > +#include <Library/MemoryAllocationLib.h>
> > +#include <Library/UefiBootServicesTableLib.h>
> > +
> > +#include <Protocol/EmbeddedGpio.h>
> > +
> > +typedef enum {
> > +  MV_GPIO_DRIVER_TYPE_SOC_CONTROLLER,
> > +} MV_GPIO_DRIVER_TYPE;
> > +
> > +typedef struct {
> > +  VENDOR_DEVICE_PATH        Header;
> > +  MV_GPIO_DRIVER_TYPE       GpioDriverType;
> > +  EFI_DEVICE_PATH_PROTOCOL  End;
> > +} MV_GPIO_DEVICE_PATH;
> > +
> > +typedef struct {
> > +  UINTN    ControllerId;
> > +  UINTN    PinNumber;
> > +  BOOLEAN  ActiveHigh;
> > +} MV_GPIO_PIN;
> > +
> > +/*
> > + * Check if the driver type matches the requested value.
> > + * In case of the success open the GPIO protocol and return.
> > + */
> > +STATIC
> > +inline
> > +EFI_STATUS
> > +EFIAPI
> > +MvGpioFindMatchingDriver (
>
> Eep!
> I missed this in v1 (I assumed I'd missed the new file indicator
> whilst scrolling) - please move these functions out of this .h file.
> https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/v/release/2.20/5_source_files/53_include_files.html#537-include-files-shall-not-generate-code-or-define-data-variables
>
> This makes me take a closer look at the rest of the patch ... and this
> just isn't a protocol - it's a library. Please restrucure it as such
> and move the header file to Include/Library rather than
> Include/Protocol.
>

Initially it was a header for MvGpioProtocol + this inline C addition
at the end... I will change it to the library.

Thanks,
Marcin

> /
>     Leif
>
> > +  IN     MV_GPIO_DRIVER_TYPE    GpioDriverType,
> > +  IN     EFI_HANDLE             HandleBuffer,
> > +  IN     EFI_DEVICE_PATH       *DevicePath,
> > +  IN OUT EMBEDDED_GPIO        **GpioProtocol
> > +  )
> > +{
> > +  MV_GPIO_DEVICE_PATH *GpioDevicePath;
> > +  EFI_STATUS           Status;
> > +
> > +  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,
> > +                    &gEmbeddedGpioProtocolGuid,
> > +                    (VOID **)GpioProtocol,
> > +                    gImageHandle,
> > +                    NULL,
> > +                    EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> > +    return Status;
> > +  }
> > +
> > +  return EFI_NOT_FOUND;
> > +}
> > +
> > +/*
> > + * Select desired protocol producer upon MV_GPIO_DRIVER_TYPE
> > + * field of driver's MV_GPIO_DEVICE_PATH.
> > + */
> > +STATIC
> > +inline
> > +EFI_STATUS
> > +EFIAPI
> > +MvGpioGetProtocol (
> > +  IN     MV_GPIO_DRIVER_TYPE    GpioDriverType,
> > +  IN OUT EMBEDDED_GPIO        **GpioProtocol
> > +  )
> > +{
> > +  EFI_DEVICE_PATH     *DevicePath;
> > +  EFI_HANDLE          *HandleBuffer;
> > +  EFI_STATUS           Status;
> > +  UINTN                HandleCount;
> > +  UINTN                Index;
> > +
> > +  /* Locate Handles of all EMBEDDED_GPIO producers */
> > +  Status = gBS->LocateHandleBuffer (ByProtocol,
> > +                  &gEmbeddedGpioProtocolGuid,
> > +                  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;
> > +    }
> > +
> > +    /*
> > +     * Check if the driver type matches the requested value.
> > +     * In case of the success open the GPIO protocol and return.
> > +     * If there is no match, repeat procedure with the next handle.
> > +     */
> > +    Status = MvGpioFindMatchingDriver (GpioDriverType,
> > +               HandleBuffer[Index],
> > +               DevicePath,
> > +               GpioProtocol);
> > +    if (Status == EFI_NOT_FOUND) {
> > +      continue;
> > +    } else if (EFI_ERROR (Status)) {
> > +      DEBUG ((DEBUG_ERROR,
> > +        "%a: Unable to open GPIO protocol\n",
> > +        __FUNCTION__));
> > +    }
> > +
> > +    gBS->FreePool (HandleBuffer);
> > +
> > +    return Status;
> > +  }
> > +
> > +  /* No matching GPIO protocol producer was found */
> > +  gBS->FreePool (HandleBuffer);
> > +
> > +  return EFI_NOT_FOUND;
> > +}
> > +
> > +#endif // __MV_GPIO_PROTOCOL_H__
> > --
> > 2.7.4
> >