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

["Pankaj Bansal" <pankaj.bansal@nxp.com>]

> -----Original Message-----
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Tuesday, March 31, 2020 6:01 PM
> To: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>
> Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Michael D Kinney
> <michael.d.kinney@intel.com>; devel@edk2.groups.io; Varun Sethi
> <V.Sethi@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>; Jon Nettleton <jon@solid-run.com>
> Subject: Re: [PATCH v2 04/28] Silicon/Maxim: Fix bug in RtcWrite in
> Ds1307RtcLib
> 
> 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.

Actually this bug was introduced because the Pcf2129 RTC driver was written
Based on I2cDxe driver, which assumed that all I2c transaction would be (reg, data)
Pair, i.e. always two operations. 

> 
> 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
> >