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

[Varun Sethi <V.Sethi@nxp.com>]

Hi Leif,
Please find my response inline.

Regards
Varun

-----Original Message-----
From: Leif Lindholm <leif@nuviainc.com> 
Sent: Monday, April 6, 2020 4:42 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: [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 <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.

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.