From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753129AbbC3IOU (ORCPT ); Mon, 30 Mar 2015 04:14:20 -0400 Received: from mailgw01.mediatek.com ([210.61.82.183]:36765 "EHLO mailgw01.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1752810AbbC3IOR (ORCPT ); Mon, 30 Mar 2015 04:14:17 -0400 X-Listener-Flag: 11101 Subject: Re: [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C controller From: Eddie Huang To: Sascha Hauer CC: Mark Rutland , Wolfram Sang , Doug Anderson , , David Box , Lee Jones , Jean Delvare , Xudong Chen , Boris BREZILLON , Pawel Moll , Liguo Zhang , Wei Yan , Bjorn Andersson , Uwe =?ISO-8859-1?Q?Kleine-K=F6nig?= , Neelesh Gupta , , Arnd Bergmann , Ian Campbell , Beniamino Galvani , Rob Herring , , Matthias Brugger , , , Anders Berg , Jim Cromie , Simon Glass , Max Schwarz , , Sascha Hauer , Kumar Gala In-Reply-To: <20150323084237.GG9742@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> Content-Type: text/plain; charset="UTF-8" Date: Mon, 30 Mar 2015 16:14:12 +0800 Message-ID: <1427703252.26464.14.camel@mtksdaap41> MIME-Version: 1.0 X-Mailer: Evolution 2.28.3 Content-Transfer-Encoding: 7bit X-MTK: N Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Sascha, On Mon, 2015-03-23 at 09:42 +0100, Sascha Hauer wrote: > On Sat, Mar 21, 2015 at 02:05:21PM +0800, Eddie Huang wrote: > > From: Xudong Chen > > > > The mediatek SoCs have I2C controller that handle I2C transfer. > > This patch include common I2C bus driver. > > This driver is compatible with I2C controller on mt65xx/mt81xx. > > > > Signed-off-by: Xudong Chen > > Signed-off-by: Liguo Zhang > > Signed-off-by: Eddie Huang > > --- > > drivers/i2c/busses/Kconfig | 9 + > > drivers/i2c/busses/Makefile | 1 + > > drivers/i2c/busses/i2c-mt65xx.c | 705 ++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 715 insertions(+) > > create mode 100644 drivers/i2c/busses/i2c-mt65xx.c > > > > + > > +static void mtk_i2c_clock_disable(struct mtk_i2c *i2c) > > +{ > > + if (i2c->have_pmic) > > + clk_disable_unprepare(i2c->clk_pmic); > > + > > + clk_disable_unprepare(i2c->clk_main); > > + clk_disable_unprepare(i2c->clk_dma); > > +} > > + > > +static inline void mtk_i2c_init_hw(struct mtk_i2c *i2c) > > Please let the compiler decide whether to inline this or not. Will fix it. > > > +{ > > + mtk_i2c_writew(I2C_SOFT_RST, i2c, OFFSET_SOFTRESET); > > + /* Set ioconfig */ > > + if (i2c->use_push_pull) > > + mtk_i2c_writew(I2C_IO_CONFIG_PUSH_PULL, i2c, OFFSET_IO_CONFIG); > > + else > > + mtk_i2c_writew(I2C_IO_CONFIG_OPEN_DRAIN, i2c, OFFSET_IO_CONFIG); > > + > > + if (i2c->platform_compat & COMPAT_MT6577) > > + mtk_i2c_writew(I2C_DCM_DISABLE, i2c, OFFSET_DCM_EN); > > + > > + mtk_i2c_writew(i2c->timing_reg, i2c, OFFSET_TIMING); > > + mtk_i2c_writew(i2c->high_speed_reg, i2c, OFFSET_HS); > > +} > > + > > [...] > > > + step_cnt = step_div; > > + sclk = hclk / (2 * sample_cnt * step_cnt); > > + if (sclk > khz) { > > + dev_dbg(i2c->dev, "%s mode: unsupported speed (%ldkhz)\n", > > + (mode == HS_MODE) ? "HS" : "ST/FT", (long int)khz); > > + return -ENOTSUPP; > > + } > > + > > + step_cnt--; > > + sample_cnt--; > > + > > + if (mode == HS_MODE) { > > This is the only place where the HS_MODE is actually tested for. > Dropping this enum and using i2c->speed_hz > MAX_FS_MODE_SPEED directly > here would improve readability. Will fix it. > > > + /* Set the hign speed mode register */ > > + i2c->timing_reg = I2C_FS_TIME_INIT_VALUE; > > + i2c->high_speed_reg = I2C_TIME_DEFAULT_VALUE | > > + (sample_cnt & I2C_TIMING_SAMPLE_COUNT_MASK) << 12 | > > + (step_cnt & I2C_TIMING_SAMPLE_COUNT_MASK) << 8; > > + } else { > > + i2c->timing_reg = > > + (sample_cnt & I2C_TIMING_SAMPLE_COUNT_MASK) << 8 | > > + (step_cnt & I2C_TIMING_STEP_DIV_MASK) << 0; > > + /* Disable the high speed transaction */ > > + i2c->high_speed_reg = I2C_TIME_CLR_VALUE; > > + } > > + > > + return 0; > > +} > > + > > [...] > > > + 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. > > > + > > + addr_reg = msgs->addr << 1; > > + if (i2c->op == I2C_MASTER_RD) > > + addr_reg |= 0x1; > > + mtk_i2c_writew(addr_reg, i2c, OFFSET_SLAVE_ADDR); > > + > > + /* Clear interrupt status */ > > + mtk_i2c_writew(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP, > > + i2c, OFFSET_INTR_STAT); > > + mtk_i2c_writew(I2C_FIFO_ADDR_CLR, i2c, OFFSET_FIFO_ADDR_CLR); > > + > > + /* 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. > > + if (!i2c->trans_stop && tmo == 0) { > > + dev_dbg(i2c->dev, "addr: %x, transfer timeout\n", msgs->addr); > > + mtk_i2c_init_hw(i2c); > > + return -ETIMEDOUT; > > + } > > + > > + spin_lock(&i2c->irqlock); > > + irqstat = i2c->irq_stat; > > + spin_unlock(&i2c->irqlock); > > A plain spin_lock can't protect you against the interrupt handler, see > https://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/c214.html. > You need at least spin_lock_irq(). > > Anyway, I think this spin_lock can be removed since it only protects the > irq_stat variable. This is written only in the interrupt handler and > then tested for in thread context. The thread waits for the interrupt > handler to be finished anyway. Will fix it. > > > +static irqreturn_t mtk_i2c_irq(int irqno, void *dev_id) > > +{ > > + struct mtk_i2c *i2c = dev_id; > > + > > + /* Clear interrupt mask */ > > + mtk_i2c_writew(~(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP), > > + i2c, OFFSET_INTR_MASK); > > + > > + spin_lock(&i2c->irqlock); > > + i2c->irq_stat = mtk_i2c_readw(i2c, OFFSET_INTR_STAT); > > + i2c->trans_stop = true; > > A struct completion seems more what you want here. This makes the > trans_stop variable unnecessary (it contains no information anyway). > > See the tegra driver as an example. OK, will change to use complete API > > > + spin_unlock(&i2c->irqlock); > > + mtk_i2c_writew(I2C_HS_NACKERR | I2C_ACKERR > > + | I2C_TRANSAC_COMP, i2c, OFFSET_INTR_STAT); > > + wake_up(&i2c->wait); > > + > > + return IRQ_HANDLED; > > +} > > + > > [...] > > > +static int mtk_i2c_probe(struct platform_device *pdev) > > +{ > > + int ret = 0; > > + struct mtk_i2c *i2c; > > + struct clk *clk; > > + unsigned int clk_src_in_hz; > > + unsigned int clk_src_div; > > + struct resource *res; > > + > > + i2c = devm_kzalloc(&pdev->dev, sizeof(struct mtk_i2c), GFP_KERNEL); > > sizeof(*i2c) instead. This will make it harder to allocate the memory > for a wrong struct size. Will fix it. > > > + if (i2c == NULL) > > + return -ENOMEM; > > + > > + ret = mtk_i2c_parse_dt(pdev->dev.of_node, i2c, &clk_src_div); > > + if (ret) > > + return -EINVAL; > > + > > + i2c->platform_compat = mtk_get_device_prop(pdev); > > + if (i2c->have_pmic && (i2c->platform_compat & COMPAT_MT6577)) > > + return -EINVAL; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + > > + i2c->base = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(i2c->base)) > > + return PTR_ERR(i2c->base); > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > + > > + i2c->pdmabase = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(i2c->pdmabase)) > > + return PTR_ERR(i2c->pdmabase); > > + > > + i2c->irqnr = platform_get_irq(pdev, 0); > > + if (i2c->irqnr <= 0) > > + return -EINVAL; > > i2c->irqnr is never used outside this function, so you can drop it from > struct mtk_i2c and make this a local variable. > > Contrary to what Uwe said this variable should be a signed variable > because platform_get_irq() returns a signed integer which may contain a > negative error code. This will never be catched if you use an unsigned > variable. > > Also you should forward the error instead of returning -EINVAL. Will fix it. > > > +static int mtk_i2c_remove(struct platform_device *pdev) > > +{ > > + struct mtk_i2c *i2c = platform_get_drvdata(pdev); > > + > > + i2c_del_adapter(&i2c->adap); > > + platform_set_drvdata(pdev, NULL); > > This is unnecessary. This pointer is unused when no driver is bound to > the device and no driver will expect it to be valid in probe(). Will fix it. Eddie From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eddie Huang Subject: Re: [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C controller Date: Mon, 30 Mar 2015 16:14:12 +0800 Message-ID: <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> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150323084237.GG9742-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> Sender: linux-i2c-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Sascha Hauer Cc: Mark Rutland , Wolfram Sang , Doug Anderson , linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, David Box , Lee Jones , Jean Delvare , Xudong Chen , Boris BREZILLON , Pawel Moll , Liguo Zhang , Wei Yan , Bjorn Andersson , Uwe =?ISO-8859-1?Q?Kleine-K=F6nig?= , Neelesh Gupta , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Arnd Bergmann , Ian Campbell , Beniamino Galvani , Rob Herring , linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Matthias Brugger List-Id: devicetree@vger.kernel.org Hi Sascha, On Mon, 2015-03-23 at 09:42 +0100, Sascha Hauer wrote: > On Sat, Mar 21, 2015 at 02:05:21PM +0800, Eddie Huang wrote: > > From: Xudong Chen > > > > The mediatek SoCs have I2C controller that handle I2C transfer. > > This patch include common I2C bus driver. > > This driver is compatible with I2C controller on mt65xx/mt81xx. > > > > Signed-off-by: Xudong Chen > > Signed-off-by: Liguo Zhang > > Signed-off-by: Eddie Huang > > --- > > drivers/i2c/busses/Kconfig | 9 + > > drivers/i2c/busses/Makefile | 1 + > > drivers/i2c/busses/i2c-mt65xx.c | 705 ++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 715 insertions(+) > > create mode 100644 drivers/i2c/busses/i2c-mt65xx.c > > > > + > > +static void mtk_i2c_clock_disable(struct mtk_i2c *i2c) > > +{ > > + if (i2c->have_pmic) > > + clk_disable_unprepare(i2c->clk_pmic); > > + > > + clk_disable_unprepare(i2c->clk_main); > > + clk_disable_unprepare(i2c->clk_dma); > > +} > > + > > +static inline void mtk_i2c_init_hw(struct mtk_i2c *i2c) > > Please let the compiler decide whether to inline this or not. Will fix it. > > > +{ > > + mtk_i2c_writew(I2C_SOFT_RST, i2c, OFFSET_SOFTRESET); > > + /* Set ioconfig */ > > + if (i2c->use_push_pull) > > + mtk_i2c_writew(I2C_IO_CONFIG_PUSH_PULL, i2c, OFFSET_IO_CONFIG); > > + else > > + mtk_i2c_writew(I2C_IO_CONFIG_OPEN_DRAIN, i2c, OFFSET_IO_CONFIG); > > + > > + if (i2c->platform_compat & COMPAT_MT6577) > > + mtk_i2c_writew(I2C_DCM_DISABLE, i2c, OFFSET_DCM_EN); > > + > > + mtk_i2c_writew(i2c->timing_reg, i2c, OFFSET_TIMING); > > + mtk_i2c_writew(i2c->high_speed_reg, i2c, OFFSET_HS); > > +} > > + > > [...] > > > + step_cnt = step_div; > > + sclk = hclk / (2 * sample_cnt * step_cnt); > > + if (sclk > khz) { > > + dev_dbg(i2c->dev, "%s mode: unsupported speed (%ldkhz)\n", > > + (mode == HS_MODE) ? "HS" : "ST/FT", (long int)khz); > > + return -ENOTSUPP; > > + } > > + > > + step_cnt--; > > + sample_cnt--; > > + > > + if (mode == HS_MODE) { > > This is the only place where the HS_MODE is actually tested for. > Dropping this enum and using i2c->speed_hz > MAX_FS_MODE_SPEED directly > here would improve readability. Will fix it. > > > + /* Set the hign speed mode register */ > > + i2c->timing_reg = I2C_FS_TIME_INIT_VALUE; > > + i2c->high_speed_reg = I2C_TIME_DEFAULT_VALUE | > > + (sample_cnt & I2C_TIMING_SAMPLE_COUNT_MASK) << 12 | > > + (step_cnt & I2C_TIMING_SAMPLE_COUNT_MASK) << 8; > > + } else { > > + i2c->timing_reg = > > + (sample_cnt & I2C_TIMING_SAMPLE_COUNT_MASK) << 8 | > > + (step_cnt & I2C_TIMING_STEP_DIV_MASK) << 0; > > + /* Disable the high speed transaction */ > > + i2c->high_speed_reg = I2C_TIME_CLR_VALUE; > > + } > > + > > + return 0; > > +} > > + > > [...] > > > + 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. > > > + > > + addr_reg = msgs->addr << 1; > > + if (i2c->op == I2C_MASTER_RD) > > + addr_reg |= 0x1; > > + mtk_i2c_writew(addr_reg, i2c, OFFSET_SLAVE_ADDR); > > + > > + /* Clear interrupt status */ > > + mtk_i2c_writew(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP, > > + i2c, OFFSET_INTR_STAT); > > + mtk_i2c_writew(I2C_FIFO_ADDR_CLR, i2c, OFFSET_FIFO_ADDR_CLR); > > + > > + /* 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. > > + if (!i2c->trans_stop && tmo == 0) { > > + dev_dbg(i2c->dev, "addr: %x, transfer timeout\n", msgs->addr); > > + mtk_i2c_init_hw(i2c); > > + return -ETIMEDOUT; > > + } > > + > > + spin_lock(&i2c->irqlock); > > + irqstat = i2c->irq_stat; > > + spin_unlock(&i2c->irqlock); > > A plain spin_lock can't protect you against the interrupt handler, see > https://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/c214.html. > You need at least spin_lock_irq(). > > Anyway, I think this spin_lock can be removed since it only protects the > irq_stat variable. This is written only in the interrupt handler and > then tested for in thread context. The thread waits for the interrupt > handler to be finished anyway. Will fix it. > > > +static irqreturn_t mtk_i2c_irq(int irqno, void *dev_id) > > +{ > > + struct mtk_i2c *i2c = dev_id; > > + > > + /* Clear interrupt mask */ > > + mtk_i2c_writew(~(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP), > > + i2c, OFFSET_INTR_MASK); > > + > > + spin_lock(&i2c->irqlock); > > + i2c->irq_stat = mtk_i2c_readw(i2c, OFFSET_INTR_STAT); > > + i2c->trans_stop = true; > > A struct completion seems more what you want here. This makes the > trans_stop variable unnecessary (it contains no information anyway). > > See the tegra driver as an example. OK, will change to use complete API > > > + spin_unlock(&i2c->irqlock); > > + mtk_i2c_writew(I2C_HS_NACKERR | I2C_ACKERR > > + | I2C_TRANSAC_COMP, i2c, OFFSET_INTR_STAT); > > + wake_up(&i2c->wait); > > + > > + return IRQ_HANDLED; > > +} > > + > > [...] > > > +static int mtk_i2c_probe(struct platform_device *pdev) > > +{ > > + int ret = 0; > > + struct mtk_i2c *i2c; > > + struct clk *clk; > > + unsigned int clk_src_in_hz; > > + unsigned int clk_src_div; > > + struct resource *res; > > + > > + i2c = devm_kzalloc(&pdev->dev, sizeof(struct mtk_i2c), GFP_KERNEL); > > sizeof(*i2c) instead. This will make it harder to allocate the memory > for a wrong struct size. Will fix it. > > > + if (i2c == NULL) > > + return -ENOMEM; > > + > > + ret = mtk_i2c_parse_dt(pdev->dev.of_node, i2c, &clk_src_div); > > + if (ret) > > + return -EINVAL; > > + > > + i2c->platform_compat = mtk_get_device_prop(pdev); > > + if (i2c->have_pmic && (i2c->platform_compat & COMPAT_MT6577)) > > + return -EINVAL; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + > > + i2c->base = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(i2c->base)) > > + return PTR_ERR(i2c->base); > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > + > > + i2c->pdmabase = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(i2c->pdmabase)) > > + return PTR_ERR(i2c->pdmabase); > > + > > + i2c->irqnr = platform_get_irq(pdev, 0); > > + if (i2c->irqnr <= 0) > > + return -EINVAL; > > i2c->irqnr is never used outside this function, so you can drop it from > struct mtk_i2c and make this a local variable. > > Contrary to what Uwe said this variable should be a signed variable > because platform_get_irq() returns a signed integer which may contain a > negative error code. This will never be catched if you use an unsigned > variable. > > Also you should forward the error instead of returning -EINVAL. Will fix it. > > > +static int mtk_i2c_remove(struct platform_device *pdev) > > +{ > > + struct mtk_i2c *i2c = platform_get_drvdata(pdev); > > + > > + i2c_del_adapter(&i2c->adap); > > + platform_set_drvdata(pdev, NULL); > > This is unnecessary. This pointer is unused when no driver is bound to > the device and no driver will expect it to be valid in probe(). Will fix it. Eddie From mboxrd@z Thu Jan 1 00:00:00 1970 From: eddie.huang@mediatek.com (Eddie Huang) Date: Mon, 30 Mar 2015 16:14:12 +0800 Subject: [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C controller In-Reply-To: <20150323084237.GG9742@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> Message-ID: <1427703252.26464.14.camel@mtksdaap41> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Sascha, On Mon, 2015-03-23 at 09:42 +0100, Sascha Hauer wrote: > On Sat, Mar 21, 2015 at 02:05:21PM +0800, Eddie Huang wrote: > > From: Xudong Chen > > > > The mediatek SoCs have I2C controller that handle I2C transfer. > > This patch include common I2C bus driver. > > This driver is compatible with I2C controller on mt65xx/mt81xx. > > > > Signed-off-by: Xudong Chen > > Signed-off-by: Liguo Zhang > > Signed-off-by: Eddie Huang > > --- > > drivers/i2c/busses/Kconfig | 9 + > > drivers/i2c/busses/Makefile | 1 + > > drivers/i2c/busses/i2c-mt65xx.c | 705 ++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 715 insertions(+) > > create mode 100644 drivers/i2c/busses/i2c-mt65xx.c > > > > + > > +static void mtk_i2c_clock_disable(struct mtk_i2c *i2c) > > +{ > > + if (i2c->have_pmic) > > + clk_disable_unprepare(i2c->clk_pmic); > > + > > + clk_disable_unprepare(i2c->clk_main); > > + clk_disable_unprepare(i2c->clk_dma); > > +} > > + > > +static inline void mtk_i2c_init_hw(struct mtk_i2c *i2c) > > Please let the compiler decide whether to inline this or not. Will fix it. > > > +{ > > + mtk_i2c_writew(I2C_SOFT_RST, i2c, OFFSET_SOFTRESET); > > + /* Set ioconfig */ > > + if (i2c->use_push_pull) > > + mtk_i2c_writew(I2C_IO_CONFIG_PUSH_PULL, i2c, OFFSET_IO_CONFIG); > > + else > > + mtk_i2c_writew(I2C_IO_CONFIG_OPEN_DRAIN, i2c, OFFSET_IO_CONFIG); > > + > > + if (i2c->platform_compat & COMPAT_MT6577) > > + mtk_i2c_writew(I2C_DCM_DISABLE, i2c, OFFSET_DCM_EN); > > + > > + mtk_i2c_writew(i2c->timing_reg, i2c, OFFSET_TIMING); > > + mtk_i2c_writew(i2c->high_speed_reg, i2c, OFFSET_HS); > > +} > > + > > [...] > > > + step_cnt = step_div; > > + sclk = hclk / (2 * sample_cnt * step_cnt); > > + if (sclk > khz) { > > + dev_dbg(i2c->dev, "%s mode: unsupported speed (%ldkhz)\n", > > + (mode == HS_MODE) ? "HS" : "ST/FT", (long int)khz); > > + return -ENOTSUPP; > > + } > > + > > + step_cnt--; > > + sample_cnt--; > > + > > + if (mode == HS_MODE) { > > This is the only place where the HS_MODE is actually tested for. > Dropping this enum and using i2c->speed_hz > MAX_FS_MODE_SPEED directly > here would improve readability. Will fix it. > > > + /* Set the hign speed mode register */ > > + i2c->timing_reg = I2C_FS_TIME_INIT_VALUE; > > + i2c->high_speed_reg = I2C_TIME_DEFAULT_VALUE | > > + (sample_cnt & I2C_TIMING_SAMPLE_COUNT_MASK) << 12 | > > + (step_cnt & I2C_TIMING_SAMPLE_COUNT_MASK) << 8; > > + } else { > > + i2c->timing_reg = > > + (sample_cnt & I2C_TIMING_SAMPLE_COUNT_MASK) << 8 | > > + (step_cnt & I2C_TIMING_STEP_DIV_MASK) << 0; > > + /* Disable the high speed transaction */ > > + i2c->high_speed_reg = I2C_TIME_CLR_VALUE; > > + } > > + > > + return 0; > > +} > > + > > [...] > > > + 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. > > > + > > + addr_reg = msgs->addr << 1; > > + if (i2c->op == I2C_MASTER_RD) > > + addr_reg |= 0x1; > > + mtk_i2c_writew(addr_reg, i2c, OFFSET_SLAVE_ADDR); > > + > > + /* Clear interrupt status */ > > + mtk_i2c_writew(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP, > > + i2c, OFFSET_INTR_STAT); > > + mtk_i2c_writew(I2C_FIFO_ADDR_CLR, i2c, OFFSET_FIFO_ADDR_CLR); > > + > > + /* 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. > > + if (!i2c->trans_stop && tmo == 0) { > > + dev_dbg(i2c->dev, "addr: %x, transfer timeout\n", msgs->addr); > > + mtk_i2c_init_hw(i2c); > > + return -ETIMEDOUT; > > + } > > + > > + spin_lock(&i2c->irqlock); > > + irqstat = i2c->irq_stat; > > + spin_unlock(&i2c->irqlock); > > A plain spin_lock can't protect you against the interrupt handler, see > https://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/c214.html. > You need at least spin_lock_irq(). > > Anyway, I think this spin_lock can be removed since it only protects the > irq_stat variable. This is written only in the interrupt handler and > then tested for in thread context. The thread waits for the interrupt > handler to be finished anyway. Will fix it. > > > +static irqreturn_t mtk_i2c_irq(int irqno, void *dev_id) > > +{ > > + struct mtk_i2c *i2c = dev_id; > > + > > + /* Clear interrupt mask */ > > + mtk_i2c_writew(~(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP), > > + i2c, OFFSET_INTR_MASK); > > + > > + spin_lock(&i2c->irqlock); > > + i2c->irq_stat = mtk_i2c_readw(i2c, OFFSET_INTR_STAT); > > + i2c->trans_stop = true; > > A struct completion seems more what you want here. This makes the > trans_stop variable unnecessary (it contains no information anyway). > > See the tegra driver as an example. OK, will change to use complete API > > > + spin_unlock(&i2c->irqlock); > > + mtk_i2c_writew(I2C_HS_NACKERR | I2C_ACKERR > > + | I2C_TRANSAC_COMP, i2c, OFFSET_INTR_STAT); > > + wake_up(&i2c->wait); > > + > > + return IRQ_HANDLED; > > +} > > + > > [...] > > > +static int mtk_i2c_probe(struct platform_device *pdev) > > +{ > > + int ret = 0; > > + struct mtk_i2c *i2c; > > + struct clk *clk; > > + unsigned int clk_src_in_hz; > > + unsigned int clk_src_div; > > + struct resource *res; > > + > > + i2c = devm_kzalloc(&pdev->dev, sizeof(struct mtk_i2c), GFP_KERNEL); > > sizeof(*i2c) instead. This will make it harder to allocate the memory > for a wrong struct size. Will fix it. > > > + if (i2c == NULL) > > + return -ENOMEM; > > + > > + ret = mtk_i2c_parse_dt(pdev->dev.of_node, i2c, &clk_src_div); > > + if (ret) > > + return -EINVAL; > > + > > + i2c->platform_compat = mtk_get_device_prop(pdev); > > + if (i2c->have_pmic && (i2c->platform_compat & COMPAT_MT6577)) > > + return -EINVAL; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + > > + i2c->base = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(i2c->base)) > > + return PTR_ERR(i2c->base); > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > + > > + i2c->pdmabase = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(i2c->pdmabase)) > > + return PTR_ERR(i2c->pdmabase); > > + > > + i2c->irqnr = platform_get_irq(pdev, 0); > > + if (i2c->irqnr <= 0) > > + return -EINVAL; > > i2c->irqnr is never used outside this function, so you can drop it from > struct mtk_i2c and make this a local variable. > > Contrary to what Uwe said this variable should be a signed variable > because platform_get_irq() returns a signed integer which may contain a > negative error code. This will never be catched if you use an unsigned > variable. > > Also you should forward the error instead of returning -EINVAL. Will fix it. > > > +static int mtk_i2c_remove(struct platform_device *pdev) > > +{ > > + struct mtk_i2c *i2c = platform_get_drvdata(pdev); > > + > > + i2c_del_adapter(&i2c->adap); > > + platform_set_drvdata(pdev, NULL); > > This is unnecessary. This pointer is unused when no driver is bound to > the device and no driver will expect it to be valid in probe(). Will fix it. Eddie