Re: [PATCH v2 04/28] Silicon/Maxim: Fix bug in RtcWrite in Ds1307RtcLib

["Leif Lindholm" <leif@nuviainc.com>]

On Fri, Mar 20, 2020 at 20:05:19 +0530, Pankaj Bansal wrote:
> From: Pankaj Bansal <pankaj.bansal@nxp.com>
> 
> There was a bug in I2C DXE implementation, which caused the Ds1307 RTC
> device to issue two operation for register write, while this is a single
> operation task. refer page 12 (Slave Receiver Mode (Write Mode)) on
> 
> https://datasheets.maximintegrated.com/en/ds/DS1307.pdf
>
> Modify ds1307 RtcWrite code accordingly.
> 
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>

So, I'm OK with this patch, but I'll mention that I prefer the design
in Silicon/NXP/Library/Pcf8563RealTimeClockLib which I think could
also be applied here. I think that might have avoided the confusion
that caused the bug.

Reviewed-by: Leif Lindholm <leif@nuviainc.com>

> ---
>  Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c b/Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c
> index 88dc198ffec8..fd7a8696e405 100644
> --- a/Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c
> +++ b/Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.c
> @@ -5,7 +5,7 @@
>    EmbeddedPkg/Library/TemplateRealTimeClockLib/RealTimeClockLib.c
>  
>    Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
> -  Copyright 2017 NXP
> +  Copyright 2017, 2020 NXP
>  
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
> @@ -84,16 +84,15 @@ RtcWrite (
>  {
>    RTC_I2C_REQUEST          Req;
>    EFI_STATUS               Status;
> +  UINT8                    Buffer[2];
>  
> -  Req.OperationCount = 2;
> +  Req.OperationCount = 1;
> +  Buffer[0] = RtcRegAddr;
> +  Buffer[1] = Val;
>  
>    Req.SetAddressOp.Flags = 0;
> -  Req.SetAddressOp.LengthInBytes = sizeof (RtcRegAddr);
> -  Req.SetAddressOp.Buffer = &RtcRegAddr;
> -
> -  Req.GetSetDateTimeOp.Flags = 0;
> -  Req.GetSetDateTimeOp.LengthInBytes = sizeof (Val);
> -  Req.GetSetDateTimeOp.Buffer = &Val;
> +  Req.SetAddressOp.LengthInBytes = sizeof (Buffer);
> +  Req.SetAddressOp.Buffer = Buffer;
>  
>    Status = mI2cMaster->StartRequest (mI2cMaster, FixedPcdGet8 (PcdI2cSlaveAddress),
>                                       (VOID *)&Req,
> -- 
> 2.17.1
>