From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753489AbbC3R0p (ORCPT ); Mon, 30 Mar 2015 13:26:45 -0400 Received: from metis.ext.pengutronix.de ([92.198.50.35]:52603 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752863AbbC3R0m (ORCPT ); Mon, 30 Mar 2015 13:26:42 -0400 Date: Mon, 30 Mar 2015 19:23:09 +0200 From: Sascha Hauer To: Eddie Huang Cc: Mark Rutland , Wolfram Sang , Doug Anderson , linux-i2c@vger.kernel.org, David Box , Lee Jones , Jean Delvare , Xudong Chen , Boris BREZILLON , Pawel Moll , Liguo Zhang , Wei Yan , Bjorn Andersson , Uwe =?iso-8859-15?Q?Kleine-K=F6nig?= , Neelesh Gupta , devicetree@vger.kernel.org, Arnd Bergmann , Ian Campbell , Beniamino Galvani , Rob Herring , linux-mediatek@lists.infradead.org, Matthias Brugger , linux-arm-kernel@lists.infradead.org, srv_heupstream@mediatek.com, Anders Berg , Jim Cromie , Simon Glass , Max Schwarz , linux-kernel@vger.kernel.org, Sascha Hauer , Kumar Gala Subject: Re: [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C controller Message-ID: <20150330172309.GB9742@pengutronix.de> References: <1426917922-61356-1-git-send-email-eddie.huang@mediatek.com> <1426917922-61356-3-git-send-email-eddie.huang@mediatek.com> <20150323084237.GG9742@pengutronix.de> <1427703252.26464.14.camel@mtksdaap41> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1427703252.26464.14.camel@mtksdaap41> X-Sent-From: Pengutronix Hildesheim X-URL: http://www.pengutronix.de/ X-IRC: #ptxdist @freenode X-Accept-Language: de,en X-Accept-Content-Type: text/plain X-Uptime: 19:11:39 up 14 days, 5:03, 51 users, load average: 0.09, 0.13, 0.13 User-Agent: Mutt/1.5.21 (2010-09-15) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c0 X-SA-Exim-Mail-From: sha@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Mar 30, 2015 at 04:14:12PM +0800, Eddie Huang wrote: > Hi Sascha, > > > > > [...] > > > > > + if (i2c->speed_hz > 400000) > > > + control_reg |= I2C_CONTROL_RS; > > > + if (i2c->op == I2C_MASTER_WRRD) > > > + control_reg |= I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS; > > > + mtk_i2c_writew(control_reg, i2c, OFFSET_CONTROL); > > > + > > > + /* set start condition */ > > > + if (i2c->speed_hz <= 100000) > > > + mtk_i2c_writew(I2C_ST_START_CON, i2c, OFFSET_EXT_CONF); > > > + else > > > + mtk_i2c_writew(I2C_FS_START_CON, i2c, OFFSET_EXT_CONF); > > > + > > > + if (~control_reg & I2C_CONTROL_RS) > > > + mtk_i2c_writew(I2C_DELAY_LEN, i2c, OFFSET_DELAY_LEN); > > > > speed <= 400000 here to make this more obvious? > There are two cases, not only speed<=400000, but I2C_MASTER_WRRD. I tend > to keep it. Still it looks strange. You only ever write this default value to the register. Putting this register write under an if() seems bogus since the same value will be in the register the next time this code is executed. It looks like you should move this register write to some initialization function. > > > + > > > + /* Enable interrupt */ > > > + mtk_i2c_writew(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP, > > > + i2c, OFFSET_INTR_MASK); > > > > Why do you enable/disable interrupts for each transfer? Enabling them > > once and just acknowledge them in the interrupt handler should be > > enough. > This can avoid unwanted I2C interrupt. For example, I2C transfer error, > and cause timeout, I2C driver report error to caller. Then I2C error > interrupt happen. So isn't the same unwanted interrupt then just delayed until you enable the interrupts again? Is this something that really happens or just something you think that might happen? Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sascha Hauer Subject: Re: [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C controller Date: Mon, 30 Mar 2015 19:23:09 +0200 Message-ID: <20150330172309.GB9742@pengutronix.de> References: <1426917922-61356-1-git-send-email-eddie.huang@mediatek.com> <1426917922-61356-3-git-send-email-eddie.huang@mediatek.com> <20150323084237.GG9742@pengutronix.de> <1427703252.26464.14.camel@mtksdaap41> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1427703252.26464.14.camel@mtksdaap41> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+glpam-linux-mediatek=m.gmane.org-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org To: Eddie Huang Cc: Mark Rutland , Wolfram Sang , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, David Box , Lee Jones , Jean Delvare , Xudong Chen , Boris BREZILLON , Arnd Bergmann , Liguo Zhang , Wei Yan , Bjorn Andersson , Uwe =?iso-8859-15?Q?Kleine-K=F6nig?= , Neelesh Gupta , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Pawel Moll , Ian Campbell , Beniamino Galvani , Rob Herring , linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Matthias Brugger , linux-arm-kernel@li List-Id: devicetree@vger.kernel.org On Mon, Mar 30, 2015 at 04:14:12PM +0800, Eddie Huang wrote: > Hi Sascha, > > > > > [...] > > > > > + if (i2c->speed_hz > 400000) > > > + control_reg |= I2C_CONTROL_RS; > > > + if (i2c->op == I2C_MASTER_WRRD) > > > + control_reg |= I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS; > > > + mtk_i2c_writew(control_reg, i2c, OFFSET_CONTROL); > > > + > > > + /* set start condition */ > > > + if (i2c->speed_hz <= 100000) > > > + mtk_i2c_writew(I2C_ST_START_CON, i2c, OFFSET_EXT_CONF); > > > + else > > > + mtk_i2c_writew(I2C_FS_START_CON, i2c, OFFSET_EXT_CONF); > > > + > > > + if (~control_reg & I2C_CONTROL_RS) > > > + mtk_i2c_writew(I2C_DELAY_LEN, i2c, OFFSET_DELAY_LEN); > > > > speed <= 400000 here to make this more obvious? > There are two cases, not only speed<=400000, but I2C_MASTER_WRRD. I tend > to keep it. Still it looks strange. You only ever write this default value to the register. Putting this register write under an if() seems bogus since the same value will be in the register the next time this code is executed. It looks like you should move this register write to some initialization function. > > > + > > > + /* Enable interrupt */ > > > + mtk_i2c_writew(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP, > > > + i2c, OFFSET_INTR_MASK); > > > > Why do you enable/disable interrupts for each transfer? Enabling them > > once and just acknowledge them in the interrupt handler should be > > enough. > This can avoid unwanted I2C interrupt. For example, I2C transfer error, > and cause timeout, I2C driver report error to caller. Then I2C error > interrupt happen. So isn't the same unwanted interrupt then just delayed until you enable the interrupts again? Is this something that really happens or just something you think that might happen? Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | From mboxrd@z Thu Jan 1 00:00:00 1970 From: s.hauer@pengutronix.de (Sascha Hauer) Date: Mon, 30 Mar 2015 19:23:09 +0200 Subject: [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C controller In-Reply-To: <1427703252.26464.14.camel@mtksdaap41> References: <1426917922-61356-1-git-send-email-eddie.huang@mediatek.com> <1426917922-61356-3-git-send-email-eddie.huang@mediatek.com> <20150323084237.GG9742@pengutronix.de> <1427703252.26464.14.camel@mtksdaap41> Message-ID: <20150330172309.GB9742@pengutronix.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, Mar 30, 2015 at 04:14:12PM +0800, Eddie Huang wrote: > Hi Sascha, > > > > > [...] > > > > > + if (i2c->speed_hz > 400000) > > > + control_reg |= I2C_CONTROL_RS; > > > + if (i2c->op == I2C_MASTER_WRRD) > > > + control_reg |= I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS; > > > + mtk_i2c_writew(control_reg, i2c, OFFSET_CONTROL); > > > + > > > + /* set start condition */ > > > + if (i2c->speed_hz <= 100000) > > > + mtk_i2c_writew(I2C_ST_START_CON, i2c, OFFSET_EXT_CONF); > > > + else > > > + mtk_i2c_writew(I2C_FS_START_CON, i2c, OFFSET_EXT_CONF); > > > + > > > + if (~control_reg & I2C_CONTROL_RS) > > > + mtk_i2c_writew(I2C_DELAY_LEN, i2c, OFFSET_DELAY_LEN); > > > > speed <= 400000 here to make this more obvious? > There are two cases, not only speed<=400000, but I2C_MASTER_WRRD. I tend > to keep it. Still it looks strange. You only ever write this default value to the register. Putting this register write under an if() seems bogus since the same value will be in the register the next time this code is executed. It looks like you should move this register write to some initialization function. > > > + > > > + /* Enable interrupt */ > > > + mtk_i2c_writew(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP, > > > + i2c, OFFSET_INTR_MASK); > > > > Why do you enable/disable interrupts for each transfer? Enabling them > > once and just acknowledge them in the interrupt handler should be > > enough. > This can avoid unwanted I2C interrupt. For example, I2C transfer error, > and cause timeout, I2C driver report error to caller. Then I2C error > interrupt happen. So isn't the same unwanted interrupt then just delayed until you enable the interrupts again? Is this something that really happens or just something you think that might happen? Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |