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

[Marcin Wojtas <mw@semihalf.com>]

2017-10-26 15:54 GMT+02:00 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.
>

Ok, thanks for clarification.

Marcin