On Mon, Apr 06, 2020 at 06:14:38 +0000, Pankaj Bansal (OSS) wrote:
>
>
> > -----Original Message-----
> > From: Leif Lindholm <l...@nuviainc.com>
> > Sent: Tuesday, March 31, 2020 5:21 PM
> > To: Pankaj Bansal (OSS) <pankaj.ban...@oss.nxp.com>
> > Cc: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com>; Michael D Kinney
> > <michael.d.kin...@intel.com>; devel@edk2.groups.io; Varun Sethi
> > <v.se...@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> > mahm...@arm.com>; Jon Nettleton <j...@solid-run.com>
> > Subject: Re: [PATCH v2 01/28] Silicon/NXP: Add I2c lib
> >
> > On Fri, Mar 20, 2020 at 20:05:16 +0530, Pankaj Bansal wrote:
> > > From: Pankaj Bansal <pankaj.ban...@nxp.com>
> > >
> > > I2c lib is going to be used in PrePeiCore sec module to get the
> > > System clock information from devices connected to i2c (like fpga
> > > or clock generator)
> > >
> > > since we don't have support of DXE modules this early in boot stage,
> > > move the i2c controller functionality in library.
> >
> > This isn't moving the functionality to a library though - it is moving
> > the functionality to a library *and* adding new features. These are
> > two separate changes that should be two separate patches.
> >
> > The content in this patch is mostly fine as the end result (but some
> > comments below).
> >
> > I suggest this patch is reordered with 2/28 and all of the splitting
> > out part takes place in that patch. This patch can then be reduced to
> > ... the bits that are currently impossible to see are changed (at
> > least the glitch fixing).
>
> Actually the I2cLib is not some bug fix over I2cDxe.
> The I2cLib has been completely re-written.
> This is because, I2cDxe was written assuming that the I2c transaction would
> always be of (reg + data) type only (i.e. two operations) .
> And also the Repeat start condition was not generated In the I2cDxe driver.
>
> This caused the I2c Peripheral drivers which were written keeping the
> controller driver
> In mind to issue two operations. Which caused bug in Pcf2129 RTC driver, that
> I am fixing in patch 3.
>
> Now I have removed these assumptions as well as added Repeat start between
> successive operations, which
> Comply with PI I2c spec.
>
> So, it would be difficult for me to merge the 1 and 2.
OK, that's fine[1] then, but that means that this commit message is
misleading. Please rewrite it to reflect the actual situation.
(And address the remaining comments on this.)
[1] It's not "fine" - this means we're ripping out and replacing even
more code that was just merged. This pattern needs to stop.
/
Leif
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56978): https://edk2.groups.io/g/devel/message/56978
Mute This Topic: https://groups.io/mt/72077423/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
