From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by mx.groups.io with SMTP id smtpd.web10.8413.1586171541631813825 for ; Mon, 06 Apr 2020 04:12:22 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=QmN0kS5W; spf=pass (domain: nuviainc.com, ip: 209.85.221.67, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f67.google.com with SMTP id v5so5978417wrp.12 for ; Mon, 06 Apr 2020 04:12:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=LGxaiEAvnVvVkqKHt0C8CBUNmM8v2AbcpHzXoipKOU0=; b=QmN0kS5WPyR2vcYRxvGjvBroAYBP4JiReuaDYo99f31zkvjiQSJ+2ZCoVySSLtfuN0 U/5XYcKKCjTP/WSdhvzU1MyjhNf1x9ecAVobdyDEYx8tTqICS/CiG10Sm32yIYACvlUK +sGbDqb3oGHMN67kZVu65fMogZ59wWZldLMNst8I0B4P8cLDxJ+shYUgvno3LgEv0pTI u3s3m2m+jngzmVP6uj5eeIHkJHnPyQc72d4IDuNJdgTgdpn/NlwZZnhVSFBhQGTgMViR 71wxcXJtmLwg2V3LKCkz/cFa9mDJucba4niHerNDFyUCA6cIlCymZ12kGbW9iLJvdf10 aNXw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=LGxaiEAvnVvVkqKHt0C8CBUNmM8v2AbcpHzXoipKOU0=; b=YRjPZPgkYAmK24yifO8Z7SSNAqOzw07rP68z+xThKDCTpruItkebvk1CO2v3GSy3DO JoLucnaJxHo3GeXp2WzvEhijkxkPnrJ91DE2QJwEOtKWS9OaDkHjdFHo5eDjDH8lLIkf +KrusDTvntTXPpYxq3IwbT4LFIIrSbDzo2hrTnTCuBNOogy4E315ruMR/6b/yWduYPX+ 2K+8jYTytVrX1BJwxxRnswDns0f3m2gKu+rkM4sC8Mf/DHoeQCoZyPR5RHmR+waV0Vja 4KnA69xzwpmQzKvFROMjVAwCVeRZ/JiTk3AP2EvQ5TuoPvEFWReJ1RiNcRATD5qYEvYv W8DA== X-Gm-Message-State: AGi0PuZLnTYSz3Wn+Xk8SStpw6hGuF9A7yx2T8a+DgVxlmBOZm78mztW OMhgpeur6w0CidPrnZvKxpS4CA== X-Google-Smtp-Source: APiQypINQIEkdSqUSaEpOw8B0kmYQE9MsLyoj+i9byVF3Lud/2l97JCBDCcv8oZreVXyok+Er2+IEw== X-Received: by 2002:adf:fc0c:: with SMTP id i12mr5784378wrr.284.1586171540074; Mon, 06 Apr 2020 04:12:20 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id m8sm23884848wmc.28.2020.04.06.04.12.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Apr 2020 04:12:19 -0700 (PDT) Date: Mon, 6 Apr 2020 12:12:17 +0100 From: "Leif Lindholm" To: "Pankaj Bansal (OSS)" Cc: Meenakshi Aggarwal , Michael D Kinney , "devel@edk2.groups.io" , Varun Sethi , Samer El-Haj-Mahmoud , Jon Nettleton Subject: Re: [PATCH v2 01/28] Silicon/NXP: Add I2c lib Message-ID: <20200406111217.GB14075@vanye> References: <20200320143543.18615-1-pankaj.bansal@oss.nxp.com> <20200320143543.18615-2-pankaj.bansal@oss.nxp.com> <20200331115114.GD7468@vanye> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Apr 06, 2020 at 06:14:38 +0000, Pankaj Bansal (OSS) wrote: > > > > -----Original Message----- > > From: Leif Lindholm > > Sent: Tuesday, March 31, 2020 5:21 PM > > To: Pankaj Bansal (OSS) > > Cc: Meenakshi Aggarwal ; Michael D Kinney > > ; devel@edk2.groups.io; Varun Sethi > > ; Samer El-Haj-Mahmoud > Mahmoud@arm.com>; Jon Nettleton > > 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 > > > > > > 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