Re: [PATCH v2 01/28] Silicon/NXP: Add I2c lib

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

On Mon, Apr 06, 2020 at 06:14:38 +0000, Pankaj Bansal (OSS) wrote:
> 
> 
> > -----Original Message-----
> > From: Leif Lindholm <leif@nuviainc.com>
> > Sent: Tuesday, March 31, 2020 5:21 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 01/28] Silicon/NXP: Add I2c lib
> > 
> > On Fri, Mar 20, 2020 at 20:05:16 +0530, Pankaj Bansal wrote:
> > > From: Pankaj Bansal <pankaj.bansal@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