Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752913AbcLGKPR (ORCPT ); Wed, 7 Dec 2016 05:15:17 -0500 Received: from mga11.intel.com ([192.55.52.93]:33307 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752425AbcLGKPP (ORCPT ); Wed, 7 Dec 2016 05:15:15 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,310,1477983600"; d="scan'208";a="795231765" Message-ID: <1481105711.21899.88.camel@linux.intel.com> Subject: Re: [PATCH V2] i2c: designware: fix wrong tx/rx fifo for ACPI From: Andy Shevchenko To: Tin Huynh , Jarkko Nikula , Mika Westerberg , Wolfram Sang Cc: linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org, Loc Ho , Thang Nguyen , Phong Vo , patches@apm.com Date: Wed, 07 Dec 2016 12:15:11 +0200 In-Reply-To: <1481011073-29143-1-git-send-email-tnhuynh@apm.com> References: <1481011073-29143-1-git-send-email-tnhuynh@apm.com> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.2-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1918 Lines: 66 On Tue, 2016-12-06 at 14:57 +0700, Tin Huynh wrote: > ACPI always sets txfifo and rxfifo to 32. This configuration will > cause problem if the IP core supports a fifo size of less than 32. > The driver should read the fifo size from the IP and select the  > smaller one of the two. > > Signed-off-by: Tin Huynh > The idea looks good, but see my comment below. > >dev); > + u32 param1, tx_fifo_depth, rx_fifo_depth; >   struct dw_i2c_dev *dev; >   struct i2c_adapter *adap; >   struct resource *mem; > @@ -246,12 +247,18 @@ static int dw_i2c_plat_probe(struct > platform_device *pdev) >   1000000); >   } >   > + param1 = i2c_dw_read_comp_param(dev); > + tx_fifo_depth = ((param1 >> 16) & 0xff) + 1; > + rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1; >   if (!dev->tx_fifo_depth) { > - u32 param1 = i2c_dw_read_comp_param(dev); > - > - dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1; > - dev->rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1; > + dev->tx_fifo_depth = tx_fifo_depth; > + dev->rx_fifo_depth = rx_fifo_depth; >   dev->adapter.nr = pdev->id; > + } else if (tx_fifo_depth) { > + dev->tx_fifo_depth = min_t(u32, dev->tx_fifo_depth, > + tx_fifo_depth > ); > + dev->rx_fifo_depth = min_t(u32, dev->rx_fifo_depth, > + rx_fifo_dept Can we move this to a separate function like static void dw_i2c_set_fifo_size(... *dev, ... id) { u32 param, tx_fifo_depth, rx_fifo_depth; param = i2c_dw_read_comp_param(dev); tx_fifo_depth = ((param >> 16) & 0xff) + 1; rx_fifo_depth = ((param >> 8)  & 0xff) + 1;   if (!dev->tx_fifo_depth) { dev->tx_fifo_depth = tx_fifo_depth; dev->rx_fifo_depth = rx_fifo_depth;   dev->adapter.nr = id; } dev->tx_fifo_depth = min_t(u32, dev->tx_fifo_depth, tx_fifo_depth); dev->rx_fifo_depth = min_t(u32, dev->rx_fifo_depth, rx_fifo_depth); } ? -- Andy Shevchenko Intel Finland Oy