Re: [PATCH edk2-platforms 06/27] Silicon/NXP: Add I2C library support for i.MX platforms

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

On Fri, Sep 21, 2018 at 08:25:57AM +0000, Chris Co wrote:
> This adds support for I2C controller on NXP i.MX platforms.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Christopher Co <christopher.co@microsoft.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  Silicon/NXP/iMXPlatformPkg/Include/iMXI2cLib.h             | 162 +++++++
>  Silicon/NXP/iMXPlatformPkg/Library/iMXI2cLib/iMXI2cLib.c   | 487 ++++++++++++++++++++
>  Silicon/NXP/iMXPlatformPkg/Library/iMXI2cLib/iMXI2cLib.inf |  35 ++
>  3 files changed, 684 insertions(+)
> 
> diff --git a/Silicon/NXP/iMXPlatformPkg/Include/iMXI2cLib.h b/Silicon/NXP/iMXPlatformPkg/Include/iMXI2cLib.h
> new file mode 100644
> index 000000000000..de8a1616fe1b
> --- /dev/null
> +++ b/Silicon/NXP/iMXPlatformPkg/Include/iMXI2cLib.h
> @@ -0,0 +1,162 @@
> +/** @file
> +*
> +*  Copyright (c) 2018 Microsoft Corporation. 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 _IMX_I2C_H_
> +#define _IMX_I2C_H_
> +
> +#pragma pack(push, 1)

I don't see where below this has any effect.
If not required, please drop.

> +
> +typedef union {
> +  UINT16 AsUint16;

Not super happy with this name.
Could it be just "Raw"? (That pattern is used frequently in EDK2.)
Same applies to similar situations below.

> +  struct {
> +    UINT16 Reserved0 : 1;
> +    UINT16 ADR : 7;
> +    UINT16 Reserved1 : 8;
> +  };
> +} IMX_I2C_IADR_REG;

IADR is the name I guess, but REG should ideally be fully written out
as REGISTER.

> +
> +typedef union {
> +  UINT16 AsUint16;
> +  struct {
> +    UINT16 IC : 6;
> +    UINT16 Reserved0 : 10;
> +  };
> +} IMX_I2C_IFDR_REG;
> +
> +typedef union {
> +  UINT16 AsUint16;
> +  struct {
> +    UINT16 Reserved0 : 2;
> +    UINT16 RSTA : 1;
> +    UINT16 TXAK : 1;
> +    UINT16 MTX : 1;
> +    UINT16 MSTA : 1;
> +    UINT16 IIEN : 1;
> +    UINT16 IEN : 1;
> +    UINT16 Reserved1 : 8;
> +  };
> +} IMX_I2C_I2CR_REG;
> +
> +#define IMX_I2C_I2SR_RXAK        0x0001
> +#define IMX_I2C_I2SR_IIF         0x0002
> +#define IMX_I2C_I2SR_SRW         0x0004
> +#define IMX_I2C_I2SR_IAL         0x0010
> +#define IMX_I2C_I2SR_IBB         0x0020
> +#define IMX_I2C_I2SR_IAAS        0x0040
> +#define IMX_I2C_I2SR_ICF         0x0080
> +
> +typedef union {
> +  UINT16 AsUint16;
> +  struct {
> +    UINT16 RXAK : 1;
> +    UINT16 IIF : 1;
> +    UINT16 SRW : 1;
> +    UINT16 Reserved0 : 1;
> +    UINT16 IAL : 1;
> +    UINT16 IBB : 1;
> +    UINT16 IAAS : 1;
> +    UINT16 ICF : 1;
> +    UINT16 Reserved1 : 8;
> +  };
> +} IMX_I2C_I2SR_REG;
> +
> +typedef union {
> +  UINT16 AsUint16;
> +  struct {
> +    UINT16 DATA : 8;
> +    UINT16 Reserved0 : 8;
> +  };
> +} IMX_I2C_I2DR_REG;
> +
> +typedef struct {
> +  IMX_I2C_IADR_REG IADR;
> +  UINT16 Pad0;
> +  IMX_I2C_IFDR_REG IFDR;
> +  UINT16 Pad1;
> +  IMX_I2C_I2CR_REG I2CR;
> +  UINT16 Pad2;
> +  IMX_I2C_I2DR_REG I2SR;
> +  UINT16 Pad3;
> +  IMX_I2C_I2DR_REG I2DR;
> +  UINT16 Pad4;
> +} IMX_I2C_REGS;

And here, REGISTERS.

> +
> +#pragma pack(pop)
> +
> +typedef struct {
> +  UINT32 ControllerAddress;
> +  UINT32 ControllerSlaveAddress;
> +  UINT32 ReferenceFreq;
> +  UINT32 TargetFreq;
> +  UINT32 SlaveAddress;
> +  UINT32 TimeoutInUs;
> +} IMX_I2C_CONFIG;
> +
> +typedef struct {
> +  UINT32 Divider;
> +  UINT32 IC;

Please expand to CamelCase.
It's not obvious what IC stands for.

> +} IMX_I2C_DIVIDER;
> +
> +/**
> +  Perform I2C read operation.
> +
> +  The iMXI2cRead perform I2C read operation by programming the I2C controller.
> +  The caller is responsible to provide I2C controller configuration.
> +
> +  @param[in]    IMX_I2C_CONFIG    Pointer to structure containing the targeted
> +                                  I2C controller to be used for I2C operation.
> +  @param[in]    RegisterAddress   Targeted device register address to start read.
> +  @param[out]   ReadBufferPtr     Caller supplied buffer that would be written
> +                                  into with data from the read operation.
> +  @param[in]    ReadBufferSize    Size of caller supplied buffer.
> +
> +  @retval   RETURN_SUCCESS        I2C Read operation succeeded.
> +  @retval   RETURN_DEVICE_ERROR   The I2C device is not functioning correctly.
> +
> +**/
> +RETURN_STATUS
> +iMXI2cRead (
> +  IN IMX_I2C_CONFIG  *I2cConfigPtr,
> +  IN UINT8            RegisterAddress,
> +  OUT UINT8          *ReadBufferPtr,
> +  IN UINT32           ReadBufferSize
> +  );
> +
> +/**
> +  Perform I2C write operation.
> +
> +  The iMXI2cWrite perform I2C write operation by programming the I2C
> +  controller. The caller is responsible to provide I2C controller
> +  configuration.
> +
> +  @param[in]    IMX_I2C_CONFIG    Pointer to structure containing the targeted
> +                                  I2C controller to be used for I2C operation.
> +  @param[in]    RegisterAddress   Targeted device register address to start write.
> +  @param[out]   WriteBufferPtr    Caller supplied buffer that contained data that
> +                                  would be read from for I2C write operation.
> +  @param[in]    WriteBufferSize   Size of caller supplied buffer.
> +
> +  @retval   RETURN_SUCCESS        I2C Write operation succeeded.
> +  @retval   RETURN_DEVICE_ERROR   The I2C device is not functioning correctly.
> +
> +**/
> +RETURN_STATUS
> +iMXI2cWrite (
> +  IN IMX_I2C_CONFIG  *I2cConfigPtr,
> +  IN UINT8            RegisterAddress,
> +  IN UINT8           *WriteBufferPtr,
> +  IN UINT32           WriteBufferSize
> +  );
> +
> +#endif
> diff --git a/Silicon/NXP/iMXPlatformPkg/Library/iMXI2cLib/iMXI2cLib.c b/Silicon/NXP/iMXPlatformPkg/Library/iMXI2cLib/iMXI2cLib.c
> new file mode 100644
> index 000000000000..6cfe04dba571
> --- /dev/null
> +++ b/Silicon/NXP/iMXPlatformPkg/Library/iMXI2cLib/iMXI2cLib.c
> @@ -0,0 +1,487 @@
> +/** @file
> +*
> +*  Copyright (c) 2018 Microsoft Corporation. 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.
> +*
> +**/
> +
> +#include <Base.h>
> +#include <Uefi.h>
> +
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/TimerLib.h>
> +
> +#include <iMXI2cLib.h>
> +
> +IMX_I2C_DIVIDER DividerValue[] = {

Global variable should have 'm' prefix if scope is this module or 'g'
if external visibility. If only this file, please also make it STATIC.

I'm not super happy about these live-coded values, but I guess adding
a bunch of defines wouldn't help much. But could you add a comment
block explaining roughly what the fields mean?

> +  { 22, 0x20, },

Please drop the second ',' on all lines - IMX_I2C_DIVIDER only has two
structure members.

> +  { 24, 0x21, },
> +  { 26, 0x22, },
> +  { 28, 0x23, },
> +  { 30, 0x00, },
> +  { 32, 0x24, },
> +  { 36, 0x25, },
> +  { 40, 0x26, },
> +  { 42, 0x03, },
> +  { 44, 0x27, },
> +  { 48, 0x28, },
> +  { 52, 0x05, },
> +  { 56, 0x29, },
> +  { 60, 0x06, },
> +  { 64, 0x2A, },
> +  { 72, 0x2B, },
> +  { 80, 0x2C, },
> +  { 88, 0x09, },
> +  { 96, 0x2D, },
> +  { 104, 0x0A, },
> +  { 112, 0x2E, },
> +  { 128, 0x2F, },
> +  { 144, 0x0C, },
> +  { 160, 0x30, },
> +  { 192, 0x31, },
> +  { 224, 0x32, },
> +  { 240, 0x0F, },
> +  { 256, 0x33, },
> +  { 288, 0x10, },
> +  { 320, 0x34, },
> +  { 384, 0x35, },
> +  { 448, 0x36, },
> +  { 480, 0x13, },
> +  { 512, 0x37, },
> +  { 576, 0x14, },
> +  { 640, 0x38, },
> +  { 768, 0x39, },
> +  { 896, 0x3A, },
> +  { 960, 0x17, },
> +  { 1024, 0x3B, },
> +  { 1152, 0x18, },
> +  { 1280, 0x3C, },
> +  { 1536, 0x3D, },
> +  { 1792, 0x3E, },
> +  { 1920, 0x1B, },
> +  { 2048, 0x3F, },
> +  { 2304, 0x1C, },
> +  { 2560, 0x1D, },
> +  { 3072, 0x1E, },
> +  { 3840, 0x1F, },
> +};
> +
> +#define DIVIDER_VALUE_TOTAL (sizeof(DividerValue) / sizeof(DividerValue[0]))

Can use BaseLib ARRAY_SIZE macro instead of adding this custom one.

> +
> +BOOLEAN
> +iMXI2cWaitStatusSet (
> +  IN  IMX_I2C_CONFIG   *I2cConfigPtr,
> +  IN  UINT16           StatusBits
> +  )
> +{
> +  IMX_I2C_REGS      *I2cRegsPtr;
> +  UINT32            Counter;
> +  IMX_I2C_I2SR_REG  I2srReg;

Same as for previous patch - no need to name the variable after the
register it's initialised from.

> +
> +  Counter = I2cConfigPtr->TimeoutInUs;
> +  I2cRegsPtr = (IMX_I2C_REGS*)I2cConfigPtr->ControllerAddress;
> +  while (Counter) {
> +    I2srReg = (IMX_I2C_I2SR_REG)MmioRead16 ((UINTN)&I2cRegsPtr->I2SR);
> +    if ((I2srReg.AsUint16 & StatusBits) == StatusBits) {
> +      return TRUE;
> +    }
> +    MicroSecondDelay (1);
> +    --Counter;
> +  }
> +
> +  return FALSE;
> +}
> +
> +BOOLEAN
> +iMXI2cWaitStatusUnSet (
> +  IN  IMX_I2C_CONFIG   *I2cConfigPtr,
> +  IN  UINT16           StatusBits
> +  )
> +{
> +  IMX_I2C_REGS      *I2cRegsPtr;
> +  UINT32            Counter;
> +  IMX_I2C_I2SR_REG  I2srReg;
> +
> +  Counter = I2cConfigPtr->TimeoutInUs;
> +  I2cRegsPtr = (IMX_I2C_REGS*)I2cConfigPtr->ControllerAddress;
> +  while (Counter) {
> +    I2srReg = (IMX_I2C_I2SR_REG)MmioRead16 ((UINTN)&I2cRegsPtr->I2SR);
> +    if ((I2srReg.AsUint16 & StatusBits) == 0) {
> +      return TRUE;
> +    }
> +    MicroSecondDelay (1);
> +    --Counter;
> +  }
> +
> +  return FALSE;
> +}
> +
> +BOOLEAN
> +iMXI2cSendByte (
> +  IN  IMX_I2C_CONFIG  *I2cConfigPtr,
> +  IN  UINT8           Data
> +  )
> +{
> +  IMX_I2C_REGS      *I2cRegsPtr;
> +  UINT32            Counter;
> +  IMX_I2C_I2SR_REG  I2srReg;
> +  UINT16            SendData;
> +
> +  SendData = Data;
> +  Counter = I2cConfigPtr->TimeoutInUs;
> +  I2cRegsPtr = (IMX_I2C_REGS*)I2cConfigPtr->ControllerAddress;
> +
> +  // Clear status and transfer byte
> +  MmioWrite16 ((UINTN)&I2cRegsPtr->I2SR, 0);
> +  MmioWrite16 ((UINTN)&I2cRegsPtr->I2DR, SendData);
> +
> +  while (Counter) {
> +    I2srReg = (IMX_I2C_I2SR_REG)MmioRead16 ((UINTN)&I2cRegsPtr->I2SR);
> +    if (I2srReg.IIF == 1) {

What does it mean if it's == 1? Can that 1 be given a descriptively
named #define?

> +      return TRUE;
> +    } else if (I2srReg.IAL == 1) {

What does it mean if it's == 1? Can that 1 be given a descriptively
named #define?

> +      DEBUG ((DEBUG_ERROR, "iMXI2cSendByte: fail 0x%04x\n", I2srReg.AsUint16));
> +      return FALSE;
> +    }
> +    MicroSecondDelay (1);
> +    --Counter;
> +  }
> +
> +  DEBUG ((DEBUG_ERROR, "iMXI2cSendByte: Fail timeout\n"));
> +  return FALSE;
> +}
> +
> +RETURN_STATUS
> +iMXI2cSetupController (
> +  IN  IMX_I2C_CONFIG *I2cConfigPtr
> +  )
> +{
> +  IMX_I2C_REGS      *I2cRegsPtr;
> +  UINT32            Divider;
> +  UINT32            DividerCount;
> +  UINT32            IfdrDiv;
> +  IMX_I2C_I2CR_REG  I2crReg;
> +
> +  I2cRegsPtr = (IMX_I2C_REGS *)I2cConfigPtr->ControllerAddress;
> +
> +  // Disable controller and clear any pending interrupt
> +  MmioWrite16 ((UINTN)&I2cRegsPtr->I2CR, 0);
> +  MmioWrite16 ((UINTN)&I2cRegsPtr->I2SR, 0);

A comment as to which operation does which?

> +
> +  // Setup Divider if reference freq is provided. If no, use value setup by
> +  // 1st boot loader

first

> +  if (I2cConfigPtr->ReferenceFreq != 0) {
> +    IfdrDiv = 0;
> +    Divider = I2cConfigPtr->ReferenceFreq / I2cConfigPtr->TargetFreq;
> +
> +    for (DividerCount = 0; DividerCount < DIVIDER_VALUE_TOTAL; ++DividerCount) {
> +      if (DividerValue[DividerCount].Divider >= Divider) {
> +        DEBUG ((
> +                 DEBUG_INFO,
> +                 "iMXI2cSetupController: Divider %d IC 0x%02x\n",
> +                 DividerValue[DividerCount].Divider,
> +                 DividerValue[DividerCount].IC));
> +        IfdrDiv = DividerValue[DividerCount].IC;
> +        break;
> +      }
> +    }
> +
> +    if (IfdrDiv == 0) {
> +      DEBUG ((
> +               DEBUG_ERROR,
> +               "iMXI2cSetupController: could not find Divider for %d\n",
> +               Divider));
> +      return RETURN_INVALID_PARAMETER;
> +    }
> +
> +    MmioWrite16 ((UINTN)&I2cRegsPtr->IFDR, IfdrDiv);
> +  }
> +
> +  // Setup slave address
> +  MmioWrite16 ((UINTN)&I2cRegsPtr->IADR,
> +               (I2cConfigPtr->ControllerSlaveAddress << 1));

Why the shift by 1? Could this use a macro?

> +
> +  // Enable controller and set to master mode.
> +  I2crReg = (IMX_I2C_I2CR_REG)MmioRead16 ((UINTN)&I2cRegsPtr->I2CR);
> +
> +  // This bit must be set before any other I2C_I2CR bits have an effect

Why?

> +  I2crReg.IEN = 1;
> +  MmioWrite16 ((UINTN)&I2cRegsPtr->I2CR, I2crReg.AsUint16);

What does this write do?

> +  MicroSecondDelay (100);
> +
> +  MmioWrite16 ((UINTN)&I2cRegsPtr->I2SR, 0);

Why this write of 0?

> +
> +  // Wait for bus to be idle
> +  if (iMXI2cWaitStatusUnSet (I2cConfigPtr, IMX_I2C_I2SR_IBB) == FALSE) {
> +    DEBUG ((DEBUG_ERROR, "iMXI2cGenerateStart: Controller remains busy\n"));
> +    return RETURN_DEVICE_ERROR;
> +  }
> +
> +  I2crReg.MSTA = 1;

What does 1 mean?

> +  MmioWrite16 ((UINTN)&I2cRegsPtr->I2CR, I2crReg.AsUint16);

What does this write do?

> +
> +  // Now wait for bus to be busy
> +  if (iMXI2cWaitStatusSet (I2cConfigPtr, IMX_I2C_I2SR_IBB) == FALSE) {
> +    DEBUG ((DEBUG_ERROR, "iMXI2cGenerateStart: Controller remains idle\n"));
> +    return RETURN_DEVICE_ERROR;
> +  }
> +
> +  return RETURN_SUCCESS;
> +}
> +
> +RETURN_STATUS
> +iMXI2cGenerateStart (
> +  IN  IMX_I2C_CONFIG  *I2cConfigPtr,
> +  IN  UINT8           RegisterAddress
> +  )
> +{
> +  IMX_I2C_REGS      *I2cRegsPtr;
> +  IMX_I2C_I2CR_REG  I2crReg;
> +  BOOLEAN           Result;
> +  RETURN_STATUS     Status;
> +
> +  I2cRegsPtr = (IMX_I2C_REGS*)I2cConfigPtr->ControllerAddress;
> +  Status = iMXI2cSetupController (I2cConfigPtr);
> +  if (RETURN_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "iMXI2cGenerateStart: Fail to setup controller %r\n",
> +            Status));
> +    return Status;
> +  }
> +
> +  // Set controller to transmit mode
> +  I2crReg = (IMX_I2C_I2CR_REG)MmioRead16 ((UINTN)&I2cRegsPtr->I2CR);
> +  I2crReg.MTX = 1;
> +  MmioWrite16 ((UINTN)&I2cRegsPtr->I2CR, I2crReg.AsUint16);
> +
> +  Result = iMXI2cSendByte (I2cConfigPtr, (I2cConfigPtr->SlaveAddress << 1));

Shift? Macro?

> +  if (Result == FALSE) {
> +    DEBUG ((
> +             DEBUG_ERROR,
> +             "iMXI2cGenerateStart: Slave address transfer fail 0x%04x\n",
> +             MmioRead16 ((UINTN)&I2cRegsPtr->I2SR)));
> +    return RETURN_DEVICE_ERROR;
> +  }
> +
> +  // Send slave register address
> +  Result = iMXI2cSendByte (I2cConfigPtr, RegisterAddress);
> +  if (Result == FALSE) {
> +    DEBUG ((
> +             DEBUG_ERROR,
> +             "iMXI2cGenerateStart: Slave register address transfer fail 0x%04x\n",
> +             MmioRead16 ((UINTN)&I2cRegsPtr->I2SR)));
> +    return RETURN_DEVICE_ERROR;
> +  }
> +
> +  return RETURN_SUCCESS;
> +}
> +
> +RETURN_STATUS
> +iMXI2cGenerateStop (
> +  IN  IMX_I2C_CONFIG *I2cConfigPtr
> +  )
> +{
> +  IMX_I2C_REGS      *I2cRegsPtr;
> +  IMX_I2C_I2CR_REG  I2crReg;
> +
> +  I2cRegsPtr = (IMX_I2C_REGS*)I2cConfigPtr->ControllerAddress;
> +  I2crReg = (IMX_I2C_I2CR_REG)MmioRead16 ((UINTN)&I2cRegsPtr->I2CR);
> +  I2crReg.MSTA = 0;

I'm getting to the point where I am wanting these fields to be given
useful names. An awful lot of #defines and comments will be necessary
otherwise.

> +  I2crReg.MTX = 0;
> +  MmioWrite16 ((UINTN)&I2cRegsPtr->I2CR, I2crReg.AsUint16);
> +
> +  // Bus should go idle
> +  if (iMXI2cWaitStatusUnSet (I2cConfigPtr, IMX_I2C_I2SR_IBB) == FALSE) {
> +    DEBUG ((DEBUG_ERROR, "iMXI2cGenerateStop: Controller remains busy\n"));
> +    return RETURN_DEVICE_ERROR;
> +  }
> +
> +  return RETURN_SUCCESS;
> +}
> +
> +/**
> +  Perform I2C read operation.
> +
> +  The iMXI2cRead perform I2C read operation by programming the I2C controller.
> +  The caller is responsible to provide I2C controller configuration.
> +
> +  @param[in]    IMX_I2C_CONFIG    Pointer to structure containing the targeted
> +                                  I2C controller to be used for I2C operation.
> +  @param[in]    RegisterAddress   Targeted device register address to start read.
> +  @param[out]   ReadBufferPtr     Caller supplied buffer that would be written
> +                                  into with data from the read operation.
> +  @param[in]    ReadBufferSize    Size of caller supplied buffer.
> +
> +  @retval   RETURN_SUCCESS        I2C Read operation succeeded.
> +  @retval   RETURN_DEVICE_ERROR   The I2C device is not functioning correctly.
> +
> +**/
> +RETURN_STATUS
> +iMXI2cRead (
> +  IN IMX_I2C_CONFIG  *I2cConfigPtr,
> +  IN UINT8            RegisterAddress,
> +  OUT UINT8          *ReadBufferPtr,
> +  IN UINT32           ReadBufferSize
> +  )
> +{
> +  IMX_I2C_REGS      *I2cRegsPtr;
> +  IMX_I2C_I2CR_REG  I2crReg;
> +  BOOLEAN           Result;
> +  RETURN_STATUS     Status;
> +
> +  I2cRegsPtr = (IMX_I2C_REGS*)I2cConfigPtr->ControllerAddress;
> +  Status = iMXI2cGenerateStart (I2cConfigPtr, RegisterAddress);
> +  if (RETURN_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "iMXI2cRead: iMXI2cGenerateStart failed %r\n", Status));
> +    goto Exit;
> +  }
> +
> +  // Send slave address again to begin read
> +  I2crReg = (IMX_I2C_I2CR_REG)MmioRead16 ((UINTN)&I2cRegsPtr->I2CR);
> +  I2crReg.RSTA = 1; // Repeated start
> +  MmioWrite16 ((UINTN)&I2cRegsPtr->I2CR, I2crReg.AsUint16);
> +  Result = iMXI2cSendByte (
> +              I2cConfigPtr,
> +              (I2cConfigPtr->SlaveAddress << 1 | 1));

| 1 - the plot thickens.
Macro please.

> +  if (Result == FALSE) {
> +    DEBUG ((
> +              DEBUG_ERROR,
> +              "iMXI2cRead: 2nd Slave address transfer failed 0x%04x\n",
> +              MmioRead16 ((UINTN)&I2cRegsPtr->I2SR)));
> +    Status = RETURN_DEVICE_ERROR;
> +    goto Exit;
> +  }
> +
> +  // Disable master mode
> +  I2crReg = (IMX_I2C_I2CR_REG)MmioRead16 ((UINTN)&I2cRegsPtr->I2CR);
> +
> +  // NXP application note AN4481 - Only one byte so do not send acknowledge
> +  if (ReadBufferSize == 1) {
> +    I2crReg.TXAK = 1;
> +  } else {
> +    I2crReg.TXAK = 0;
> +  }
> +
> +  I2crReg.MTX = 0;
> +  MmioWrite16 ((UINTN)&I2cRegsPtr->I2CR, I2crReg.AsUint16);
> +
> +  // A data transfer can now be initiated by a read from I2C_I2DR in Slave
> +  // Receive mode.
> +  MmioWrite16 ((UINTN)&I2cRegsPtr->I2SR, 0);
> +  MmioRead16 ((UINTN)&I2cRegsPtr->I2DR);
> +
> +  do {
> +    // Wait for transfer to complete
> +    if (iMXI2cWaitStatusSet (I2cConfigPtr, IMX_I2C_I2SR_IIF) == FALSE) {
> +      DEBUG ((DEBUG_ERROR, "iMXI2cRead: waiting for read fail\n"));
> +      Status = RETURN_DEVICE_ERROR;
> +      goto Exit;
> +    }
> +
> +    if (iMXI2cWaitStatusSet (I2cConfigPtr, IMX_I2C_I2SR_ICF) == FALSE) {
> +      DEBUG ((DEBUG_ERROR, "iMXI2cRead: waiting for read fail\n"));
> +      Status = RETURN_DEVICE_ERROR;
> +      goto Exit;
> +    }
> +
> +    // Before the last byte is read, a Stop signal must be generated
> +    if (ReadBufferSize == 1) {
> +      Status = iMXI2cGenerateStop (
> +                 I2cConfigPtr);

No need to wrap this line.

> +      if (RETURN_ERROR (Status)) {
> +        DEBUG ((DEBUG_ERROR, "iMXI2cRead: iMXI2cGenerateStop fail %r\n", Status));
> +        goto Exit;
> +      }
> +    }
> +
> +    if (ReadBufferSize == 2) {

For second to last byte?
Could use a comment. And possibly comment above at == 1 could be
expanded to explain why.

> +      I2crReg = (IMX_I2C_I2CR_REG)MmioRead16 ((UINTN)&I2cRegsPtr->I2CR);
> +      I2crReg.TXAK = 1;

What does this 1 mean? #define?

> +      MmioWrite16 ((UINTN)&I2cRegsPtr->I2CR, I2crReg.AsUint16);
> +    }
> +
> +    MmioWrite16 ((UINTN)&I2cRegsPtr->I2SR, 0);

Why write 0?

> +
> +    *ReadBufferPtr = MmioRead8 ((UINTN)&I2cRegsPtr->I2DR);
> +    ++ReadBufferPtr;
> +    --ReadBufferSize;
> +  } while (ReadBufferSize > 0);
> +
> +Exit:
> +  Status = iMXI2cGenerateStop (I2cConfigPtr);
> +  if (RETURN_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "iMXI2cRead: Final iMXI2cGenerateStop fail %r\n", Status));
> +  }
> +
> +  return Status;
> +}
> +
> +/**
> +  Perform I2C write operation.
> +
> +  The iMXI2cWrite perform I2C write operation by programming the I2C
> +  controller. The caller is responsible to provide I2C controller
> +  configuration.
> +
> +  @param[in]    IMX_I2C_CONFIG    Pointer to structure containing the targeted
> +                                  I2C controller to be used for I2C operation.
> +  @param[in]    RegisterAddress   Targeted device register address to start write.
> +  @param[out]   WriteBufferPtr    Caller supplied buffer that contained data that
> +                                  would be read from for I2C write operation.
> +  @param[in]    WriteBufferSize   Size of caller supplied buffer.
> +
> +  @retval   RETURN_SUCCESS        I2C Write operation succeeded.
> +  @retval   RETURN_DEVICE_ERROR   The I2C device is not functioning correctly.
> +
> +**/
> +RETURN_STATUS
> +iMXI2cWrite (
> +  IN IMX_I2C_CONFIG  *I2cConfigPtr,
> +  IN UINT8            RegisterAddress,
> +  IN UINT8           *WriteBufferPtr,
> +  IN UINT32           WriteBufferSize
> +  )
> +{
> +  IMX_I2C_REGS    *I2cRegsPtr;
> +  BOOLEAN         Result;
> +  RETURN_STATUS   Status;
> +
> +  I2cRegsPtr = (IMX_I2C_REGS*)I2cConfigPtr->ControllerAddress;
> +  Status = iMXI2cGenerateStart (I2cConfigPtr, RegisterAddress);
> +  if (RETURN_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "iMXI2cWrite: iMXI2cGenerateStart fail %r\n", Status));
> +    goto Exit;
> +  }
> +
> +  while (WriteBufferSize > 0) {
> +    Result = iMXI2cSendByte (I2cConfigPtr, *WriteBufferPtr);
> +    if (Result == FALSE) {
> +      DEBUG ((
> +               DEBUG_ERROR,

Could skip that line break.

/
    Leif

> +               "iMXI2cWrite: Slave address transfer fail 0x%04x\n",
> +               MmioRead16 ((UINTN)&I2cRegsPtr->I2SR)));
> +      Status = RETURN_DEVICE_ERROR;
> +      goto Exit;
> +    }
> +
> +    ++WriteBufferPtr;
> +    --WriteBufferSize;
> +  }
> +
> +Exit:
> +  Status = iMXI2cGenerateStop (I2cConfigPtr);
> +  if (RETURN_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "iMXI2cWrite: iMXI2cGenerateStop fail %r\n", Status));
> +  }
> +
> +  return Status;
> +}
> diff --git a/Silicon/NXP/iMXPlatformPkg/Library/iMXI2cLib/iMXI2cLib.inf b/Silicon/NXP/iMXPlatformPkg/Library/iMXI2cLib/iMXI2cLib.inf
> new file mode 100644
> index 000000000000..ad968e88b1da
> --- /dev/null
> +++ b/Silicon/NXP/iMXPlatformPkg/Library/iMXI2cLib/iMXI2cLib.inf
> @@ -0,0 +1,35 @@
> +## @file
> +#
> +#  Copyright (c) 2018 Microsoft Corporation. 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.
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = iMXI2cLib
> +  FILE_GUID                      = C4E4A003-8AEB-4C9B-8E72-D2BAD2134BDE
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = iMXI2cLib
> +
> +[Packages]
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdePkg/MdePkg.dec
> +  Silicon/NXP/iMXPlatformPkg/iMXPlatformPkg.dec
> +
> +[LibraryClasses]
> +  BaseMemoryLib
> +  DebugLib
> +  IoLib
> +  TimerLib
> +
> +[Sources.common]
> +  iMXI2cLib.c
> -- 
> 2.16.2.gvfs.1.33.gf5370f1
>