Re: [edk2-devel] [PATCH edk2-platforms 1/1] Silicon/NXP: rework IoAccessLib to use single function pointer struct

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 4/23/20 12:09 PM, Leif Lindholm wrote:
> For whatever reason, the function pointer structs (and their retrieval
> functions) were split up on data size boundary - separate for 16-bit,
> 32-bit, and 64-bit.
> 
> This makes the already tedious boilerplate require to deal with these
> hardware quirks even worse, so unify them into a single function pointer
> struct and retrieval function.
> 
> Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> Cc: Pankaj Bansal <pankaj.bansal@nxp.com>
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
> 
> Meenakshi, Pankaj - I have no hardware to verify that this patch works,
> but it builds cleanlyi and isn't exactly complex.
> I will need R-b from at least one of you, and ideally Tested-by as well.
> 
>   Silicon/NXP/Include/Library/IoAccessLib.h     | 52 ++----------
>   Silicon/NXP/Library/IoAccessLib/IoAccessLib.c | 82 ++++---------------
>   2 files changed, 22 insertions(+), 112 deletions(-)
> 
> diff --git a/Silicon/NXP/Include/Library/IoAccessLib.h b/Silicon/NXP/Include/Library/IoAccessLib.h
> index 0b708d544fa7..0f5b19dcf149 100644
> --- a/Silicon/NXP/Include/Library/IoAccessLib.h
> +++ b/Silicon/NXP/Include/Library/IoAccessLib.h
> @@ -15,40 +15,26 @@
>   ///  Structure to have pointer to R/W
>   ///  Mmio operations for 16 bits.
>   ///
> -typedef struct _MMIO_OPERATIONS_16 {
> +typedef struct _MMIO_OPERATIONS {
>     UINT16 (*Read16) (UINTN Address);
>     UINT16 (*Write16) (UINTN Address, UINT16 Value);
>     UINT16 (*Or16) (UINTN Address, UINT16 OrData);
>     UINT16 (*And16) (UINTN Address, UINT16 AndData);
>     UINT16 (*AndThenOr16) (UINTN Address, UINT16 AndData, UINT16 OrData);
> -} MMIO_OPERATIONS_16;
> -
> -///
> -///  Structure to have pointer to R/W
> -///  Mmio operations for 32 bits.
> -///
> -typedef struct _MMIO_OPERATIONS_32 {
>     UINT32 (*Read32) (UINTN Address);
>     UINT32 (*Write32) (UINTN Address, UINT32 Value);
>     UINT32 (*Or32) (UINTN Address, UINT32 OrData);
>     UINT32 (*And32) (UINTN Address, UINT32 AndData);
>     UINT32 (*AndThenOr32) (UINTN Address, UINT32 AndData, UINT32 OrData);
> -} MMIO_OPERATIONS_32;
> -
> -///
> -///  Structure to have pointer to R/W
> -///  Mmio operations for 64 bits.
> -///
> -typedef struct _MMIO_OPERATIONS_64 {
>     UINT64 (*Read64) (UINTN Address);
>     UINT64 (*Write64) (UINTN Address, UINT64 Value);
>     UINT64 (*Or64) (UINTN Address, UINT64 OrData);
>     UINT64 (*And64) (UINTN Address, UINT64 AndData);
>     UINT64 (*AndThenOr64) (UINTN Address, UINT64 AndData, UINT64 OrData);
> -} MMIO_OPERATIONS_64;
> +} MMIO_OPERATIONS;
>   
>   /**
> -  Function to return pointer to 16 bit Mmio operations.
> +  Function to return pointer to Mmio operations.
>   
>     @param  Swap  Flag to tell if Swap is needed or not
>                   on Mmio Operations.
> @@ -56,36 +42,8 @@ typedef struct _MMIO_OPERATIONS_64 {
>     @return       Pointer to Mmio Operations.
>   
>   **/
> -MMIO_OPERATIONS_16 *
> -GetMmioOperations16  (
> -  IN  BOOLEAN  Swap
> -  );
> -
> -/**
> -  Function to return pointer to 32 bit Mmio operations.
> -
> -  @param  Swap  Flag to tell if Swap is needed or not
> -                on Mmio Operations.
> -
> -  @return       Pointer to Mmio Operations.
> -
> -**/
> -MMIO_OPERATIONS_32 *
> -GetMmioOperations32  (
> -  IN  BOOLEAN  Swap
> -  );
> -
> -/**
> -  Function to return pointer to 64 bit Mmio operations.
> -
> -  @param  Swap  Flag to tell if Swap is needed or not
> -                on Mmio Operations.
> -
> -  @return       Pointer to Mmio Operations.
> -
> -**/
> -MMIO_OPERATIONS_64 *
> -GetMmioOperations64  (
> +MMIO_OPERATIONS *
> +GetMmioOperations  (
>     IN  BOOLEAN  Swap
>     );
>   
> diff --git a/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c b/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c
> index 6ed83d019a6e..33039afda40f 100644
> --- a/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c
> +++ b/Silicon/NXP/Library/IoAccessLib/IoAccessLib.c
> @@ -301,39 +301,17 @@ SwapMmioAnd64 (
>     return MmioAnd64 (Address, SwapBytes64 (AndData));
>   }
>   
> -STATIC MMIO_OPERATIONS_16 SwappingFunctions16 = {
> +STATIC MMIO_OPERATIONS SwappingFunctions = {
>     SwapMmioRead16,
>     SwapMmioWrite16,
>     SwapMmioOr16,
>     SwapMmioAnd16,
>     SwapMmioAndThenOr16,
> -};
> -
> -STATIC MMIO_OPERATIONS_16 NonSwappingFunctions16 = {
> -  MmioRead16,
> -  MmioWrite16,
> -  MmioOr16,
> -  MmioAnd16,
> -  MmioAndThenOr16,
> -};
> -
> -STATIC MMIO_OPERATIONS_32 SwappingFunctions32 = {
>     SwapMmioRead32,
>     SwapMmioWrite32,
>     SwapMmioOr32,
>     SwapMmioAnd32,
>     SwapMmioAndThenOr32,
> -};
> -
> -STATIC MMIO_OPERATIONS_32 NonSwappingFunctions32 = {
> -  MmioRead32,
> -  MmioWrite32,
> -  MmioOr32,
> -  MmioAnd32,
> -  MmioAndThenOr32,
> -};
> -
> -STATIC MMIO_OPERATIONS_64 SwappingFunctions64 = {
>     SwapMmioRead64,
>     SwapMmioWrite64,
>     SwapMmioOr64,
> @@ -341,7 +319,17 @@ STATIC MMIO_OPERATIONS_64 SwappingFunctions64 = {
>     SwapMmioAndThenOr64,
>   };
>   
> -STATIC MMIO_OPERATIONS_64 NonSwappingFunctions64 = {
> +STATIC MMIO_OPERATIONS NonSwappingFunctions = {
> +  MmioRead16,
> +  MmioWrite16,
> +  MmioOr16,
> +  MmioAnd16,
> +  MmioAndThenOr16,
> +  MmioRead32,
> +  MmioWrite32,
> +  MmioOr32,
> +  MmioAnd32,
> +  MmioAndThenOr32,
>     MmioRead64,
>     MmioWrite64,
>     MmioOr64,
> @@ -350,7 +338,7 @@ STATIC MMIO_OPERATIONS_64 NonSwappingFunctions64 = {
>   };
>   
>   /**
> -  Function to return pointer to 16 bit Mmio operations.
> +  Function to return pointer to Mmio operations.
>   
>     @param  Swap  Flag to tell if Swap is needed or not
>                   on Mmio Operations.
> @@ -358,47 +346,11 @@ STATIC MMIO_OPERATIONS_64 NonSwappingFunctions64 = {
>     @return       Pointer to Mmio Operations.
>   
>   **/
> -MMIO_OPERATIONS_16 *
> -GetMmioOperations16 (BOOLEAN Swap) {
> +MMIO_OPERATIONS *
> +GetMmioOperations (BOOLEAN Swap) {
>     if (Swap) {
> -    return &SwappingFunctions16;
> +    return &SwappingFunctions;
>     } else {
> -    return &NonSwappingFunctions16;
> -  }
> -}
> -
> -/**
> -  Function to return pointer to 32 bit Mmio operations.
> -
> -  @param  Swap  Flag to tell if Swap is needed or not
> -                on Mmio Operations.
> -
> -  @return       Pointer to Mmio Operations.
> -
> -**/
> -MMIO_OPERATIONS_32 *
> -GetMmioOperations32 (BOOLEAN Swap) {
> -  if (Swap) {
> -    return &SwappingFunctions32;
> -  } else {
> -    return &NonSwappingFunctions32;
> -  }
> -}
> -
> -/**
> -  Function to return pointer to 64 bit Mmio operations.
> -
> -  @param  Swap  Flag to tell if Swap is needed or not
> -                on Mmio Operations.
> -
> -  @return       Pointer to Mmio Operations.
> -
> -**/
> -MMIO_OPERATIONS_64 *
> -GetMmioOperations64 (BOOLEAN Swap) {
> -  if (Swap) {
> -    return &SwappingFunctions64;
> -  } else {
> -    return &NonSwappingFunctions64;
> +    return &NonSwappingFunctions;
>     }
>   }
> 

Nice simplification.
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>