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