Re: [platforms: PATCH 06/12] Marvell/Drivers: MvBoardDesc: Extend protocol with GPIO support

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

On Sat, Oct 20, 2018 at 03:57:35AM +0200, Marcin Wojtas wrote:
> Introduce new callback that can provide information
> about GPIO SoC controllers, as well as on-board
> I2C IO expanders. According ArmadaSoCDescLib
> ArmadaBoardDescLib routines are used for
> obtaining required data.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf |  1 +
>  Silicon/Marvell/Include/Protocol/BoardDesc.h         |  8 ++++
>  Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c   | 48 ++++++++++++++++++++
>  3 files changed, 57 insertions(+)
> 
> diff --git a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf
> index 41f72d6..0b93948 100644
> --- a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf
> +++ b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf
> @@ -47,6 +47,7 @@
>    Silicon/Marvell/Marvell.dec
>  
>  [LibraryClasses]
> +  ArmadaBoardDescLib
>    ArmadaSoCDescLib
>    DebugLib
>    MemoryAllocationLib
> diff --git a/Silicon/Marvell/Include/Protocol/BoardDesc.h b/Silicon/Marvell/Include/Protocol/BoardDesc.h
> index 1d57a16..e06d689 100644
> --- a/Silicon/Marvell/Include/Protocol/BoardDesc.h
> +++ b/Silicon/Marvell/Include/Protocol/BoardDesc.h
> @@ -50,6 +50,13 @@ EFI_STATUS
>  
>  typedef
>  EFI_STATUS
> +(EFIAPI *MV_BOARD_DESC_GPIO_GET) (
> +  IN MARVELL_BOARD_DESC_PROTOCOL  *This,
> +  IN OUT MV_BOARD_GPIO_DESC      **GpioDesc
> +  );
> +
> +typedef
> +EFI_STATUS
>  (EFIAPI *MV_BOARD_DESC_I2C_GET) (
>    IN MARVELL_BOARD_DESC_PROTOCOL  *This,
>    IN OUT MV_BOARD_I2C_DESC       **I2cDesc
> @@ -106,6 +113,7 @@ VOID
>  struct _MARVELL_BOARD_DESC_PROTOCOL {
>    MV_BOARD_DESC_AHCI_GET         BoardDescAhciGet;
>    MV_BOARD_DESC_COMPHY_GET       BoardDescComPhyGet;
> +  MV_BOARD_DESC_GPIO_GET         BoardDescGpioGet;
>    MV_BOARD_DESC_I2C_GET          BoardDescI2cGet;
>    MV_BOARD_DESC_MDIO_GET         BoardDescMdioGet;
>    MV_BOARD_DESC_PP2_GET          BoardDescPp2Get;
> diff --git a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c
> index 39dc06c..948a6a0 100644
> --- a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c
> +++ b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c
> @@ -100,6 +100,53 @@ MvBoardDescComPhyGet (
>  
>  STATIC
>  EFI_STATUS
> +MvBoardDescGpioGet (
> +  IN MARVELL_BOARD_DESC_PROTOCOL  *This,
> +  IN OUT MV_BOARD_GPIO_DESC      **GpioDesc
> +  )
> +{
> +  MV_I2C_IO_EXPANDER_DESC *I2cIoExpanderDesc;
> +  UINTN SoCGpioCount, I2cIoExpanderCount;
> +  STATIC MV_BOARD_GPIO_DESC *BoardDesc;
> +  MV_SOC_GPIO_DESC *SoCDesc;
> +  EFI_STATUS Status;
> +
> +  if (BoardDesc != NULL) {
> +    *GpioDesc = BoardDesc;
> +    return EFI_SUCCESS;
> +  }

For a completely opposite comment to the previous patch:
I would much prefer for BoardDesc to be a globally visible variable
with a g prefix and proper namespace.

Even though it itself is not globally visible, the above maneuver
makes it effectively so, but not semantically.
It's not like there's a million call sites, so I would prefer if the
check was there rather than in this function.

> +
> +  /* Get SoC data about all available GPIO controllers */
> +  Status = ArmadaSoCDescGpioGet (&SoCDesc, &SoCGpioCount);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  /* Get per-board information about all available I2C IO expanders */
> +  Status = ArmadaBoardDescGpioGet (&I2cIoExpanderDesc, &I2cIoExpanderCount);

Same I2C->GPIO comments apply here.

> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  /* Allocate and fill board description */
> +  BoardDesc = AllocateZeroPool (sizeof (MV_BOARD_GPIO_DESC));
> +  if (BoardDesc == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: Cannot allocate memory\n", __FUNCTION__));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  BoardDesc->SoC = SoCDesc;
> +  BoardDesc->GpioDevCount = SoCGpioCount;
> +  BoardDesc->I2cIoExpanderDesc = I2cIoExpanderDesc;
> +  BoardDesc->I2cIoExpanderCount = I2cIoExpanderCount;
> +
> +  *GpioDesc = BoardDesc;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +STATIC
> +EFI_STATUS
>  MvBoardDescI2cGet (
>    IN MARVELL_BOARD_DESC_PROTOCOL  *This,
>    IN OUT MV_BOARD_I2C_DESC       **I2cDesc
> @@ -556,6 +603,7 @@ MvBoardDescInitProtocol (
>  {
>    BoardDescProtocol->BoardDescAhciGet = MvBoardDescAhciGet;
>    BoardDescProtocol->BoardDescComPhyGet = MvBoardDescComPhyGet;
> +  BoardDescProtocol->BoardDescGpioGet = MvBoardDescGpioGet;
>    BoardDescProtocol->BoardDescI2cGet = MvBoardDescI2cGet;

Even more so because of the two lines above :)

/
    Leif

>    BoardDescProtocol->BoardDescMdioGet = MvBoardDescMdioGet;
>    BoardDescProtocol->BoardDescPp2Get = MvBoardDescPp2Get;
> -- 
> 2.7.4
>