Re: [edk2] [platforms: PATCH v2 07/12] Marvell/Protocol: Introduce GPIO helper header
Hi Leif,
wt., 15 sty 2019 o 00:12 Leif Lindholm napisał(a):
>
> On Thu, Jan 10, 2019 at 02:44:34AM +0100, Marcin Wojtas wrote:
> > From: jinghua
> >
> > 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
> > ---
> > 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 000..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
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +
> > +typedef enum {
> > + MV_GPIO_DRIVER_TYPE_SOC_CONTROLLER,
> > +} MV_GPIO_DRIVER_TYPE;
> > +
> > +typedef struct {
> > + VENDOR_DEVICE_PATHHeader;
> > + MV_GPIO_DRIVER_TYPE GpioDriverType;
> > + EFI_DEVICE_PATH_PROTOCOL End;
> > +} MV_GPIO_DEVICE_PATH;
> > +
> > +typedef struct {
> > + UINTNControllerId;
> > + UINTNPinNumber;
> > + 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_TYPEGpioDriverType,
> > + 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_TYPEGpioDriverType,
> > + IN OUT EMBEDDED_GPIO**GpioProtocol
> > + )
> > +{
> > + EFI_DEVICE_PATH *DevicePath;
> > + EFI_HANDLE *HandleBuffer;
> > + EFI_STATUS Status;
> > + UINTNHandleCount;
> > + UINTNIndex;
> > +
> > + /* Locate Handles of all EMBEDDED_GPIO producers */
> > + Status = gBS->LocateHand
Re: [edk2] [platforms: PATCH v2 07/12] Marvell/Protocol: Introduce GPIO helper header
On Thu, Jan 10, 2019 at 02:44:34AM +0100, Marcin Wojtas wrote:
> From: jinghua
>
> 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
> ---
> 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 000..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
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +typedef enum {
> + MV_GPIO_DRIVER_TYPE_SOC_CONTROLLER,
> +} MV_GPIO_DRIVER_TYPE;
> +
> +typedef struct {
> + VENDOR_DEVICE_PATHHeader;
> + MV_GPIO_DRIVER_TYPE GpioDriverType;
> + EFI_DEVICE_PATH_PROTOCOL End;
> +} MV_GPIO_DEVICE_PATH;
> +
> +typedef struct {
> + UINTNControllerId;
> + UINTNPinNumber;
> + 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.
/
Leif
> + IN MV_GPIO_DRIVER_TYPEGpioDriverType,
> + 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_TYPEGpioDriverType,
> + IN OUT EMBEDDED_GPIO**GpioProtocol
> + )
> +{
> + EFI_DEVICE_PATH *DevicePath;
> + EFI_HANDLE *HandleBuffer;
> + EFI_STATUS Status;
> + UINTNHandleCount;
> + UINTNIndex;
> +
> + /* 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 */
> +
