Re: [[PATCH v2] 1/7] EmbeddedPkg: Fix DwEmmc driver bugs

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

Hi Tien Hock,

Thanks for splitting the patches back up, and sorry for taking so long
to get around to reviewing. But could you please add some descriptive
subject lines back as well?

Most of my comments on this series are purely coding style related, a
couple are not.

On Fri, May 03, 2019 at 11:26:57AM +0800, tien.hock.loh@intel.com wrote:
> From: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> 
> Added ACMD6 for SD support. For SD, after CMD55 is sent, the next command
> should be an application command, which should not expect data
> 
> Signed-off-by: "Tien Hock, Loh" <tien.hock.loh@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> index adc6b06..fa24802 100644
> --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> @@ -39,6 +39,7 @@ DWEMMC_IDMAC_DESCRIPTOR   *gpIdmacDesc;
>  EFI_GUID mDwEmmcDevicePathGuid = EFI_CALLER_ID_GUID;
>  STATIC UINT32 mDwEmmcCommand;
>  STATIC UINT32 mDwEmmcArgument;
> +STATIC BOOLEAN mIsACmd = FALSE;

Could we move this variable into DwEmmcSendCommand and drop the 'm'?

>  
>  EFI_STATUS
>  DwEmmcReadBlockData (
> @@ -321,6 +322,15 @@ DwEmmcSendCommand (
>      Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC |
>             BIT_CMD_SEND_INIT;
>      break;
> +  case MMC_INDX(6):
> +    if(mIsACmd) {
> +      Cmd = BIT_CMD_RESPONSE_EXPECT ;

Drop space before ;.

> +    }
> +    else {

} else {

> +      Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_DATA_EXPECTED |
> +            BIT_CMD_READ;
> +    }
> +    break;
>    case MMC_INDX(7):
>      if (Argument)
>          Cmd = BIT_CMD_RESPONSE_EXPECT | BIT_CMD_CHECK_RESPONSE_CRC;
> @@ -371,6 +381,11 @@ DwEmmcSendCommand (
>    }
>  
>    Cmd |= MMC_GET_INDX(MmcCmd) | BIT_CMD_USE_HOLD_REG | BIT_CMD_START;
> +
> +  if(MMC_INDX(55) == MMC_GET_INDX(MmcCmd))
> +    mIsACmd = TRUE;
> +  else
> +    mIsACmd = FALSE;

  if () {
  } else {
  }

There should also be spaces between MMC_INDX and (55), as well as
between MMC_GET_INDX (MmcCmd). Surrounding code does not follow the
style, but I would still prefer to keep to this for new/modified code.

I also think we should add IsACmd = FALSE to the default: case.

/
    Leif

>    if (IsPendingReadCommand (Cmd) || IsPendingWriteCommand (Cmd)) {
>      mDwEmmcCommand = Cmd;
>      mDwEmmcArgument = Argument;
> -- 
> 2.2.2
>