[PATCH v4 0/2] ARM: mediatek: Add driver for Mediatek I2C controller

[PATCH v4 0/2] ARM: mediatek: Add driver for Mediatek I2C controller

[Eddie Huang]

This series is the fourth version of Mediatek SoCs I2C controller common
bus driver.

Change in v4:
Modify to support i2c_adapter_quirks base on Wolfram's patch [1].
Remove check transfer size and WRRD combine code. Instead, fill quirk
property and let i2c_check_for_quirks to do the filter.

This driver is based on 3.19-rc1.

MTK I2C HW has some limitations.
1. Only support one i2c_msg number. One exception is WRRD (write then read)
mode. WRRD can have two i2c_msg numbers.

2. Mediatek I2C controller support WRRD(write then read) mode, in WRRD
mode the Repeat Start will be issued between 2 messages.
In this driver if 2 messages is first write then read, the driver will
combine 2 messages using Write-Read mode so the RS will be issued between
the 2 messages.

3. The max transfer data length is 255 in one message. In WRRD mode, the
max data length of second msg is 31.

MT8135 and MT6589 can control I2C pins on PMIC(MT6397) by setting the i2c
registers in MT8135 side. In this case, driver should set OFFSET_PATH_DIR
bit first, the operation on other registers are still the same.
For now MT6589/MT8135 support this, MT6577/MT6595/MT8127 do not support.
For example, If want to use I2C4/5/6 pins on MT8135 just need to enable
the pinmux, else if want to use I2C pins on PMIC(MT6397) need to add
"mediatek,have-pmic" property in the .dts file of each platform.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/314804.html

Xudong Chen (2):
  dt-bindings: Add I2C bindings for mt65xx/mt81xx.
  I2C: mediatek: Add driver for MediaTek I2C controller

 .../devicetree/bindings/i2c/i2c-mt6577.txt         |  40 ++
 drivers/i2c/busses/Kconfig                         |   9 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-mt65xx.c                    | 710 +++++++++++++++++++++
 4 files changed, 760 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mt6577.txt
 create mode 100644 drivers/i2c/busses/i2c-mt65xx.c

--
1.8.1.1.dirty

[PATCH v4 1/2] dt-bindings: Add I2C bindings for mt65xx/mt81xx.

[Eddie Huang]

From: Xudong Chen <xudong.chen@mediatek.com>

Add devicetree bindings for Mediatek Soc I2C driver.

Signed-off-by: Xudong Chen <xudong.chen@mediatek.com>
Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
---
 .../devicetree/bindings/i2c/i2c-mt6577.txt         | 40 ++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mt6577.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mt6577.txt b/Documentation/devicetree/bindings/i2c/i2c-mt6577.txt
new file mode 100644
index 0000000..08f86c7
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-mt6577.txt
@@ -0,0 +1,40 @@
+* Mediatek's I2C controller
+
+The Mediatek's I2C controller is used to interface with I2C devices.
+
+Required properties:
+  - compatible: value should be either of the following.
+      (a) "mediatek,mt6577-i2c", for i2c compatible with mt6577 i2c.
+      (b) "mediatek,mt6589-i2c", for i2c compatible with mt6589 i2c.
+      (c) "mediatek,mt8127-i2c", for i2c compatible with mt8127 i2c.
+      (d) "mediatek,mt8135-i2c", for i2c compatible with mt8135 i2c.
+  - reg: physical base address of the controller and dma base, length of memory
+    mapped region.
+  - interrupts: interrupt number to the cpu.
+  - clock-div: the fixed value for frequency divider of clock source in i2c
+    module. Each IC may be different.
+  - clocks: clock name from clock manager
+  - clock-names: Must include "main" and "dma", if enable have-pmic need include
+    "pmic" extra.
+
+Optional properties:
+  - clock-frequency: Frequency in Hz of the bus when transfer, the default value
+    is 100000.
+  - mediatek,have-pmic: platform can control i2c form special pmic side.
+    Only mt6589 and mt8135 support this feature.
+  - mediatek,use-push-pull: IO config use push-pull mode.
+
+Example:
+
+	i2c0: i2c@1100d000 {
+			compatible = "mediatek,mt6577-i2c";
+			reg = <0x1100d000 0x70>,
+			      <0x11000300 0x80>;
+			interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_LOW>;
+			clock-frequency = <100000>;
+			mediatek,have-pmic;
+			clock-div = <16>;
+			clocks = <&i2c0_ck>, <&ap_dma_ck>;
+			clock-names = "main", "dma";
+	};
+
-- 
1.8.1.1.dirty

[PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller

[Eddie Huang]

From: Xudong Chen <xudong.chen-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>

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 <xudong.chen-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Signed-off-by: Eddie Huang <eddie.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
 drivers/i2c/busses/Kconfig      |   9 +
 drivers/i2c/busses/Makefile     |   1 +
 drivers/i2c/busses/i2c-mt65xx.c | 710 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 720 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-mt65xx.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 31e8308..7c76693 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -582,6 +582,15 @@ config I2C_MPC
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-mpc.
 
+config I2C_MT65XX
+	tristate "MediaTek I2C adapter"
+	depends on ARCH_MEDIATEK
+	help
+	  This selects the MediaTek(R) Integrated Inter Circuit bus driver
+	  for MT65xx and MT81xx.
+	  If you want to use MediaTek(R) I2C interface, say Y or M here.
+	  If unsure, say N.
+
 config I2C_MV64XXX
 	tristate "Marvell mv64xxx I2C Controller"
 	depends on MV64X60 || PLAT_ORION || ARCH_SUNXI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 56388f6..59cbb4b 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -56,6 +56,7 @@ obj-$(CONFIG_I2C_IOP3XX)	+= i2c-iop3xx.o
 obj-$(CONFIG_I2C_KEMPLD)	+= i2c-kempld.o
 obj-$(CONFIG_I2C_MESON)		+= i2c-meson.o
 obj-$(CONFIG_I2C_MPC)		+= i2c-mpc.o
+obj-$(CONFIG_I2C_MT65XX)       += i2c-mt65xx.o
 obj-$(CONFIG_I2C_MV64XXX)	+= i2c-mv64xxx.o
 obj-$(CONFIG_I2C_MXS)		+= i2c-mxs.o
 obj-$(CONFIG_I2C_NOMADIK)	+= i2c-nomadik.o
diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
new file mode 100644
index 0000000..21bbe3b
--- /dev/null
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -0,0 +1,710 @@
+/*
+ * Copyright (c) 2014 MediaTek Inc.
+ * Author: Xudong.chen <xudong.chen-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/sched.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/wait.h>
+#include <linux/mm.h>
+#include <linux/dma-mapping.h>
+#include <linux/scatterlist.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/clk.h>
+
+#define I2C_HS_NACKERR			(1 << 2)
+#define I2C_ACKERR			(1 << 1)
+#define I2C_TRANSAC_COMP		(1 << 0)
+#define I2C_TRANSAC_START		(1 << 0)
+#define I2C_TIMING_STEP_DIV_MASK	(0x3f << 0)
+#define I2C_TIMING_SAMPLE_COUNT_MASK	(0x7 << 0)
+#define I2C_TIMING_SAMPLE_DIV_MASK	(0x7 << 8)
+#define I2C_TIMING_DATA_READ_MASK	(0x7 << 12)
+#define I2C_DCM_DISABLE			0x0000
+#define I2C_IO_CONFIG_OPEN_DRAIN	0x0003
+#define I2C_IO_CONFIG_PUSH_PULL		0x0000
+#define I2C_SOFT_RST			0x0001
+#define I2C_FIFO_ADDR_CLR		0x0001
+#define I2C_DELAY_LEN			0x0002
+#define I2C_ST_START_CON		0x8001
+#define I2C_FS_START_CON		0x1800
+#define I2C_TIME_CLR_VALUE		0x0000
+#define I2C_TIME_DEFAULT_VALUE		0x0003
+#define I2C_FS_TIME_INIT_VALUE		0x1303
+#define I2C_WRRD_TRANAC_VALUE		0x0002
+#define I2C_RD_TRANAC_VALUE		0x0001
+
+#define I2C_DMA_CON_TX			0x0000
+#define I2C_DMA_CON_RX			0x0001
+#define I2C_DMA_START_EN		0x0001
+#define I2C_DMA_INT_FLAG_NONE		0x0000
+#define I2C_DMA_CLR_FLAG		0x0000
+
+#define I2C_DEFAUT_SPEED		100000	/* hz */
+#define MAX_FS_MODE_SPEED		400000
+#define MAX_HS_MODE_SPEED		3400000
+#define MAX_DMA_TRANS_SIZE		255
+#define MAX_WRRD_TRANS_SIZE		31
+#define MAX_SAMPLE_CNT_DIV		8
+#define MAX_STEP_CNT_DIV		64
+#define MAX_HS_STEP_CNT_DIV		8
+
+#define I2C_CONTROL_RS                  (0x1 << 1)
+#define I2C_CONTROL_DMA_EN              (0x1 << 2)
+#define I2C_CONTROL_CLK_EXT_EN          (0x1 << 3)
+#define I2C_CONTROL_DIR_CHANGE          (0x1 << 4)
+#define I2C_CONTROL_ACKERR_DET_EN       (0x1 << 5)
+#define I2C_CONTROL_TRANSFER_LEN_CHANGE (0x1 << 6)
+#define I2C_CONTROL_WRAPPER             (0x1 << 0)
+
+#define COMPAT_MT6577			(0x1 << 0)
+#define COMPAT_MT6589			(0x1 << 1)
+
+#define I2C_DRV_NAME		"mt-i2c"
+
+enum DMA_REGS_OFFSET {
+	OFFSET_INT_FLAG = 0x0,
+	OFFSET_INT_EN = 0x04,
+	OFFSET_EN = 0x08,
+	OFFSET_CON = 0x18,
+	OFFSET_TX_MEM_ADDR = 0x1c,
+	OFFSET_RX_MEM_ADDR = 0x20,
+	OFFSET_TX_LEN = 0x24,
+	OFFSET_RX_LEN = 0x28,
+};
+
+enum i2c_trans_st_rs {
+	I2C_TRANS_STOP = 0,
+	I2C_TRANS_REPEATED_START,
+};
+
+enum {
+	FS_MODE,
+	HS_MODE,
+};
+
+enum mtk_trans_op {
+	I2C_MASTER_WR = 1,
+	I2C_MASTER_RD,
+	I2C_MASTER_WRRD,
+};
+
+enum I2C_REGS_OFFSET {
+	OFFSET_DATA_PORT = 0x0,
+	OFFSET_SLAVE_ADDR = 0x04,
+	OFFSET_INTR_MASK = 0x08,
+	OFFSET_INTR_STAT = 0x0c,
+	OFFSET_CONTROL = 0x10,
+	OFFSET_TRANSFER_LEN = 0x14,
+	OFFSET_TRANSAC_LEN = 0x18,
+	OFFSET_DELAY_LEN = 0x1c,
+	OFFSET_TIMING = 0x20,
+	OFFSET_START = 0x24,
+	OFFSET_EXT_CONF = 0x28,
+	OFFSET_FIFO_STAT = 0x30,
+	OFFSET_FIFO_THRESH = 0x34,
+	OFFSET_FIFO_ADDR_CLR = 0x38,
+	OFFSET_IO_CONFIG = 0x40,
+	OFFSET_RSV_DEBUG = 0x44,
+	OFFSET_HS = 0x48,
+	OFFSET_SOFTRESET = 0x50,
+	OFFSET_DCM_EN = 0x54,
+	OFFSET_PATH_DIR = 0x60,
+	OFFSET_DEBUGSTAT = 0x64,
+	OFFSET_DEBUGCTRL = 0x68,
+	OFFSET_TRANSFER_LEN_AUX = 0x6c,
+};
+
+struct mtk_i2c_data {
+	unsigned int clk_frequency;	/* bus speed in Hz */
+	unsigned int flags;
+	unsigned int clk_src_div;
+};
+
+struct i2c_dma_buf {
+	u8 *vaddr;
+	dma_addr_t paddr;
+};
+
+struct mtk_i2c {
+	struct i2c_adapter adap;	/* i2c host adapter */
+	struct device *dev;
+	wait_queue_head_t wait;		/* i2c transfer wait queue */
+	/* set in i2c probe */
+	void __iomem *base;		/* i2c base addr */
+	void __iomem *pdmabase;		/* dma base address*/
+	int irqnr;			/* i2c interrupt number */
+	struct i2c_dma_buf dma_buf;	/* memory alloc for DMA mode */
+	struct clk *clk_main;		/* main clock for i2c bus */
+	struct clk *clk_dma;		/* DMA clock for i2c via DMA */
+	struct clk *clk_pmic;		/* PMIC clock for i2c from PMIC */
+	bool have_pmic;			/* can use i2c pins from PMIC */
+	bool use_push_pull;		/* IO config push-pull mode */
+	u32 platform_compat;		/* platform compatible data */
+	/* set when doing the transfer */
+	u16 irq_stat;			/* interrupt status */
+	unsigned int speed_hz;		/* The speed in transfer */
+	bool trans_stop;		/* i2c transfer stop */
+	enum mtk_trans_op op;
+	u16 msg_len;
+	u8 *msg_buf;			/* pointer to msg data */
+	u16 msg_aux_len;		/* WRRD mode to set AUX_LEN register*/
+	u16 addr;	/* 7bit slave address, without read/write bit */
+	u16 timing_reg;
+	u16 high_speed_reg;
+};
+
+static const struct of_device_id mtk_i2c_of_match[] = {
+	{ .compatible = "mediatek,mt6577-i2c", .data = (void *)COMPAT_MT6577 },
+	{ .compatible = "mediatek,mt6589-i2c", .data = (void *)COMPAT_MT6589 },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mtk_i2c_match);
+
+static inline void i2c_writew(u16 value, struct mtk_i2c *i2c, u8 offset)
+{
+	writew(value, i2c->base + offset);
+}
+
+static inline u16 i2c_readw(struct mtk_i2c *i2c, u8 offset)
+{
+	return readw(i2c->base + offset);
+}
+
+static inline void i2c_writel_dma(u32 value, struct mtk_i2c *i2c, u8 offset)
+{
+	writel(value, i2c->pdmabase + offset);
+}
+
+static int mtk_i2c_clock_enable(struct mtk_i2c *i2c)
+{
+	int ret;
+
+	ret = clk_prepare_enable(i2c->clk_dma);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(i2c->clk_main);
+	if (ret)
+		goto err_main;
+
+	if (i2c->have_pmic) {
+		ret = clk_prepare_enable(i2c->clk_pmic);
+		if (ret)
+			goto err_pmic;
+	}
+	return 0;
+
+err_pmic:
+	clk_disable_unprepare(i2c->clk_main);
+err_main:
+	clk_disable_unprepare(i2c->clk_dma);
+
+	return ret;
+}
+
+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 void free_i2c_dma_bufs(struct mtk_i2c *i2c)
+{
+	dma_free_coherent(i2c->adap.dev.parent, PAGE_SIZE,
+		i2c->dma_buf.vaddr, i2c->dma_buf.paddr);
+}
+
+static inline void mtk_i2c_init_hw(struct mtk_i2c *i2c)
+{
+	i2c_writew(I2C_SOFT_RST, i2c, OFFSET_SOFTRESET);
+	/* Set ioconfig */
+	if (i2c->use_push_pull)
+		i2c_writew(I2C_IO_CONFIG_PUSH_PULL, i2c, OFFSET_IO_CONFIG);
+	else
+		i2c_writew(I2C_IO_CONFIG_OPEN_DRAIN, i2c, OFFSET_IO_CONFIG);
+
+	if (i2c->platform_compat & COMPAT_MT6577)
+		i2c_writew(I2C_DCM_DISABLE, i2c, OFFSET_DCM_EN);
+
+	i2c_writew(i2c->timing_reg, i2c, OFFSET_TIMING);
+	i2c_writew(i2c->high_speed_reg, i2c, OFFSET_HS);
+}
+
+/* calculate i2c port speed */
+static int i2c_set_speed(struct mtk_i2c *i2c, unsigned int clk_src_in_hz)
+{
+	int mode;
+	unsigned int khz;
+	unsigned int step_cnt;
+	unsigned int sample_cnt;
+	unsigned int sclk;
+	unsigned int hclk;
+	unsigned int max_step_cnt;
+	unsigned int sample_div = MAX_SAMPLE_CNT_DIV;
+	unsigned int step_div;
+	unsigned int min_div;
+	unsigned int best_mul;
+	unsigned int cnt_mul;
+
+	if (i2c->speed_hz > MAX_HS_MODE_SPEED) {
+		return -EINVAL;
+	} else if (i2c->speed_hz > MAX_FS_MODE_SPEED) {
+		mode = HS_MODE;
+		max_step_cnt = MAX_HS_STEP_CNT_DIV;
+	} else {
+		mode = FS_MODE;
+		max_step_cnt = MAX_STEP_CNT_DIV;
+	}
+
+	step_div = max_step_cnt;
+	/* Find the best combination */
+	khz = i2c->speed_hz / 1000;
+	hclk = clk_src_in_hz / 1000;
+	min_div = ((hclk >> 1) + khz - 1) / khz;
+	best_mul = MAX_SAMPLE_CNT_DIV * max_step_cnt;
+
+	for (sample_cnt = 1; sample_cnt <= MAX_SAMPLE_CNT_DIV; sample_cnt++) {
+		step_cnt = (min_div + sample_cnt - 1) / sample_cnt;
+		cnt_mul = step_cnt * sample_cnt;
+		if (step_cnt > max_step_cnt)
+			continue;
+
+		if (cnt_mul < best_mul) {
+			best_mul = cnt_mul;
+			sample_div = sample_cnt;
+			step_div = step_cnt;
+			if (best_mul == min_div)
+				break;
+		}
+	}
+
+	sample_cnt = sample_div;
+	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) {
+		/* 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;
+}
+
+static int mtk_i2c_do_transfer(struct mtk_i2c *i2c)
+{
+	u16 addr_reg;
+	u16 control_reg;
+	int tmo = i2c->adap.timeout;
+
+	i2c->trans_stop = false;
+	i2c->irq_stat = 0;
+
+	/* If use i2c pin from PMIC mt6397 side, need set PATH_DIR first */
+	if (i2c->have_pmic)
+		i2c_writew(I2C_CONTROL_WRAPPER, i2c, OFFSET_PATH_DIR);
+
+	control_reg = I2C_CONTROL_ACKERR_DET_EN |
+		I2C_CONTROL_CLK_EXT_EN | I2C_CONTROL_DMA_EN;
+	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;
+	i2c_writew(control_reg, i2c, OFFSET_CONTROL);
+
+	/* set start condition */
+	if (i2c->speed_hz <= 100000)
+		i2c_writew(I2C_ST_START_CON, i2c, OFFSET_EXT_CONF);
+	else
+		i2c_writew(I2C_FS_START_CON, i2c, OFFSET_EXT_CONF);
+
+	if (~control_reg & I2C_CONTROL_RS)
+		i2c_writew(I2C_DELAY_LEN, i2c, OFFSET_DELAY_LEN);
+
+	addr_reg = i2c->addr << 1;
+	if (i2c->op == I2C_MASTER_RD)
+		addr_reg |= 0x1;
+	i2c_writew(addr_reg, i2c, OFFSET_SLAVE_ADDR);
+
+	/* Clear interrupt status */
+	i2c_writew(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP,
+		i2c, OFFSET_INTR_STAT);
+	i2c_writew(I2C_FIFO_ADDR_CLR, i2c, OFFSET_FIFO_ADDR_CLR);
+
+	/* Enable interrupt */
+	i2c_writew(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP,
+		i2c, OFFSET_INTR_MASK);
+
+	/* Set transfer and transaction len */
+	if (i2c->op == I2C_MASTER_WRRD) {
+		i2c_writew(i2c->msg_len | (i2c->msg_aux_len) << 8,
+			i2c, OFFSET_TRANSFER_LEN);
+		i2c_writew(I2C_WRRD_TRANAC_VALUE, i2c, OFFSET_TRANSAC_LEN);
+	} else {
+		i2c_writew(i2c->msg_len, i2c, OFFSET_TRANSFER_LEN);
+		i2c_writew(I2C_RD_TRANAC_VALUE, i2c, OFFSET_TRANSAC_LEN);
+	}
+
+	/* Prepare buffer data to start transfer */
+	if (i2c->op == I2C_MASTER_RD) {
+		i2c_writel_dma(I2C_DMA_INT_FLAG_NONE, i2c, OFFSET_INT_FLAG);
+		i2c_writel_dma(I2C_DMA_CON_RX, i2c, OFFSET_CON);
+		i2c_writel_dma((u32)i2c->msg_buf, i2c, OFFSET_RX_MEM_ADDR);
+		i2c_writel_dma(i2c->msg_len, i2c, OFFSET_RX_LEN);
+	} else if (i2c->op == I2C_MASTER_WR) {
+		i2c_writel_dma(I2C_DMA_INT_FLAG_NONE, i2c, OFFSET_INT_FLAG);
+		i2c_writel_dma(I2C_DMA_CON_TX, i2c, OFFSET_CON);
+		i2c_writel_dma((u32)i2c->msg_buf, i2c, OFFSET_TX_MEM_ADDR);
+		i2c_writel_dma(i2c->msg_len, i2c, OFFSET_TX_LEN);
+	} else {
+		i2c_writel_dma(I2C_DMA_CLR_FLAG, i2c, OFFSET_INT_FLAG);
+		i2c_writel_dma(I2C_DMA_CLR_FLAG, i2c, OFFSET_CON);
+		i2c_writel_dma((u32)i2c->msg_buf, i2c, OFFSET_TX_MEM_ADDR);
+		i2c_writel_dma((u32)i2c->dma_buf.paddr, i2c,
+			OFFSET_RX_MEM_ADDR);
+		i2c_writel_dma(i2c->msg_len, i2c, OFFSET_TX_LEN);
+		i2c_writel_dma(i2c->msg_aux_len, i2c, OFFSET_RX_LEN);
+	}
+
+	/* flush before sending start */
+	mb();
+	i2c_writel_dma(I2C_DMA_START_EN, i2c, OFFSET_EN);
+	i2c_writew(I2C_TRANSAC_START, i2c, OFFSET_START);
+
+	tmo = wait_event_timeout(i2c->wait, i2c->trans_stop, tmo);
+	if (tmo == 0) {
+		dev_dbg(i2c->dev, "addr: %x, transfer timeout\n", i2c->addr);
+		mtk_i2c_init_hw(i2c);
+		return -ETIMEDOUT;
+	}
+
+	if (i2c->irq_stat & (I2C_HS_NACKERR | I2C_ACKERR)) {
+		dev_dbg(i2c->dev, "addr: %x, transfer ACK error\n", i2c->addr);
+		mtk_i2c_init_hw(i2c);
+		return -EREMOTEIO;
+	}
+
+	dev_dbg(i2c->dev, "i2c transfer done.\n");
+
+	return 0;
+}
+
+static inline void mtk_i2c_copy_to_dma(struct mtk_i2c *i2c, struct i2c_msg *msg)
+{
+	/* if the operate is write, need to copy the data to DMA memory */
+	if (!(msg->flags & I2C_M_RD))
+		memcpy(i2c->dma_buf.vaddr, msg->buf, msg->len);
+}
+
+static inline void mtk_i2c_copy_from_dma(struct mtk_i2c *i2c,
+	struct i2c_msg *msg)
+{
+	/* if the operate is read, need to copy the data from DMA memory */
+	if (msg->flags & I2C_M_RD)
+		memcpy(msg->buf, i2c->dma_buf.vaddr, msg->len);
+}
+
+static int mtk_i2c_transfer(struct i2c_adapter *adap,
+	struct i2c_msg msgs[], int num)
+{
+	int ret;
+	struct mtk_i2c *i2c = i2c_get_adapdata(adap);
+
+	ret = mtk_i2c_clock_enable(i2c);
+	if (ret)
+		return ret;
+
+	if (msgs->addr == 0) {
+		dev_dbg(i2c->dev, " addr is invalid.\n");
+		ret = -EINVAL;
+		goto err_exit;
+	}
+
+	if (msgs->buf == NULL) {
+		dev_dbg(i2c->dev, " data buffer is NULL.\n");
+		ret = -EINVAL;
+		goto err_exit;
+	}
+
+	i2c->addr = msgs->addr;
+	i2c->msg_len = msgs->len;
+	i2c->msg_buf = msgs->buf;
+
+	if (msgs->flags & I2C_M_RD)
+		i2c->op = I2C_MASTER_RD;
+	else
+		i2c->op = I2C_MASTER_WR;
+
+	/* combined two messages into one transaction */
+	if (num > 1) {
+		i2c->msg_aux_len = (msgs + 1)->len;
+		i2c->op = I2C_MASTER_WRRD;
+	}
+
+	/*
+	 * always use DMA mode.
+	 * 1st when write need copy the data of message to dma memory
+	 * 2nd when read need copy the DMA data to the message buffer.
+	 */
+	mtk_i2c_copy_to_dma(i2c, msgs);
+	i2c->msg_buf = (u8 *)i2c->dma_buf.paddr;
+	ret = mtk_i2c_do_transfer(i2c);
+	if (ret < 0)
+		goto err_exit;
+
+	if (i2c->op == I2C_MASTER_WRRD)
+		mtk_i2c_copy_from_dma(i2c, msgs + 1);
+	else
+		mtk_i2c_copy_from_dma(i2c, msgs);
+
+	/* the return value is number of executed messages */
+	ret = num;
+
+err_exit:
+	mtk_i2c_clock_disable(i2c);
+	return ret;
+}
+
+static irqreturn_t mtk_i2c_irq(int irqno, void *dev_id)
+{
+	struct mtk_i2c *i2c = dev_id;
+
+	/* Clear interrupt mask */
+	i2c_writew(~(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP),
+		i2c, OFFSET_INTR_MASK);
+
+	i2c->irq_stat = i2c_readw(i2c, OFFSET_INTR_STAT);
+	i2c_writew(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP,
+		i2c, OFFSET_INTR_STAT);
+	i2c->trans_stop = true;
+	wake_up(&i2c->wait);
+
+	return IRQ_HANDLED;
+}
+
+static u32 mtk_i2c_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm mtk_i2c_algorithm = {
+	.master_xfer = mtk_i2c_transfer,
+	.functionality = mtk_i2c_functionality,
+};
+
+static inline u32 mtk_get_device_prop(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+
+	match = of_match_node(mtk_i2c_of_match, pdev->dev.of_node);
+	return (u32)match->data;
+}
+
+static int mtk_i2c_parse_dt(struct device_node *np, struct mtk_i2c *i2c,
+	unsigned int *clk_src_div)
+{
+	i2c->speed_hz = I2C_DEFAUT_SPEED;
+	of_property_read_u32(np, "clock-frequency", &i2c->speed_hz);
+	of_property_read_u32(np, "clock-div", clk_src_div);
+	if (*clk_src_div == 0)
+		return -EINVAL;
+
+	i2c->have_pmic = of_property_read_bool(np, "mediatek,have-pmic");
+	i2c->use_push_pull =
+		of_property_read_bool(np, "mediatek,use-push-pull");
+
+	return 0;
+}
+
+static const struct i2c_adapter_quirks mt6577_i2c_quirks = {
+	.flags = I2C_ADAPTER_QUIRK_COMB_WRITE_THEN_READ,
+	.max_num_msgs = 1,
+	.max_write_len = MAX_DMA_TRANS_SIZE,
+	.max_read_len = MAX_DMA_TRANS_SIZE,
+	.max_comb_write_len = MAX_DMA_TRANS_SIZE,
+	.max_comb_read_len = MAX_WRRD_TRANS_SIZE,
+};
+
+static int mtk_i2c_probe(struct platform_device *pdev)
+{
+	int ret = 0;
+	struct mtk_i2c *i2c;
+	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);
+	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;
+
+	ret = devm_request_irq(&pdev->dev, i2c->irqnr, mtk_i2c_irq,
+		IRQF_TRIGGER_NONE, I2C_DRV_NAME, i2c);
+	if (ret < 0) {
+		dev_err(&pdev->dev,
+			"Request I2C IRQ %d fail\n", i2c->irqnr);
+		return ret;
+	}
+
+	i2c->adap.dev.of_node = pdev->dev.of_node;
+	i2c->dev = &i2c->adap.dev;
+	i2c->adap.dev.parent = &pdev->dev;
+	i2c->adap.owner = THIS_MODULE;
+	i2c->adap.algo = &mtk_i2c_algorithm;
+	i2c->adap.quirks = &mt6577_i2c_quirks;
+	i2c->adap.algo_data = NULL;
+	i2c->adap.timeout = 2 * HZ;
+	i2c->adap.retries = 1;
+
+	i2c->clk_main = devm_clk_get(&pdev->dev, "main");
+	if (IS_ERR(i2c->clk_main)) {
+		dev_err(&pdev->dev, "cannot get main clock\n");
+		return PTR_ERR(i2c->clk_main);
+	}
+
+	i2c->clk_dma = devm_clk_get(&pdev->dev, "dma");
+	if (IS_ERR(i2c->clk_dma)) {
+		dev_err(&pdev->dev, "cannot get dma clock\n");
+		return PTR_ERR(i2c->clk_dma);
+	}
+
+	if (i2c->have_pmic) {
+		i2c->clk_pmic = devm_clk_get(&pdev->dev, "pmic");
+		if (IS_ERR(i2c->clk_pmic)) {
+			dev_err(&pdev->dev, "cannot get pmic clock\n");
+			return PTR_ERR(i2c->clk_pmic);
+		}
+		clk_src_in_hz = clk_get_rate(i2c->clk_pmic) / clk_src_div;
+	} else {
+		clk_src_in_hz = clk_get_rate(i2c->clk_main) / clk_src_div;
+	}
+
+	dev_dbg(&pdev->dev, "clock source %p,clock src frequency %d\n",
+		i2c->clk_main, clk_src_in_hz);
+	strlcpy(i2c->adap.name, I2C_DRV_NAME, sizeof(i2c->adap.name));
+	init_waitqueue_head(&i2c->wait);
+
+	ret = i2c_set_speed(i2c, clk_src_in_hz);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to set the speed.\n");
+		return -EINVAL;
+	}
+
+	ret = mtk_i2c_clock_enable(i2c);
+	if (ret) {
+		dev_err(&pdev->dev, "clock enable failed!\n");
+		return ret;
+	}
+	mtk_i2c_init_hw(i2c);
+	mtk_i2c_clock_disable(i2c);
+
+	i2c->dma_buf.vaddr = dma_alloc_coherent(&pdev->dev,
+		PAGE_SIZE, &i2c->dma_buf.paddr, GFP_KERNEL);
+	if (i2c->dma_buf.vaddr == NULL) {
+		dev_err(&pdev->dev, "dma_alloc_coherent fail\n");
+		return -ENOMEM;
+	}
+
+	i2c_set_adapdata(&i2c->adap, i2c);
+	ret = i2c_add_adapter(&i2c->adap);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to add i2c bus to i2c core\n");
+		free_i2c_dma_bufs(i2c);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, i2c);
+
+	return 0;
+}
+
+static int mtk_i2c_remove(struct platform_device *pdev)
+{
+	struct mtk_i2c *i2c = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&i2c->adap);
+	free_i2c_dma_bufs(i2c);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static struct platform_driver mtk_i2c_driver = {
+	.probe = mtk_i2c_probe,
+	.remove = mtk_i2c_remove,
+	.driver = {
+		.name = I2C_DRV_NAME,
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(mtk_i2c_of_match),
+	},
+};
+
+module_platform_driver(mtk_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("MediaTek I2C Bus Driver");
+MODULE_AUTHOR("Xudong Chen <xudong.chen-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>");
-- 
1.8.1.1.dirty

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller

[Uwe Kleine-König]

Hello,

On Fri, Jan 16, 2015 at 06:33:38PM +0800, Eddie Huang wrote:
> +config I2C_MT65XX
> +	tristate "MediaTek I2C adapter"
> +	depends on ARCH_MEDIATEK
depends on ARCH_MEDIATEK || COMPILE_TEST
default ARCH_MEDIATEK

would be nice to have to get better compile coverage.

> +struct mtk_i2c {
> +	struct i2c_adapter adap;	/* i2c host adapter */
> +	struct device *dev;
> +	wait_queue_head_t wait;		/* i2c transfer wait queue */
> +	/* set in i2c probe */
> +	void __iomem *base;		/* i2c base addr */
> +	void __iomem *pdmabase;		/* dma base address*/
> +	int irqnr;			/* i2c interrupt number */
irqs are unsigned quantities

> +	struct i2c_dma_buf dma_buf;	/* memory alloc for DMA mode */
> +	struct clk *clk_main;		/* main clock for i2c bus */
> +	struct clk *clk_dma;		/* DMA clock for i2c via DMA */
> +	struct clk *clk_pmic;		/* PMIC clock for i2c from PMIC */
> +	bool have_pmic;			/* can use i2c pins from PMIC */
> +	bool use_push_pull;		/* IO config push-pull mode */
> +	u32 platform_compat;		/* platform compatible data */
> +	/* set when doing the transfer */
> +	u16 irq_stat;			/* interrupt status */
> +	unsigned int speed_hz;		/* The speed in transfer */
> +	bool trans_stop;		/* i2c transfer stop */
> +	enum mtk_trans_op op;
> +	u16 msg_len;
> +	u8 *msg_buf;			/* pointer to msg data */
> +	u16 msg_aux_len;		/* WRRD mode to set AUX_LEN register*/
> +	u16 addr;	/* 7bit slave address, without read/write bit */
Wouldn't it be easier to maintain a pointer to the message to be
transferred?

> +	u16 timing_reg;
> +	u16 high_speed_reg;
> +};
> +
> +static const struct of_device_id mtk_i2c_of_match[] = {
> +	{ .compatible = "mediatek,mt6577-i2c", .data = (void *)COMPAT_MT6577 },
> +	{ .compatible = "mediatek,mt6589-i2c", .data = (void *)COMPAT_MT6589 },
> +	{},
There is usually no , after the sentinel entry.

> +};
> +MODULE_DEVICE_TABLE(of, mtk_i2c_match);
> +
> +static inline void i2c_writew(u16 value, struct mtk_i2c *i2c, u8 offset)
> +{
> +	writew(value, i2c->base + offset);
> +}
hmm, these simple wrappers are fine in general for me because they might
ease debugging or changing the accessor-function. Still "i2c_writew"
sounds too generic. IMHO you should spend the few chars to make this
mtk_i2c_writew. And to match my taste completely, move the driver data
parameter to the front. (But there are too much different tastes out
there to really request a certain style here.) Same for the other
wrappers of course.

> +	/* Prepare buffer data to start transfer */
> +	if (i2c->op == I2C_MASTER_RD) {
> +		i2c_writel_dma(I2C_DMA_INT_FLAG_NONE, i2c, OFFSET_INT_FLAG);
> +		i2c_writel_dma(I2C_DMA_CON_RX, i2c, OFFSET_CON);
> +		i2c_writel_dma((u32)i2c->msg_buf, i2c, OFFSET_RX_MEM_ADDR);
> +		i2c_writel_dma(i2c->msg_len, i2c, OFFSET_RX_LEN);
> +	} else if (i2c->op == I2C_MASTER_WR) {
> +		i2c_writel_dma(I2C_DMA_INT_FLAG_NONE, i2c, OFFSET_INT_FLAG);
> +		i2c_writel_dma(I2C_DMA_CON_TX, i2c, OFFSET_CON);
> +		i2c_writel_dma((u32)i2c->msg_buf, i2c, OFFSET_TX_MEM_ADDR);
> +		i2c_writel_dma(i2c->msg_len, i2c, OFFSET_TX_LEN);
> +	} else {
		/* i2c->op == I2C_MASTER_WRRD */

> +		i2c_writel_dma(I2C_DMA_CLR_FLAG, i2c, OFFSET_INT_FLAG);
> +		i2c_writel_dma(I2C_DMA_CLR_FLAG, i2c, OFFSET_CON);
> +		i2c_writel_dma((u32)i2c->msg_buf, i2c, OFFSET_TX_MEM_ADDR);
> +		i2c_writel_dma((u32)i2c->dma_buf.paddr, i2c,
> +			OFFSET_RX_MEM_ADDR);
> +		i2c_writel_dma(i2c->msg_len, i2c, OFFSET_TX_LEN);
> +		i2c_writel_dma(i2c->msg_aux_len, i2c, OFFSET_RX_LEN);
> +	}
> +
> +	/* flush before sending start */
> +	mb();
> +	i2c_writel_dma(I2C_DMA_START_EN, i2c, OFFSET_EN);
> +	i2c_writew(I2C_TRANSAC_START, i2c, OFFSET_START);
> +
> +	tmo = wait_event_timeout(i2c->wait, i2c->trans_stop, tmo);
If the request completes just when wait_event_timeout returned 0 you
shouldn't throw it away.

> +	if (tmo == 0) {
> +		dev_dbg(i2c->dev, "addr: %x, transfer timeout\n", i2c->addr);
> +		mtk_i2c_init_hw(i2c);
> +		return -ETIMEDOUT;
> +	}
> [...]
> +	if (msgs->addr == 0) {
> +		dev_dbg(i2c->dev, " addr is invalid.\n");
Is this a hardware limitation?

I'd remove the leading space in the message. (Applies also to other
places.)

> +		ret = -EINVAL;
> +		goto err_exit;
> +	}
> +
> +	if (msgs->buf == NULL) {
> +		dev_dbg(i2c->dev, " data buffer is NULL.\n");
> +		ret = -EINVAL;
> +		goto err_exit;
> +	}
> +
> +	i2c->addr = msgs->addr;
> +	i2c->msg_len = msgs->len;
> +	i2c->msg_buf = msgs->buf;
> +
> +	if (msgs->flags & I2C_M_RD)
> +		i2c->op = I2C_MASTER_RD;
> +	else
> +		i2c->op = I2C_MASTER_WR;
> +
> +	/* combined two messages into one transaction */
> +	if (num > 1) {
> +		i2c->msg_aux_len = (msgs + 1)->len;
> +		i2c->op = I2C_MASTER_WRRD;
> +	}
This means "write then read", right? You should check here that the
first message is really a write and the 2nd a read then.
Can this happen at all with the quirks defined below (.max_num_msgs =
1)?

> +	/*
> +	 * always use DMA mode.
Out of interest: Did you benchmark DMA vs manual mode for a typical
(what ever that means) scenario?

> +	 * 1st when write need copy the data of message to dma memory
> +	 * 2nd when read need copy the DMA data to the message buffer.
> +	 */
> +	mtk_i2c_copy_to_dma(i2c, msgs);
> +	i2c->msg_buf = (u8 *)i2c->dma_buf.paddr;
> +	ret = mtk_i2c_do_transfer(i2c);
> +	if (ret < 0)
> +		goto err_exit;
> +
> +	if (i2c->op == I2C_MASTER_WRRD)
> +		mtk_i2c_copy_from_dma(i2c, msgs + 1);
> +	else
> +		mtk_i2c_copy_from_dma(i2c, msgs);
> +
> +	/* the return value is number of executed messages */
> +	ret = num;
> +
> +err_exit:
> +	mtk_i2c_clock_disable(i2c);
> +	return ret;
> +}
> +
> +static irqreturn_t mtk_i2c_irq(int irqno, void *dev_id)
> +{
> +	struct mtk_i2c *i2c = dev_id;
> +
> +	/* Clear interrupt mask */
> +	i2c_writew(~(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP),
> +		i2c, OFFSET_INTR_MASK);
What is the effect of this. Does it disable further irqs?

> +	i2c->irq_stat = i2c_readw(i2c, OFFSET_INTR_STAT);
Maybe you need locking here to modify your driver data in the irq?

> +	i2c_writew(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP,
> +		i2c, OFFSET_INTR_STAT);
This is the ack? Then this is racy; I guess you want

	i2c_writew(i2c->irq_stat & (I2C_HS_NACKERR | I2C_ACKERR ...),
		   i2c, OFFSET_INTR_STAT);

then.

> +	i2c->trans_stop = true;
> +	wake_up(&i2c->wait);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static u32 mtk_i2c_functionality(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL;
Does your hardware handle 10bit addresses that nice that there is
nothing visible in the driver apart from this functionality
announcement?

> +}
> +
> +static const struct i2c_algorithm mtk_i2c_algorithm = {
> +	.master_xfer = mtk_i2c_transfer,
> +	.functionality = mtk_i2c_functionality,
> +};
> +
> +static inline u32 mtk_get_device_prop(struct platform_device *pdev)
> +{
> +	const struct of_device_id *match;
> +
> +	match = of_match_node(mtk_i2c_of_match, pdev->dev.of_node);
> +	return (u32)match->data;
> +}
> +
> +static int mtk_i2c_parse_dt(struct device_node *np, struct mtk_i2c *i2c,
> +	unsigned int *clk_src_div)
> +{
> +	i2c->speed_hz = I2C_DEFAUT_SPEED;
> +	of_property_read_u32(np, "clock-frequency", &i2c->speed_hz);
> +	of_property_read_u32(np, "clock-div", clk_src_div);
You should check the return value of of_property_read_u32.

> [...]
> +	ret = devm_request_irq(&pdev->dev, i2c->irqnr, mtk_i2c_irq,
> +		IRQF_TRIGGER_NONE, I2C_DRV_NAME, i2c);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev,
> +			"Request I2C IRQ %d fail\n", i2c->irqnr);
> +		return ret;
> +	}
I think the devm_request_irq should go near the end of probing.
Otherwise the irq might fire before the used resources are ready.

> +
> +	i2c->adap.dev.of_node = pdev->dev.of_node;
> +	i2c->dev = &i2c->adap.dev;
> +	i2c->adap.dev.parent = &pdev->dev;
> +	i2c->adap.owner = THIS_MODULE;
> +	i2c->adap.algo = &mtk_i2c_algorithm;
> +	i2c->adap.quirks = &mt6577_i2c_quirks;
> +	i2c->adap.algo_data = NULL;
No need to initialize this to NULL as the struct was allocated using
kzalloc.

> +	i2c->adap.timeout = 2 * HZ;
> +	i2c->adap.retries = 1;
> +
> +	i2c->clk_main = devm_clk_get(&pdev->dev, "main");
> +	if (IS_ERR(i2c->clk_main)) {
> +		dev_err(&pdev->dev, "cannot get main clock\n");
> +		return PTR_ERR(i2c->clk_main);
> +	}
> +
> +	i2c->clk_dma = devm_clk_get(&pdev->dev, "dma");
> +	if (IS_ERR(i2c->clk_dma)) {
> +		dev_err(&pdev->dev, "cannot get dma clock\n");
> +		return PTR_ERR(i2c->clk_dma);
> +	}
> +
> +	if (i2c->have_pmic) {
> +		i2c->clk_pmic = devm_clk_get(&pdev->dev, "pmic");
> +		if (IS_ERR(i2c->clk_pmic)) {
> +			dev_err(&pdev->dev, "cannot get pmic clock\n");
> +			return PTR_ERR(i2c->clk_pmic);
> +		}
> +		clk_src_in_hz = clk_get_rate(i2c->clk_pmic) / clk_src_div;
> +	} else {
> +		clk_src_in_hz = clk_get_rate(i2c->clk_main) / clk_src_div;
> +	}
This can be simplified, i.e. the common line can go after the if block
and then the else branch can be dropped.

> +	dev_dbg(&pdev->dev, "clock source %p,clock src frequency %d\n",
> +		i2c->clk_main, clk_src_in_hz);
> +	strlcpy(i2c->adap.name, I2C_DRV_NAME, sizeof(i2c->adap.name));
> +	init_waitqueue_head(&i2c->wait);
> +
> +	ret = i2c_set_speed(i2c, clk_src_in_hz);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to set the speed.\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = mtk_i2c_clock_enable(i2c);
> +	if (ret) {
> +		dev_err(&pdev->dev, "clock enable failed!\n");
> +		return ret;
> +	}
> +	mtk_i2c_init_hw(i2c);
> +	mtk_i2c_clock_disable(i2c);
> +
> +	i2c->dma_buf.vaddr = dma_alloc_coherent(&pdev->dev,
> +		PAGE_SIZE, &i2c->dma_buf.paddr, GFP_KERNEL);
> +	if (i2c->dma_buf.vaddr == NULL) {
> +		dev_err(&pdev->dev, "dma_alloc_coherent fail\n");
> +		return -ENOMEM;
> +	}
> +
> +	i2c_set_adapdata(&i2c->adap, i2c);
> +	ret = i2c_add_adapter(&i2c->adap);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to add i2c bus to i2c core\n");
> +		free_i2c_dma_bufs(i2c);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, i2c);
> +
> +	return 0;
> +}
> +
> +static int mtk_i2c_remove(struct platform_device *pdev)
> +{
> +	struct mtk_i2c *i2c = platform_get_drvdata(pdev);
> +
> +	i2c_del_adapter(&i2c->adap);
> +	free_i2c_dma_bufs(i2c);
> +	platform_set_drvdata(pdev, NULL);
> +
Here you need to make sure that no irq is running when i2c_del_adapter
is called.

> +	return 0;
> +}
> +
> +static struct platform_driver mtk_i2c_driver = {
> +	.probe = mtk_i2c_probe,
> +	.remove = mtk_i2c_remove,
> +	.driver = {
> +		.name = I2C_DRV_NAME,
> +		.owner = THIS_MODULE,
You don't need to set .owner any more. That's included in
module_platform_driver since some time.

> +		.of_match_table = of_match_ptr(mtk_i2c_of_match),
> +	},
> +};
> +
> +module_platform_driver(mtk_i2c_driver);
> +
> +MODULE_LICENSE("GPL");
MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("MediaTek I2C Bus Driver");
> +MODULE_AUTHOR("Xudong Chen <xudong.chen-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>");

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller

[Eddie Huang]

Hi Uwe,

On Sun, 2015-01-18 at 11:18 +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Jan 16, 2015 at 06:33:38PM +0800, Eddie Huang wrote:
> > +config I2C_MT65XX
> > +	tristate "MediaTek I2C adapter"
> > +	depends on ARCH_MEDIATEK
> depends on ARCH_MEDIATEK || COMPILE_TEST
> default ARCH_MEDIATEK
> 
> would be nice to have to get better compile coverage.
OK

> 
> > +struct mtk_i2c {
> > +	struct i2c_adapter adap;	/* i2c host adapter */
> > +	struct device *dev;
> > +	wait_queue_head_t wait;		/* i2c transfer wait queue */
> > +	/* set in i2c probe */
> > +	void __iomem *base;		/* i2c base addr */
> > +	void __iomem *pdmabase;		/* dma base address*/
> > +	int irqnr;			/* i2c interrupt number */
> irqs are unsigned quantities
OK

> 
> > +	struct i2c_dma_buf dma_buf;	/* memory alloc for DMA mode */
> > +	struct clk *clk_main;		/* main clock for i2c bus */
> > +	struct clk *clk_dma;		/* DMA clock for i2c via DMA */
> > +	struct clk *clk_pmic;		/* PMIC clock for i2c from PMIC */
> > +	bool have_pmic;			/* can use i2c pins from PMIC */
> > +	bool use_push_pull;		/* IO config push-pull mode */
> > +	u32 platform_compat;		/* platform compatible data */
> > +	/* set when doing the transfer */
> > +	u16 irq_stat;			/* interrupt status */
> > +	unsigned int speed_hz;		/* The speed in transfer */
> > +	bool trans_stop;		/* i2c transfer stop */
> > +	enum mtk_trans_op op;
> > +	u16 msg_len;
> > +	u8 *msg_buf;			/* pointer to msg data */
> > +	u16 msg_aux_len;		/* WRRD mode to set AUX_LEN register*/
> > +	u16 addr;	/* 7bit slave address, without read/write bit */
> Wouldn't it be easier to maintain a pointer to the message to be
> transferred?
I think use mtk_i2c pointer is more flexible than maintain a pointer to
message.

> 
> > +	u16 timing_reg;
> > +	u16 high_speed_reg;
> > +};
> > +
> > +static const struct of_device_id mtk_i2c_of_match[] = {
> > +	{ .compatible = "mediatek,mt6577-i2c", .data = (void *)COMPAT_MT6577 },
> > +	{ .compatible = "mediatek,mt6589-i2c", .data = (void *)COMPAT_MT6589 },
> > +	{},
> There is usually no , after the sentinel entry.
OK

> 
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_i2c_match);
> > +
> > +static inline void i2c_writew(u16 value, struct mtk_i2c *i2c, u8 offset)
> > +{
> > +	writew(value, i2c->base + offset);
> > +}
> hmm, these simple wrappers are fine in general for me because they might
> ease debugging or changing the accessor-function. Still "i2c_writew"
> sounds too generic. IMHO you should spend the few chars to make this
> mtk_i2c_writew. And to match my taste completely, move the driver data
> parameter to the front. (But there are too much different tastes out
> there to really request a certain style here.) Same for the other
> wrappers of course.
Sure, I will add mtk_ prefix.

> 
> > +	/* Prepare buffer data to start transfer */
> > +	if (i2c->op == I2C_MASTER_RD) {
> > +		i2c_writel_dma(I2C_DMA_INT_FLAG_NONE, i2c, OFFSET_INT_FLAG);
> > +		i2c_writel_dma(I2C_DMA_CON_RX, i2c, OFFSET_CON);
> > +		i2c_writel_dma((u32)i2c->msg_buf, i2c, OFFSET_RX_MEM_ADDR);
> > +		i2c_writel_dma(i2c->msg_len, i2c, OFFSET_RX_LEN);
> > +	} else if (i2c->op == I2C_MASTER_WR) {
> > +		i2c_writel_dma(I2C_DMA_INT_FLAG_NONE, i2c, OFFSET_INT_FLAG);
> > +		i2c_writel_dma(I2C_DMA_CON_TX, i2c, OFFSET_CON);
> > +		i2c_writel_dma((u32)i2c->msg_buf, i2c, OFFSET_TX_MEM_ADDR);
> > +		i2c_writel_dma(i2c->msg_len, i2c, OFFSET_TX_LEN);
> > +	} else {
> 		/* i2c->op == I2C_MASTER_WRRD */
> 
> > +		i2c_writel_dma(I2C_DMA_CLR_FLAG, i2c, OFFSET_INT_FLAG);
> > +		i2c_writel_dma(I2C_DMA_CLR_FLAG, i2c, OFFSET_CON);
> > +		i2c_writel_dma((u32)i2c->msg_buf, i2c, OFFSET_TX_MEM_ADDR);
> > +		i2c_writel_dma((u32)i2c->dma_buf.paddr, i2c,
> > +			OFFSET_RX_MEM_ADDR);
> > +		i2c_writel_dma(i2c->msg_len, i2c, OFFSET_TX_LEN);
> > +		i2c_writel_dma(i2c->msg_aux_len, i2c, OFFSET_RX_LEN);
> > +	}
> > +
> > +	/* flush before sending start */
> > +	mb();
> > +	i2c_writel_dma(I2C_DMA_START_EN, i2c, OFFSET_EN);
> > +	i2c_writew(I2C_TRANSAC_START, i2c, OFFSET_START);
> > +
> > +	tmo = wait_event_timeout(i2c->wait, i2c->trans_stop, tmo);
> If the request completes just when wait_event_timeout returned 0 you
> shouldn't throw it away.
OK, add check transfer complete in tmo == 0 case.

> > +	if (tmo == 0) {
> > +		dev_dbg(i2c->dev, "addr: %x, transfer timeout\n", i2c->addr);
> > +		mtk_i2c_init_hw(i2c);
> > +		return -ETIMEDOUT;
> > +	}
> > [...]
> > +	if (msgs->addr == 0) {
> > +		dev_dbg(i2c->dev, " addr is invalid.\n");
> Is this a hardware limitation?
No. addr 0 should be reserved for special purpose. No client should use
addr 0. I think driver should not block transfer addr 0, I will remove
this.

> 
> I'd remove the leading space in the message. (Applies also to other
> places.)
Of course

> 
> > +		ret = -EINVAL;
> > +		goto err_exit;
> > +	}
> > +
> > +	if (msgs->buf == NULL) {
> > +		dev_dbg(i2c->dev, " data buffer is NULL.\n");
> > +		ret = -EINVAL;
> > +		goto err_exit;
> > +	}
> > +
> > +	i2c->addr = msgs->addr;
> > +	i2c->msg_len = msgs->len;
> > +	i2c->msg_buf = msgs->buf;
> > +
> > +	if (msgs->flags & I2C_M_RD)
> > +		i2c->op = I2C_MASTER_RD;
> > +	else
> > +		i2c->op = I2C_MASTER_WR;
> > +
> > +	/* combined two messages into one transaction */
> > +	if (num > 1) {
> > +		i2c->msg_aux_len = (msgs + 1)->len;
> > +		i2c->op = I2C_MASTER_WRRD;
> > +	}
> This means "write then read", right? You should check here that the
> first message is really a write and the 2nd a read then.
> Can this happen at all with the quirks defined below (.max_num_msgs =
> 1)?
Yes, mean write then read. Indeed, add check is better.
If msg number is 1, means normal write or read, not "write then read".

> > +	/*
> > +	 * always use DMA mode.
> Out of interest: Did you benchmark DMA vs manual mode for a typical
> (what ever that means) scenario?
I think performance don't go to far, but always use DMA make driver
simplified.

> 
> > +	 * 1st when write need copy the data of message to dma memory
> > +	 * 2nd when read need copy the DMA data to the message buffer.
> > +	 */
> > +	mtk_i2c_copy_to_dma(i2c, msgs);
> > +	i2c->msg_buf = (u8 *)i2c->dma_buf.paddr;
> > +	ret = mtk_i2c_do_transfer(i2c);
> > +	if (ret < 0)
> > +		goto err_exit;
> > +
> > +	if (i2c->op == I2C_MASTER_WRRD)
> > +		mtk_i2c_copy_from_dma(i2c, msgs + 1);
> > +	else
> > +		mtk_i2c_copy_from_dma(i2c, msgs);
> > +
> > +	/* the return value is number of executed messages */
> > +	ret = num;
> > +
> > +err_exit:
> > +	mtk_i2c_clock_disable(i2c);
> > +	return ret;
> > +}
> > +
> > +static irqreturn_t mtk_i2c_irq(int irqno, void *dev_id)
> > +{
> > +	struct mtk_i2c *i2c = dev_id;
> > +
> > +	/* Clear interrupt mask */
> > +	i2c_writew(~(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP),
> > +		i2c, OFFSET_INTR_MASK);
> What is the effect of this. Does it disable further irqs?
Yes. interrupt enable in mtk_i2c_do_transfer, disable in mtk_i2c_irq.

> 
> > +	i2c->irq_stat = i2c_readw(i2c, OFFSET_INTR_STAT);
> Maybe you need locking here to modify your driver data in the irq?
OK
> 
> > +	i2c_writew(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP,
> > +		i2c, OFFSET_INTR_STAT);
> This is the ack? Then this is racy; I guess you want
Clear interrupt

> 
> 	i2c_writew(i2c->irq_stat & (I2C_HS_NACKERR | I2C_ACKERR ...),
> 		   i2c, OFFSET_INTR_STAT);
> 
Yes. you are right. This can avoid unnecessary write 1 bit.

> then.
> 
> > +	i2c->trans_stop = true;
> > +	wake_up(&i2c->wait);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static u32 mtk_i2c_functionality(struct i2c_adapter *adap)
> > +{
> > +	return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL;
> Does your hardware handle 10bit addresses that nice that there is
> nothing visible in the driver apart from this functionality
> announcement?
Our hardware has this capability, but not implement in this driver.
Remove I2C_FUNC_10BIT_ADDR in next round.

> 
> > +}
> > +
> > +static const struct i2c_algorithm mtk_i2c_algorithm = {
> > +	.master_xfer = mtk_i2c_transfer,
> > +	.functionality = mtk_i2c_functionality,
> > +};
> > +
> > +static inline u32 mtk_get_device_prop(struct platform_device *pdev)
> > +{
> > +	const struct of_device_id *match;
> > +
> > +	match = of_match_node(mtk_i2c_of_match, pdev->dev.of_node);
> > +	return (u32)match->data;
> > +}
> > +
> > +static int mtk_i2c_parse_dt(struct device_node *np, struct mtk_i2c *i2c,
> > +	unsigned int *clk_src_div)
> > +{
> > +	i2c->speed_hz = I2C_DEFAUT_SPEED;
> > +	of_property_read_u32(np, "clock-frequency", &i2c->speed_hz);
> > +	of_property_read_u32(np, "clock-div", clk_src_div);
> You should check the return value of of_property_read_u32.
OK
> 
> > [...]
> > +	ret = devm_request_irq(&pdev->dev, i2c->irqnr, mtk_i2c_irq,
> > +		IRQF_TRIGGER_NONE, I2C_DRV_NAME, i2c);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev,
> > +			"Request I2C IRQ %d fail\n", i2c->irqnr);
> > +		return ret;
> > +	}
> I think the devm_request_irq should go near the end of probing.
> Otherwise the irq might fire before the used resources are ready.
OK

> 
> > +
> > +	i2c->adap.dev.of_node = pdev->dev.of_node;
> > +	i2c->dev = &i2c->adap.dev;
> > +	i2c->adap.dev.parent = &pdev->dev;
> > +	i2c->adap.owner = THIS_MODULE;
> > +	i2c->adap.algo = &mtk_i2c_algorithm;
> > +	i2c->adap.quirks = &mt6577_i2c_quirks;
> > +	i2c->adap.algo_data = NULL;
> No need to initialize this to NULL as the struct was allocated using
> kzalloc.
OK

> 
> > +	i2c->adap.timeout = 2 * HZ;
> > +	i2c->adap.retries = 1;
> > +
> > +	i2c->clk_main = devm_clk_get(&pdev->dev, "main");
> > +	if (IS_ERR(i2c->clk_main)) {
> > +		dev_err(&pdev->dev, "cannot get main clock\n");
> > +		return PTR_ERR(i2c->clk_main);
> > +	}
> > +
> > +	i2c->clk_dma = devm_clk_get(&pdev->dev, "dma");
> > +	if (IS_ERR(i2c->clk_dma)) {
> > +		dev_err(&pdev->dev, "cannot get dma clock\n");
> > +		return PTR_ERR(i2c->clk_dma);
> > +	}
> > +
> > +	if (i2c->have_pmic) {
> > +		i2c->clk_pmic = devm_clk_get(&pdev->dev, "pmic");
> > +		if (IS_ERR(i2c->clk_pmic)) {
> > +			dev_err(&pdev->dev, "cannot get pmic clock\n");
> > +			return PTR_ERR(i2c->clk_pmic);
> > +		}
> > +		clk_src_in_hz = clk_get_rate(i2c->clk_pmic) / clk_src_div;
> > +	} else {
> > +		clk_src_in_hz = clk_get_rate(i2c->clk_main) / clk_src_div;
> > +	}
> This can be simplified, i.e. the common line can go after the if block
> and then the else branch can be dropped.
One is clk_pmic, the other is clk_main, one way to reduce else branch is
use variable to store clk and set to i2c->clk_main by default. Then call
clk_get_rate at last. I don't know whether this is a better way.

> 
> > +	dev_dbg(&pdev->dev, "clock source %p,clock src frequency %d\n",
> > +		i2c->clk_main, clk_src_in_hz);
> > +	strlcpy(i2c->adap.name, I2C_DRV_NAME, sizeof(i2c->adap.name));
> > +	init_waitqueue_head(&i2c->wait);
> > +
> > +	ret = i2c_set_speed(i2c, clk_src_in_hz);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to set the speed.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = mtk_i2c_clock_enable(i2c);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "clock enable failed!\n");
> > +		return ret;
> > +	}
> > +	mtk_i2c_init_hw(i2c);
> > +	mtk_i2c_clock_disable(i2c);
> > +
> > +	i2c->dma_buf.vaddr = dma_alloc_coherent(&pdev->dev,
> > +		PAGE_SIZE, &i2c->dma_buf.paddr, GFP_KERNEL);
> > +	if (i2c->dma_buf.vaddr == NULL) {
> > +		dev_err(&pdev->dev, "dma_alloc_coherent fail\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	i2c_set_adapdata(&i2c->adap, i2c);
> > +	ret = i2c_add_adapter(&i2c->adap);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to add i2c bus to i2c core\n");
> > +		free_i2c_dma_bufs(i2c);
> > +		return ret;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, i2c);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_i2c_remove(struct platform_device *pdev)
> > +{
> > +	struct mtk_i2c *i2c = platform_get_drvdata(pdev);
> > +
> > +	i2c_del_adapter(&i2c->adap);
> > +	free_i2c_dma_bufs(i2c);
> > +	platform_set_drvdata(pdev, NULL);
> > +
> Here you need to make sure that no irq is running when i2c_del_adapter
> is called.
OK, add check here

> 
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver mtk_i2c_driver = {
> > +	.probe = mtk_i2c_probe,
> > +	.remove = mtk_i2c_remove,
> > +	.driver = {
> > +		.name = I2C_DRV_NAME,
> > +		.owner = THIS_MODULE,
> You don't need to set .owner any more. That's included in
> module_platform_driver since some time.
OK
> 
> > +		.of_match_table = of_match_ptr(mtk_i2c_of_match),
> > +	},
> > +};
> > +
> > +module_platform_driver(mtk_i2c_driver);
> > +
> > +MODULE_LICENSE("GPL");
> MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("MediaTek I2C Bus Driver");
> > +MODULE_AUTHOR("Xudong Chen <xudong.chen-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>");
> 
> Best regards
> Uwe
> 

Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller

[Yingjoe Chen]

Hi Uwe,

Thanks for your review,

On Wed, 2015-01-21 at 11:13 +0800, Eddie Huang wrote:
<...>
> > > +		ret = -EINVAL;
> > > +		goto err_exit;
> > > +	}
> > > +
> > > +	if (msgs->buf == NULL) {
> > > +		dev_dbg(i2c->dev, " data buffer is NULL.\n");
> > > +		ret = -EINVAL;
> > > +		goto err_exit;
> > > +	}
> > > +
> > > +	i2c->addr = msgs->addr;
> > > +	i2c->msg_len = msgs->len;
> > > +	i2c->msg_buf = msgs->buf;
> > > +
> > > +	if (msgs->flags & I2C_M_RD)
> > > +		i2c->op = I2C_MASTER_RD;
> > > +	else
> > > +		i2c->op = I2C_MASTER_WR;
> > > +
> > > +	/* combined two messages into one transaction */
> > > +	if (num > 1) {
> > > +		i2c->msg_aux_len = (msgs + 1)->len;
> > > +		i2c->op = I2C_MASTER_WRRD;
> > > +	}
> > This means "write then read", right? You should check here that the
> > first message is really a write and the 2nd a read then.
> > Can this happen at all with the quirks defined below (.max_num_msgs =
> > 1)?
> Yes, mean write then read. Indeed, add check is better.
> If msg number is 1, means normal write or read, not "write then read".

The quirks will increase the message count and check 'write then read'
for us. We don't have to add check here.


<...>
> > > +static int mtk_i2c_remove(struct platform_device *pdev)
> > > +{
> > > +	struct mtk_i2c *i2c = platform_get_drvdata(pdev);
> > > +
> > > +	i2c_del_adapter(&i2c->adap);
> > > +	free_i2c_dma_bufs(i2c);
> > > +	platform_set_drvdata(pdev, NULL);
> > > +
> > Here you need to make sure that no irq is running when i2c_del_adapter
> > is called.
> OK, add check here

I thought after i2c_del_adapter() is complete, all i2c_transfer for this
adapter is completed. If this is true, then i2c clock is already off and
we won't have any on-going transfer/pending irq.

Joe.C

Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller

[Uwe Kleine-König]

Hello,

On Wed, Jan 21, 2015 at 02:30:09PM +0800, Yingjoe Chen wrote:
> On Wed, 2015-01-21 at 11:13 +0800, Eddie Huang wrote:
> <...>
> > > > +		ret = -EINVAL;
> > > > +		goto err_exit;
> > > > +	}
> > > > +
> > > > +	if (msgs->buf == NULL) {
> > > > +		dev_dbg(i2c->dev, " data buffer is NULL.\n");
> > > > +		ret = -EINVAL;
> > > > +		goto err_exit;
> > > > +	}
> > > > +
> > > > +	i2c->addr = msgs->addr;
> > > > +	i2c->msg_len = msgs->len;
> > > > +	i2c->msg_buf = msgs->buf;
> > > > +
> > > > +	if (msgs->flags & I2C_M_RD)
> > > > +		i2c->op = I2C_MASTER_RD;
> > > > +	else
> > > > +		i2c->op = I2C_MASTER_WR;
> > > > +
> > > > +	/* combined two messages into one transaction */
> > > > +	if (num > 1) {
> > > > +		i2c->msg_aux_len = (msgs + 1)->len;
> > > > +		i2c->op = I2C_MASTER_WRRD;
> > > > +	}
> > > This means "write then read", right? You should check here that the
> > > first message is really a write and the 2nd a read then.
> > > Can this happen at all with the quirks defined below (.max_num_msgs =
> > > 1)?
> > Yes, mean write then read. Indeed, add check is better.
> > If msg number is 1, means normal write or read, not "write then read".
> 
> The quirks will increase the message count and check 'write then read'
> for us. We don't have to add check here.
I have to admit I don't know that quirks stuff, so it's well possible
that I'm wrong here.
 
> > > > +static int mtk_i2c_remove(struct platform_device *pdev)
> > > > +{
> > > > +	struct mtk_i2c *i2c = platform_get_drvdata(pdev);
> > > > +
> > > > +	i2c_del_adapter(&i2c->adap);
> > > > +	free_i2c_dma_bufs(i2c);
> > > > +	platform_set_drvdata(pdev, NULL);
> > > > +
> > > Here you need to make sure that no irq is running when i2c_del_adapter
> > > is called.
> > OK, add check here
> 
> I thought after i2c_del_adapter() is complete, all i2c_transfer for this
> adapter is completed. If this is true, then i2c clock is already off and
> we won't have any on-going transfer/pending irq.
Consider that there is an ongoing transaction and before it completes
the adapter-device is unbound from the driver. Then i2c_del_adapter is
called which frees the resources managed by the core, then the device's
completion irq triggers and the freed adapter is used which probably
results in an oops.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller

[Yingjoe Chen]

On Wed, 2015-01-21 at 09:15 +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, Jan 21, 2015 at 02:30:09PM +0800, Yingjoe Chen wrote:
> > On Wed, 2015-01-21 at 11:13 +0800, Eddie Huang wrote:
> > <...>
> > > > > +		ret = -EINVAL;
> > > > > +		goto err_exit;
> > > > > +	}
> > > > > +
> > > > > +	if (msgs->buf == NULL) {
> > > > > +		dev_dbg(i2c->dev, " data buffer is NULL.\n");
> > > > > +		ret = -EINVAL;
> > > > > +		goto err_exit;
> > > > > +	}
> > > > > +
> > > > > +	i2c->addr = msgs->addr;
> > > > > +	i2c->msg_len = msgs->len;
> > > > > +	i2c->msg_buf = msgs->buf;
> > > > > +
> > > > > +	if (msgs->flags & I2C_M_RD)
> > > > > +		i2c->op = I2C_MASTER_RD;
> > > > > +	else
> > > > > +		i2c->op = I2C_MASTER_WR;
> > > > > +
> > > > > +	/* combined two messages into one transaction */
> > > > > +	if (num > 1) {
> > > > > +		i2c->msg_aux_len = (msgs + 1)->len;
> > > > > +		i2c->op = I2C_MASTER_WRRD;
> > > > > +	}
> > > > This means "write then read", right? You should check here that the
> > > > first message is really a write and the 2nd a read then.
> > > > Can this happen at all with the quirks defined below (.max_num_msgs =
> > > > 1)?
> > > Yes, mean write then read. Indeed, add check is better.
> > > If msg number is 1, means normal write or read, not "write then read".
> > 
> > The quirks will increase the message count and check 'write then read'
> > for us. We don't have to add check here.
> I have to admit I don't know that quirks stuff, so it's well possible
> that I'm wrong here.
>  
> > > > > +static int mtk_i2c_remove(struct platform_device *pdev)
> > > > > +{
> > > > > +	struct mtk_i2c *i2c = platform_get_drvdata(pdev);
> > > > > +
> > > > > +	i2c_del_adapter(&i2c->adap);
> > > > > +	free_i2c_dma_bufs(i2c);
> > > > > +	platform_set_drvdata(pdev, NULL);
> > > > > +
> > > > Here you need to make sure that no irq is running when i2c_del_adapter
> > > > is called.
> > > OK, add check here
> > 
> > I thought after i2c_del_adapter() is complete, all i2c_transfer for this
> > adapter is completed. If this is true, then i2c clock is already off and
> > we won't have any on-going transfer/pending irq.
> Consider that there is an ongoing transaction and before it completes
> the adapter-device is unbound from the driver. Then i2c_del_adapter is
> called which frees the resources managed by the core, then the device's
> completion irq triggers and the freed adapter is used which probably
> results in an oops.

Not sure if I missed anything. i2c_transfer() is a synchronize call. If
we fixed timeout issue you mentioned in mtk_i2c_transfer(), it will turn
off clock before it return, which disable any transaction and clear all
pending irq.

Your scenario can only happens when one thread is still running in
i2c_transfer/algo->master_xfer and the other thread is trying to remove
the device. If that happened, then every device data access in
mtk_i2c_transfer might cause oops. I looked at some i2c drivers and
can't find any checking for this case, I can't find anything prevent i2c
device removal before pending i2c_transfer complete either. Would you
give me an example?

Joe.C

Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller

[Uwe Kleine-König]

Hello,

On Wed, Jan 21, 2015 at 08:49:40PM +0800, Yingjoe Chen wrote:
> On Wed, 2015-01-21 at 09:15 +0100, Uwe Kleine-König wrote:
> > On Wed, Jan 21, 2015 at 02:30:09PM +0800, Yingjoe Chen wrote:
> > > On Wed, 2015-01-21 at 11:13 +0800, Eddie Huang wrote:
> > > <...>
> > > > > > +		ret = -EINVAL;
> > > > > > +		goto err_exit;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (msgs->buf == NULL) {
> > > > > > +		dev_dbg(i2c->dev, " data buffer is NULL.\n");
> > > > > > +		ret = -EINVAL;
> > > > > > +		goto err_exit;
> > > > > > +	}
> > > > > > +
> > > > > > +	i2c->addr = msgs->addr;
> > > > > > +	i2c->msg_len = msgs->len;
> > > > > > +	i2c->msg_buf = msgs->buf;
> > > > > > +
> > > > > > +	if (msgs->flags & I2C_M_RD)
> > > > > > +		i2c->op = I2C_MASTER_RD;
> > > > > > +	else
> > > > > > +		i2c->op = I2C_MASTER_WR;
> > > > > > +
> > > > > > +	/* combined two messages into one transaction */
> > > > > > +	if (num > 1) {
> > > > > > +		i2c->msg_aux_len = (msgs + 1)->len;
> > > > > > +		i2c->op = I2C_MASTER_WRRD;
> > > > > > +	}
> > > > > This means "write then read", right? You should check here that the
> > > > > first message is really a write and the 2nd a read then.
> > > > > Can this happen at all with the quirks defined below (.max_num_msgs =
> > > > > 1)?
> > > > Yes, mean write then read. Indeed, add check is better.
> > > > If msg number is 1, means normal write or read, not "write then read".
> > > 
> > > The quirks will increase the message count and check 'write then read'
> > > for us. We don't have to add check here.
> > I have to admit I don't know that quirks stuff, so it's well possible
> > that I'm wrong here.
> >  
> > > > > > +static int mtk_i2c_remove(struct platform_device *pdev)
> > > > > > +{
> > > > > > +	struct mtk_i2c *i2c = platform_get_drvdata(pdev);
> > > > > > +
> > > > > > +	i2c_del_adapter(&i2c->adap);
> > > > > > +	free_i2c_dma_bufs(i2c);
> > > > > > +	platform_set_drvdata(pdev, NULL);
> > > > > > +
> > > > > Here you need to make sure that no irq is running when i2c_del_adapter
> > > > > is called.
> > > > OK, add check here
> > > 
> > > I thought after i2c_del_adapter() is complete, all i2c_transfer for this
> > > adapter is completed. If this is true, then i2c clock is already off and
> > > we won't have any on-going transfer/pending irq.
> > Consider that there is an ongoing transaction and before it completes
> > the adapter-device is unbound from the driver. Then i2c_del_adapter is
> > called which frees the resources managed by the core, then the device's
> > completion irq triggers and the freed adapter is used which probably
> > results in an oops.
> 
> Not sure if I missed anything. i2c_transfer() is a synchronize call. If
> we fixed timeout issue you mentioned in mtk_i2c_transfer(), it will turn
> off clock before it return, which disable any transaction and clear all
> pending irq.
There is no synchronization to prevent unbinding the i2c-bus device
while there is a i2c transfer on the wire. i2c_del_adapter only takes
i2c-core.c's &core_lock while i2c_transfer takes &adapter->bus_lock.
If you want to test for it: do something like that:

	while true; do dd if=/sys/bus/i2c/.../eeprom of=/dev/null; done

and while this is running do:

	cd /sys/bus/platform/drivers/mt-i2c
	while true; do
		echo 1100d000.i2c > unbind;
		sleep 1;
		echo 1100d000.i2c > bind;
		sleep 1;
	done
 
> Your scenario can only happens when one thread is still running in
> i2c_transfer/algo->master_xfer and the other thread is trying to remove
> the device. If that happened, then every device data access in
> mtk_i2c_transfer might cause oops. I looked at some i2c drivers and
> can't find any checking for this case, I can't find anything prevent i2c
> device removal before pending i2c_transfer complete either. Would you
> give me an example?
I just noticed that even "my" driver is affected. If the above recipe
makes your driver barf there is something to fix, if not ... hmm, then
maybe there is more synchronization than I'm aware of or my recipe is
wrong.

At least another driver author believed me:
http://thread.gmane.org/gmane.linux.drivers.i2c/21531/focus=21662

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller

[Yingjoe Chen]

Hi,

On Wed, 2015-01-21 at 16:31 +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, Jan 21, 2015 at 08:49:40PM +0800, Yingjoe Chen wrote:
> > On Wed, 2015-01-21 at 09:15 +0100, Uwe Kleine-König wrote:
<...>
> > > > > > > +static int mtk_i2c_remove(struct platform_device *pdev)
> > > > > > > +{
> > > > > > > +	struct mtk_i2c *i2c = platform_get_drvdata(pdev);
> > > > > > > +
> > > > > > > +	i2c_del_adapter(&i2c->adap);
> > > > > > > +	free_i2c_dma_bufs(i2c);
> > > > > > > +	platform_set_drvdata(pdev, NULL);
> > > > > > > +
> > > > > > Here you need to make sure that no irq is running when i2c_del_adapter
> > > > > > is called.
> > > > > OK, add check here
> > > > 
> > > > I thought after i2c_del_adapter() is complete, all i2c_transfer for this
> > > > adapter is completed. If this is true, then i2c clock is already off and
> > > > we won't have any on-going transfer/pending irq.
> > > Consider that there is an ongoing transaction and before it completes
> > > the adapter-device is unbound from the driver. Then i2c_del_adapter is
> > > called which frees the resources managed by the core, then the device's
> > > completion irq triggers and the freed adapter is used which probably
> > > results in an oops.
> > 
> > Not sure if I missed anything. i2c_transfer() is a synchronize call. If
> > we fixed timeout issue you mentioned in mtk_i2c_transfer(), it will turn
> > off clock before it return, which disable any transaction and clear all
> > pending irq.
> There is no synchronization to prevent unbinding the i2c-bus device
> while there is a i2c transfer on the wire. i2c_del_adapter only takes
> i2c-core.c's &core_lock while i2c_transfer takes &adapter->bus_lock.
> If you want to test for it: do something like that:
> 
> 	while true; do dd if=/sys/bus/i2c/.../eeprom of=/dev/null; done
> 
> and while this is running do:
> 
> 	cd /sys/bus/platform/drivers/mt-i2c
> 	while true; do
> 		echo 1100d000.i2c > unbind;
> 		sleep 1;
> 		echo 1100d000.i2c > bind;
> 		sleep 1;
> 	done
>  
> > Your scenario can only happens when one thread is still running in
> > i2c_transfer/algo->master_xfer and the other thread is trying to remove
> > the device. If that happened, then every device data access in
> > mtk_i2c_transfer might cause oops. I looked at some i2c drivers and
> > can't find any checking for this case, I can't find anything prevent i2c
> > device removal before pending i2c_transfer complete either. Would you
> > give me an example?
> I just noticed that even "my" driver is affected. If the above recipe
> makes your driver barf there is something to fix, if not ... hmm, then
> maybe there is more synchronization than I'm aware of or my recipe is
> wrong.
> 
> At least another driver author believed me:
> http://thread.gmane.org/gmane.linux.drivers.i2c/21531/focus=21662

If there is no synchronization mechanism then every driver is affected.
We should add the check in the core instead of fixing it in every
driver. We could take the bus_lock in i2c_del_adapter() and check if the
adapter is removed in __i2c_transfer(), this should fix the problem.

Wolfram, what do you think about this? Is there anything we are missing?

Joe.C


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller

[Uwe Kleine-König]

Hello,

On Wed, Jan 21, 2015 at 11:13:24AM +0800, Eddie Huang wrote:
> > > +	/* set when doing the transfer */
> > > +	u16 irq_stat;			/* interrupt status */
> > > +	unsigned int speed_hz;		/* The speed in transfer */
> > > +	bool trans_stop;		/* i2c transfer stop */
> > > +	enum mtk_trans_op op;
> > > +	u16 msg_len;
> > > +	u8 *msg_buf;			/* pointer to msg data */
> > > +	u16 msg_aux_len;		/* WRRD mode to set AUX_LEN register*/
> > > +	u16 addr;	/* 7bit slave address, without read/write bit */
> > Wouldn't it be easier to maintain a pointer to the message to be
> > transferred?
> I think use mtk_i2c pointer is more flexible than maintain a pointer to
> message.
Not sure you understood what I intended to suggest. My idea was to drop

	u16 msg_len;
	u8 *msg_buf;
	u16 msg_aux_len; // maybe
	u16 addr;

from struct mtk_i2c and add a struct i2c_msg *msg instead. Up to you to
decide.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] I2C: mediatek: Add driver for MediaTek I2C controller

[Eddie Huang]

Hi Uwe,

On Wed, 2015-01-21 at 09:20 +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, Jan 21, 2015 at 11:13:24AM +0800, Eddie Huang wrote:
> > > > +	/* set when doing the transfer */
> > > > +	u16 irq_stat;			/* interrupt status */
> > > > +	unsigned int speed_hz;		/* The speed in transfer */
> > > > +	bool trans_stop;		/* i2c transfer stop */
> > > > +	enum mtk_trans_op op;
> > > > +	u16 msg_len;
> > > > +	u8 *msg_buf;			/* pointer to msg data */
> > > > +	u16 msg_aux_len;		/* WRRD mode to set AUX_LEN register*/
> > > > +	u16 addr;	/* 7bit slave address, without read/write bit */
> > > Wouldn't it be easier to maintain a pointer to the message to be
> > > transferred?
> > I think use mtk_i2c pointer is more flexible than maintain a pointer to
> > message.
> Not sure you understood what I intended to suggest. My idea was to drop
> 
> 	u16 msg_len;
> 	u8 *msg_buf;
> 	u16 msg_aux_len; // maybe
> 	u16 addr;
> 
> from struct mtk_i2c and add a struct i2c_msg *msg instead. Up to you to
> decide.
> 
Because this driver pass mtk_i2c pointer between functions, it's
flexible to use any member of struct mtk_i2c. The good thing is avoid
passing one more struct i2c_msg parameter between functions, or another
struct copy. The bad thing is make struct mtk_i2c larger. I prefer to
keep as it is now.

Best Regards
Eddie