Re: [PATCH 02/19] Silicon/NXP: changes to use I2clib in i2cdxe

["Leif Lindholm" <leif@nuviainc.com>]

On Fri, Feb 07, 2020 at 18:13:11 +0530, Pankaj Bansal wrote:
> I2c lib contains the i2c controller functionality. this can be used
> in I2c DXE driver to communicate with i2c devices.

This is one patch.

> There was a bug in I2C DXE implementation, which caused the Ds1307 RTC
> device to issue two operation for register write, while this is a single
> operation task. refer page 12 (Slave Receiver Mode (Write Mode)) on
> 
> https://datasheets.maximintegrated.com/en/ds/DS1307.pdf
> 
> with i2c lib implementation, this bug has been fixed. so modify ds1307
> code accordingly.

This is a separate patch.
Please split it out separately.
Ideally the change in I2clib would happen in the same patch.

> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
>  .../Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c |  23 +-
>  Silicon/NXP/Drivers/I2cDxe/I2cDxe.c           | 526 +-----------------
>  Silicon/NXP/Drivers/I2cDxe/I2cDxe.h           |  44 +-
>  Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf         |   3 +-
>  4 files changed, 31 insertions(+), 565 deletions(-)
> 
> diff --git a/Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c b/Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c
> index 88dc198ffe..444e011248 100644
> --- a/Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c
> +++ b/Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c
> @@ -5,7 +5,7 @@
>    EmbeddedPkg/Library/TemplateRealTimeClockLib/RealTimeClockLib.c
>  
>    Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
> -  Copyright 2017 NXP
> +  Copyright 2017, 2020 NXP
>  
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
> @@ -28,6 +28,11 @@ STATIC EFI_I2C_MASTER_PROTOCOL    *mI2cMaster;
>  
>  /**
>    Read RTC register.
> +  Data Read-Slave Transmitter Mode
> +
> +  <Slave Address> <Word Address (n)> <Slave Address> <Data(n)> <Data(n+1)> <Data(n+2)> <Data(n+X)>
> +
> +  The first byte is received and handled as in the slave receiver mode.

And while I greatly approve of the improved comments (especially if
they were wrapped at 80 characters), this is yet another separate patch.

>  
>    @param  RtcRegAddr       Register offset of RTC to be read.
>  
> @@ -69,6 +74,9 @@ RtcRead (
>  
>  /**
>    Write RTC register.
> +  Data Write-Slave Receiver Mode
> +
> +  <Slave Address> <Word Address (n)> <Data(n)> <Data(n+1)> <Data(n+X)>
>  
>    @param  RtcRegAddr       Register offset of RTC to write.
>    @param  Val              Value to be written
> @@ -84,16 +92,15 @@ RtcWrite (
>  {
>    RTC_I2C_REQUEST          Req;
>    EFI_STATUS               Status;
> +  UINT8                    Buffer[2];
>  
> -  Req.OperationCount = 2;
> +  Req.OperationCount = 1;
> +  Buffer[0] = RtcRegAddr;
> +  Buffer[1] = Val;
>  
>    Req.SetAddressOp.Flags = 0;
> -  Req.SetAddressOp.LengthInBytes = sizeof (RtcRegAddr);
> -  Req.SetAddressOp.Buffer = &RtcRegAddr;
> -
> -  Req.GetSetDateTimeOp.Flags = 0;
> -  Req.GetSetDateTimeOp.LengthInBytes = sizeof (Val);
> -  Req.GetSetDateTimeOp.Buffer = &Val;
> +  Req.SetAddressOp.LengthInBytes = sizeof (Buffer);
> +  Req.SetAddressOp.Buffer = Buffer;
>  
>    Status = mI2cMaster->StartRequest (mI2cMaster, FixedPcdGet8 (PcdI2cSlaveAddress),
>                                       (VOID *)&Req,
> diff --git a/Silicon/NXP/Drivers/I2cDxe/I2cDxe.c b/Silicon/NXP/Drivers/I2cDxe/I2cDxe.c
> index 853c426fbc..8e3a82efca 100644
> --- a/Silicon/NXP/Drivers/I2cDxe/I2cDxe.c
> +++ b/Silicon/NXP/Drivers/I2cDxe/I2cDxe.c
> @@ -1,7 +1,7 @@
>  /** I2cDxe.c
>    I2c driver APIs for read, write, initialize, set speed and reset
>  
> -  Copyright 2017-2019 NXP
> +  Copyright 2017-2020 NXP
>  
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
> @@ -15,6 +15,7 @@
>  #include <Library/TimerLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/UefiLib.h>
> +#include <Library/I2cLib.h>
>  
>  #include "I2cDxe.h"
>  
> @@ -25,444 +26,6 @@ STATIC CONST EFI_I2C_CONTROLLER_CAPABILITIES mI2cControllerCapabilities = {
>    0
>  };
>  
> -STATIC CONST CLK_DIV mClkDiv[] = {
> -  { 20,  0x00 }, { 22, 0x01 }, { 24, 0x02 }, { 26, 0x03 },
> -  { 28,  0x04 }, { 30, 0x05 }, { 32, 0x09 }, { 34, 0x06 },
> -  { 36,  0x0A }, { 40, 0x07 }, { 44, 0x0C }, { 48, 0x0D },
> -  { 52,  0x43 }, { 56, 0x0E }, { 60, 0x45 }, { 64, 0x12 },
> -  { 68,  0x0F }, { 72, 0x13 }, { 80, 0x14 }, { 88, 0x15 },
> -  { 96,  0x19 }, { 104, 0x16 }, { 112, 0x1A }, { 128, 0x17 },
> -  { 136, 0x4F }, { 144, 0x1C }, { 160, 0x1D }, { 176, 0x55 },
> -  { 192, 0x1E }, { 208, 0x56 }, { 224, 0x22 }, { 228, 0x24 },
> -  { 240, 0x1F }, { 256, 0x23 }, { 288, 0x5C }, { 320, 0x25 },
> -  { 384, 0x26 }, { 448, 0x2A }, { 480, 0x27 }, { 512, 0x2B },
> -  { 576, 0x2C }, { 640, 0x2D }, { 768, 0x31 }, { 896, 0x32 },
> -  { 960, 0x2F }, { 1024, 0x33 }, { 1152, 0x34 }, { 1280, 0x35 },
> -  { 1536, 0x36 }, { 1792, 0x3A }, { 1920, 0x37 }, { 2048, 0x3B },
> -  { 2304, 0x3C }, { 2560, 0x3D }, { 3072, 0x3E }, { 3584, 0x7A },
> -  { 3840, 0x3F }, { 4096, 0x7B }, { 5120, 0x7D }, { 6144, 0x7E }
> -};
> -
> -/**
> -  Calculate and return proper clock divider
> -
> -  @param  Rate       desired clock rate
> -
> -  @retval ClkDiv     Index value used to get Bus Clock Rate
> -
> -**/
> -STATIC
> -UINT8
> -GetClkDivIndex (
> -  IN  UINT32         Rate
> -  )
> -{
> -  UINTN              ClkRate;
> -  UINT32             Div;
> -  UINT8              Index;
> -
> -  Index = 0;
> -  ClkRate = GetBusFrequency ();
> -
> -  Div = (ClkRate + Rate - 1) / Rate;
> -
> -  if (Div < mClkDiv[0].SCLDivider) {
> -    return 0;
> -  }
> -
> -  do {
> -    if (mClkDiv[Index].SCLDivider >= Div ) {
> -      return Index;
> -    }
> -    Index++;
> -  } while (Index < ARRAY_SIZE (mClkDiv));
> -
> -  return (ARRAY_SIZE (mClkDiv) - 1);
> -}
> -
> -/**
> -  Function used to check if i2c is in mentioned state or not
> -
> -  @param   I2cRegs        Pointer to I2C registers
> -  @param   State          i2c state need to be checked
> -
> -  @retval  EFI_NOT_READY  Arbitration was lost
> -  @retval  EFI_TIMEOUT    Timeout occured
> -  @retval  CurrState      Value of state register
> -
> -**/
> -STATIC
> -EFI_STATUS
> -WaitForI2cState (
> -  IN  I2C_REGS            *I2cRegs,
> -  IN  UINT32              State
> -  )
> -{
> -  UINT8                   CurrState;
> -  UINT64                  Count;
> -
> -  for (Count = 0; Count < I2C_STATE_RETRIES; Count++) {
> -    MemoryFence ();
> -    CurrState = MmioRead8 ((UINTN)&I2cRegs->I2cSr);
> -    if (CurrState & I2C_SR_IAL) {
> -      MmioWrite8 ((UINTN)&I2cRegs->I2cSr, CurrState | I2C_SR_IAL);
> -      return EFI_NOT_READY;
> -    }
> -
> -    if ((CurrState & (State >> 8)) == (UINT8)State) {
> -      return CurrState;
> -    }
> -  }
> -
> -  return EFI_TIMEOUT;
> -}
> -
> -/**
> -  Function to transfer byte on i2c
> -
> -  @param   I2cRegs        Pointer to i2c registers
> -  @param   Byte           Byte to be transferred on i2c bus
> -
> -  @retval  EFI_NOT_READY  Arbitration was lost
> -  @retval  EFI_TIMEOUT    Timeout occured
> -  @retval  EFI_NOT_FOUND  ACK was not recieved
> -  @retval  EFI_SUCCESS    Data transfer was succesful
> -
> -**/
> -STATIC
> -EFI_STATUS
> -TransferByte (
> -  IN  I2C_REGS            *I2cRegs,
> -  IN  UINT8               Byte
> -  )
> -{
> -  EFI_STATUS              RetVal;
> -
> -  MmioWrite8 ((UINTN)&I2cRegs->I2cSr, I2C_SR_IIF_CLEAR);
> -  MmioWrite8 ((UINTN)&I2cRegs->I2cDr, Byte);
> -
> -  RetVal = WaitForI2cState (I2cRegs, IIF);
> -  if ((RetVal == EFI_TIMEOUT) || (RetVal == EFI_NOT_READY)) {
> -    return RetVal;
> -  }
> -
> -  if (RetVal & I2C_SR_RX_NO_AK) {
> -    return EFI_NOT_FOUND;
> -  }
> -
> -  return EFI_SUCCESS;
> -}
> -
> -/**
> -  Function to stop transaction on i2c bus
> -
> -  @param   I2cRegs          Pointer to i2c registers
> -
> -  @retval  EFI_NOT_READY    Arbitration was lost
> -  @retval  EFI_TIMEOUT      Timeout occured
> -  @retval  EFI_SUCCESS      Stop operation was successful
> -
> -**/
> -STATIC
> -EFI_STATUS
> -I2cStop (
> -  IN  I2C_REGS             *I2cRegs
> -  )
> -{
> -  EFI_STATUS               RetVal;
> -  UINT32                   Temp;
> -
> -  Temp = MmioRead8 ((UINTN)&I2cRegs->I2cCr);
> -
> -  Temp &= ~(I2C_CR_MSTA | I2C_CR_MTX);
> -  MmioWrite8 ((UINTN)&I2cRegs->I2cCr, Temp);
> -
> -  RetVal = WaitForI2cState (I2cRegs, BUS_IDLE);
> -
> -  if (RetVal < 0) {
> -    return RetVal;
> -  } else {
> -    return EFI_SUCCESS;
> -  }
> -}
> -
> -/**
> -  Function to send start signal, Chip Address and
> -  memory offset
> -
> -  @param   I2cRegs         Pointer to i2c base registers
> -  @param   Chip            Chip Address
> -  @param   Offset          Slave memory's offset
> -  @param   AddressLength   length of chip address
> -
> -  @retval  EFI_NOT_READY   Arbitration lost
> -  @retval  EFI_TIMEOUT     Failed to initialize data transfer in predefined time
> -  @retval  EFI_NOT_FOUND   ACK was not recieved
> -  @retval  EFI_SUCCESS     Read was successful
> -
> -**/
> -STATIC
> -EFI_STATUS
> -InitTransfer (
> -  IN  I2C_REGS             *I2cRegs,
> -  IN  UINT8                Chip,
> -  IN  UINT32               Offset,
> -  IN  INT32                AddressLength
> -  )
> -{
> -  UINT32                   Temp;
> -  EFI_STATUS               RetVal;
> -
> -  // Enable I2C controller
> -  if (MmioRead8 ((UINTN)&I2cRegs->I2cCr) & I2C_CR_IDIS) {
> -    MmioWrite8 ((UINTN)&I2cRegs->I2cCr, I2C_CR_IEN);
> -  }
> -
> -  if (MmioRead8 ((UINTN)&I2cRegs->I2cAdr) == (Chip << 1)) {
> -    MmioWrite8 ((UINTN)&I2cRegs->I2cAdr, (Chip << 1) ^ 2);
> -  }
> -
> -  MmioWrite8 ((UINTN)&I2cRegs->I2cSr, I2C_SR_IIF_CLEAR);
> -  RetVal = WaitForI2cState (I2cRegs, BUS_IDLE);
> -  if ((RetVal == EFI_TIMEOUT) || (RetVal == EFI_NOT_READY)) {
> -    return RetVal;
> -  }
> -
> -  // Start I2C transaction
> -  Temp = MmioRead8 ((UINTN)&I2cRegs->I2cCr);
> -  // set to master mode
> -  Temp |= I2C_CR_MSTA;
> -  MmioWrite8 ((UINTN)&I2cRegs->I2cCr, Temp);
> -
> -  RetVal = WaitForI2cState (I2cRegs, BUS_BUSY);
> -  if ((RetVal == EFI_TIMEOUT) || (RetVal == EFI_NOT_READY)) {
> -    return RetVal;
> -  }
> -
> -  Temp |= I2C_CR_MTX | I2C_CR_TX_NO_AK;
> -  MmioWrite8 ((UINTN)&I2cRegs->I2cCr, Temp);
> -
> -  // write slave Address
> -  RetVal = TransferByte (I2cRegs, Chip << 1);
> -  if (RetVal != EFI_SUCCESS) {
> -    return RetVal;
> -  }
> -
> -  if (AddressLength >= 0) {
> -    while (AddressLength--) {
> -      RetVal = TransferByte (I2cRegs, (Offset >> (AddressLength * 8)) & 0xff);
> -      if (RetVal != EFI_SUCCESS)
> -        return RetVal;
> -    }
> -  }
> -  return EFI_SUCCESS;
> -}
> -
> -/**
> -  Function to check if i2c bus is idle
> -
> -  @param   Base          Pointer to base address of I2c controller
> -
> -  @retval  EFI_SUCCESS
> -
> -**/
> -STATIC
> -INT32
> -I2cBusIdle (
> -  IN  VOID               *Base
> -  )
> -{
> -  return EFI_SUCCESS;
> -}
> -
> -/**
> -  Function to initiate data transfer on i2c bus
> -
> -  @param   I2cRegs         Pointer to i2c base registers
> -  @param   Chip            Chip Address
> -  @param   Offset          Slave memory's offset
> -  @param   AddressLength   length of chip address
> -
> -  @retval  EFI_NOT_READY   Arbitration lost
> -  @retval  EFI_TIMEOUT     Failed to initialize data transfer in predefined time
> -  @retval  EFI_NOT_FOUND   ACK was not recieved
> -  @retval  EFI_SUCCESS     Read was successful
> -
> -**/
> -STATIC
> -EFI_STATUS
> -InitDataTransfer (
> -  IN  I2C_REGS             *I2cRegs,
> -  IN  UINT8                Chip,
> -  IN  UINT32               Offset,
> -  IN  INT32                AddressLength
> -  )
> -{
> -  EFI_STATUS               RetVal;
> -  INT32                    Retry;
> -
> -  for (Retry = 0; Retry < RETRY_COUNT; Retry++) {
> -    RetVal = InitTransfer (I2cRegs, Chip, Offset, AddressLength);
> -    if (RetVal == EFI_SUCCESS) {
> -      return EFI_SUCCESS;
> -    }
> -
> -    I2cStop (I2cRegs);
> -
> -    if (EFI_NOT_FOUND == RetVal) {
> -      return RetVal;
> -    }
> -
> -    // Disable controller
> -    if (RetVal != EFI_NOT_READY) {
> -      MmioWrite8 ((UINTN)&I2cRegs->I2cCr, I2C_CR_IDIS);
> -    }
> -
> -    if (I2cBusIdle (I2cRegs) < 0) {
> -      break;
> -    }
> -  }
> -  return RetVal;
> -}
> -
> -/**
> -  Function to read data using i2c bus
> -
> -  @param   BaseAddr        I2c Controller Base Address
> -  @param   Chip            Address of slave device from where data to be read
> -  @param   Offset          Offset of slave memory
> -  @param   AddressLength   Address length of slave
> -  @param   Buffer          A pointer to the destination buffer for the data
> -  @param   Len             Length of data to be read
> -
> -  @retval  EFI_NOT_READY   Arbitration lost
> -  @retval  EFI_TIMEOUT     Failed to initialize data transfer in predefined time
> -  @retval  EFI_NOT_FOUND   ACK was not recieved
> -  @retval  EFI_SUCCESS     Read was successful
> -
> -**/
> -STATIC
> -EFI_STATUS
> -I2cDataRead (
> -  IN  UINTN                BaseAddr,
> -  IN  UINT8                Chip,
> -  IN  UINT32               Offset,
> -  IN  UINT32               AddressLength,
> -  IN  UINT8                *Buffer,
> -  IN  UINT32               Len
> -  )
> -{
> -  EFI_STATUS               RetVal;
> -  UINT32                   Temp;
> -  INT32                    I;
> -  I2C_REGS                 *I2cRegs;
> -
> -  I2cRegs = (I2C_REGS *)(BaseAddr);
> -
> -  RetVal = InitDataTransfer (I2cRegs, Chip, Offset, AddressLength);
> -  if (RetVal != EFI_SUCCESS) {
> -    return RetVal;
> -  }
> -
> -  Temp = MmioRead8 ((UINTN)&I2cRegs->I2cCr);
> -  Temp |= I2C_CR_RSTA;
> -  MmioWrite8 ((UINTN)&I2cRegs->I2cCr, Temp);
> -
> -  RetVal = TransferByte (I2cRegs, (Chip << 1) | 1);
> -  if (RetVal != EFI_SUCCESS) {
> -    I2cStop (I2cRegs);
> -    return RetVal;
> -  }
> -
> -  // setup bus to read data
> -  Temp = MmioRead8 ((UINTN)&I2cRegs->I2cCr);
> -  Temp &= ~(I2C_CR_MTX | I2C_CR_TX_NO_AK);
> -  if (Len == 1) {
> -    Temp |= I2C_CR_TX_NO_AK;
> -  }
> -
> -  MmioWrite8 ((UINTN)&I2cRegs->I2cCr, Temp);
> -  MmioWrite8 ((UINTN)&I2cRegs->I2cSr, I2C_SR_IIF_CLEAR);
> -
> -  // Dummy Read to initiate recieve operation
> -  MmioRead8 ((UINTN)&I2cRegs->I2cDr);
> -
> -  for (I = 0; I < Len; I++) {
> -    RetVal = WaitForI2cState (I2cRegs, IIF);
> -    if ((RetVal == EFI_TIMEOUT) || (RetVal == EFI_NOT_READY)) {
> -       I2cStop (I2cRegs);
> -       return RetVal;
> -    }
> -    //
> -    // It must generate STOP before read I2DR to prevent
> -    // controller from generating another clock cycle
> -    //
> -    if (I == (Len - 1)) {
> -      I2cStop (I2cRegs);
> -    } else if (I == (Len - 2)) {
> -      Temp = MmioRead8 ((UINTN)&I2cRegs->I2cCr);
> -      Temp |= I2C_CR_TX_NO_AK;
> -      MmioWrite8 ((UINTN)&I2cRegs->I2cCr, Temp);
> -    }
> -    MmioWrite8 ((UINTN)&I2cRegs->I2cSr, I2C_SR_IIF_CLEAR);
> -    Buffer[I] = MmioRead8 ((UINTN)&I2cRegs->I2cDr);
> -  }
> -
> -  I2cStop (I2cRegs);
> -
> -  return EFI_SUCCESS;
> -}
> -
> -/**
> -  Function to write data using i2c bus
> -
> -  @param   BaseAddr        I2c Controller Base Address
> -  @param   Chip            Address of slave device where data to be written
> -  @param   Offset          Offset of slave memory
> -  @param   AddressLength   Address length of slave
> -  @param   Buffer          A pointer to the source buffer for the data
> -  @param   Len             Length of data to be write
> -
> -  @retval  EFI_NOT_READY   Arbitration lost
> -  @retval  EFI_TIMEOUT     Failed to initialize data transfer in predefined time
> -  @retval  EFI_NOT_FOUND   ACK was not recieved
> -  @retval  EFI_SUCCESS     Read was successful
> -
> -**/
> -STATIC
> -EFI_STATUS
> -I2cDataWrite (
> -  IN  UINTN                BaseAddr,
> -  IN  UINT8                Chip,
> -  IN  UINT32               Offset,
> -  IN  INT32                AddressLength,
> -  OUT UINT8                *Buffer,
> -  IN  INT32                Len
> -  )
> -{
> -  EFI_STATUS               RetVal;
> -  I2C_REGS                 *I2cRegs;
> -  INT32                    I;
> -
> -  I2cRegs = (I2C_REGS *)BaseAddr;
> -
> -  RetVal = InitDataTransfer (I2cRegs, Chip, Offset, AddressLength);
> -  if (RetVal != EFI_SUCCESS) {
> -    return RetVal;
> -  }
> -
> -  // Write operation
> -  for (I = 0; I < Len; I++) {
> -    RetVal = TransferByte (I2cRegs, Buffer[I]);
> -    if (RetVal != EFI_SUCCESS) {
> -      break;
> -    }
> -  }
> -
> -  I2cStop (I2cRegs);
> -  return RetVal;
> -}
> -
>  /**
>    Function to set i2c bus frequency
>  
> @@ -479,22 +42,17 @@ SetBusFrequency (
>    IN OUT UINTN                       *BusClockHertz
>   )
>  {
> -  I2C_REGS                 *I2cRegs;
> -  UINT8                    ClkId;
> -  UINT8                    SpeedId;
> +  UINTN                    I2cBase;
> +  UINT64                   I2cClock;
>    NXP_I2C_MASTER           *I2c;
>  
>    I2c = NXP_I2C_FROM_THIS (This);
>  
> -  I2cRegs = (I2C_REGS *)(I2c->Dev->Resources[0].AddrRangeMin);
> -
> -  ClkId = GetClkDivIndex (*BusClockHertz);
> -  SpeedId = mClkDiv[ClkId].BusClockRate;
> +  I2cBase = (UINTN)(I2c->Dev->Resources[0].AddrRangeMin);
>  
> -  // Store divider value
> -  MmioWrite8 ((UINTN)&I2cRegs->I2cFdr, SpeedId);
> +  I2cClock = GetBusFrequency ();
>  
> -  MemoryFence ();
> +  I2cInitialize (I2cBase, I2cClock, *BusClockHertz);
>  
>    return EFI_SUCCESS;
>  }
> @@ -513,19 +71,6 @@ Reset (
>    IN CONST EFI_I2C_MASTER_PROTOCOL *This
>    )
>  {
> -  I2C_REGS                         *I2cRegs;
> -  NXP_I2C_MASTER                   *I2c;
> -
> -  I2c = NXP_I2C_FROM_THIS (This);
> -
> -  I2cRegs = (I2C_REGS *)(I2c->Dev->Resources[0].AddrRangeMin);
> -
> -  // Reset module
> -  MmioWrite8 ((UINTN)&I2cRegs->I2cCr, I2C_CR_IDIS);
> -  MmioWrite8 ((UINTN)&I2cRegs->I2cSr, 0);
> -
> -  MemoryFence ();
> -
>    return EFI_SUCCESS;
>  }
>  
> @@ -540,62 +85,17 @@ StartRequest (
>    OUT EFI_STATUS                   *I2cStatus       OPTIONAL
>    )
>  {
> -  NXP_I2C_MASTER                   *I2c;
> -  UINT32                           Count;
> -  INT32                            RetVal;
> -  UINT32                           Length;
> -  UINT8                            *Buffer;
> -  UINT32                           Flag;
> -  UINT32                           RegAddress;
> -  UINT32                           OffsetLength;
> -
> -  RegAddress = 0;
> +  NXP_I2C_MASTER           *I2c;
> +  UINTN                    I2cBase;
> +  EFI_STATUS               Status;
>  
>    I2c = NXP_I2C_FROM_THIS (This);
>  
> -  if (RequestPacket->OperationCount <= 0) {
> -    DEBUG ((DEBUG_ERROR,"%a: Operation count is not valid %d\n",
> -           __FUNCTION__, RequestPacket->OperationCount));
> -    return EFI_INVALID_PARAMETER;
> -  }
> +  I2cBase = (UINTN)(I2c->Dev->Resources[0].AddrRangeMin);
>  
> -  OffsetLength = RequestPacket->Operation[0].LengthInBytes;
> -  RegAddress = *RequestPacket->Operation[0].Buffer;
> -
> -  for (Count = 1; Count < RequestPacket->OperationCount; Count++) {
> -    Flag = RequestPacket->Operation[Count].Flags;
> -    Length = RequestPacket->Operation[Count].LengthInBytes;
> -    Buffer = RequestPacket->Operation[Count].Buffer;
> -
> -    if (Length <= 0) {
> -      DEBUG ((DEBUG_ERROR,"%a: Invalid length of buffer %d\n",
> -             __FUNCTION__, Length));
> -      return EFI_INVALID_PARAMETER;
> -    }
> -
> -    if (Flag == I2C_FLAG_READ) {
> -      RetVal = I2cDataRead (I2c->Dev->Resources[0].AddrRangeMin, SlaveAddress,
> -                            RegAddress, OffsetLength, Buffer, Length);
> -      if (RetVal != EFI_SUCCESS) {
> -        DEBUG ((DEBUG_ERROR,"%a: I2c read operation failed (error %d)\n",
> -               __FUNCTION__, RetVal));
> -        return RetVal;
> -      }
> -    } else if (Flag == I2C_FLAG_WRITE) {
> -      RetVal = I2cDataWrite (I2c->Dev->Resources[0].AddrRangeMin, SlaveAddress,
> -                             RegAddress, OffsetLength, Buffer, Length);
> -      if (RetVal != EFI_SUCCESS) {
> -        DEBUG ((DEBUG_ERROR,"%a: I2c write operation failed (error %d)\n",
> -               __FUNCTION__, RetVal));
> -        return RetVal;
> -      }
> -    } else {
> -      DEBUG ((DEBUG_ERROR,"%a: Invalid Flag %d\n", __FUNCTION__, Flag));
> -      return EFI_INVALID_PARAMETER;
> -    }
> -  }
> +  Status = I2cBusXfer (I2cBase, SlaveAddress, RequestPacket);
>  
> -  return EFI_SUCCESS;
> +  return Status;
>  }
>  
>  EFI_STATUS
> diff --git a/Silicon/NXP/Drivers/I2cDxe/I2cDxe.h b/Silicon/NXP/Drivers/I2cDxe/I2cDxe.h
> index 02a29a5cf2..88316f3133 100644
> --- a/Silicon/NXP/Drivers/I2cDxe/I2cDxe.h
> +++ b/Silicon/NXP/Drivers/I2cDxe/I2cDxe.h
> @@ -1,7 +1,7 @@
>  /** I2cDxe.h
>    Header defining the constant, base address amd function for I2C controller
>  
> -  Copyright 2017-2019 NXP
> +  Copyright 2017-2020 NXP
>  
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
> @@ -16,32 +16,6 @@
>  #include <Protocol/I2cMaster.h>
>  #include <Protocol/NonDiscoverableDevice.h>
>  
> -#define I2C_CR_IIEN               (1 << 6)
> -#define I2C_CR_MSTA               (1 << 5)
> -#define I2C_CR_MTX                (1 << 4)
> -#define I2C_CR_TX_NO_AK           (1 << 3)
> -#define I2C_CR_RSTA               (1 << 2)
> -
> -#define I2C_SR_ICF                (1 << 7)
> -#define I2C_SR_IBB                (1 << 5)
> -#define I2C_SR_IAL                (1 << 4)
> -#define I2C_SR_IIF                (1 << 1)
> -#define I2C_SR_RX_NO_AK           (1 << 0)
> -
> -#define I2C_CR_IEN                (0 << 7)
> -#define I2C_CR_IDIS               (1 << 7)
> -#define I2C_SR_IIF_CLEAR          (1 << 1)
> -
> -#define BUS_IDLE                  (0 | (I2C_SR_IBB << 8))
> -#define BUS_BUSY                  (I2C_SR_IBB | (I2C_SR_IBB << 8))
> -#define IIF                       (I2C_SR_IIF | (I2C_SR_IIF << 8))
> -
> -#define I2C_FLAG_WRITE            0x0
> -
> -#define I2C_STATE_RETRIES         50000
> -
> -#define RETRY_COUNT               3
> -
>  #define NXP_I2C_SIGNATURE         SIGNATURE_32 ('N', 'I', '2', 'C')
>  #define NXP_I2C_FROM_THIS(a)      CR ((a), NXP_I2C_MASTER, \
>                                      I2cMaster, NXP_I2C_SIGNATURE)
> @@ -63,22 +37,6 @@ typedef struct {
>    NON_DISCOVERABLE_DEVICE         *Dev;
>  } NXP_I2C_MASTER;
>  
> -/**
> -  Record defining i2c registers
> -**/
> -typedef struct {
> -  UINT8     I2cAdr;
> -  UINT8     I2cFdr;
> -  UINT8     I2cCr;
> -  UINT8     I2cSr;
> -  UINT8     I2cDr;
> -} I2C_REGS;
> -
> -typedef struct {
> -  UINT16   SCLDivider;
> -  UINT16   BusClockRate;
> -} CLK_DIV;
> -
>  extern
>  UINT64
>  GetBusFrequency (
> diff --git a/Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf b/Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf
> index 0c0bf63bb2..784139065f 100644
> --- a/Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf
> +++ b/Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf
> @@ -3,7 +3,7 @@
>  #  Component description file for I2c driver
>  #
>  #  Copyright (c) 2015, Freescale Semiconductor, Inc. All rights reserved.
> -#  Copyright 2017-2019 NXP
> +#  Copyright 2017-2020 NXP
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -35,6 +35,7 @@
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
>    UefiLib
> +  I2cLib

Please sort libraryclasses alphabetically.

/
    Leif

>  
>  [Guids]
>    gNxpNonDiscoverableI2cMasterGuid
> -- 
> 2.17.1
>