> -----Original Message-----
> From: Leif Lindholm <leif.lindh...@linaro.org>
> Sent: Friday, May 3, 2019 8:11 PM
> To: Loh, Tien Hock <tien.hock....@intel.com>
> Cc: devel@edk2.groups.io; thlo...@gmail.com; Ard Biesheuvel
> <ard.biesheu...@linaro.org>
> Subject: Re: [[PATCH v2] 5/7] EmbeddedPkg: Fix DwEmmc driver bugs
>
> On Fri, May 03, 2019 at 11:27:01AM +0800, tien.hock....@intel.com wrote:
> > From: "Tien Hock, Loh" <tien.hock....@intel.com>
> >
> > Send command when MMC ask for response in
> DwEmmcReceiveResponse, and
> > command is a pending command (eg. DMA needs to be set up first)
> >
> > Signed-off-by: "Tien Hock, Loh" <tien.hock....@intel.com>
> > Cc: Leif Lindholm <leif.lindh...@linaro.org>
> > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> > ---
> > EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > index 32572a9..a69d9ab 100644
> > --- a/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > +++ b/EmbeddedPkg/Drivers/DwEmmcDxe/DwEmmcDxe.c
> > @@ -398,8 +398,11 @@ DwEmmcSendCommand (
> > mDwEmmcCommand = Cmd;
> > mDwEmmcArgument = Argument;
> > } else {
> > + mDwEmmcCommand = Cmd;
> > + mDwEmmcArgument = Argument;
> > Status = SendCommand (Cmd, Argument);
> > }
> > +
>
> I agree a space looks better here, but please don't add unrelated whitespace
> as part of a functional change.
OK noted.
>
> > return Status;
> > }
> >
> > @@ -410,6 +413,11 @@ DwEmmcReceiveResponse (
> > IN UINT32* Buffer
> > )
> > {
> > + EFI_STATUS Status = EFI_SUCCESS;
> > +
> > + if(IsPendingReadCommand (mDwEmmcCommand) ||
> > + IsPendingWriteCommand(mDwEmmcCommand))
>
> {
>
> > + Status = SendCommand (mDwEmmcCommand, mDwEmmcArgument);
>
> }
>
> > +
> > if (Buffer == NULL) {
> > return EFI_INVALID_PARAMETER;
> > }
>
> Should this test not come first in the function?
> If the code is relying on the side effect of the SendCommand () being
> performed even if Buffer is invalid, that needs a very detailed comment.
Yes I'll move it to after the buffer null check.
>
> /
> Leif
>
>
> > @@ -427,7 +435,7 @@ DwEmmcReceiveResponse (
> > Buffer[2] = MmioRead32 (DWEMMC_RESP2);
> > Buffer[3] = MmioRead32 (DWEMMC_RESP3);
> > }
> > - return EFI_SUCCESS;
> > + return Status;
> > }
> >
> > VOID
> > --
> > 2.2.2
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#40241): https://edk2.groups.io/g/devel/message/40241
Mute This Topic: https://groups.io/mt/31480081/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
