Re: [platforms: PATCH 09/12] Marvell/Drivers: I2c: Use common header for macros

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

On Sat, Oct 20, 2018 at 03:57:38AM +0200, Marcin Wojtas wrote:
> Hitherto I2c solution used same macros, defined in multiple
> places. Move them to a new common header.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  Silicon/Marvell/Drivers/I2c/MvEepromDxe/MvEepromDxe.h | 10 -------
>  Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h       | 17 ++---------
>  Silicon/Marvell/Include/Protocol/MvI2c.h              | 31 ++++++++++++++++++++
>  Silicon/Marvell/Applications/EepromCmd/EepromCmd.c    |  5 +---
>  Silicon/Marvell/Drivers/I2c/MvEepromDxe/MvEepromDxe.c |  3 +-
>  Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c       |  4 +--
>  6 files changed, 37 insertions(+), 33 deletions(-)
>  create mode 100644 Silicon/Marvell/Include/Protocol/MvI2c.h
> 
> diff --git a/Silicon/Marvell/Drivers/I2c/MvEepromDxe/MvEepromDxe.h b/Silicon/Marvell/Drivers/I2c/MvEepromDxe/MvEepromDxe.h
> index b1af645..c32ee48 100644
> --- a/Silicon/Marvell/Drivers/I2c/MvEepromDxe/MvEepromDxe.h
> +++ b/Silicon/Marvell/Drivers/I2c/MvEepromDxe/MvEepromDxe.h
> @@ -41,16 +41,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  
>  #define MAX_BUFFER_LENGTH 64
>  
> -/*
> - * I2C_FLAG_NORESTART is not part of PI spec, it allows to continue
> - * transmission without repeated start operation.
> - * FIXME: This flag is also defined in Drivers/I2c/MvI2cDxe/MvI2cDxe.h
> - * and it's important to have both version synced. This solution is
> - * temporary and shared flag should be used by both files.
> - * Situation is analogous with I2C_GUID, which also should be common, but is
> - * for now defined same way in two header files.
> - */
> -#define I2C_FLAG_NORESTART 0x00000002
>  #define I2C_GUID \
>    { \
>    0xadc1901b, 0xb83c, 0x4831, { 0x8f, 0x59, 0x70, 0x89, 0x8f, 0x26, 0x57, 0x1e } \
> diff --git a/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h b/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h
> index 3c9beaf..6850c34 100644
> --- a/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h
> +++ b/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.h
> @@ -32,8 +32,8 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  
>  *******************************************************************************/
>  
> -#ifndef __MV_I2C_H__
> -#define __MV_I2C_H__
> +#ifndef __MV_I2C_DXE_H__
> +#define __MV_I2C_DXE_H__
>  
>  #include <Uefi.h>
>  
> @@ -75,17 +75,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  #define I2C_FAST           0x2
>  #define I2C_FASTEST        0x3
>  
> -/*
> - * I2C_FLAG_NORESTART is not part of PI spec, it allows to continue
> - * transmission without repeated start operation.
> - * FIXME: This flag is also defined in
> - * Platforms/Marvell/Include/Protocol/Eeprom.h and it's important to have both
> - * version synced. This solution is temporary and shared flag should be used by
> - * both files.
> - * Situation is analogous with I2C_GUID, which also should be common, but is
> - * for now defined same way in two header files.
> - */
> -#define I2C_FLAG_NORESTART 0x00000002
>  #define I2C_GUID \
>    { \
>    0xadc1901b, 0xb83c, 0x4831, { 0x8f, 0x59, 0x70, 0x89, 0x8f, 0x26, 0x57, 0x1e } \
> @@ -266,4 +255,4 @@ MvI2cEnableConf (
>    IN EFI_STATUS                                          *I2cStatus OPTIONAL
>    );
>  
> -#endif // __MV_I2C_H__
> +#endif // __MV_I2C_DXE_H__
> diff --git a/Silicon/Marvell/Include/Protocol/MvI2c.h b/Silicon/Marvell/Include/Protocol/MvI2c.h
> new file mode 100644
> index 0000000..d8e644e
> --- /dev/null
> +++ b/Silicon/Marvell/Include/Protocol/MvI2c.h
> @@ -0,0 +1,31 @@
> +/**
> +*
> +*  Copyright (c) 2018, Marvell International Ltd. All rights reserved.
> +*
> +*  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_I2C_H__
> +#define __MV_I2C_H__
> +
> +/*
> + * I2C_FLAG_NORESTART is not part of PI spec, it allows to continue
> + * transmission without repeated start operation.
> + */
> +#define I2C_FLAG_NORESTART             0x00000002
> +
> +/*
> + * Helper macros for maintaining multiple I2C buses
> + * and devices defined via EFI_I2C_DEVICE.
> + */
> +#define I2C_DEVICE_ADDRESS(Index)      ((Index) & MAX_UINT16)
> +#define I2C_DEVICE_BUS(Index)          ((Index) >> 16)
> +#define I2C_DEVICE_INDEX(Bus, Address) (((Address) & MAX_UINT16) | (Bus) << 16)
> +
> +#endif
> diff --git a/Silicon/Marvell/Applications/EepromCmd/EepromCmd.c b/Silicon/Marvell/Applications/EepromCmd/EepromCmd.c
> index f43e411..a390f23 100644
> --- a/Silicon/Marvell/Applications/EepromCmd/EepromCmd.c
> +++ b/Silicon/Marvell/Applications/EepromCmd/EepromCmd.c
> @@ -52,10 +52,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  #include <Library/UefiBootServicesTableLib.h>
>  
>  #include <Protocol/Eeprom.h>
> -
> -#define I2C_DEVICE_INDEX(bus, address) (((address) & 0xffff) | (bus) << 16)
> -#define I2C_DEVICE_ADDRESS(index) ((index) & 0xffff)
> -#define I2C_DEVICE_BUS(index) ((index) >> 16)
> +#include <Protocol/MvI2c.h>
>  
>  CONST CHAR16 ShellEepromFileName[] = L"ShellCommand";
>  EFI_HANDLE ShellEepromHiiHandle = NULL;
> diff --git a/Silicon/Marvell/Drivers/I2c/MvEepromDxe/MvEepromDxe.c b/Silicon/Marvell/Drivers/I2c/MvEepromDxe/MvEepromDxe.c
> index 9d2f650..90d0d1e 100644
> --- a/Silicon/Marvell/Drivers/I2c/MvEepromDxe/MvEepromDxe.c
> +++ b/Silicon/Marvell/Drivers/I2c/MvEepromDxe/MvEepromDxe.c
> @@ -35,6 +35,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  #include <Protocol/DriverBinding.h>
>  #include <Protocol/I2cIo.h>
>  #include <Protocol/Eeprom.h>
> +#include <Protocol/MvI2c.h>
>  
>  #include <Library/BaseLib.h>
>  #include <Library/BaseMemoryLib.h>
> @@ -49,8 +50,6 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  
>  #include "MvEepromDxe.h"
>  
> -#define I2C_DEVICE_INDEX(bus, address) (((address) & 0xffff) | (bus) << 16)
> -
>  STATIC CONST EFI_GUID I2cGuid = I2C_GUID;
>  
>  EFI_DRIVER_BINDING_PROTOCOL gDriverBindingProtocol = {
> diff --git a/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c b/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> index 9ec4929..582e2b9 100755
> --- a/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> +++ b/Silicon/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> @@ -37,6 +37,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  #include <Protocol/I2cEnumerate.h>
>  #include <Protocol/I2cBusConfigurationManagement.h>
>  #include <Protocol/DevicePath.h>
> +#include <Protocol/MvI2c.h>
>  
>  #include <Library/BaseLib.h>
>  #include <Library/IoLib.h>
> @@ -628,9 +629,6 @@ MvI2cStartRequest (
>  
>  STATIC CONST EFI_GUID DevGuid = I2C_GUID;
>  
> -#define I2C_DEVICE_INDEX(bus, address) (((address) & 0xffff) | (bus) << 16)
> -#define I2C_DEVICE_ADDRESS(index) ((index) & 0xffff)
> -
>  STATIC
>  EFI_STATUS
>  MvI2cAllocDevice (
> -- 
> 2.7.4
>