LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v5 0/3] ARM: mediatek: Add driver for Mediatek I2C controller 
@ 2015-03-21  6:05 Eddie Huang
  2015-03-21  6:05 ` [PATCH v5 1/3] dt-bindings: Add I2C bindings for mt65xx/mt81xx Eddie Huang
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Eddie Huang @ 2015-03-21  6:05 UTC (permalink / raw)
  To: Wolfram Sang, Matthias Brugger, Uwe Kleine-König
  Cc: srv_heupstream, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, Jean Delvare, David Box, Arnd Bergmann,
	Doug Anderson, Max Schwarz, Boris BREZILLON, Anders Berg,
	Neelesh Gupta, Lee Jones, Simon Glass, Jim Cromie, Wei Yan,
	Bjorn Andersson, Beniamino Galvani, Xudong Chen, Liguo Zhang,
	linux-i2c, devicetree, linux-kernel, linux-arm-kernel,
	Sascha Hauer, linux-mediatek

This series is for Mediatek SoCs I2C controller common bus driver.

Earlier MTK SoC ((for example, MT6589, MT8135)) I2C HW has some limitationes.
New generation SoC like MT8173 fix these 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.

This driver is based on 4.0-rc1.

Change in v5:
Apply new i2c_adapter_quirks patch [2]. Change to use dam_map_single to map
dma buffer. Add spinlock to fix race condition. Check of_property_read_u32
return value. Remove I2C_FUNC_10BIT_ADDR capability due to driver not implement.
Add MT8173 I2C 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.


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

Eddie Huang (1):
  I2C: mediatek: Add driver for MediaTek MT8173 I2C controller

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         |  41 ++
 drivers/i2c/busses/Kconfig                         |   9 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-mt65xx.c                    | 748 +++++++++++++++++++++
 4 files changed, 799 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



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v5 1/3] dt-bindings: Add I2C bindings for mt65xx/mt81xx.
  2015-03-21  6:05 [PATCH v5 0/3] ARM: mediatek: Add driver for Mediatek I2C controller Eddie Huang
@ 2015-03-21  6:05 ` Eddie Huang
  2015-03-23  8:45   ` Sascha Hauer
  2015-03-21  6:05 ` [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C controller Eddie Huang
  2015-03-21  6:05 ` [PATCH v5 3/3] I2C: mediatek: Add driver for MediaTek MT8173 " Eddie Huang
  2 siblings, 1 reply; 14+ messages in thread
From: Eddie Huang @ 2015-03-21  6:05 UTC (permalink / raw)
  To: Wolfram Sang, Matthias Brugger, Uwe Kleine-König
  Cc: srv_heupstream, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, Jean Delvare, David Box, Arnd Bergmann,
	Doug Anderson, Max Schwarz, Boris BREZILLON, Anders Berg,
	Neelesh Gupta, Lee Jones, Simon Glass, Jim Cromie, Wei Yan,
	Bjorn Andersson, Beniamino Galvani, Xudong Chen, Liguo Zhang,
	linux-i2c, devicetree, linux-kernel, linux-arm-kernel,
	Sascha Hauer, linux-mediatek, 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         | 41 ++++++++++++++++++++++
 1 file changed, 41 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..eff52c1
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-mt6577.txt
@@ -0,0 +1,41 @@
+* 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.
+      (e) "mediatek,mt8173-i2c", for i2c compatible with mt8173 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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C controller
  2015-03-21  6:05 [PATCH v5 0/3] ARM: mediatek: Add driver for Mediatek I2C controller Eddie Huang
  2015-03-21  6:05 ` [PATCH v5 1/3] dt-bindings: Add I2C bindings for mt65xx/mt81xx Eddie Huang
@ 2015-03-21  6:05 ` Eddie Huang
  2015-03-23  8:42   ` Sascha Hauer
  2015-03-21  6:05 ` [PATCH v5 3/3] I2C: mediatek: Add driver for MediaTek MT8173 " Eddie Huang
  2 siblings, 1 reply; 14+ messages in thread
From: Eddie Huang @ 2015-03-21  6:05 UTC (permalink / raw)
  To: Wolfram Sang, Matthias Brugger, Uwe Kleine-König
  Cc: srv_heupstream, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, Jean Delvare, David Box, Arnd Bergmann,
	Doug Anderson, Max Schwarz, Boris BREZILLON, Anders Berg,
	Neelesh Gupta, Lee Jones, Simon Glass, Jim Cromie, Wei Yan,
	Bjorn Andersson, Beniamino Galvani, Xudong Chen, Liguo Zhang,
	linux-i2c, devicetree, linux-kernel, linux-arm-kernel,
	Sascha Hauer, linux-mediatek, Eddie Huang

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

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@mediatek.com>
Signed-off-by: Liguo Zhang <liguo.zhang@mediatek.com>
Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
---
 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

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 22da9c2..243c4ec 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -602,6 +602,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 || COMPILE_TEST
+	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 3638feb..372a711 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -58,6 +58,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..3940112
--- /dev/null
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -0,0 +1,705 @@
+/*
+ * Copyright (c) 2014 MediaTek Inc.
+ * Author: Xudong.chen <xudong.chen@mediatek.com>
+ *
+ * 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_MSG_NUM_MT6577		1
+#define MAX_DMA_TRANS_SIZE_MT6577	255
+#define MAX_WRRD_TRANS_SIZE_MT6577	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 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*/
+	unsigned int irqnr;		/* i2c interrupt number */
+	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 */
+	spinlock_t irqlock;		/* interrupt lock */
+	unsigned int speed_hz;		/* The speed in transfer */
+	bool trans_stop;		/* i2c transfer stop */
+	enum mtk_trans_op op;
+	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 mtk_i2c_writew(u16 value, struct mtk_i2c *i2c, u8 offset)
+{
+	writew(value, i2c->base + offset);
+}
+
+static inline u16 mtk_i2c_readw(struct mtk_i2c *i2c, u8 offset)
+{
+	return readw(i2c->base + offset);
+}
+
+static inline void mtk_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 inline void mtk_i2c_init_hw(struct mtk_i2c *i2c)
+{
+	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);
+}
+
+/* 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, struct i2c_msg *msgs)
+{
+	u16 addr_reg;
+	u16 control_reg;
+	u16 irqstat;
+	dma_addr_t rpaddr = 0;
+	dma_addr_t wpaddr = 0;
+	int tmo = i2c->adap.timeout;
+
+	i2c->trans_stop = false;
+	spin_lock(&i2c->irqlock);
+	i2c->irq_stat = 0;
+	spin_unlock(&i2c->irqlock);
+
+	/* If use i2c pin from PMIC mt6397 side, need set PATH_DIR first */
+	if (i2c->have_pmic)
+		mtk_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;
+	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);
+
+	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);
+
+	/* Set transfer and transaction len */
+	if (i2c->op == I2C_MASTER_WRRD) {
+		mtk_i2c_writew(msgs->len | ((msgs + 1)->len) << 8,
+			i2c, OFFSET_TRANSFER_LEN);
+		mtk_i2c_writew(I2C_WRRD_TRANAC_VALUE, i2c, OFFSET_TRANSAC_LEN);
+	} else {
+		mtk_i2c_writew(msgs->len, i2c, OFFSET_TRANSFER_LEN);
+		mtk_i2c_writew(I2C_RD_TRANAC_VALUE, i2c, OFFSET_TRANSAC_LEN);
+	}
+
+	/* Prepare buffer data to start transfer */
+	if (i2c->op == I2C_MASTER_RD) {
+		mtk_i2c_writel_dma(I2C_DMA_INT_FLAG_NONE, i2c, OFFSET_INT_FLAG);
+		mtk_i2c_writel_dma(I2C_DMA_CON_RX, i2c, OFFSET_CON);
+		rpaddr = dma_map_single(i2c->adap.dev.parent, msgs->buf,
+						msgs->len, DMA_FROM_DEVICE);
+		if (dma_mapping_error(i2c->adap.dev.parent, rpaddr))
+			return -ENOMEM;
+		mtk_i2c_writel_dma((u32)rpaddr, i2c, OFFSET_RX_MEM_ADDR);
+		mtk_i2c_writel_dma(msgs->len, i2c, OFFSET_RX_LEN);
+	} else if (i2c->op == I2C_MASTER_WR) {
+		mtk_i2c_writel_dma(I2C_DMA_INT_FLAG_NONE, i2c, OFFSET_INT_FLAG);
+		mtk_i2c_writel_dma(I2C_DMA_CON_TX, i2c, OFFSET_CON);
+		wpaddr = dma_map_single(i2c->adap.dev.parent, msgs->buf,
+						msgs->len, DMA_TO_DEVICE);
+		if (dma_mapping_error(i2c->adap.dev.parent, wpaddr))
+			return -ENOMEM;
+		mtk_i2c_writel_dma((u32)wpaddr, i2c, OFFSET_TX_MEM_ADDR);
+		mtk_i2c_writel_dma(msgs->len, i2c, OFFSET_TX_LEN);
+	} else {
+		mtk_i2c_writel_dma(I2C_DMA_CLR_FLAG, i2c, OFFSET_INT_FLAG);
+		mtk_i2c_writel_dma(I2C_DMA_CLR_FLAG, i2c, OFFSET_CON);
+		wpaddr = dma_map_single(i2c->adap.dev.parent, msgs->buf,
+						msgs->len, DMA_TO_DEVICE);
+		if (dma_mapping_error(i2c->adap.dev.parent, wpaddr))
+			return -ENOMEM;
+		rpaddr = dma_map_single(i2c->adap.dev.parent, (msgs + 1)->buf,
+						(msgs + 1)->len,
+						DMA_FROM_DEVICE);
+		if (dma_mapping_error(i2c->adap.dev.parent, rpaddr)) {
+			dma_unmap_single(i2c->adap.dev.parent, wpaddr,
+						msgs->len, DMA_TO_DEVICE);
+			return -ENOMEM;
+		}
+		mtk_i2c_writel_dma((u32)wpaddr, i2c, OFFSET_TX_MEM_ADDR);
+		mtk_i2c_writel_dma((u32)rpaddr, i2c, OFFSET_RX_MEM_ADDR);
+		mtk_i2c_writel_dma(msgs->len, i2c, OFFSET_TX_LEN);
+		mtk_i2c_writel_dma((msgs + 1)->len, i2c, OFFSET_RX_LEN);
+	}
+
+	/* flush before sending start */
+	mb();
+	mtk_i2c_writel_dma(I2C_DMA_START_EN, i2c, OFFSET_EN);
+	mtk_i2c_writew(I2C_TRANSAC_START, i2c, OFFSET_START);
+
+	tmo = wait_event_timeout(i2c->wait, i2c->trans_stop, tmo);
+
+	if (i2c->op == I2C_MASTER_WR) {
+		dma_unmap_single(i2c->adap.dev.parent, wpaddr,
+					msgs->len, DMA_TO_DEVICE);
+	} else if (i2c->op == I2C_MASTER_RD) {
+		dma_unmap_single(i2c->adap.dev.parent, rpaddr,
+					msgs->len, DMA_FROM_DEVICE);
+	} else {
+		dma_unmap_single(i2c->adap.dev.parent, wpaddr, msgs->len,
+					DMA_TO_DEVICE);
+		dma_unmap_single(i2c->adap.dev.parent, rpaddr, (msgs + 1)->len,
+					DMA_FROM_DEVICE);
+	}
+
+	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);
+
+	if (irqstat & (I2C_HS_NACKERR | I2C_ACKERR)) {
+		dev_dbg(i2c->dev, "addr: %x, transfer ACK error\n", msgs->addr);
+		mtk_i2c_init_hw(i2c);
+		return -EREMOTEIO;
+	}
+
+	return 0;
+}
+
+static int mtk_i2c_transfer(struct i2c_adapter *adap,
+	struct i2c_msg msgs[], int num)
+{
+	int ret;
+	int left_num = num;
+	struct mtk_i2c *i2c = i2c_get_adapdata(adap);
+
+	ret = mtk_i2c_clock_enable(i2c);
+	if (ret)
+		return ret;
+
+	if (msgs->buf == NULL) {
+		dev_dbg(i2c->dev, "data buffer is NULL.\n");
+		ret = -EINVAL;
+		goto err_exit;
+	}
+
+	if (msgs->flags & I2C_M_RD)
+		i2c->op = I2C_MASTER_RD;
+	else
+		i2c->op = I2C_MASTER_WR;
+
+	if (num > 1) {
+		/* combined two messages into one transaction */
+		i2c->op = I2C_MASTER_WRRD;
+		left_num--;
+	}
+
+	/* always use DMA mode. */
+	ret = mtk_i2c_do_transfer(i2c, msgs);
+	if (ret < 0)
+		goto err_exit;
+
+	/* 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 */
+	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;
+	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 u32 mtk_i2c_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm mtk_i2c_algorithm = {
+	.master_xfer = mtk_i2c_transfer,
+	.functionality = mtk_i2c_functionality,
+};
+
+static inline unsigned long 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 (unsigned long)match->data;
+}
+
+static int mtk_i2c_parse_dt(struct device_node *np, struct mtk_i2c *i2c,
+	unsigned int *clk_src_div)
+{
+	int ret;
+
+	i2c->speed_hz = I2C_DEFAUT_SPEED;
+	ret = of_property_read_u32(np, "clock-frequency", &i2c->speed_hz);
+	if (ret < 0)
+		return ret;
+
+	ret = of_property_read_u32(np, "clock-div", clk_src_div);
+	if (ret < 0)
+		return ret;
+
+	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_AQ_COMB_WRITE_THEN_READ,
+	.max_num_msgs = MAX_MSG_NUM_MT6577,
+	.max_write_len = MAX_DMA_TRANS_SIZE_MT6577,
+	.max_read_len = MAX_DMA_TRANS_SIZE_MT6577,
+	.max_comb_1st_msg_len = MAX_DMA_TRANS_SIZE_MT6577,
+	.max_comb_2nd_msg_len = MAX_WRRD_TRANS_SIZE_MT6577,
+};
+
+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);
+	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;
+
+	spin_lock_init(&i2c->irqlock);
+
+	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.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);
+	}
+
+	clk = i2c->clk_main;
+	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 = i2c->clk_pmic;
+	}
+	clk_src_in_hz = clk_get_rate(clk) / 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);
+
+	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_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");
+		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);
+	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,
+		.of_match_table = of_match_ptr(mtk_i2c_of_match),
+	},
+};
+
+module_platform_driver(mtk_i2c_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("MediaTek I2C Bus Driver");
+MODULE_AUTHOR("Xudong Chen <xudong.chen@mediatek.com>");
-- 
1.8.1.1.dirty


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v5 3/3] I2C: mediatek: Add driver for MediaTek MT8173 I2C controller
  2015-03-21  6:05 [PATCH v5 0/3] ARM: mediatek: Add driver for Mediatek I2C controller Eddie Huang
  2015-03-21  6:05 ` [PATCH v5 1/3] dt-bindings: Add I2C bindings for mt65xx/mt81xx Eddie Huang
  2015-03-21  6:05 ` [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C controller Eddie Huang
@ 2015-03-21  6:05 ` Eddie Huang
  2015-03-23  7:39   ` Sascha Hauer
  2 siblings, 1 reply; 14+ messages in thread
From: Eddie Huang @ 2015-03-21  6:05 UTC (permalink / raw)
  To: Wolfram Sang, Matthias Brugger, Uwe Kleine-König
  Cc: srv_heupstream, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, Jean Delvare, David Box, Arnd Bergmann,
	Doug Anderson, Max Schwarz, Boris BREZILLON, Anders Berg,
	Neelesh Gupta, Lee Jones, Simon Glass, Jim Cromie, Wei Yan,
	Bjorn Andersson, Beniamino Galvani, Xudong Chen, Liguo Zhang,
	linux-i2c, devicetree, linux-kernel, linux-arm-kernel,
	Sascha Hauer, linux-mediatek, Eddie Huang

Add mediatek MT8173 I2C controller driver. Compare to I2C controller
of earlier mediatek SoC, MT8173 fix write-then-read limitation, and
also increase message size to 64kb.

Signed-off-by: Xudong Chen <xudong.chen@mediatek.com>
Signed-off-by: Liguo Zhang <liguo.zhang@mediatek.com>
Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
---
 drivers/i2c/busses/i2c-mt65xx.c | 107 ++++++++++++++++++++++++++++------------
 1 file changed, 75 insertions(+), 32 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index 3940112..17607c8 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -33,10 +33,13 @@
 #include <linux/of_irq.h>
 #include <linux/clk.h>
 
+#define I2C_RS_TRANSFER			(1 << 4)
 #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_RS_MUL_CNFG			(1 << 15)
+#define I2C_RS_MUL_TRIG			(1 << 14)
 #define I2C_TIMING_STEP_DIV_MASK	(0x3f << 0)
 #define I2C_TIMING_SAMPLE_COUNT_MASK	(0x7 << 0)
 #define I2C_TIMING_SAMPLE_DIV_MASK	(0x7 << 8)
@@ -67,6 +70,9 @@
 #define MAX_MSG_NUM_MT6577		1
 #define MAX_DMA_TRANS_SIZE_MT6577	255
 #define MAX_WRRD_TRANS_SIZE_MT6577	31
+#define MAX_MSG_NUM_MT8173		65535
+#define MAX_DMA_TRANS_SIZE_MT8173	65535
+#define MAX_WRRD_TRANS_SIZE_MT8173	65535
 #define MAX_SAMPLE_CNT_DIV		8
 #define MAX_STEP_CNT_DIV		64
 #define MAX_HS_STEP_CNT_DIV		8
@@ -81,6 +87,7 @@
 
 #define COMPAT_MT6577			(0x1 << 0)
 #define COMPAT_MT6589			(0x1 << 1)
+#define COMPAT_MT8173			(0x1 << 2)
 
 #define I2C_DRV_NAME		"mt-i2c"
 
@@ -172,6 +179,7 @@ struct mtk_i2c {
 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 },
+	{ .compatible = "mediatek,mt8173-i2c", .data = (void *)COMPAT_MT8173 },
 	{}
 };
 MODULE_DEVICE_TABLE(of, mtk_i2c_match);
@@ -236,7 +244,7 @@ static inline void mtk_i2c_init_hw(struct mtk_i2c *i2c)
 	else
 		mtk_i2c_writew(I2C_IO_CONFIG_OPEN_DRAIN, i2c, OFFSET_IO_CONFIG);
 
-	if (i2c->platform_compat & COMPAT_MT6577)
+	if (i2c->platform_compat & (COMPAT_MT6577 | COMPAT_MT8173))
 		mtk_i2c_writew(I2C_DCM_DISABLE, i2c, OFFSET_DCM_EN);
 
 	mtk_i2c_writew(i2c->timing_reg, i2c, OFFSET_TIMING);
@@ -320,10 +328,12 @@ static int i2c_set_speed(struct mtk_i2c *i2c, unsigned int clk_src_in_hz)
 	return 0;
 }
 
-static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs)
+static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
+					int num, int left_num)
 {
 	u16 addr_reg;
 	u16 control_reg;
+	u16 start_reg = 0;
 	u16 irqstat;
 	dma_addr_t rpaddr = 0;
 	dma_addr_t wpaddr = 0;
@@ -344,6 +354,8 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs)
 		control_reg |= I2C_CONTROL_RS;
 	if (i2c->op == I2C_MASTER_WRRD)
 		control_reg |= I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS;
+	if (left_num >= 1)
+		control_reg |= I2C_CONTROL_RS;
 	mtk_i2c_writew(control_reg, i2c, OFFSET_CONTROL);
 
 	/* set start condition */
@@ -361,13 +373,13 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs)
 	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_RS_TRANSFER | 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);
+	mtk_i2c_writew(I2C_RS_TRANSFER | I2C_HS_NACKERR | I2C_ACKERR
+			| I2C_TRANSAC_COMP, i2c, OFFSET_INTR_MASK);
 
 	/* Set transfer and transaction len */
 	if (i2c->op == I2C_MASTER_WRRD) {
@@ -376,7 +388,7 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs)
 		mtk_i2c_writew(I2C_WRRD_TRANAC_VALUE, i2c, OFFSET_TRANSAC_LEN);
 	} else {
 		mtk_i2c_writew(msgs->len, i2c, OFFSET_TRANSFER_LEN);
-		mtk_i2c_writew(I2C_RD_TRANAC_VALUE, i2c, OFFSET_TRANSAC_LEN);
+		mtk_i2c_writew(num, i2c, OFFSET_TRANSAC_LEN);
 	}
 
 	/* Prepare buffer data to start transfer */
@@ -422,7 +434,17 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs)
 	/* flush before sending start */
 	mb();
 	mtk_i2c_writel_dma(I2C_DMA_START_EN, i2c, OFFSET_EN);
-	mtk_i2c_writew(I2C_TRANSAC_START, i2c, OFFSET_START);
+
+	if (i2c->platform_compat & (COMPAT_MT6577 | COMPAT_MT6589)) {
+		start_reg = I2C_TRANSAC_START;
+	} else if (i2c->platform_compat & COMPAT_MT8173) {
+		if (left_num >= 1)
+			start_reg = I2C_TRANSAC_START | I2C_RS_MUL_CNFG
+					| I2C_RS_MUL_TRIG;
+		else
+			start_reg = I2C_TRANSAC_START | I2C_RS_MUL_TRIG;
+	}
+	mtk_i2c_writew(start_reg, i2c, OFFSET_START);
 
 	tmo = wait_event_timeout(i2c->wait, i2c->trans_stop, tmo);
 
@@ -455,6 +477,10 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs)
 		return -EREMOTEIO;
 	}
 
+	if (irqstat & I2C_RS_TRANSFER)
+		dev_dbg(i2c->dev, "addr: %x, restart transfer interrupt.\n",
+				msgs->addr);
+
 	return 0;
 }
 
@@ -469,28 +495,33 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
 	if (ret)
 		return ret;
 
-	if (msgs->buf == NULL) {
-		dev_dbg(i2c->dev, "data buffer is NULL.\n");
-		ret = -EINVAL;
-		goto err_exit;
-	}
-
-	if (msgs->flags & I2C_M_RD)
-		i2c->op = I2C_MASTER_RD;
-	else
-		i2c->op = I2C_MASTER_WR;
+	while (left_num--) {
+		if (msgs->buf == NULL) {
+			dev_dbg(i2c->dev, "data buffer is NULL.\n");
+			ret = -EINVAL;
+			goto err_exit;
+		}
 
-	if (num > 1) {
-		/* combined two messages into one transaction */
-		i2c->op = I2C_MASTER_WRRD;
-		left_num--;
-	}
+		if (msgs->flags & I2C_M_RD)
+			i2c->op = I2C_MASTER_RD;
+		else
+			i2c->op = I2C_MASTER_WR;
+
+		if (i2c->platform_compat & (COMPAT_MT6577 | COMPAT_MT6589)) {
+			if (num > 1) {
+				/* combined two messages into one transaction */
+				i2c->op = I2C_MASTER_WRRD;
+				left_num--;
+			}
+		}
 
-	/* always use DMA mode. */
-	ret = mtk_i2c_do_transfer(i2c, msgs);
-	if (ret < 0)
-		goto err_exit;
+		/* always use DMA mode. */
+		ret = mtk_i2c_do_transfer(i2c, msgs, num, left_num);
+		if (ret < 0)
+			goto err_exit;
 
+		msgs++;
+	}
 	/* the return value is number of executed messages */
 	ret = num;
 
@@ -504,14 +535,14 @@ 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);
+	mtk_i2c_writew(~(I2C_RS_TRANSFER | 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;
 	spin_unlock(&i2c->irqlock);
-	mtk_i2c_writew(I2C_HS_NACKERR | I2C_ACKERR
+	mtk_i2c_writew(I2C_RS_TRANSFER | I2C_HS_NACKERR | I2C_ACKERR
 			| I2C_TRANSAC_COMP, i2c, OFFSET_INTR_STAT);
 	wake_up(&i2c->wait);
 
@@ -569,6 +600,14 @@ static const struct i2c_adapter_quirks mt6577_i2c_quirks = {
 	.max_comb_2nd_msg_len = MAX_WRRD_TRANS_SIZE_MT6577,
 };
 
+static const struct i2c_adapter_quirks mt8173_i2c_quirks = {
+	.max_num_msgs = MAX_MSG_NUM_MT8173,
+	.max_write_len = MAX_DMA_TRANS_SIZE_MT8173,
+	.max_read_len = MAX_DMA_TRANS_SIZE_MT8173,
+	.max_comb_1st_msg_len = MAX_DMA_TRANS_SIZE_MT8173,
+	.max_comb_2nd_msg_len = MAX_WRRD_TRANS_SIZE_MT8173,
+};
+
 static int mtk_i2c_probe(struct platform_device *pdev)
 {
 	int ret = 0;
@@ -587,7 +626,8 @@ static int mtk_i2c_probe(struct platform_device *pdev)
 		return -EINVAL;
 
 	i2c->platform_compat = mtk_get_device_prop(pdev);
-	if (i2c->have_pmic && (i2c->platform_compat & COMPAT_MT6577))
+	if (i2c->have_pmic && (i2c->platform_compat &
+			(COMPAT_MT6577 | COMPAT_MT8173)))
 		return -EINVAL;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -613,7 +653,10 @@ static int mtk_i2c_probe(struct platform_device *pdev)
 	i2c->adap.dev.parent = &pdev->dev;
 	i2c->adap.owner = THIS_MODULE;
 	i2c->adap.algo = &mtk_i2c_algorithm;
-	i2c->adap.quirks = &mt6577_i2c_quirks;
+	if (i2c->platform_compat & COMPAT_MT8173)
+		i2c->adap.quirks = &mt8173_i2c_quirks;
+	else
+		i2c->adap.quirks = &mt6577_i2c_quirks;
 	i2c->adap.timeout = 2 * HZ;
 	i2c->adap.retries = 1;
 
-- 
1.8.1.1.dirty


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 3/3] I2C: mediatek: Add driver for MediaTek MT8173 I2C controller
  2015-03-21  6:05 ` [PATCH v5 3/3] I2C: mediatek: Add driver for MediaTek MT8173 " Eddie Huang
@ 2015-03-23  7:39   ` Sascha Hauer
  2015-03-30  8:05     ` Eddie Huang
  0 siblings, 1 reply; 14+ messages in thread
From: Sascha Hauer @ 2015-03-23  7:39 UTC (permalink / raw)
  To: Eddie Huang
  Cc: Wolfram Sang, Matthias Brugger, Uwe Kleine-König,
	Mark Rutland, linux-kernel, linux-i2c, David Box, Lee Jones,
	Jean Delvare, Xudong Chen, Boris BREZILLON, Arnd Bergmann,
	Liguo Zhang, Wei Yan, Bjorn Andersson, Neelesh Gupta, devicetree,
	Pawel Moll, Ian Campbell, Beniamino Galvani, Rob Herring,
	linux-mediatek, linux-arm-kernel, srv_heupstream, Anders Berg,
	Jim Cromie, Simon Glass, Max Schwarz, Doug Anderson,
	Sascha Hauer, Kumar Gala

On Sat, Mar 21, 2015 at 02:05:22PM +0800, Eddie Huang wrote:
> Add mediatek MT8173 I2C controller driver. Compare to I2C controller
> of earlier mediatek SoC, MT8173 fix write-then-read limitation, and
> also increase message size to 64kb.
> 

[...]

> +static const struct i2c_adapter_quirks mt8173_i2c_quirks = {
> +	.max_num_msgs = MAX_MSG_NUM_MT8173,
> +	.max_write_len = MAX_DMA_TRANS_SIZE_MT8173,
> +	.max_read_len = MAX_DMA_TRANS_SIZE_MT8173,
> +	.max_comb_1st_msg_len = MAX_DMA_TRANS_SIZE_MT8173,
> +	.max_comb_2nd_msg_len = MAX_WRRD_TRANS_SIZE_MT8173,
> +};
> +
>  static int mtk_i2c_probe(struct platform_device *pdev)
>  {
>  	int ret = 0;
> @@ -587,7 +626,8 @@ static int mtk_i2c_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  
>  	i2c->platform_compat = mtk_get_device_prop(pdev);
> -	if (i2c->have_pmic && (i2c->platform_compat & COMPAT_MT6577))
> +	if (i2c->have_pmic && (i2c->platform_compat &
> +			(COMPAT_MT6577 | COMPAT_MT8173)))
>  		return -EINVAL;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -613,7 +653,10 @@ static int mtk_i2c_probe(struct platform_device *pdev)
>  	i2c->adap.dev.parent = &pdev->dev;
>  	i2c->adap.owner = THIS_MODULE;
>  	i2c->adap.algo = &mtk_i2c_algorithm;
> -	i2c->adap.quirks = &mt6577_i2c_quirks;
> +	if (i2c->platform_compat & COMPAT_MT8173)
> +		i2c->adap.quirks = &mt8173_i2c_quirks;
> +	else
> +		i2c->adap.quirks = &mt6577_i2c_quirks;

Instead of putting an integer into struct of_device_id you should
introduce a

struct mtk_i2c_data {
	struct i2c_adapter_quirks quirks;
	int compat;
	/* Additional SoC specific data */
};

and put a pointer to this directly into the of_device_id. This way you
need less casting and can put newly discovered differences diretly into
some data struct and don't have to introduce if(socxy) ... else ...
everytime.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C controller
  2015-03-21  6:05 ` [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C controller Eddie Huang
@ 2015-03-23  8:42   ` Sascha Hauer
  2015-03-30  8:14     ` Eddie Huang
  0 siblings, 1 reply; 14+ messages in thread
From: Sascha Hauer @ 2015-03-23  8:42 UTC (permalink / raw)
  To: Eddie Huang
  Cc: Wolfram Sang, Matthias Brugger, Uwe Kleine-König,
	Mark Rutland, linux-kernel, linux-i2c, David Box, Lee Jones,
	Jean Delvare, Xudong Chen, Boris BREZILLON, Arnd Bergmann,
	Liguo Zhang, Wei Yan, Bjorn Andersson, Neelesh Gupta, devicetree,
	Pawel Moll, Ian Campbell, Beniamino Galvani, Rob Herring,
	linux-mediatek, linux-arm-kernel, srv_heupstream, Anders Berg,
	Jim Cromie, Simon Glass, Max Schwarz, Doug Anderson,
	Sascha Hauer, Kumar Gala

On Sat, Mar 21, 2015 at 02:05:21PM +0800, Eddie Huang wrote:
> From: Xudong Chen <xudong.chen@mediatek.com>
> 
> 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@mediatek.com>
> Signed-off-by: Liguo Zhang <liguo.zhang@mediatek.com>
> Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> ---
>  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.

> +{
> +	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.

> +		/* 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?

> +
> +	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.

> +	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.

> +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.

> +	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.

> +	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.

> +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().

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 1/3] dt-bindings: Add I2C bindings for mt65xx/mt81xx.
  2015-03-21  6:05 ` [PATCH v5 1/3] dt-bindings: Add I2C bindings for mt65xx/mt81xx Eddie Huang
@ 2015-03-23  8:45   ` Sascha Hauer
  0 siblings, 0 replies; 14+ messages in thread
From: Sascha Hauer @ 2015-03-23  8:45 UTC (permalink / raw)
  To: Eddie Huang
  Cc: Wolfram Sang, Matthias Brugger, Uwe Kleine-König,
	Mark Rutland, linux-kernel, linux-i2c, David Box, Lee Jones,
	Jean Delvare, Xudong Chen, Boris BREZILLON, Arnd Bergmann,
	Liguo Zhang, Wei Yan, Bjorn Andersson, Neelesh Gupta, devicetree,
	Pawel Moll, Ian Campbell, Beniamino Galvani, Rob Herring,
	linux-mediatek, linux-arm-kernel, srv_heupstream, Anders Berg,
	Jim Cromie, Simon Glass, Max Schwarz, Doug Anderson,
	Sascha Hauer, Kumar Gala

On Sat, Mar 21, 2015 at 02:05:20PM +0800, Eddie Huang wrote:
> 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         | 41 ++++++++++++++++++++++
>  1 file changed, 41 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..eff52c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mt6577.txt
> @@ -0,0 +1,41 @@
> +* 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.
> +      (e) "mediatek,mt8173-i2c", for i2c compatible with mt8173 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>;

Since 100000 is the default you should probably pick another speed for
this example or drop the property.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 3/3] I2C: mediatek: Add driver for MediaTek MT8173 I2C controller
  2015-03-23  7:39   ` Sascha Hauer
@ 2015-03-30  8:05     ` Eddie Huang
  0 siblings, 0 replies; 14+ messages in thread
From: Eddie Huang @ 2015-03-30  8:05 UTC (permalink / raw)
  To: Sascha Hauer
  Cc: Mark Rutland, Wolfram Sang, Doug Anderson, linux-i2c, David Box,
	Lee Jones, Jean Delvare, Xudong Chen, Boris BREZILLON,
	Pawel Moll, Liguo Zhang, Wei Yan, Bjorn Andersson,
	Uwe Kleine-König, Neelesh Gupta, devicetree, Arnd Bergmann,
	Ian Campbell, Beniamino Galvani, Rob Herring, linux-mediatek,
	Matthias Brugger, linux-arm-kernel, srv_heupstream, Anders Berg,
	Jim Cromie, Simon Glass, Max Schwarz, linux-kernel, Sascha Hauer,
	Kumar Gala

Hi Sascha,

On Mon, 2015-03-23 at 08:39 +0100, Sascha Hauer wrote:
> On Sat, Mar 21, 2015 at 02:05:22PM +0800, Eddie Huang wrote:
> > Add mediatek MT8173 I2C controller driver. Compare to I2C controller
> > of earlier mediatek SoC, MT8173 fix write-then-read limitation, and
> > also increase message size to 64kb.
> > 
> 
> [...]
> 
> > +static const struct i2c_adapter_quirks mt8173_i2c_quirks = {
> > +	.max_num_msgs = MAX_MSG_NUM_MT8173,
> > +	.max_write_len = MAX_DMA_TRANS_SIZE_MT8173,
> > +	.max_read_len = MAX_DMA_TRANS_SIZE_MT8173,
> > +	.max_comb_1st_msg_len = MAX_DMA_TRANS_SIZE_MT8173,
> > +	.max_comb_2nd_msg_len = MAX_WRRD_TRANS_SIZE_MT8173,
> > +};
> > +
> >  static int mtk_i2c_probe(struct platform_device *pdev)
> >  {
> >  	int ret = 0;
> > @@ -587,7 +626,8 @@ static int mtk_i2c_probe(struct platform_device *pdev)
> >  		return -EINVAL;
> >  
> >  	i2c->platform_compat = mtk_get_device_prop(pdev);
> > -	if (i2c->have_pmic && (i2c->platform_compat & COMPAT_MT6577))
> > +	if (i2c->have_pmic && (i2c->platform_compat &
> > +			(COMPAT_MT6577 | COMPAT_MT8173)))
> >  		return -EINVAL;
> >  
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > @@ -613,7 +653,10 @@ static int mtk_i2c_probe(struct platform_device *pdev)
> >  	i2c->adap.dev.parent = &pdev->dev;
> >  	i2c->adap.owner = THIS_MODULE;
> >  	i2c->adap.algo = &mtk_i2c_algorithm;
> > -	i2c->adap.quirks = &mt6577_i2c_quirks;
> > +	if (i2c->platform_compat & COMPAT_MT8173)
> > +		i2c->adap.quirks = &mt8173_i2c_quirks;
> > +	else
> > +		i2c->adap.quirks = &mt6577_i2c_quirks;
> 
> Instead of putting an integer into struct of_device_id you should
> introduce a
> 
> struct mtk_i2c_data {
> 	struct i2c_adapter_quirks quirks;
> 	int compat;
> 	/* Additional SoC specific data */
> };
> 
> and put a pointer to this directly into the of_device_id. This way you
> need less casting and can put newly discovered differences diretly into
> some data struct and don't have to introduce if(socxy) ... else ...
> everytime.
> 

Thanks your suggestion. I will use capability struct like 

struct mtk_i2c_compatible {
	unsigned char pmic_i2c;
	unsigned char dcm;
	unsigned char auto_restart;
	const struct i2c_adapter_quirks *quirks;
};

And pass in of_device_id, then remove all if(socxy).... else ... in
driver.

Eddie






^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C controller
  2015-03-23  8:42   ` Sascha Hauer
@ 2015-03-30  8:14     ` Eddie Huang
  2015-03-30 17:23       ` Sascha Hauer
  0 siblings, 1 reply; 14+ messages in thread
From: Eddie Huang @ 2015-03-30  8:14 UTC (permalink / raw)
  To: Sascha Hauer
  Cc: Mark Rutland, Wolfram Sang, Doug Anderson, linux-i2c, David Box,
	Lee Jones, Jean Delvare, Xudong Chen, Boris BREZILLON,
	Pawel Moll, Liguo Zhang, Wei Yan, Bjorn Andersson,
	Uwe Kleine-König, Neelesh Gupta, devicetree, Arnd Bergmann,
	Ian Campbell, Beniamino Galvani, Rob Herring, linux-mediatek,
	Matthias Brugger, linux-arm-kernel, srv_heupstream, Anders Berg,
	Jim Cromie, Simon Glass, Max Schwarz, linux-kernel, Sascha Hauer,
	Kumar Gala

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 <xudong.chen@mediatek.com>
> > 
> > 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@mediatek.com>
> > Signed-off-by: Liguo Zhang <liguo.zhang@mediatek.com>
> > Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> > ---
> >  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




^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C controller
  2015-03-30  8:14     ` Eddie Huang
@ 2015-03-30 17:23       ` Sascha Hauer
  2015-03-31  7:08         ` Eddie Huang
  0 siblings, 1 reply; 14+ messages in thread
From: Sascha Hauer @ 2015-03-30 17:23 UTC (permalink / raw)
  To: Eddie Huang
  Cc: Mark Rutland, Wolfram Sang, Doug Anderson, linux-i2c, David Box,
	Lee Jones, Jean Delvare, Xudong Chen, Boris BREZILLON,
	Pawel Moll, Liguo Zhang, Wei Yan, Bjorn Andersson,
	Uwe Kleine-König, Neelesh Gupta, devicetree, Arnd Bergmann,
	Ian Campbell, Beniamino Galvani, Rob Herring, linux-mediatek,
	Matthias Brugger, linux-arm-kernel, srv_heupstream, Anders Berg,
	Jim Cromie, Simon Glass, Max Schwarz, linux-kernel, Sascha Hauer,
	Kumar Gala

On Mon, Mar 30, 2015 at 04:14:12PM +0800, Eddie Huang wrote:
> Hi Sascha,
> 
> > 
> > [...]
> > 
> > > +	if (i2c->speed_hz > 400000)
> > > +		control_reg |= I2C_CONTROL_RS;
> > > +	if (i2c->op == I2C_MASTER_WRRD)
> > > +		control_reg |= I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS;
> > > +	mtk_i2c_writew(control_reg, i2c, OFFSET_CONTROL);
> > > +
> > > +	/* set start condition */
> > > +	if (i2c->speed_hz <= 100000)
> > > +		mtk_i2c_writew(I2C_ST_START_CON, i2c, OFFSET_EXT_CONF);
> > > +	else
> > > +		mtk_i2c_writew(I2C_FS_START_CON, i2c, OFFSET_EXT_CONF);
> > > +
> > > +	if (~control_reg & I2C_CONTROL_RS)
> > > +		mtk_i2c_writew(I2C_DELAY_LEN, i2c, OFFSET_DELAY_LEN);
> > 
> > speed <= 400000 here to make this more obvious?
> There are two cases, not only speed<=400000, but I2C_MASTER_WRRD. I tend
> to keep it.

Still it looks strange. You only ever write this default value to the
register. Putting this register write under an if() seems bogus since
the same value will be in the register the next time this code is
executed. It looks like you should move this register write to some
initialization function.

> > > +
> > > +	/* Enable interrupt */
> > > +	mtk_i2c_writew(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP,
> > > +		i2c, OFFSET_INTR_MASK);
> > 
> > Why do you enable/disable interrupts for each transfer? Enabling them
> > once and just acknowledge them in the interrupt handler should be
> > enough.
> This can avoid unwanted I2C interrupt. For example, I2C transfer error,
> and cause timeout, I2C driver report error to caller. Then I2C error
> interrupt happen.

So isn't the same unwanted interrupt then just delayed until you enable
the interrupts again? Is this something that really happens or just
something you think that might happen?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C controller
  2015-03-30 17:23       ` Sascha Hauer
@ 2015-03-31  7:08         ` Eddie Huang
  2015-03-31 11:50           ` Eddie Huang
  0 siblings, 1 reply; 14+ messages in thread
From: Eddie Huang @ 2015-03-31  7:08 UTC (permalink / raw)
  To: Sascha Hauer
  Cc: Mark Rutland, Wolfram Sang, linux-kernel, linux-i2c, David Box,
	Lee Jones, Jean Delvare, Xudong Chen, Boris BREZILLON,
	Arnd Bergmann, Liguo Zhang, Wei Yan, Bjorn Andersson,
	Uwe Kleine-König, Neelesh Gupta, devicetree, Pawel Moll,
	Ian Campbell, Beniamino Galvani, Rob Herring, linux-mediatek,
	Matthias Brugger, linux-arm-kernel, srv_heupstream, Anders Berg,
	Jim Cromie, Simon Glass, Max Schwarz, Doug Anderson,
	Sascha Hauer, Kumar Gala

Hi Sascha,

On Mon, 2015-03-30 at 19:23 +0200, Sascha Hauer wrote:
> On Mon, Mar 30, 2015 at 04:14:12PM +0800, Eddie Huang wrote:
> > Hi Sascha,
> > 
> > > 
> > > [...]
> > > 
> > > > +	if (i2c->speed_hz > 400000)
> > > > +		control_reg |= I2C_CONTROL_RS;
> > > > +	if (i2c->op == I2C_MASTER_WRRD)
> > > > +		control_reg |= I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS;
> > > > +	mtk_i2c_writew(control_reg, i2c, OFFSET_CONTROL);
> > > > +
> > > > +	/* set start condition */
> > > > +	if (i2c->speed_hz <= 100000)
> > > > +		mtk_i2c_writew(I2C_ST_START_CON, i2c, OFFSET_EXT_CONF);
> > > > +	else
> > > > +		mtk_i2c_writew(I2C_FS_START_CON, i2c, OFFSET_EXT_CONF);
> > > > +
> > > > +	if (~control_reg & I2C_CONTROL_RS)
> > > > +		mtk_i2c_writew(I2C_DELAY_LEN, i2c, OFFSET_DELAY_LEN);
> > > 
> > > speed <= 400000 here to make this more obvious?
> > There are two cases, not only speed<=400000, but I2C_MASTER_WRRD. I tend
> > to keep it.
> 
> Still it looks strange. You only ever write this default value to the
> register. Putting this register write under an if() seems bogus since
> the same value will be in the register the next time this code is
> executed. It looks like you should move this register write to some
> initialization function.
OK, move to mtk_i2c_init_hw function

> 
> > > > +
> > > > +	/* Enable interrupt */
> > > > +	mtk_i2c_writew(I2C_HS_NACKERR | I2C_ACKERR | I2C_TRANSAC_COMP,
> > > > +		i2c, OFFSET_INTR_MASK);
> > > 
> > > Why do you enable/disable interrupts for each transfer? Enabling them
> > > once and just acknowledge them in the interrupt handler should be
> > > enough.
> > This can avoid unwanted I2C interrupt. For example, I2C transfer error,
> > and cause timeout, I2C driver report error to caller. Then I2C error
> > interrupt happen.
> 
> So isn't the same unwanted interrupt then just delayed until you enable
> the interrupts again? Is this something that really happens or just
> something you think that might happen?
> 
Clear interrupt status before enable interrupt, so won't get unwanted
interrupt again. I just think this might happen, and it's not harmful to
enable/disable interrupt in transfer function and can get extra benefit
to avoid unnecessary interrupt.  Tegra I2C driver i2c-tegra.c also do
the same thing.

Eddie




^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C controller
  2015-03-31  7:08         ` Eddie Huang
@ 2015-03-31 11:50           ` Eddie Huang
  2015-03-31 17:52             ` Sascha Hauer
  0 siblings, 1 reply; 14+ messages in thread
From: Eddie Huang @ 2015-03-31 11:50 UTC (permalink / raw)
  To: Sascha Hauer
  Cc: Mark Rutland, Wolfram Sang, Doug Anderson, linux-i2c, David Box,
	Lee Jones, Jean Delvare, Xudong Chen (陈旭东),
	Boris BREZILLON, Pawel Moll,
	Liguo Zhang (张立国),
	Wei Yan, Bjorn Andersson, Uwe Kleine-Konig, Neelesh Gupta,
	devicetree, Arnd Bergmann, Ian Campbell, Beniamino Galvani,
	Rob Herring, linux-mediatek, Matthias Brugger, linux-arm-kernel,
	srv_heupstream, Anders Berg, Jim Cromie, Simon Glass,
	Max Schwarz, linux-kernel, Sascha Hauer, Kumar Gala

Hi Sascha,

On Tue, 2015-03-31 at 15:08 +0800, Eddie Huang wrote:
> Hi Sascha,
> 
> On Mon, 2015-03-30 at 19:23 +0200, Sascha Hauer wrote:
> > On Mon, Mar 30, 2015 at 04:14:12PM +0800, Eddie Huang wrote:
> > > Hi Sascha,
> > >
> > > >
> > > > [...]
> > > >
> > > > > +       if (i2c->speed_hz > 400000)
> > > > > +               control_reg |= I2C_CONTROL_RS;
> > > > > +       if (i2c->op == I2C_MASTER_WRRD)
> > > > > +               control_reg |= I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS;
> > > > > +       mtk_i2c_writew(control_reg, i2c, OFFSET_CONTROL);
> > > > > +
> > > > > +       /* set start condition */
> > > > > +       if (i2c->speed_hz <= 100000)
> > > > > +               mtk_i2c_writew(I2C_ST_START_CON, i2c, OFFSET_EXT_CONF);
> > > > > +       else
> > > > > +               mtk_i2c_writew(I2C_FS_START_CON, i2c, OFFSET_EXT_CONF);
> > > > > +
> > > > > +       if (~control_reg & I2C_CONTROL_RS)
> > > > > +               mtk_i2c_writew(I2C_DELAY_LEN, i2c, OFFSET_DELAY_LEN);
> > > >
> > > > speed <= 400000 here to make this more obvious?
> > > There are two cases, not only speed<=400000, but I2C_MASTER_WRRD. I tend
> > > to keep it.
> >
> > Still it looks strange. You only ever write this default value to the
> > register. Putting this register write under an if() seems bogus since
> > the same value will be in the register the next time this code is
> > executed. It looks like you should move this register write to some
> > initialization function.
> OK, move to mtk_i2c_init_hw function
> 
Sorry for my negligence. Write-then-read (I2C_MASTER_WRRD) decides
according to each I2C transfer, so I still need to judge whether set
I2C_CONTROL_RS and DELAY_LEN here. Other control setting can move to
init function.

Eddie




^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C controller
  2015-03-31 11:50           ` Eddie Huang
@ 2015-03-31 17:52             ` Sascha Hauer
  2015-04-01  2:11               ` Eddie Huang
  0 siblings, 1 reply; 14+ messages in thread
From: Sascha Hauer @ 2015-03-31 17:52 UTC (permalink / raw)
  To: Eddie Huang
  Cc: Mark Rutland, Wolfram Sang, Doug Anderson, linux-i2c, David Box,
	Lee Jones, Jean Delvare, Xudong Chen (陈旭东),
	Boris BREZILLON, Pawel Moll,
	Liguo Zhang (张立国),
	Wei Yan, Bjorn Andersson, Uwe Kleine-Konig, Neelesh Gupta,
	devicetree, Arnd Bergmann, Ian Campbell, Beniamino Galvani,
	Rob Herring, linux-mediatek, Matthias Brugger, linux-arm-kernel,
	srv_heupstream, Anders Berg, Jim Cromie, Simon Glass,
	Max Schwarz, linux-kernel, Sascha Hauer, Kumar Gala

On Tue, Mar 31, 2015 at 07:50:58PM +0800, Eddie Huang wrote:
> Hi Sascha,
> 
> On Tue, 2015-03-31 at 15:08 +0800, Eddie Huang wrote:
> > Hi Sascha,
> > 
> > > > > speed <= 400000 here to make this more obvious?
> > > > There are two cases, not only speed<=400000, but I2C_MASTER_WRRD. I tend
> > > > to keep it.
> > >
> > > Still it looks strange. You only ever write this default value to the
> > > register. Putting this register write under an if() seems bogus since
> > > the same value will be in the register the next time this code is
> > > executed. It looks like you should move this register write to some
> > > initialization function.
> > OK, move to mtk_i2c_init_hw function
> > 
> Sorry for my negligence. Write-then-read (I2C_MASTER_WRRD) decides
> according to each I2C transfer, so I still need to judge whether set
> I2C_CONTROL_RS and DELAY_LEN here. Other control setting can move to
> init function.

You need to decide whether you have to set the I2C_CONTROL_RS bit, but
you don't need to write the same value to the DELAY_LEN register each
time. It won't change under your hood.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C controller
  2015-03-31 17:52             ` Sascha Hauer
@ 2015-04-01  2:11               ` Eddie Huang
  0 siblings, 0 replies; 14+ messages in thread
From: Eddie Huang @ 2015-04-01  2:11 UTC (permalink / raw)
  To: Sascha Hauer
  Cc: Mark Rutland, Wolfram Sang, Doug Anderson, linux-i2c, David Box,
	Lee Jones, Jean Delvare, Xudong Chen (陈旭东),
	Boris BREZILLON, Pawel Moll,
	Liguo Zhang (张立国),
	Wei Yan, Bjorn Andersson, Uwe Kleine-Konig, Neelesh Gupta,
	devicetree, Arnd Bergmann, Ian Campbell, Beniamino Galvani,
	Rob Herring, linux-mediatek, Matthias Brugger, linux-arm-kernel,
	srv_heupstream, Anders Berg, Jim Cromie, Simon Glass,
	Max Schwarz, linux-kernel, Sascha Hauer, Kumar Gala

Hi Sascha,

On Tue, 2015-03-31 at 19:52 +0200, Sascha Hauer wrote:
> On Tue, Mar 31, 2015 at 07:50:58PM +0800, Eddie Huang wrote:
> > Hi Sascha,
> > 
> > On Tue, 2015-03-31 at 15:08 +0800, Eddie Huang wrote:
> > > Hi Sascha,
> > > 
> > > > > > speed <= 400000 here to make this more obvious?
> > > > > There are two cases, not only speed<=400000, but I2C_MASTER_WRRD. I tend
> > > > > to keep it.
> > > >
> > > > Still it looks strange. You only ever write this default value to the
> > > > register. Putting this register write under an if() seems bogus since
> > > > the same value will be in the register the next time this code is
> > > > executed. It looks like you should move this register write to some
> > > > initialization function.
> > > OK, move to mtk_i2c_init_hw function
> > > 
> > Sorry for my negligence. Write-then-read (I2C_MASTER_WRRD) decides
> > according to each I2C transfer, so I still need to judge whether set
> > I2C_CONTROL_RS and DELAY_LEN here. Other control setting can move to
> > init function.
> 
> You need to decide whether you have to set the I2C_CONTROL_RS bit, but
> you don't need to write the same value to the DELAY_LEN register each
> time. It won't change under your hood.
> 
Yes, you are right. DELAY_LEN should only be set one time in init
function. Thanks your correction.

Eddie



^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-04-01  2:11 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-21  6:05 [PATCH v5 0/3] ARM: mediatek: Add driver for Mediatek I2C controller Eddie Huang
2015-03-21  6:05 ` [PATCH v5 1/3] dt-bindings: Add I2C bindings for mt65xx/mt81xx Eddie Huang
2015-03-23  8:45   ` Sascha Hauer
2015-03-21  6:05 ` [PATCH v5 2/3] I2C: mediatek: Add driver for MediaTek I2C controller Eddie Huang
2015-03-23  8:42   ` Sascha Hauer
2015-03-30  8:14     ` Eddie Huang
2015-03-30 17:23       ` Sascha Hauer
2015-03-31  7:08         ` Eddie Huang
2015-03-31 11:50           ` Eddie Huang
2015-03-31 17:52             ` Sascha Hauer
2015-04-01  2:11               ` Eddie Huang
2015-03-21  6:05 ` [PATCH v5 3/3] I2C: mediatek: Add driver for MediaTek MT8173 " Eddie Huang
2015-03-23  7:39   ` Sascha Hauer
2015-03-30  8:05     ` Eddie Huang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).