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 <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Leif Lindholm <[email protected]>
> Cc: Michael D Kinney <[email protected]>
> ---
> 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
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
