Re: [platforms: PATCH 01/10] Marvell/Drivers: MvI2cDxe: Abort transaction immediately upon fail

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

On Thu, Oct 26, 2017 at 03:19:36PM +0200, Marcin Wojtas wrote:
> Hi Leif,
> 
> 2017-10-26 14:51 GMT+02:00 Leif Lindholm <leif.lindholm@linaro.org>:
> > On Thu, Oct 26, 2017 at 03:19:28AM +0200, Marcin Wojtas wrote:
> >> From: David Greeson <dgreeson@cisco.com>
> >>
> >> Although the I2C transaction routines were prepared to
> >> return their status, they were never used. This could
> >> cause bus lock-up e.g. in case of failing to send a
> >> slave address, the data transfer was attempted to be
> >> continued anyway.
> >>
> >> This patch fixes faulty behavior by checking transaction
> >> status and stopping it immediately, once the fail
> >> is detected.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: David Greeson <dgreeson@cisco.com>
> >> [Style adjustment and cleanup]
> >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> >> ---
> >>  Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c | 34 +++++++++++++++++---
> >>  1 file changed, 29 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> >> index d85ee0b..7faf1f7 100755
> >> --- a/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> >> +++ b/Platform/Marvell/Drivers/I2c/MvI2cDxe/MvI2cDxe.c
> >> @@ -565,6 +565,7 @@ MvI2cStartRequest (
> >>    UINTN Transmitted;
> >>    I2C_MASTER_CONTEXT *I2cMasterContext = I2C_SC_FROM_MASTER(This);
> >>    EFI_I2C_OPERATION *Operation;
> >> +  EFI_STATUS Status = EFI_SUCCESS;
> >>
> >>    ASSERT (RequestPacket != NULL);
> >>    ASSERT (I2cMasterContext != NULL);
> >> @@ -574,35 +575,58 @@ MvI2cStartRequest (
> >>      ReadMode = Operation->Flags & I2C_FLAG_READ;
> >>
> >>      if (Count == 0) {
> >> -      MvI2cStart ( I2cMasterContext,
> >> +      Status = MvI2cStart (I2cMasterContext,
> >>                     (SlaveAddress << 1) | ReadMode,
> >>                     I2C_TRANSFER_TIMEOUT
> >
> > Much as I appreciate seeing this form of the code, since it simplifies
> > seeing the functional changes, this does cause those lines left
> > unchanges to no longer conform to coding style.
> > Can you please adjust throughout for a v2?
> >
> 
> No problem. I of course saw style violations, but I gave up on them
> for "no mix of functional improvements and style cleanups" contraint
> :) I will correct the modified function calls.

Clarification: this is and has always been _unrelated_ style cleanups.
Any statement that is actually being modified should be conformant
afterwards.

Thanks.

/
    Leif