Re: [PATCH edk2-platforms v1 16/38] Silicon/Hisilicon/D06: Add I2C delay for HNS auto config

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

On Tue, Jul 24, 2018 at 03:09:00PM +0800, Ming Huang wrote:
> From: shaochangliang <shaochangliang@huawei.com>
> 
> Because I2C Port5 salve device connect under 95545 device,

salve -> slave
What is 95545?

> it will cost more time to access I2C slave device, so add
> delay time for HNS auto config.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: shaochangliang <shaochangliang@huawei.com>
> Signed-off-by: Ming Huang <ming.huang@linaro.org>
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> ---
>  Silicon/Hisilicon/Library/I2CLib/I2CLib.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/Silicon/Hisilicon/Library/I2CLib/I2CLib.c b/Silicon/Hisilicon/Library/I2CLib/I2CLib.c
> index 16636987a6..fa8c510f36 100644
> --- a/Silicon/Hisilicon/Library/I2CLib/I2CLib.c
> +++ b/Silicon/Hisilicon/Library/I2CLib/I2CLib.c
> @@ -248,7 +248,11 @@ CheckI2CTimeOut (
>    if (Transfer == I2CTx) {
>      ulFifo = I2C_GetTxStatus (Socket,Port);
>      while (ulFifo != 0) {
> -      I2C_Delay(2);
> +      if (Port == 5) {

What is special about 5?
Can it be given a descriptive #define?

> +        I2C_Delay(1000);

Please add a comment on why this special treatment is needed.
Space before '('
.
> +      } else {
> +        I2C_Delay(2);

Space before '('

> +      }
>        if (++ulTimes > I2C_READ_TIMEOUT) {
>          (VOID)I2C_Disable (Socket, Port);
>          return EFI_TIMEOUT;
> @@ -259,7 +263,11 @@ CheckI2CTimeOut (
>    else {
>      ulFifo = I2C_GetRxStatus (Socket,Port);
>      while (ulFifo == 0) {
> -      I2C_Delay(2);
> +      if (Port == 5) {
> +        I2C_Delay(1000);
> +      } else {
> +        I2C_Delay(2);
> +      }

All the same comments as for previous instance.

>        if (++ulTimes > I2C_READ_TIMEOUT) {
>          (VOID)I2C_Disable (Socket, Port);
>          return EFI_TIMEOUT;
> @@ -359,7 +367,7 @@ I2CWrite(I2C_DEVICE *I2cInfo, UINT16 InfoOffset, UINT32 ulLength, UINT8 *pBuf)
>          ulFifo = I2C_GetTxStatus(I2cInfo->Socket,I2cInfo->Port);
>          while(ulFifo > I2C_TXRX_THRESHOLD)
>          {
> -            I2C_Delay(2);
> +            I2C_Delay(1000);

Space before '('.

/
    Leif

>              if(++ulTimes > I2C_READ_TIMEOUT)
>              {
>                  (VOID)I2C_Disable(I2cInfo->Socket, I2cInfo->Port);
> -- 
> 2.17.0
>