Hi Leif,
Please find my response inline.

Regards
Varun

-----Original Message-----
From: Leif Lindholm <l...@nuviainc.com> 
Sent: Monday, April 6, 2020 4:42 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: [EXT] Re: [PATCH v2 01/28] Silicon/NXP: Add I2c lib

Caution: EXT Email

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.

I agree Leif, we will take care of this going forward. Some of the 
modifications (as per the patchset) were done by Pankaj after the initial 
patches got merged. We thought that it would be a good idea to post these 
patches in oneshot. We will re-visit our upstream patch submission strategy and 
make sure that there are no conflicts going forward. We are already aligning 
upstream patches for other platforms with Pankaj's patchset.

Really appreciate your help.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#57109): https://edk2.groups.io/g/devel/message/57109
Mute This Topic: https://groups.io/mt/72892621/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Reply via email to