[PATCH v2 0/5] Add i2c support for MediaTek mt8512

[PATCH v2 0/5] Add i2c support for MediaTek mt8512

[mingming lee]

From: Mingming Lee <Mingming.Lee@mediatek.com>

This patch series adds basic i2c support for MediaTek MT8512 EMMC boards.

---
Changes for v2:
   - using error number defined in include/linux/errno.h.
   - add Mediatek i2c controller driver to ARM MEDIATEK in MAINTAINERS.
   - add bindings for mediatek i2c driver.

Mingming Lee (5):
  i2c: mediatek: add basic driver support
  ARM: dts: Mediatek: add i2c node support for mt8512
  configs: mt8512: Enable I2C related configs
  dt-binding: i2c: add bindings for mediatek i2c driver
  MAINTAINERS: add i2c driver to ARM MEDIATEK

 MAINTAINERS                              |   2 +
 arch/arm/dts/mt8512-bm1-emmc.dts         |  12 +
 arch/arm/dts/mt8512.dtsi                 |  38 +-
 configs/mt8512_bm1_emmc_defconfig        |   3 +
 doc/device-tree-bindings/i2c/i2c-mtk.txt |  39 ++
 drivers/i2c/Kconfig                      |   9 +
 drivers/i2c/Makefile                     |   1 +
 drivers/i2c/mt_i2c.c                     | 617 +++++++++++++++++++++++
 8 files changed, 720 insertions(+), 1 deletion(-)
 create mode 100644 doc/device-tree-bindings/i2c/i2c-mtk.txt
 create mode 100644 drivers/i2c/mt_i2c.c

-- 
2.18.0

[PATCH v2 1/5] i2c: mediatek: add basic driver support

[mingming lee]

From: Mingming Lee <Mingming.Lee@mediatek.com>

Add MediaTek I2C basic driver

Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>

---
Changes for v2:
   - using error number defined in include/linux/errno.h
---
 drivers/i2c/Kconfig  |   9 +
 drivers/i2c/Makefile |   1 +
 drivers/i2c/mt_i2c.c | 617 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 627 insertions(+)
 create mode 100644 drivers/i2c/mt_i2c.c

diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
index dec6dc9dfa..103688ed36 100644
--- a/drivers/i2c/Kconfig
+++ b/drivers/i2c/Kconfig
@@ -159,6 +159,15 @@ config SYS_I2C_MESON
 	  internal buffer holding up to 8 bytes for transfers and supports
 	  both 7-bit and 10-bit addresses.
 
+config SYS_I2C_MTK
+	bool "MediaTek I2C driver"
+	default n
+	help
+	  This selects the MediaTek Integrated Inter Circuit bus driver.
+	  The I2C bus adapter is the base for some other I2C client, eg: touch, sensors.
+	  If you want to use MediaTek I2C interface, say Y here.
+	  If unsure, say N.
+
 config SYS_I2C_MXC
 	bool "NXP MXC I2C driver"
 	help
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index e851ec462e..7227742f8d 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_SYS_I2C_LPC32XX) += lpc32xx_i2c.o
 obj-$(CONFIG_SYS_I2C_MESON) += meson_i2c.o
 obj-$(CONFIG_SYS_I2C_MVTWSI) += mvtwsi.o
 obj-$(CONFIG_SYS_I2C_MXC) += mxc_i2c.o
+obj-$(CONFIG_SYS_I2C_MTK) += mt_i2c.o
 obj-$(CONFIG_SYS_I2C_NEXELL) += nx_i2c.o
 obj-$(CONFIG_SYS_I2C_OCTEON) += octeon_i2c.o
 obj-$(CONFIG_SYS_I2C_OMAP24XX) += omap24xx_i2c.o
diff --git a/drivers/i2c/mt_i2c.c b/drivers/i2c/mt_i2c.c
new file mode 100644
index 0000000000..be6262d3d2
--- /dev/null
+++ b/drivers/i2c/mt_i2c.c
@@ -0,0 +1,617 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MediaTek I2C Interface driver
+ *
+ * Copyright (C) 2020 MediaTek Inc.
+ * Author: Mingming Lee <Mingming.Lee@mediatek.com>
+ */
+
+#include <asm/cache.h>
+#include <asm/io.h>
+#include <common.h>
+#include <clk.h>
+#include <dm.h>
+#include <i2c.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+
+#define I2C_OK			0
+
+#define I2C_RS_TRANSFER		BIT(4)
+#define I2C_HS_NACKERR		BIT(2)
+#define I2C_ACKERR		BIT(1)
+#define I2C_TRANSAC_COMP	BIT(0)
+#define I2C_TRANSAC_START	BIT(0)
+#define I2C_RS_MUL_CNFG		BIT(15)
+#define I2C_RS_MUL_TRIG		BIT(14)
+#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_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_DMA_HARD_RST		0x0002
+
+#define MAX_ST_MODE_SPEED		100000
+#define MAX_FS_MODE_SPEED		400000
+#define MAX_HS_MODE_SPEED		3400000
+#define MAX_SAMPLE_CNT_DIV		8
+#define MAX_STEP_CNT_DIV		64
+#define MAX_HS_STEP_CNT_DIV		8
+#define I2C_DEFAULT_CLK_DIV		4
+
+#define MAX_I2C_ADDR		0x7F
+#define MAX_I2C_LEN		0xFF
+#define TRANS_ADDR_ONLY		BIT(8)
+#define TRANSFER_TIMEOUT		50000  /* us */
+#define I2C_FIFO_STAT1_MASK		0x001F
+#define TIMING_SAMPLE_OFFSET	8
+#define HS_SAMPLE_OFFSET	12
+#define HS_STEP_OFFSET		8
+
+#define I2C_CONTROL_WRAPPER		BIT(0)
+#define I2C_CONTROL_RS			BIT(1)
+#define I2C_CONTROL_DMA_EN		BIT(2)
+#define I2C_CONTROL_CLK_EXT_EN		BIT(3)
+#define I2C_CONTROL_DIR_CHANGE		BIT(4)
+#define I2C_CONTROL_ACKERR_DET_EN	BIT(5)
+#define I2C_CONTROL_TRANSFER_LEN_CHANGE BIT(6)
+#define I2C_CONTROL_DMAACK		BIT(8)
+#define I2C_CONTROL_ASYNC		BIT(9)
+
+enum I2C_REGS_OFFSET {
+	OFFSET_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_STAT1 = 0x2C,
+	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_DEBUGSTAT = 0x64,
+	OFFSET_DEBUGCTRL = 0x68,
+	OFFSET_TRANSFER_LEN_AUX = 0x6C,
+	OFFSET_CLOCK_DIV = 0x70,
+	OFFSET_SCL_HL_RATIO = 0x74,
+	OFFSET_SCL_HS_HL_RATIO = 0x78,
+	OFFSET_SCL_MIS_COMP_POINT = 0x7C,
+	OFFSET_STA_STOP_AC_TIME = 0x80,
+	OFFSET_HS_STA_STOP_AC_TIME = 0x84,
+	OFFSET_DATA_TIME = 0x88,
+};
+
+enum DMA_REGS_OFFSET {
+	OFFSET_INT_FLAG = 0x0,
+	OFFSET_INT_EN = 0x04,
+	OFFSET_EN = 0x08,
+	OFFSET_RST = 0x0C,
+	OFFSET_CON = 0x18,
+	OFFSET_TX_MEM_ADDR = 0x1C,
+	OFFSET_RX_MEM_ADDR = 0x20,
+	OFFSET_TX_LEN = 0x24,
+	OFFSET_RX_LEN = 0x28,
+};
+
+enum mtk_trans_op {
+	I2C_MASTER_WR = 1,
+	I2C_MASTER_RD,
+	I2C_MASTER_WRRD,
+};
+
+struct mtk_i2c_soc_data {
+	u8 dma_sync: 1;
+};
+
+struct mtk_i2c_priv {
+	/* set in i2c probe */
+	void __iomem *base;			/* i2c base addr */
+	void __iomem *pdmabase;		/* dma base address*/
+	struct clk clk_main;		/* main clock for i2c bus */
+	struct clk clk_dma;		    /* DMA clock for i2c via DMA */
+	const struct mtk_i2c_soc_data *soc_data;
+	enum mtk_trans_op op;
+	bool   zero_len;
+	bool   pushpull;            /* open drain */
+	bool   filter_msg;          /* filter msg error log */
+	bool   auto_restart;
+	bool ignore_restart_irq;
+	u32 speed;                 /* hz */
+	u8 *tx_buff;
+	u8 *rx_buff;
+};
+
+static inline void i2c_writel(struct mtk_i2c_priv *priv, u8 offset, u32 value)
+{
+	writel(value, (priv->base + offset));
+}
+
+static inline u32 i2c_readl(struct mtk_i2c_priv *priv, u8 offset)
+{
+	return readl(priv->base + offset);
+}
+
+static void mtk_i2c_clk_enable(struct mtk_i2c_priv *priv)
+{
+	clk_enable(&priv->clk_main);
+	clk_enable(&priv->clk_dma);
+}
+
+static void mtk_i2c_clk_disable(struct mtk_i2c_priv *priv)
+{
+	clk_disable(&priv->clk_dma);
+	clk_disable(&priv->clk_main);
+}
+
+static void mtk_i2c_init_hw(struct mtk_i2c_priv *priv)
+{
+	u16 control_reg;
+
+	writel(I2C_DMA_HARD_RST, priv->pdmabase + OFFSET_RST);
+	writel(I2C_DMA_CLR_FLAG, priv->pdmabase + OFFSET_RST);
+	i2c_writel(priv, OFFSET_SOFTRESET, I2C_SOFT_RST);
+	/* set ioconfig */
+	if (priv->pushpull)
+		i2c_writel(priv, OFFSET_IO_CONFIG, I2C_IO_CONFIG_PUSH_PULL);
+	else
+		i2c_writel(priv, OFFSET_IO_CONFIG, I2C_IO_CONFIG_OPEN_DRAIN);
+
+	i2c_writel(priv, OFFSET_DCM_EN, I2C_DCM_DISABLE);
+	control_reg = I2C_CONTROL_ACKERR_DET_EN | I2C_CONTROL_CLK_EXT_EN;
+	if (priv->soc_data->dma_sync)
+		control_reg |= I2C_CONTROL_DMAACK | I2C_CONTROL_ASYNC;
+	i2c_writel(priv, OFFSET_CONTROL, control_reg);
+	i2c_writel(priv, OFFSET_DELAY_LEN, I2C_DELAY_LEN);
+}
+
+static int mtk_i2c_calculate_speed(struct mtk_i2c_priv *priv,
+				   u32 clk_src,
+				   u32 target_speed,
+				   u32 *timing_step_cnt,
+				   u32 *timing_sample_cnt)
+{
+	u32 step_cnt;
+	u32 sample_cnt;
+	u32 max_step_cnt;
+	u32 base_sample_cnt = MAX_SAMPLE_CNT_DIV;
+	u32 base_step_cnt;
+	u32 opt_div;
+	u32 best_mul;
+	u32 cnt_mul;
+
+	if (target_speed > MAX_HS_MODE_SPEED)
+		target_speed = MAX_HS_MODE_SPEED;
+
+	if (target_speed > MAX_FS_MODE_SPEED)
+		max_step_cnt = MAX_HS_STEP_CNT_DIV;
+	else
+		max_step_cnt = MAX_STEP_CNT_DIV;
+
+	base_step_cnt = max_step_cnt;
+	/* Find the best combination */
+	opt_div = DIV_ROUND_UP(clk_src >> 1, target_speed);
+	best_mul = MAX_SAMPLE_CNT_DIV * max_step_cnt;
+
+	/* Search for the best pair (sample_cnt, step_cnt) with
+	 * 0 < sample_cnt < MAX_SAMPLE_CNT_DIV
+	 * 0 < step_cnt < max_step_cnt
+	 * sample_cnt * step_cnt >= opt_div
+	 * optimizing for sample_cnt * step_cnt being minimal
+	 */
+	for (sample_cnt = 1; sample_cnt <= MAX_SAMPLE_CNT_DIV; sample_cnt++) {
+		step_cnt = DIV_ROUND_UP(opt_div, sample_cnt);
+		cnt_mul = step_cnt * sample_cnt;
+		if (step_cnt > max_step_cnt)
+			continue;
+
+		if (cnt_mul < best_mul) {
+			best_mul = cnt_mul;
+			base_sample_cnt = sample_cnt;
+			base_step_cnt = step_cnt;
+			if (best_mul == opt_div)
+				break;
+		}
+	}
+
+	sample_cnt = base_sample_cnt;
+	step_cnt = base_step_cnt;
+
+	if ((clk_src / (2 * sample_cnt * step_cnt)) > target_speed) {
+		/* In this case, hardware can't support such
+		 * low i2c_bus_freq
+		 */
+		debug("Unsupported speed(%uhz)\n", target_speed);
+		return -EINVAL;
+	}
+
+	*timing_step_cnt = step_cnt - 1;
+	*timing_sample_cnt = sample_cnt - 1;
+	return 0;
+}
+
+static int mtk_i2c_set_speed(struct udevice *dev, uint speed)
+{
+	u32 clk_src;
+	u32 step_cnt;
+	u32 sample_cnt;
+	u16 timing_reg;
+	u16 high_speed_reg;
+	int ret = 0;
+	struct mtk_i2c_priv *priv = dev_get_priv(dev);
+
+	priv->speed = speed;
+	mtk_i2c_clk_enable(priv);
+	clk_src = clk_get_rate(&priv->clk_main) /
+			I2C_DEFAULT_CLK_DIV;
+	i2c_writel(priv, OFFSET_CLOCK_DIV, (I2C_DEFAULT_CLK_DIV - 1));
+	if (priv->speed > MAX_FS_MODE_SPEED) {
+		/* Set master code speed register */
+		ret = mtk_i2c_calculate_speed(priv, clk_src, MAX_FS_MODE_SPEED,
+					      &step_cnt, &sample_cnt);
+		if (ret < 0)
+			goto exit;
+
+		timing_reg = (sample_cnt << TIMING_SAMPLE_OFFSET) | step_cnt;
+		writel(timing_reg, priv->base + OFFSET_TIMING);
+		/* Set the high speed mode register */
+		ret = mtk_i2c_calculate_speed(priv, clk_src, priv->speed,
+					      &step_cnt, &sample_cnt);
+		if (ret < 0)
+			goto exit;
+
+		high_speed_reg = I2C_TIME_DEFAULT_VALUE |
+				(sample_cnt << HS_SAMPLE_OFFSET) |
+				(step_cnt << HS_STEP_OFFSET);
+		writel(high_speed_reg, priv->base + OFFSET_HS);
+	} else {
+		ret = mtk_i2c_calculate_speed(priv, clk_src, priv->speed,
+					      &step_cnt, &sample_cnt);
+		if (ret < 0)
+			goto exit;
+
+		timing_reg = (sample_cnt << TIMING_SAMPLE_OFFSET) | step_cnt;
+		/* Disable the high speed transaction */
+		high_speed_reg = I2C_TIME_CLR_VALUE;
+		writel(timing_reg, priv->base + OFFSET_TIMING);
+		writel(high_speed_reg, priv->base + OFFSET_HS);
+	}
+exit:
+	mtk_i2c_clk_disable(priv);
+	return ret;
+}
+
+static int mtk_i2c_do_transfer(struct mtk_i2c_priv *priv,
+			       struct i2c_msg *msgs,
+			       int num, int left_num)
+{
+	u16 addr_reg;
+	u16 start_reg;
+	u16 control_reg;
+	u16 restart_flag = 0;
+	u16	irq_stat = 0;
+	u8 trans_error = 0;
+	bool tmo = false;
+	u8 *ptr = msgs->buf;
+	u16 data_size = msgs->len;
+	u32 tmo_poll = 0;
+	int ret;
+
+	if (priv->auto_restart)
+		restart_flag = I2C_RS_TRANSFER;
+
+	control_reg = i2c_readl(priv, OFFSET_CONTROL) &
+		~(I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS);
+
+	if (priv->speed > MAX_FS_MODE_SPEED || num > 1)
+		control_reg |= I2C_CONTROL_RS;
+
+	if (priv->op == I2C_MASTER_WRRD)
+		control_reg |= I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS;
+	control_reg |= I2C_CONTROL_DMA_EN;
+	i2c_writel(priv, OFFSET_CONTROL, control_reg);
+
+	/* set start condition */
+	if (priv->speed <= MAX_ST_MODE_SPEED)
+		i2c_writel(priv, OFFSET_EXT_CONF, I2C_ST_START_CON);
+	else
+		i2c_writel(priv, OFFSET_EXT_CONF, I2C_FS_START_CON);
+
+	addr_reg = msgs->addr << 1;
+	if (priv->op == I2C_MASTER_RD)
+		addr_reg |= I2C_M_RD;
+	if (priv->zero_len)
+		i2c_writel(priv, OFFSET_SLAVE_ADDR, addr_reg | TRANS_ADDR_ONLY);
+	else
+		i2c_writel(priv, OFFSET_SLAVE_ADDR, addr_reg);
+
+	/* clear interrupt status */
+	i2c_writel(priv, OFFSET_INTR_STAT, restart_flag | I2C_HS_NACKERR |
+						I2C_ACKERR | I2C_TRANSAC_COMP);
+		i2c_writel(priv, OFFSET_FIFO_ADDR_CLR, I2C_FIFO_ADDR_CLR);
+
+	/* enable interrupt */
+	i2c_writel(priv, OFFSET_INTR_MASK, restart_flag | I2C_HS_NACKERR |
+						I2C_ACKERR | I2C_TRANSAC_COMP);
+
+	/* set transfer and transaction len */
+	if (priv->op == I2C_MASTER_WRRD) {
+		i2c_writel(priv, OFFSET_TRANSFER_LEN, msgs->len);
+		i2c_writel(priv, OFFSET_TRANSFER_LEN_AUX,
+			   (msgs + 1)->len);
+		i2c_writel(priv, OFFSET_TRANSAC_LEN, I2C_WRRD_TRANAC_VALUE);
+	} else {
+		i2c_writel(priv, OFFSET_TRANSFER_LEN, msgs->len);
+		i2c_writel(priv, OFFSET_TRANSAC_LEN, num);
+	}
+
+	/* prepare buffer data to start transfer */
+	if (priv->op == I2C_MASTER_RD) {
+		writel(I2C_DMA_INT_FLAG_NONE, priv->pdmabase +
+		OFFSET_INT_FLAG);
+		writel(I2C_DMA_CON_RX, priv->pdmabase + OFFSET_CON);
+		priv->rx_buff = (u8 *)noncached_alloc(msgs->len,
+				ARCH_DMA_MINALIGN);
+		writel((u32)priv->rx_buff, priv->pdmabase +
+		OFFSET_RX_MEM_ADDR);
+		writel(msgs->len, priv->pdmabase + OFFSET_RX_LEN);
+	} else if (priv->op == I2C_MASTER_WR) {
+		writel(I2C_DMA_INT_FLAG_NONE, priv->pdmabase +
+			  OFFSET_INT_FLAG);
+		writel(I2C_DMA_CON_TX, priv->pdmabase + OFFSET_CON);
+		priv->tx_buff = (u8 *)noncached_alloc(msgs->len,
+				ARCH_DMA_MINALIGN);
+		memcpy(priv->tx_buff, msgs->buf, msgs->len);
+		writel((u32)priv->tx_buff, priv->pdmabase +
+			  OFFSET_TX_MEM_ADDR);
+		writel(msgs->len, priv->pdmabase + OFFSET_TX_LEN);
+	} else {
+		writel(I2C_DMA_CLR_FLAG, priv->pdmabase +
+			OFFSET_INT_FLAG);
+		writel(I2C_DMA_CLR_FLAG, priv->pdmabase + OFFSET_CON);
+		priv->tx_buff = (u8 *)noncached_alloc(msgs->len,
+				ARCH_DMA_MINALIGN);
+		priv->rx_buff = (u8 *)noncached_alloc((msgs + 1)->len,
+				ARCH_DMA_MINALIGN);
+		memcpy(priv->tx_buff, msgs->buf, msgs->len);
+		writel((u32)priv->tx_buff, priv->pdmabase +
+			  OFFSET_TX_MEM_ADDR);
+		writel((u32)priv->rx_buff, priv->pdmabase +
+			  OFFSET_RX_MEM_ADDR);
+		writel(msgs->len, priv->pdmabase + OFFSET_TX_LEN);
+		writel((msgs + 1)->len, priv->pdmabase +
+			  OFFSET_RX_LEN);
+	}
+	writel(I2C_DMA_START_EN, priv->pdmabase + OFFSET_EN);
+
+	if (!priv->auto_restart) {
+		start_reg = I2C_TRANSAC_START;
+	} else {
+		start_reg = I2C_TRANSAC_START | I2C_RS_MUL_TRIG;
+		if (left_num >= 1)
+			start_reg |= I2C_RS_MUL_CNFG;
+	}
+	i2c_writel(priv, OFFSET_START, start_reg);
+
+	for (;;) {
+		irq_stat = i2c_readl(priv, OFFSET_INTR_STAT);
+
+		/* ignore the first restart irq after the master code */
+		if (priv->ignore_restart_irq && (irq_stat | restart_flag)) {
+			priv->ignore_restart_irq = false;
+			irq_stat = 0;
+			writel(I2C_RS_MUL_CNFG | I2C_RS_MUL_TRIG |
+			       I2C_TRANSAC_START, priv->base + OFFSET_START);
+		}
+
+		if (irq_stat & (I2C_TRANSAC_COMP | restart_flag)) {
+			tmo = false;
+			if (irq_stat & (I2C_HS_NACKERR | I2C_ACKERR))
+				trans_error = 1;
+
+			break;
+		}
+		udelay(1);
+		if (tmo_poll++ >= TRANSFER_TIMEOUT) {
+			tmo = true;
+			break;
+		}
+	}
+	/* clear interrupt mask */
+	i2c_writel(priv, OFFSET_INTR_MASK, ~(restart_flag | I2C_HS_NACKERR |
+		  I2C_ACKERR | I2C_TRANSAC_COMP));
+
+	if (!tmo && trans_error == 0) {
+		/* transfer success ,we need to get data from fifo */
+		if (priv->op == I2C_MASTER_RD || priv->op == I2C_MASTER_WRRD) {
+			ptr = priv->op == I2C_MASTER_RD ?
+					msgs->buf : (msgs + 1)->buf;
+			data_size = priv->op == I2C_MASTER_RD ?
+					msgs->len : (msgs + 1)->len;
+			memcpy(ptr, priv->rx_buff, data_size);
+		}
+	} else {
+		/* timeout or ACKERR */
+		if (tmo) {
+			ret = -ETIMEDOUT;
+			if (!priv->filter_msg)
+				debug("transfer timeout! addr: 0x%x,\n",
+				      msgs->addr);
+		} else {
+			ret = -ENXIO;
+			if (!priv->filter_msg)
+				debug("ACKERR! addr: 0x%x,IRQ:0x%x\n",
+				      msgs->addr, irq_stat);
+		}
+		mtk_i2c_init_hw(priv);
+		return ret;
+	}
+	return 0;
+}
+
+static int mtk_i2c_transfer(struct udevice *dev, struct i2c_msg *msg,
+			    int nmsgs)
+{
+	int ret;
+	int left_num;
+	u8 num_cnt;
+	struct mtk_i2c_priv *priv = dev_get_priv(dev);
+
+	priv->auto_restart = true;
+	left_num = nmsgs;
+	mtk_i2c_clk_enable(priv);
+	for (num_cnt = 0; num_cnt < nmsgs; num_cnt++) {
+		if (((msg + num_cnt)->addr) > MAX_I2C_ADDR) {
+			ret = -EINVAL;
+			goto err_exit;
+		}
+		if ((msg + num_cnt)->len > MAX_I2C_LEN) {
+			ret = -EINVAL;
+			goto err_exit;
+		}
+	}
+
+	/* checking if we can skip restart and optimize using WRRD mode */
+	if (priv->auto_restart && nmsgs == 2) {
+		if (!(msg[0].flags & I2C_M_RD) && (msg[1].flags & I2C_M_RD) &&
+		    msg[0].addr == msg[1].addr) {
+			priv->auto_restart = false;
+		}
+	}
+
+	if (priv->auto_restart && nmsgs >= 2 && priv->speed > MAX_FS_MODE_SPEED)
+		/* ignore the first restart irq after the master code,
+		 * otherwise the first transfer will be discarded.
+		 */
+		priv->ignore_restart_irq = true;
+	else
+		priv->ignore_restart_irq = false;
+
+	while (left_num--) {
+		/*priv->zero_len = true
+		 *transfer slave address only to support devices detect
+		 */
+		if (!msg->buf)
+			priv->zero_len = true;
+		else
+			priv->zero_len = false;
+
+		if (msg->flags & I2C_M_RD)
+			priv->op = I2C_MASTER_RD;
+		else
+			priv->op = I2C_MASTER_WR;
+
+		if (!priv->auto_restart) {
+			if (nmsgs > 1) {
+				/* combined two messages into one transaction */
+				priv->op = I2C_MASTER_WRRD;
+				left_num--;
+			}
+		}
+		ret = mtk_i2c_do_transfer(priv, msg, nmsgs, left_num);
+		if (ret < 0)
+			goto err_exit;
+		msg++;
+	}
+	ret = I2C_OK;
+
+err_exit:
+	mtk_i2c_clk_disable(priv);
+	return ret;
+}
+
+static int mtk_i2c_ofdata_to_platdata(struct udevice *dev)
+{
+	int ret;
+	struct mtk_i2c_priv *priv = dev_get_priv(dev);
+
+	priv->base = dev_remap_addr_index(dev, 0);
+	priv->pdmabase = dev_remap_addr_index(dev, 1);
+	ret = clk_get_by_index(dev, 0, &priv->clk_main);
+	if (ret)
+		return ret;
+
+	ret = clk_get_by_index(dev, 1, &priv->clk_dma);
+
+	return ret;
+}
+
+static int mtk_i2c_probe(struct udevice *dev)
+{
+	struct mtk_i2c_priv *priv = dev_get_priv(dev);
+
+	priv->soc_data = (struct mtk_i2c_soc_data *)dev_get_driver_data(dev);
+	mtk_i2c_clk_enable(priv);
+	mtk_i2c_init_hw(priv);
+	mtk_i2c_clk_disable(priv);
+	return 0;
+}
+
+static int mtk_i2c_deblock(struct udevice *dev)
+{
+	struct mtk_i2c_priv *priv = dev_get_priv(dev);
+
+	mtk_i2c_clk_enable(priv);
+	mtk_i2c_init_hw(priv);
+	mtk_i2c_clk_disable(priv);
+	return 0;
+}
+
+static const struct mtk_i2c_soc_data mt8518_soc_data = {
+	.dma_sync = 0,
+};
+
+static const struct mtk_i2c_soc_data mt8512_soc_data = {
+	.dma_sync = 1,
+};
+
+static const struct dm_i2c_ops mtk_i2c_ops = {
+	.xfer		= mtk_i2c_transfer,
+	.set_bus_speed	= mtk_i2c_set_speed,
+	.deblock	= mtk_i2c_deblock,
+};
+
+static const struct udevice_id mtk_i2c_ids[] = {
+	{
+		.compatible = "mediatek,mt8518-i2c",
+		.data = (ulong)&mt8518_soc_data,
+	},
+	{
+		.compatible = "mediatek,mt8512-i2c",
+		.data = (ulong)&mt8512_soc_data,
+	},
+	{
+	}
+};
+
+U_BOOT_DRIVER(mtk_i2c) = {
+	.name		= "mtk_i2c",
+	.id		= UCLASS_I2C,
+	.of_match	= mtk_i2c_ids,
+	.ofdata_to_platdata = mtk_i2c_ofdata_to_platdata,
+	.probe		= mtk_i2c_probe,
+	.priv_auto_alloc_size = sizeof(struct mtk_i2c_priv),
+	.ops		= &mtk_i2c_ops,
+};
-- 
2.18.0

[PATCH v2 1/5] i2c: mediatek: add basic driver support

[Simon Glass]

Hi Mingming,

On Wed, 30 Sep 2020 at 02:22, mingming lee <mingming.lee@mediatek.com> wrote:
>
> From: Mingming Lee <Mingming.Lee@mediatek.com>
>
> Add MediaTek I2C basic driver
>
> Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
>
> ---
> Changes for v2:
>    - using error number defined in include/linux/errno.h
> ---
>  drivers/i2c/Kconfig  |   9 +
>  drivers/i2c/Makefile |   1 +
>  drivers/i2c/mt_i2c.c | 617 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 627 insertions(+)
>  create mode 100644 drivers/i2c/mt_i2c.c

This looks good to me. Some nits below.

>
> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> index dec6dc9dfa..103688ed36 100644
> --- a/drivers/i2c/Kconfig
> +++ b/drivers/i2c/Kconfig
> @@ -159,6 +159,15 @@ config SYS_I2C_MESON
>           internal buffer holding up to 8 bytes for transfers and supports
>           both 7-bit and 10-bit addresses.
>
> +config SYS_I2C_MTK
> +       bool "MediaTek I2C driver"
> +       default n

Not needed

> +       help
> +         This selects the MediaTek Integrated Inter Circuit bus driver.
> +         The I2C bus adapter is the base for some other I2C client, eg: touch, sensors.
> +         If you want to use MediaTek I2C interface, say Y here.
> +         If unsure, say N.
> +
>  config SYS_I2C_MXC
>         bool "NXP MXC I2C driver"
>         help
> diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
> index e851ec462e..7227742f8d 100644
> --- a/drivers/i2c/Makefile
> +++ b/drivers/i2c/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_SYS_I2C_LPC32XX) += lpc32xx_i2c.o
>  obj-$(CONFIG_SYS_I2C_MESON) += meson_i2c.o
>  obj-$(CONFIG_SYS_I2C_MVTWSI) += mvtwsi.o
>  obj-$(CONFIG_SYS_I2C_MXC) += mxc_i2c.o
> +obj-$(CONFIG_SYS_I2C_MTK) += mt_i2c.o
>  obj-$(CONFIG_SYS_I2C_NEXELL) += nx_i2c.o
>  obj-$(CONFIG_SYS_I2C_OCTEON) += octeon_i2c.o
>  obj-$(CONFIG_SYS_I2C_OMAP24XX) += omap24xx_i2c.o
> diff --git a/drivers/i2c/mt_i2c.c b/drivers/i2c/mt_i2c.c
> new file mode 100644
> index 0000000000..be6262d3d2
> --- /dev/null
> +++ b/drivers/i2c/mt_i2c.c
> @@ -0,0 +1,617 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MediaTek I2C Interface driver
> + *
> + * Copyright (C) 2020 MediaTek Inc.
> + * Author: Mingming Lee <Mingming.Lee@mediatek.com>
> + */
> +
> +#include <asm/cache.h>
> +#include <asm/io.h>

Check the include-file ordering here:

https://www.denx.de/wiki/U-Boot/CodingStyle

> +#include <common.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <i2c.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +
> +#define I2C_OK                 0

Drop this and just use 0

> +
> +#define I2C_RS_TRANSFER                BIT(4)
> +#define I2C_HS_NACKERR         BIT(2)
> +#define I2C_ACKERR             BIT(1)
> +#define I2C_TRANSAC_COMP       BIT(0)
> +#define I2C_TRANSAC_START      BIT(0)
> +#define I2C_RS_MUL_CNFG                BIT(15)
> +#define I2C_RS_MUL_TRIG                BIT(14)
> +#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_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_DMA_HARD_RST               0x0002
> +
> +#define MAX_ST_MODE_SPEED              100000
> +#define MAX_FS_MODE_SPEED              400000
> +#define MAX_HS_MODE_SPEED              3400000
> +#define MAX_SAMPLE_CNT_DIV             8
> +#define MAX_STEP_CNT_DIV               64
> +#define MAX_HS_STEP_CNT_DIV            8
> +#define I2C_DEFAULT_CLK_DIV            4
> +
> +#define MAX_I2C_ADDR           0x7F
> +#define MAX_I2C_LEN            0xFF

Can you use lower-case hex?

> +#define TRANS_ADDR_ONLY                BIT(8)
> +#define TRANSFER_TIMEOUT               50000  /* us */
> +#define I2C_FIFO_STAT1_MASK            0x001F
> +#define TIMING_SAMPLE_OFFSET   8
> +#define HS_SAMPLE_OFFSET       12
> +#define HS_STEP_OFFSET         8
> +
> +#define I2C_CONTROL_WRAPPER            BIT(0)
> +#define I2C_CONTROL_RS                 BIT(1)
> +#define I2C_CONTROL_DMA_EN             BIT(2)
> +#define I2C_CONTROL_CLK_EXT_EN         BIT(3)
> +#define I2C_CONTROL_DIR_CHANGE         BIT(4)
> +#define I2C_CONTROL_ACKERR_DET_EN      BIT(5)
> +#define I2C_CONTROL_TRANSFER_LEN_CHANGE BIT(6)
> +#define I2C_CONTROL_DMAACK             BIT(8)
> +#define I2C_CONTROL_ASYNC              BIT(9)
> +
> +enum I2C_REGS_OFFSET {
> +       OFFSET_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,

Perhaps REG instead of OFFSET since it is shorter?

> +       OFFSET_START = 0x24,
> +       OFFSET_EXT_CONF = 0x28,
> +       OFFSET_FIFO_STAT1 = 0x2C,
> +       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_DEBUGSTAT = 0x64,
> +       OFFSET_DEBUGCTRL = 0x68,
> +       OFFSET_TRANSFER_LEN_AUX = 0x6C,
> +       OFFSET_CLOCK_DIV = 0x70,
> +       OFFSET_SCL_HL_RATIO = 0x74,
> +       OFFSET_SCL_HS_HL_RATIO = 0x78,
> +       OFFSET_SCL_MIS_COMP_POINT = 0x7C,
> +       OFFSET_STA_STOP_AC_TIME = 0x80,
> +       OFFSET_HS_STA_STOP_AC_TIME = 0x84,
> +       OFFSET_DATA_TIME = 0x88,
> +};
> +
> +enum DMA_REGS_OFFSET {
> +       OFFSET_INT_FLAG = 0x0,
> +       OFFSET_INT_EN = 0x04,
> +       OFFSET_EN = 0x08,
> +       OFFSET_RST = 0x0C,
> +       OFFSET_CON = 0x18,
> +       OFFSET_TX_MEM_ADDR = 0x1C,
> +       OFFSET_RX_MEM_ADDR = 0x20,
> +       OFFSET_TX_LEN = 0x24,
> +       OFFSET_RX_LEN = 0x28,
> +};
> +
> +enum mtk_trans_op {
> +       I2C_MASTER_WR = 1,
> +       I2C_MASTER_RD,
> +       I2C_MASTER_WRRD,
> +};
> +
> +struct mtk_i2c_soc_data {
> +       u8 dma_sync: 1;
> +};
> +
> +struct mtk_i2c_priv {
> +       /* set in i2c probe */
> +       void __iomem *base;                     /* i2c base addr */
> +       void __iomem *pdmabase;         /* dma base address*/
> +       struct clk clk_main;            /* main clock for i2c bus */
> +       struct clk clk_dma;                 /* DMA clock for i2c via DMA */
> +       const struct mtk_i2c_soc_data *soc_data;
> +       enum mtk_trans_op op;
> +       bool   zero_len;
> +       bool   pushpull;            /* open drain */
> +       bool   filter_msg;          /* filter msg error log */
> +       bool   auto_restart;
> +       bool ignore_restart_irq;
> +       u32 speed;                 /* hz */
> +       u8 *tx_buff;
> +       u8 *rx_buff;

Please add comments where they are currently missing.

> +};
> +
> +static inline void i2c_writel(struct mtk_i2c_priv *priv, u8 offset, u32 value)

uint for args. There is no point in using u8, etc. and it just
potentially increases code size. Please fix globally

> +{
> +       writel(value, (priv->base + offset));
> +}
> +
> +static inline u32 i2c_readl(struct mtk_i2c_priv *priv, u8 offset)
> +{
> +       return readl(priv->base + offset);
> +}
> +
> +static void mtk_i2c_clk_enable(struct mtk_i2c_priv *priv)
> +{
> +       clk_enable(&priv->clk_main);

Error check.

> +       clk_enable(&priv->clk_dma);
> +}
> +
> +static void mtk_i2c_clk_disable(struct mtk_i2c_priv *priv)
> +{
> +       clk_disable(&priv->clk_dma);
> +       clk_disable(&priv->clk_main);

Error checks.

These two functions should return error codes.

> +}
> +
> +static void mtk_i2c_init_hw(struct mtk_i2c_priv *priv)
> +{
> +       u16 control_reg;
> +
> +       writel(I2C_DMA_HARD_RST, priv->pdmabase + OFFSET_RST);
> +       writel(I2C_DMA_CLR_FLAG, priv->pdmabase + OFFSET_RST);
> +       i2c_writel(priv, OFFSET_SOFTRESET, I2C_SOFT_RST);
> +       /* set ioconfig */
> +       if (priv->pushpull)
> +               i2c_writel(priv, OFFSET_IO_CONFIG, I2C_IO_CONFIG_PUSH_PULL);
> +       else
> +               i2c_writel(priv, OFFSET_IO_CONFIG, I2C_IO_CONFIG_OPEN_DRAIN);
> +
> +       i2c_writel(priv, OFFSET_DCM_EN, I2C_DCM_DISABLE);
> +       control_reg = I2C_CONTROL_ACKERR_DET_EN | I2C_CONTROL_CLK_EXT_EN;
> +       if (priv->soc_data->dma_sync)
> +               control_reg |= I2C_CONTROL_DMAACK | I2C_CONTROL_ASYNC;
> +       i2c_writel(priv, OFFSET_CONTROL, control_reg);
> +       i2c_writel(priv, OFFSET_DELAY_LEN, I2C_DELAY_LEN);
> +}
> +
> +static int mtk_i2c_calculate_speed(struct mtk_i2c_priv *priv,
> +                                  u32 clk_src,
> +                                  u32 target_speed,
> +                                  u32 *timing_step_cnt,
> +                                  u32 *timing_sample_cnt)

function comment

> +{
> +       u32 step_cnt;
> +       u32 sample_cnt;
> +       u32 max_step_cnt;
> +       u32 base_sample_cnt = MAX_SAMPLE_CNT_DIV;
> +       u32 base_step_cnt;
> +       u32 opt_div;
> +       u32 best_mul;
> +       u32 cnt_mul;

uint?

I don't see why these need to be u32. Please fix globally

> +
> +       if (target_speed > MAX_HS_MODE_SPEED)
> +               target_speed = MAX_HS_MODE_SPEED;
> +
> +       if (target_speed > MAX_FS_MODE_SPEED)
> +               max_step_cnt = MAX_HS_STEP_CNT_DIV;
> +       else
> +               max_step_cnt = MAX_STEP_CNT_DIV;
> +
> +       base_step_cnt = max_step_cnt;
> +       /* Find the best combination */
> +       opt_div = DIV_ROUND_UP(clk_src >> 1, target_speed);
> +       best_mul = MAX_SAMPLE_CNT_DIV * max_step_cnt;
> +
> +       /* Search for the best pair (sample_cnt, step_cnt) with
> +        * 0 < sample_cnt < MAX_SAMPLE_CNT_DIV
> +        * 0 < step_cnt < max_step_cnt
> +        * sample_cnt * step_cnt >= opt_div
> +        * optimizing for sample_cnt * step_cnt being minimal
> +        */
> +       for (sample_cnt = 1; sample_cnt <= MAX_SAMPLE_CNT_DIV; sample_cnt++) {
> +               step_cnt = DIV_ROUND_UP(opt_div, sample_cnt);
> +               cnt_mul = step_cnt * sample_cnt;
> +               if (step_cnt > max_step_cnt)
> +                       continue;
> +
> +               if (cnt_mul < best_mul) {
> +                       best_mul = cnt_mul;
> +                       base_sample_cnt = sample_cnt;
> +                       base_step_cnt = step_cnt;
> +                       if (best_mul == opt_div)
> +                               break;
> +               }
> +       }
> +
> +       sample_cnt = base_sample_cnt;
> +       step_cnt = base_step_cnt;
> +
> +       if ((clk_src / (2 * sample_cnt * step_cnt)) > target_speed) {
> +               /* In this case, hardware can't support such
> +                * low i2c_bus_freq
> +                */
> +               debug("Unsupported speed(%uhz)\n", target_speed);
> +               return -EINVAL;
> +       }
> +
> +       *timing_step_cnt = step_cnt - 1;
> +       *timing_sample_cnt = sample_cnt - 1;

blank line before final return (please fix globally)

> +       return 0;
> +}
> +
> +static int mtk_i2c_set_speed(struct udevice *dev, uint speed)
> +{
> +       u32 clk_src;
> +       u32 step_cnt;
> +       u32 sample_cnt;
> +       u16 timing_reg;
> +       u16 high_speed_reg;
> +       int ret = 0;
> +       struct mtk_i2c_priv *priv = dev_get_priv(dev);
> +
> +       priv->speed = speed;
> +       mtk_i2c_clk_enable(priv);
> +       clk_src = clk_get_rate(&priv->clk_main) /
> +                       I2C_DEFAULT_CLK_DIV;
> +       i2c_writel(priv, OFFSET_CLOCK_DIV, (I2C_DEFAULT_CLK_DIV - 1));
> +       if (priv->speed > MAX_FS_MODE_SPEED) {
> +               /* Set master code speed register */
> +               ret = mtk_i2c_calculate_speed(priv, clk_src, MAX_FS_MODE_SPEED,
> +                                             &step_cnt, &sample_cnt);
> +               if (ret < 0)
> +                       goto exit;
> +
> +               timing_reg = (sample_cnt << TIMING_SAMPLE_OFFSET) | step_cnt;
> +               writel(timing_reg, priv->base + OFFSET_TIMING);
> +               /* Set the high speed mode register */
> +               ret = mtk_i2c_calculate_speed(priv, clk_src, priv->speed,
> +                                             &step_cnt, &sample_cnt);
> +               if (ret < 0)
> +                       goto exit;
> +
> +               high_speed_reg = I2C_TIME_DEFAULT_VALUE |
> +                               (sample_cnt << HS_SAMPLE_OFFSET) |
> +                               (step_cnt << HS_STEP_OFFSET);
> +               writel(high_speed_reg, priv->base + OFFSET_HS);
> +       } else {
> +               ret = mtk_i2c_calculate_speed(priv, clk_src, priv->speed,
> +                                             &step_cnt, &sample_cnt);
> +               if (ret < 0)
> +                       goto exit;
> +
> +               timing_reg = (sample_cnt << TIMING_SAMPLE_OFFSET) | step_cnt;
> +               /* Disable the high speed transaction */
> +               high_speed_reg = I2C_TIME_CLR_VALUE;
> +               writel(timing_reg, priv->base + OFFSET_TIMING);
> +               writel(high_speed_reg, priv->base + OFFSET_HS);
> +       }
> +exit:
> +       mtk_i2c_clk_disable(priv);
> +       return ret;
> +}
> +
> +static int mtk_i2c_do_transfer(struct mtk_i2c_priv *priv,
> +                              struct i2c_msg *msgs,
> +                              int num, int left_num)

comment

> +{
> +       u16 addr_reg;
> +       u16 start_reg;
> +       u16 control_reg;
> +       u16 restart_flag = 0;
> +       u16     irq_stat = 0;
> +       u8 trans_error = 0;
> +       bool tmo = false;
> +       u8 *ptr = msgs->buf;
> +       u16 data_size = msgs->len;
> +       u32 tmo_poll = 0;
> +       int ret;
> +
> +       if (priv->auto_restart)
> +               restart_flag = I2C_RS_TRANSFER;
> +
> +       control_reg = i2c_readl(priv, OFFSET_CONTROL) &
> +               ~(I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS);
> +
> +       if (priv->speed > MAX_FS_MODE_SPEED || num > 1)
> +               control_reg |= I2C_CONTROL_RS;
> +
> +       if (priv->op == I2C_MASTER_WRRD)
> +               control_reg |= I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS;
> +       control_reg |= I2C_CONTROL_DMA_EN;
> +       i2c_writel(priv, OFFSET_CONTROL, control_reg);
> +
> +       /* set start condition */
> +       if (priv->speed <= MAX_ST_MODE_SPEED)
> +               i2c_writel(priv, OFFSET_EXT_CONF, I2C_ST_START_CON);
> +       else
> +               i2c_writel(priv, OFFSET_EXT_CONF, I2C_FS_START_CON);
> +
> +       addr_reg = msgs->addr << 1;
> +       if (priv->op == I2C_MASTER_RD)
> +               addr_reg |= I2C_M_RD;
> +       if (priv->zero_len)
> +               i2c_writel(priv, OFFSET_SLAVE_ADDR, addr_reg | TRANS_ADDR_ONLY);
> +       else
> +               i2c_writel(priv, OFFSET_SLAVE_ADDR, addr_reg);
> +
> +       /* clear interrupt status */
> +       i2c_writel(priv, OFFSET_INTR_STAT, restart_flag | I2C_HS_NACKERR |
> +                                               I2C_ACKERR | I2C_TRANSAC_COMP);
> +               i2c_writel(priv, OFFSET_FIFO_ADDR_CLR, I2C_FIFO_ADDR_CLR);
> +
> +       /* enable interrupt */
> +       i2c_writel(priv, OFFSET_INTR_MASK, restart_flag | I2C_HS_NACKERR |
> +                                               I2C_ACKERR | I2C_TRANSAC_COMP);
> +
> +       /* set transfer and transaction len */
> +       if (priv->op == I2C_MASTER_WRRD) {
> +               i2c_writel(priv, OFFSET_TRANSFER_LEN, msgs->len);
> +               i2c_writel(priv, OFFSET_TRANSFER_LEN_AUX,
> +                          (msgs + 1)->len);
> +               i2c_writel(priv, OFFSET_TRANSAC_LEN, I2C_WRRD_TRANAC_VALUE);
> +       } else {
> +               i2c_writel(priv, OFFSET_TRANSFER_LEN, msgs->len);
> +               i2c_writel(priv, OFFSET_TRANSAC_LEN, num);
> +       }
> +
> +       /* prepare buffer data to start transfer */
> +       if (priv->op == I2C_MASTER_RD) {
> +               writel(I2C_DMA_INT_FLAG_NONE, priv->pdmabase +
> +               OFFSET_INT_FLAG);
> +               writel(I2C_DMA_CON_RX, priv->pdmabase + OFFSET_CON);
> +               priv->rx_buff = (u8 *)noncached_alloc(msgs->len,
> +                               ARCH_DMA_MINALIGN);
> +               writel((u32)priv->rx_buff, priv->pdmabase +
> +               OFFSET_RX_MEM_ADDR);
> +               writel(msgs->len, priv->pdmabase + OFFSET_RX_LEN);
> +       } else if (priv->op == I2C_MASTER_WR) {
> +               writel(I2C_DMA_INT_FLAG_NONE, priv->pdmabase +
> +                         OFFSET_INT_FLAG);
> +               writel(I2C_DMA_CON_TX, priv->pdmabase + OFFSET_CON);
> +               priv->tx_buff = (u8 *)noncached_alloc(msgs->len,
> +                               ARCH_DMA_MINALIGN);
> +               memcpy(priv->tx_buff, msgs->buf, msgs->len);
> +               writel((u32)priv->tx_buff, priv->pdmabase +
> +                         OFFSET_TX_MEM_ADDR);
> +               writel(msgs->len, priv->pdmabase + OFFSET_TX_LEN);
> +       } else {
> +               writel(I2C_DMA_CLR_FLAG, priv->pdmabase +
> +                       OFFSET_INT_FLAG);
> +               writel(I2C_DMA_CLR_FLAG, priv->pdmabase + OFFSET_CON);
> +               priv->tx_buff = (u8 *)noncached_alloc(msgs->len,
> +                               ARCH_DMA_MINALIGN);

Do you need the cast?

> +               priv->rx_buff = (u8 *)noncached_alloc((msgs + 1)->len,
> +                               ARCH_DMA_MINALIGN);
> +               memcpy(priv->tx_buff, msgs->buf, msgs->len);
> +               writel((u32)priv->tx_buff, priv->pdmabase +
> +                         OFFSET_TX_MEM_ADDR);
> +               writel((u32)priv->rx_buff, priv->pdmabase +
> +                         OFFSET_RX_MEM_ADDR);
> +               writel(msgs->len, priv->pdmabase + OFFSET_TX_LEN);
> +               writel((msgs + 1)->len, priv->pdmabase +
> +                         OFFSET_RX_LEN);
> +       }
> +       writel(I2C_DMA_START_EN, priv->pdmabase + OFFSET_EN);
> +
> +       if (!priv->auto_restart) {
> +               start_reg = I2C_TRANSAC_START;
> +       } else {
> +               start_reg = I2C_TRANSAC_START | I2C_RS_MUL_TRIG;
> +               if (left_num >= 1)
> +                       start_reg |= I2C_RS_MUL_CNFG;
> +       }
> +       i2c_writel(priv, OFFSET_START, start_reg);
> +
> +       for (;;) {
> +               irq_stat = i2c_readl(priv, OFFSET_INTR_STAT);
> +
> +               /* ignore the first restart irq after the master code */
> +               if (priv->ignore_restart_irq && (irq_stat | restart_flag)) {
> +                       priv->ignore_restart_irq = false;
> +                       irq_stat = 0;
> +                       writel(I2C_RS_MUL_CNFG | I2C_RS_MUL_TRIG |
> +                              I2C_TRANSAC_START, priv->base + OFFSET_START);
> +               }
> +
> +               if (irq_stat & (I2C_TRANSAC_COMP | restart_flag)) {
> +                       tmo = false;
> +                       if (irq_stat & (I2C_HS_NACKERR | I2C_ACKERR))
> +                               trans_error = 1;
> +
> +                       break;
> +               }
> +               udelay(1);
> +               if (tmo_poll++ >= TRANSFER_TIMEOUT) {
> +                       tmo = true;
> +                       break;
> +               }
> +       }
> +       /* clear interrupt mask */
> +       i2c_writel(priv, OFFSET_INTR_MASK, ~(restart_flag | I2C_HS_NACKERR |
> +                 I2C_ACKERR | I2C_TRANSAC_COMP));
> +
> +       if (!tmo && trans_error == 0) {
> +               /* transfer success ,we need to get data from fifo */
> +               if (priv->op == I2C_MASTER_RD || priv->op == I2C_MASTER_WRRD) {
> +                       ptr = priv->op == I2C_MASTER_RD ?
> +                                       msgs->buf : (msgs + 1)->buf;
> +                       data_size = priv->op == I2C_MASTER_RD ?
> +                                       msgs->len : (msgs + 1)->len;
> +                       memcpy(ptr, priv->rx_buff, data_size);
> +               }
> +       } else {
> +               /* timeout or ACKERR */
> +               if (tmo) {
> +                       ret = -ETIMEDOUT;
> +                       if (!priv->filter_msg)
> +                               debug("transfer timeout! addr: 0x%x,\n",
> +                                     msgs->addr);
> +               } else {
> +                       ret = -ENXIO;

Should that be REMOTEIO? Up to you.

> +                       if (!priv->filter_msg)
> +                               debug("ACKERR! addr: 0x%x,IRQ:0x%x\n",
> +                                     msgs->addr, irq_stat);
> +               }
> +               mtk_i2c_init_hw(priv);
> +               return ret;
> +       }
> +       return 0;
> +}
> +
> +static int mtk_i2c_transfer(struct udevice *dev, struct i2c_msg *msg,
> +                           int nmsgs)
> +{
> +       int ret;
> +       int left_num;
> +       u8 num_cnt;

uint (please fix globally)

> +       struct mtk_i2c_priv *priv = dev_get_priv(dev);
> +
> +       priv->auto_restart = true;
> +       left_num = nmsgs;
> +       mtk_i2c_clk_enable(priv);

error check

> +       for (num_cnt = 0; num_cnt < nmsgs; num_cnt++) {
> +               if (((msg + num_cnt)->addr) > MAX_I2C_ADDR) {
> +                       ret = -EINVAL;
> +                       goto err_exit;
> +               }
> +               if ((msg + num_cnt)->len > MAX_I2C_LEN) {
> +                       ret = -EINVAL;
> +                       goto err_exit;
> +               }
> +       }
> +
> +       /* checking if we can skip restart and optimize using WRRD mode */

check

> +       if (priv->auto_restart && nmsgs == 2) {
> +               if (!(msg[0].flags & I2C_M_RD) && (msg[1].flags & I2C_M_RD) &&
> +                   msg[0].addr == msg[1].addr) {
> +                       priv->auto_restart = false;
> +               }
> +       }
> +
> +       if (priv->auto_restart && nmsgs >= 2 && priv->speed > MAX_FS_MODE_SPEED)
> +               /* ignore the first restart irq after the master code,
> +                * otherwise the first transfer will be discarded.
> +                */
> +               priv->ignore_restart_irq = true;
> +       else
> +               priv->ignore_restart_irq = false;
> +
> +       while (left_num--) {
> +               /*priv->zero_len = true

What is that comment for?

> +                *transfer slave address only to support devices detect
> +                */
> +               if (!msg->buf)
> +                       priv->zero_len = true;
> +               else
> +                       priv->zero_len = false;
> +
> +               if (msg->flags & I2C_M_RD)
> +                       priv->op = I2C_MASTER_RD;
> +               else
> +                       priv->op = I2C_MASTER_WR;
> +
> +               if (!priv->auto_restart) {
> +                       if (nmsgs > 1) {
> +                               /* combined two messages into one transaction */
> +                               priv->op = I2C_MASTER_WRRD;
> +                               left_num--;
> +                       }
> +               }
> +               ret = mtk_i2c_do_transfer(priv, msg, nmsgs, left_num);
> +               if (ret < 0)
> +                       goto err_exit;
> +               msg++;
> +       }
> +       ret = I2C_OK;

ret = 0

> +
> +err_exit:
> +       mtk_i2c_clk_disable(priv);

error check

> +       return ret;
> +}
> +
> +static int mtk_i2c_ofdata_to_platdata(struct udevice *dev)
> +{
> +       int ret;
> +       struct mtk_i2c_priv *priv = dev_get_priv(dev);
> +
> +       priv->base = dev_remap_addr_index(dev, 0);
> +       priv->pdmabase = dev_remap_addr_index(dev, 1);
> +       ret = clk_get_by_index(dev, 0, &priv->clk_main);
> +       if (ret)
> +               return ret;

consider log_ret()ret or log_msg_ret(ret)

> +
> +       ret = clk_get_by_index(dev, 1, &priv->clk_dma);
> +
> +       return ret;
> +}
> +
> +static int mtk_i2c_probe(struct udevice *dev)
> +{
> +       struct mtk_i2c_priv *priv = dev_get_priv(dev);
> +
> +       priv->soc_data = (struct mtk_i2c_soc_data *)dev_get_driver_data(dev);
> +       mtk_i2c_clk_enable(priv);
> +       mtk_i2c_init_hw(priv);
> +       mtk_i2c_clk_disable(priv);
> +       return 0;
> +}
> +
> +static int mtk_i2c_deblock(struct udevice *dev)
> +{
> +       struct mtk_i2c_priv *priv = dev_get_priv(dev);
> +
> +       mtk_i2c_clk_enable(priv);
> +       mtk_i2c_init_hw(priv);
> +       mtk_i2c_clk_disable(priv);
> +       return 0;
> +}
> +
> +static const struct mtk_i2c_soc_data mt8518_soc_data = {
> +       .dma_sync = 0,
> +};
> +
> +static const struct mtk_i2c_soc_data mt8512_soc_data = {
> +       .dma_sync = 1,
> +};
> +
> +static const struct dm_i2c_ops mtk_i2c_ops = {
> +       .xfer           = mtk_i2c_transfer,
> +       .set_bus_speed  = mtk_i2c_set_speed,
> +       .deblock        = mtk_i2c_deblock,
> +};
> +
> +static const struct udevice_id mtk_i2c_ids[] = {
> +       {
> +               .compatible = "mediatek,mt8518-i2c",
> +               .data = (ulong)&mt8518_soc_data,
> +       },
> +       {

Put the above { on the line above

> +               .compatible = "mediatek,mt8512-i2c",
> +               .data = (ulong)&mt8512_soc_data,
> +       },
> +       {
> +       }

same here

> +};
> +
> +U_BOOT_DRIVER(mtk_i2c) = {
> +       .name           = "mtk_i2c",
> +       .id             = UCLASS_I2C,
> +       .of_match       = mtk_i2c_ids,
> +       .ofdata_to_platdata = mtk_i2c_ofdata_to_platdata,
> +       .probe          = mtk_i2c_probe,
> +       .priv_auto_alloc_size = sizeof(struct mtk_i2c_priv),
> +       .ops            = &mtk_i2c_ops,
> +};
> --
> 2.18.0

Regards,
SImon

[PATCH v2 1/5] i2c: mediatek: add basic driver support

[Mingming Lee]

hi Simon,

Thank you for your carefully reveiw, and I think I would modify all of
them in next version.

On Mon, 2020-10-05 at 18:02 -0600, Simon Glass wrote:
> Hi Mingming,
> 
> On Wed, 30 Sep 2020 at 02:22, mingming lee <mingming.lee@mediatek.com> wrote:
> >
> > From: Mingming Lee <Mingming.Lee@mediatek.com>
> >
> > Add MediaTek I2C basic driver
> >
> > Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
> >
> > ---
> > Changes for v2:
> >    - using error number defined in include/linux/errno.h
> > ---
> >  drivers/i2c/Kconfig  |   9 +
> >  drivers/i2c/Makefile |   1 +
> >  drivers/i2c/mt_i2c.c | 617 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 627 insertions(+)
> >  create mode 100644 drivers/i2c/mt_i2c.c
> 
> This looks good to me. Some nits below.
> 
> >
> > diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> > index dec6dc9dfa..103688ed36 100644
> > --- a/drivers/i2c/Kconfig
> > +++ b/drivers/i2c/Kconfig
> > @@ -159,6 +159,15 @@ config SYS_I2C_MESON
> >           internal buffer holding up to 8 bytes for transfers and supports
> >           both 7-bit and 10-bit addresses.
> >
> > +config SYS_I2C_MTK
> > +       bool "MediaTek I2C driver"
> > +       default n
> 
> Not needed

I would remove it.

> 
> > +       help
> > +         This selects the MediaTek Integrated Inter Circuit bus driver.
> > +         The I2C bus adapter is the base for some other I2C client, eg: touch, sensors.
> > +         If you want to use MediaTek I2C interface, say Y here.
> > +         If unsure, say N.
> > +
> >  config SYS_I2C_MXC
> >         bool "NXP MXC I2C driver"
> >         help
> > diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
> > index e851ec462e..7227742f8d 100644
> > --- a/drivers/i2c/Makefile
> > +++ b/drivers/i2c/Makefile
> > @@ -28,6 +28,7 @@ obj-$(CONFIG_SYS_I2C_LPC32XX) += lpc32xx_i2c.o
> >  obj-$(CONFIG_SYS_I2C_MESON) += meson_i2c.o
> >  obj-$(CONFIG_SYS_I2C_MVTWSI) += mvtwsi.o
> >  obj-$(CONFIG_SYS_I2C_MXC) += mxc_i2c.o
> > +obj-$(CONFIG_SYS_I2C_MTK) += mt_i2c.o
> >  obj-$(CONFIG_SYS_I2C_NEXELL) += nx_i2c.o
> >  obj-$(CONFIG_SYS_I2C_OCTEON) += octeon_i2c.o
> >  obj-$(CONFIG_SYS_I2C_OMAP24XX) += omap24xx_i2c.o
> > diff --git a/drivers/i2c/mt_i2c.c b/drivers/i2c/mt_i2c.c
> > new file mode 100644
> > index 0000000000..be6262d3d2
> > --- /dev/null
> > +++ b/drivers/i2c/mt_i2c.c
> > @@ -0,0 +1,617 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * MediaTek I2C Interface driver
> > + *
> > + * Copyright (C) 2020 MediaTek Inc.
> > + * Author: Mingming Lee <Mingming.Lee@mediatek.com>
> > + */
> > +
> > +#include <asm/cache.h>
> > +#include <asm/io.h>
> 
> Check the include-file ordering here:
> 
> https://www.denx.de/wiki/U-Boot/CodingStyle
> 

I would modify the order.

> > +#include <common.h>
> > +#include <clk.h>
> > +#include <dm.h>
> > +#include <i2c.h>
> > +#include <linux/delay.h>
> > +#include <linux/errno.h>
> > +
> > +#define I2C_OK                 0
> 
> Drop this and just use 0
> 

I would drop this.

> > +
> > +#define I2C_RS_TRANSFER                BIT(4)
> > +#define I2C_HS_NACKERR         BIT(2)
> > +#define I2C_ACKERR             BIT(1)
> > +#define I2C_TRANSAC_COMP       BIT(0)
> > +#define I2C_TRANSAC_START      BIT(0)
> > +#define I2C_RS_MUL_CNFG                BIT(15)
> > +#define I2C_RS_MUL_TRIG                BIT(14)
> > +#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_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_DMA_HARD_RST               0x0002
> > +
> > +#define MAX_ST_MODE_SPEED              100000
> > +#define MAX_FS_MODE_SPEED              400000
> > +#define MAX_HS_MODE_SPEED              3400000
> > +#define MAX_SAMPLE_CNT_DIV             8
> > +#define MAX_STEP_CNT_DIV               64
> > +#define MAX_HS_STEP_CNT_DIV            8
> > +#define I2C_DEFAULT_CLK_DIV            4
> > +
> > +#define MAX_I2C_ADDR           0x7F
> > +#define MAX_I2C_LEN            0xFF
> 
> Can you use lower-case hex?
> 

I would modify it.

> > +#define TRANS_ADDR_ONLY                BIT(8)
> > +#define TRANSFER_TIMEOUT               50000  /* us */
> > +#define I2C_FIFO_STAT1_MASK            0x001F
> > +#define TIMING_SAMPLE_OFFSET   8
> > +#define HS_SAMPLE_OFFSET       12
> > +#define HS_STEP_OFFSET         8
> > +
> > +#define I2C_CONTROL_WRAPPER            BIT(0)
> > +#define I2C_CONTROL_RS                 BIT(1)
> > +#define I2C_CONTROL_DMA_EN             BIT(2)
> > +#define I2C_CONTROL_CLK_EXT_EN         BIT(3)
> > +#define I2C_CONTROL_DIR_CHANGE         BIT(4)
> > +#define I2C_CONTROL_ACKERR_DET_EN      BIT(5)
> > +#define I2C_CONTROL_TRANSFER_LEN_CHANGE BIT(6)
> > +#define I2C_CONTROL_DMAACK             BIT(8)
> > +#define I2C_CONTROL_ASYNC              BIT(9)
> > +
> > +enum I2C_REGS_OFFSET {
> > +       OFFSET_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,
> 
> Perhaps REG instead of OFFSET since it is shorter?
> 
> > +       OFFSET_START = 0x24,
> > +       OFFSET_EXT_CONF = 0x28,
> > +       OFFSET_FIFO_STAT1 = 0x2C,
> > +       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_DEBUGSTAT = 0x64,
> > +       OFFSET_DEBUGCTRL = 0x68,
> > +       OFFSET_TRANSFER_LEN_AUX = 0x6C,
> > +       OFFSET_CLOCK_DIV = 0x70,
> > +       OFFSET_SCL_HL_RATIO = 0x74,
> > +       OFFSET_SCL_HS_HL_RATIO = 0x78,
> > +       OFFSET_SCL_MIS_COMP_POINT = 0x7C,
> > +       OFFSET_STA_STOP_AC_TIME = 0x80,
> > +       OFFSET_HS_STA_STOP_AC_TIME = 0x84,
> > +       OFFSET_DATA_TIME = 0x88,
> > +};
> > +
> > +enum DMA_REGS_OFFSET {
> > +       OFFSET_INT_FLAG = 0x0,
> > +       OFFSET_INT_EN = 0x04,
> > +       OFFSET_EN = 0x08,
> > +       OFFSET_RST = 0x0C,
> > +       OFFSET_CON = 0x18,
> > +       OFFSET_TX_MEM_ADDR = 0x1C,
> > +       OFFSET_RX_MEM_ADDR = 0x20,
> > +       OFFSET_TX_LEN = 0x24,
> > +       OFFSET_RX_LEN = 0x28,
> > +};
> > +
> > +enum mtk_trans_op {
> > +       I2C_MASTER_WR = 1,
> > +       I2C_MASTER_RD,
> > +       I2C_MASTER_WRRD,
> > +};
> > +
> > +struct mtk_i2c_soc_data {
> > +       u8 dma_sync: 1;
> > +};
> > +
> > +struct mtk_i2c_priv {
> > +       /* set in i2c probe */
> > +       void __iomem *base;                     /* i2c base addr */
> > +       void __iomem *pdmabase;         /* dma base address*/
> > +       struct clk clk_main;            /* main clock for i2c bus */
> > +       struct clk clk_dma;                 /* DMA clock for i2c via DMA */
> > +       const struct mtk_i2c_soc_data *soc_data;
> > +       enum mtk_trans_op op;
> > +       bool   zero_len;
> > +       bool   pushpull;            /* open drain */
> > +       bool   filter_msg;          /* filter msg error log */
> > +       bool   auto_restart;
> > +       bool ignore_restart_irq;
> > +       u32 speed;                 /* hz */
> > +       u8 *tx_buff;
> > +       u8 *rx_buff;
> 
> Please add comments where they are currently missing.
> 
> > +};
> > +
> > +static inline void i2c_writel(struct mtk_i2c_priv *priv, u8 offset, u32 value)
> 
> uint for args. There is no point in using u8, etc. and it just
> potentially increases code size. Please fix globally
> 

It is better to use uint and I would modify them to uint.
> > +{
> > +       writel(value, (priv->base + offset));
> > +}
> > +
> > +static inline u32 i2c_readl(struct mtk_i2c_priv *priv, u8 offset)
> > +{
> > +       return readl(priv->base + offset);
> > +}
> > +
> > +static void mtk_i2c_clk_enable(struct mtk_i2c_priv *priv)
> > +{
> > +       clk_enable(&priv->clk_main);
> 
> Error check.

I would add error check.

> 
> > +       clk_enable(&priv->clk_dma);
> > +}
> > +
> > +static void mtk_i2c_clk_disable(struct mtk_i2c_priv *priv)
> > +{
> > +       clk_disable(&priv->clk_dma);
> > +       clk_disable(&priv->clk_main);
> 
> Error checks.
> 
> These two functions should return error codes.
> 
> > +}
> > +
> > +static void mtk_i2c_init_hw(struct mtk_i2c_priv *priv)
> > +{
> > +       u16 control_reg;
> > +
> > +       writel(I2C_DMA_HARD_RST, priv->pdmabase + OFFSET_RST);
> > +       writel(I2C_DMA_CLR_FLAG, priv->pdmabase + OFFSET_RST);
> > +       i2c_writel(priv, OFFSET_SOFTRESET, I2C_SOFT_RST);
> > +       /* set ioconfig */
> > +       if (priv->pushpull)
> > +               i2c_writel(priv, OFFSET_IO_CONFIG, I2C_IO_CONFIG_PUSH_PULL);
> > +       else
> > +               i2c_writel(priv, OFFSET_IO_CONFIG, I2C_IO_CONFIG_OPEN_DRAIN);
> > +
> > +       i2c_writel(priv, OFFSET_DCM_EN, I2C_DCM_DISABLE);
> > +       control_reg = I2C_CONTROL_ACKERR_DET_EN | I2C_CONTROL_CLK_EXT_EN;
> > +       if (priv->soc_data->dma_sync)
> > +               control_reg |= I2C_CONTROL_DMAACK | I2C_CONTROL_ASYNC;
> > +       i2c_writel(priv, OFFSET_CONTROL, control_reg);
> > +       i2c_writel(priv, OFFSET_DELAY_LEN, I2C_DELAY_LEN);
> > +}
> > +
> > +static int mtk_i2c_calculate_speed(struct mtk_i2c_priv *priv,
> > +                                  u32 clk_src,
> > +                                  u32 target_speed,
> > +                                  u32 *timing_step_cnt,
> > +                                  u32 *timing_sample_cnt)
> 
> function comment
> 

I would add function comment.

> > +{
> > +       u32 step_cnt;
> > +       u32 sample_cnt;
> > +       u32 max_step_cnt;
> > +       u32 base_sample_cnt = MAX_SAMPLE_CNT_DIV;
> > +       u32 base_step_cnt;
> > +       u32 opt_div;
> > +       u32 best_mul;
> > +       u32 cnt_mul;
> 
> uint?
> 
> I don't see why these need to be u32. Please fix globally
> 

I would modify it to uint.
> > +
> > +       if (target_speed > MAX_HS_MODE_SPEED)
> > +               target_speed = MAX_HS_MODE_SPEED;
> > +
> > +       if (target_speed > MAX_FS_MODE_SPEED)
> > +               max_step_cnt = MAX_HS_STEP_CNT_DIV;
> > +       else
> > +               max_step_cnt = MAX_STEP_CNT_DIV;
> > +
> > +       base_step_cnt = max_step_cnt;
> > +       /* Find the best combination */
> > +       opt_div = DIV_ROUND_UP(clk_src >> 1, target_speed);
> > +       best_mul = MAX_SAMPLE_CNT_DIV * max_step_cnt;
> > +
> > +       /* Search for the best pair (sample_cnt, step_cnt) with
> > +        * 0 < sample_cnt < MAX_SAMPLE_CNT_DIV
> > +        * 0 < step_cnt < max_step_cnt
> > +        * sample_cnt * step_cnt >= opt_div
> > +        * optimizing for sample_cnt * step_cnt being minimal
> > +        */
> > +       for (sample_cnt = 1; sample_cnt <= MAX_SAMPLE_CNT_DIV; sample_cnt++) {
> > +               step_cnt = DIV_ROUND_UP(opt_div, sample_cnt);
> > +               cnt_mul = step_cnt * sample_cnt;
> > +               if (step_cnt > max_step_cnt)
> > +                       continue;
> > +
> > +               if (cnt_mul < best_mul) {
> > +                       best_mul = cnt_mul;
> > +                       base_sample_cnt = sample_cnt;
> > +                       base_step_cnt = step_cnt;
> > +                       if (best_mul == opt_div)
> > +                               break;
> > +               }
> > +       }
> > +
> > +       sample_cnt = base_sample_cnt;
> > +       step_cnt = base_step_cnt;
> > +
> > +       if ((clk_src / (2 * sample_cnt * step_cnt)) > target_speed) {
> > +               /* In this case, hardware can't support such
> > +                * low i2c_bus_freq
> > +                */
> > +               debug("Unsupported speed(%uhz)\n", target_speed);
> > +               return -EINVAL;
> > +       }
> > +
> > +       *timing_step_cnt = step_cnt - 1;
> > +       *timing_sample_cnt = sample_cnt - 1;
> 
> blank line before final return (please fix globally)
> 

I would modify it.

> > +       return 0;
> > +}
> > +
> > +static int mtk_i2c_set_speed(struct udevice *dev, uint speed)
> > +{
> > +       u32 clk_src;
> > +       u32 step_cnt;
> > +       u32 sample_cnt;
> > +       u16 timing_reg;
> > +       u16 high_speed_reg;
> > +       int ret = 0;
> > +       struct mtk_i2c_priv *priv = dev_get_priv(dev);
> > +
> > +       priv->speed = speed;
> > +       mtk_i2c_clk_enable(priv);
> > +       clk_src = clk_get_rate(&priv->clk_main) /
> > +                       I2C_DEFAULT_CLK_DIV;
> > +       i2c_writel(priv, OFFSET_CLOCK_DIV, (I2C_DEFAULT_CLK_DIV - 1));
> > +       if (priv->speed > MAX_FS_MODE_SPEED) {
> > +               /* Set master code speed register */
> > +               ret = mtk_i2c_calculate_speed(priv, clk_src, MAX_FS_MODE_SPEED,
> > +                                             &step_cnt, &sample_cnt);
> > +               if (ret < 0)
> > +                       goto exit;
> > +
> > +               timing_reg = (sample_cnt << TIMING_SAMPLE_OFFSET) | step_cnt;
> > +               writel(timing_reg, priv->base + OFFSET_TIMING);
> > +               /* Set the high speed mode register */
> > +               ret = mtk_i2c_calculate_speed(priv, clk_src, priv->speed,
> > +                                             &step_cnt, &sample_cnt);
> > +               if (ret < 0)
> > +                       goto exit;
> > +
> > +               high_speed_reg = I2C_TIME_DEFAULT_VALUE |
> > +                               (sample_cnt << HS_SAMPLE_OFFSET) |
> > +                               (step_cnt << HS_STEP_OFFSET);
> > +               writel(high_speed_reg, priv->base + OFFSET_HS);
> > +       } else {
> > +               ret = mtk_i2c_calculate_speed(priv, clk_src, priv->speed,
> > +                                             &step_cnt, &sample_cnt);
> > +               if (ret < 0)
> > +                       goto exit;
> > +
> > +               timing_reg = (sample_cnt << TIMING_SAMPLE_OFFSET) | step_cnt;
> > +               /* Disable the high speed transaction */
> > +               high_speed_reg = I2C_TIME_CLR_VALUE;
> > +               writel(timing_reg, priv->base + OFFSET_TIMING);
> > +               writel(high_speed_reg, priv->base + OFFSET_HS);
> > +       }
> > +exit:
> > +       mtk_i2c_clk_disable(priv);
> > +       return ret;
> > +}
> > +
> > +static int mtk_i2c_do_transfer(struct mtk_i2c_priv *priv,
> > +                              struct i2c_msg *msgs,
> > +                              int num, int left_num)
> 
> comment
> 

I would add function comment.
> > +{
> > +       u16 addr_reg;
> > +       u16 start_reg;
> > +       u16 control_reg;
> > +       u16 restart_flag = 0;
> > +       u16     irq_stat = 0;
> > +       u8 trans_error = 0;
> > +       bool tmo = false;
> > +       u8 *ptr = msgs->buf;
> > +       u16 data_size = msgs->len;
> > +       u32 tmo_poll = 0;
> > +       int ret;
> > +
> > +       if (priv->auto_restart)
> > +               restart_flag = I2C_RS_TRANSFER;
> > +
> > +       control_reg = i2c_readl(priv, OFFSET_CONTROL) &
> > +               ~(I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS);
> > +
> > +       if (priv->speed > MAX_FS_MODE_SPEED || num > 1)
> > +               control_reg |= I2C_CONTROL_RS;
> > +
> > +       if (priv->op == I2C_MASTER_WRRD)
> > +               control_reg |= I2C_CONTROL_DIR_CHANGE | I2C_CONTROL_RS;
> > +       control_reg |= I2C_CONTROL_DMA_EN;
> > +       i2c_writel(priv, OFFSET_CONTROL, control_reg);
> > +
> > +       /* set start condition */
> > +       if (priv->speed <= MAX_ST_MODE_SPEED)
> > +               i2c_writel(priv, OFFSET_EXT_CONF, I2C_ST_START_CON);
> > +       else
> > +               i2c_writel(priv, OFFSET_EXT_CONF, I2C_FS_START_CON);
> > +
> > +       addr_reg = msgs->addr << 1;
> > +       if (priv->op == I2C_MASTER_RD)
> > +               addr_reg |= I2C_M_RD;
> > +       if (priv->zero_len)
> > +               i2c_writel(priv, OFFSET_SLAVE_ADDR, addr_reg | TRANS_ADDR_ONLY);
> > +       else
> > +               i2c_writel(priv, OFFSET_SLAVE_ADDR, addr_reg);
> > +
> > +       /* clear interrupt status */
> > +       i2c_writel(priv, OFFSET_INTR_STAT, restart_flag | I2C_HS_NACKERR |
> > +                                               I2C_ACKERR | I2C_TRANSAC_COMP);
> > +               i2c_writel(priv, OFFSET_FIFO_ADDR_CLR, I2C_FIFO_ADDR_CLR);
> > +
> > +       /* enable interrupt */
> > +       i2c_writel(priv, OFFSET_INTR_MASK, restart_flag | I2C_HS_NACKERR |
> > +                                               I2C_ACKERR | I2C_TRANSAC_COMP);
> > +
> > +       /* set transfer and transaction len */
> > +       if (priv->op == I2C_MASTER_WRRD) {
> > +               i2c_writel(priv, OFFSET_TRANSFER_LEN, msgs->len);
> > +               i2c_writel(priv, OFFSET_TRANSFER_LEN_AUX,
> > +                          (msgs + 1)->len);
> > +               i2c_writel(priv, OFFSET_TRANSAC_LEN, I2C_WRRD_TRANAC_VALUE);
> > +       } else {
> > +               i2c_writel(priv, OFFSET_TRANSFER_LEN, msgs->len);
> > +               i2c_writel(priv, OFFSET_TRANSAC_LEN, num);
> > +       }
> > +
> > +       /* prepare buffer data to start transfer */
> > +       if (priv->op == I2C_MASTER_RD) {
> > +               writel(I2C_DMA_INT_FLAG_NONE, priv->pdmabase +
> > +               OFFSET_INT_FLAG);
> > +               writel(I2C_DMA_CON_RX, priv->pdmabase + OFFSET_CON);
> > +               priv->rx_buff = (u8 *)noncached_alloc(msgs->len,
> > +                               ARCH_DMA_MINALIGN);
> > +               writel((u32)priv->rx_buff, priv->pdmabase +
> > +               OFFSET_RX_MEM_ADDR);
> > +               writel(msgs->len, priv->pdmabase + OFFSET_RX_LEN);
> > +       } else if (priv->op == I2C_MASTER_WR) {
> > +               writel(I2C_DMA_INT_FLAG_NONE, priv->pdmabase +
> > +                         OFFSET_INT_FLAG);
> > +               writel(I2C_DMA_CON_TX, priv->pdmabase + OFFSET_CON);
> > +               priv->tx_buff = (u8 *)noncached_alloc(msgs->len,
> > +                               ARCH_DMA_MINALIGN);
> > +               memcpy(priv->tx_buff, msgs->buf, msgs->len);
> > +               writel((u32)priv->tx_buff, priv->pdmabase +
> > +                         OFFSET_TX_MEM_ADDR);
> > +               writel(msgs->len, priv->pdmabase + OFFSET_TX_LEN);
> > +       } else {
> > +               writel(I2C_DMA_CLR_FLAG, priv->pdmabase +
> > +                       OFFSET_INT_FLAG);
> > +               writel(I2C_DMA_CLR_FLAG, priv->pdmabase + OFFSET_CON);
> > +               priv->tx_buff = (u8 *)noncached_alloc(msgs->len,
> > +                               ARCH_DMA_MINALIGN);
> 
> Do you need the cast?

yes,noncached_alloc() returns uinit and priv->tx_buff is u8 *, 
if do not using cast, it would have warning -Wint-conversion.

> 
> > +               priv->rx_buff = (u8 *)noncached_alloc((msgs + 1)->len,
> > +                               ARCH_DMA_MINALIGN);
> > +               memcpy(priv->tx_buff, msgs->buf, msgs->len);
> > +               writel((u32)priv->tx_buff, priv->pdmabase +
> > +                         OFFSET_TX_MEM_ADDR);
> > +               writel((u32)priv->rx_buff, priv->pdmabase +
> > +                         OFFSET_RX_MEM_ADDR);
> > +               writel(msgs->len, priv->pdmabase + OFFSET_TX_LEN);
> > +               writel((msgs + 1)->len, priv->pdmabase +
> > +                         OFFSET_RX_LEN);
> > +       }
> > +       writel(I2C_DMA_START_EN, priv->pdmabase + OFFSET_EN);
> > +
> > +       if (!priv->auto_restart) {
> > +               start_reg = I2C_TRANSAC_START;
> > +       } else {
> > +               start_reg = I2C_TRANSAC_START | I2C_RS_MUL_TRIG;
> > +               if (left_num >= 1)
> > +                       start_reg |= I2C_RS_MUL_CNFG;
> > +       }
> > +       i2c_writel(priv, OFFSET_START, start_reg);
> > +
> > +       for (;;) {
> > +               irq_stat = i2c_readl(priv, OFFSET_INTR_STAT);
> > +
> > +               /* ignore the first restart irq after the master code */
> > +               if (priv->ignore_restart_irq && (irq_stat | restart_flag)) {
> > +                       priv->ignore_restart_irq = false;
> > +                       irq_stat = 0;
> > +                       writel(I2C_RS_MUL_CNFG | I2C_RS_MUL_TRIG |
> > +                              I2C_TRANSAC_START, priv->base + OFFSET_START);
> > +               }
> > +
> > +               if (irq_stat & (I2C_TRANSAC_COMP | restart_flag)) {
> > +                       tmo = false;
> > +                       if (irq_stat & (I2C_HS_NACKERR | I2C_ACKERR))
> > +                               trans_error = 1;
> > +
> > +                       break;
> > +               }
> > +               udelay(1);
> > +               if (tmo_poll++ >= TRANSFER_TIMEOUT) {
> > +                       tmo = true;
> > +                       break;
> > +               }
> > +       }
> > +       /* clear interrupt mask */
> > +       i2c_writel(priv, OFFSET_INTR_MASK, ~(restart_flag | I2C_HS_NACKERR |
> > +                 I2C_ACKERR | I2C_TRANSAC_COMP));
> > +
> > +       if (!tmo && trans_error == 0) {
> > +               /* transfer success ,we need to get data from fifo */
> > +               if (priv->op == I2C_MASTER_RD || priv->op == I2C_MASTER_WRRD) {
> > +                       ptr = priv->op == I2C_MASTER_RD ?
> > +                                       msgs->buf : (msgs + 1)->buf;
> > +                       data_size = priv->op == I2C_MASTER_RD ?
> > +                                       msgs->len : (msgs + 1)->len;
> > +                       memcpy(ptr, priv->rx_buff, data_size);
> > +               }
> > +       } else {
> > +               /* timeout or ACKERR */
> > +               if (tmo) {
> > +                       ret = -ETIMEDOUT;
> > +                       if (!priv->filter_msg)
> > +                               debug("transfer timeout! addr: 0x%x,\n",
> > +                                     msgs->addr);
> > +               } else {
> > +                       ret = -ENXIO;
> 
> Should that be REMOTEIO? Up to you.
> 

I would modify it to EREMOTEIO.

> > +                       if (!priv->filter_msg)
> > +                               debug("ACKERR! addr: 0x%x,IRQ:0x%x\n",
> > +                                     msgs->addr, irq_stat);
> > +               }
> > +               mtk_i2c_init_hw(priv);
> > +               return ret;
> > +       }
> > +       return 0;
> > +}
> > +
> > +static int mtk_i2c_transfer(struct udevice *dev, struct i2c_msg *msg,
> > +                           int nmsgs)
> > +{
> > +       int ret;
> > +       int left_num;
> > +       u8 num_cnt;
> 
> uint (please fix globally)

I would fixed this globally.

> 
> > +       struct mtk_i2c_priv *priv = dev_get_priv(dev);
> > +
> > +       priv->auto_restart = true;
> > +       left_num = nmsgs;
> > +       mtk_i2c_clk_enable(priv);
> 
> error check

I would do error check globally.

> 
> > +       for (num_cnt = 0; num_cnt < nmsgs; num_cnt++) {
> > +               if (((msg + num_cnt)->addr) > MAX_I2C_ADDR) {
> > +                       ret = -EINVAL;
> > +                       goto err_exit;
> > +               }
> > +               if ((msg + num_cnt)->len > MAX_I2C_LEN) {
> > +                       ret = -EINVAL;
> > +                       goto err_exit;
> > +               }
> > +       }
> > +
> > +       /* checking if we can skip restart and optimize using WRRD mode */
> 
> check
> 

I would modify this.

> > +       if (priv->auto_restart && nmsgs == 2) {
> > +               if (!(msg[0].flags & I2C_M_RD) && (msg[1].flags & I2C_M_RD) &&
> > +                   msg[0].addr == msg[1].addr) {
> > +                       priv->auto_restart = false;
> > +               }
> > +       }
> > +
> > +       if (priv->auto_restart && nmsgs >= 2 && priv->speed > MAX_FS_MODE_SPEED)
> > +               /* ignore the first restart irq after the master code,
> > +                * otherwise the first transfer will be discarded.
> > +                */
> > +               priv->ignore_restart_irq = true;
> > +       else
> > +               priv->ignore_restart_irq = false;
> > +
> > +       while (left_num--) {
> > +               /*priv->zero_len = true
> 
> What is that comment for?

This is for debug before and I would drop it.

> 
> > +                *transfer slave address only to support devices detect
> > +                */
> > +               if (!msg->buf)
> > +                       priv->zero_len = true;
> > +               else
> > +                       priv->zero_len = false;
> > +
> > +               if (msg->flags & I2C_M_RD)
> > +                       priv->op = I2C_MASTER_RD;
> > +               else
> > +                       priv->op = I2C_MASTER_WR;
> > +
> > +               if (!priv->auto_restart) {
> > +                       if (nmsgs > 1) {
> > +                               /* combined two messages into one transaction */
> > +                               priv->op = I2C_MASTER_WRRD;
> > +                               left_num--;
> > +                       }
> > +               }
> > +               ret = mtk_i2c_do_transfer(priv, msg, nmsgs, left_num);
> > +               if (ret < 0)
> > +                       goto err_exit;
> > +               msg++;
> > +       }
> > +       ret = I2C_OK;
> 
> ret = 0
> 
> > +
> > +err_exit:
> > +       mtk_i2c_clk_disable(priv);
> 
> error check
> 
> > +       return ret;
> > +}
> > +
> > +static int mtk_i2c_ofdata_to_platdata(struct udevice *dev)
> > +{
> > +       int ret;
> > +       struct mtk_i2c_priv *priv = dev_get_priv(dev);
> > +
> > +       priv->base = dev_remap_addr_index(dev, 0);
> > +       priv->pdmabase = dev_remap_addr_index(dev, 1);
> > +       ret = clk_get_by_index(dev, 0, &priv->clk_main);
> > +       if (ret)
> > +               return ret;
> 
> consider log_ret()ret or log_msg_ret(ret)
> 

I would modify it.

> > +
> > +       ret = clk_get_by_index(dev, 1, &priv->clk_dma);
> > +
> > +       return ret;
> > +}
> > +
> > +static int mtk_i2c_probe(struct udevice *dev)
> > +{
> > +       struct mtk_i2c_priv *priv = dev_get_priv(dev);
> > +
> > +       priv->soc_data = (struct mtk_i2c_soc_data *)dev_get_driver_data(dev);
> > +       mtk_i2c_clk_enable(priv);
> > +       mtk_i2c_init_hw(priv);
> > +       mtk_i2c_clk_disable(priv);
> > +       return 0;
> > +}
> > +
> > +static int mtk_i2c_deblock(struct udevice *dev)
> > +{
> > +       struct mtk_i2c_priv *priv = dev_get_priv(dev);
> > +
> > +       mtk_i2c_clk_enable(priv);
> > +       mtk_i2c_init_hw(priv);
> > +       mtk_i2c_clk_disable(priv);
> > +       return 0;
> > +}
> > +
> > +static const struct mtk_i2c_soc_data mt8518_soc_data = {
> > +       .dma_sync = 0,
> > +};
> > +
> > +static const struct mtk_i2c_soc_data mt8512_soc_data = {
> > +       .dma_sync = 1,
> > +};
> > +
> > +static const struct dm_i2c_ops mtk_i2c_ops = {
> > +       .xfer           = mtk_i2c_transfer,
> > +       .set_bus_speed  = mtk_i2c_set_speed,
> > +       .deblock        = mtk_i2c_deblock,
> > +};
> > +
> > +static const struct udevice_id mtk_i2c_ids[] = {
> > +       {
> > +               .compatible = "mediatek,mt8518-i2c",
> > +               .data = (ulong)&mt8518_soc_data,
> > +       },
> > +       {
> 
> Put the above { on the line above
> 

I would modify it.

> > +               .compatible = "mediatek,mt8512-i2c",
> > +               .data = (ulong)&mt8512_soc_data,
> > +       },
> > +       {
> > +       }
> 
> same here
> 
> > +};
> > +
> > +U_BOOT_DRIVER(mtk_i2c) = {
> > +       .name           = "mtk_i2c",
> > +       .id             = UCLASS_I2C,
> > +       .of_match       = mtk_i2c_ids,
> > +       .ofdata_to_platdata = mtk_i2c_ofdata_to_platdata,
> > +       .probe          = mtk_i2c_probe,
> > +       .priv_auto_alloc_size = sizeof(struct mtk_i2c_priv),
> > +       .ops            = &mtk_i2c_ops,
> > +};
> > --
> > 2.18.0
> 
> Regards,
> SImon

[PATCH v2 2/5] ARM: dts: Mediatek: add i2c node support for mt8512

[mingming lee]

From: Mingming Lee <Mingming.Lee@mediatek.com>

add i2c dts node support for mt8512

Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
---
 arch/arm/dts/mt8512-bm1-emmc.dts | 12 ++++++++++
 arch/arm/dts/mt8512.dtsi         | 38 +++++++++++++++++++++++++++++++-
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/arch/arm/dts/mt8512-bm1-emmc.dts b/arch/arm/dts/mt8512-bm1-emmc.dts
index 296ed93b9e..7c02539fca 100644
--- a/arch/arm/dts/mt8512-bm1-emmc.dts
+++ b/arch/arm/dts/mt8512-bm1-emmc.dts
@@ -45,6 +45,18 @@
 	};
 };
 
+&i2c0 {
+	status = "okay";
+};
+
+&i2c1 {
+	status = "okay";
+};
+
+&i2c2 {
+	status = "okay";
+};
+
 &mmc0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&mmc0_pins_default>;
diff --git a/arch/arm/dts/mt8512.dtsi b/arch/arm/dts/mt8512.dtsi
index 01a02a7ebf..39e94993c1 100644
--- a/arch/arm/dts/mt8512.dtsi
+++ b/arch/arm/dts/mt8512.dtsi
@@ -90,6 +90,42 @@
 		reg = <0x10200a80 0x50>;
 	};
 
+	i2c0: i2c at 11007000 {
+		compatible = "mediatek,mt8512-i2c";
+		reg = <0x11007000 0x1000>,
+			  <0x11000080 0x80>;
+		clocks = <&infracfg CLK_INFRA_I2C0_AXI>,
+					<&infracfg CLK_INFRA_AP_DMA>;
+		clock-names = "main", "dma";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "disabled";
+	};
+
+	i2c1: i2c at 10019000 {
+		compatible = "mediatek,mt8512-i2c";
+		reg = <0x10019000 0x1000>,
+			  <0x11000100 0x80>;
+		clocks = <&infracfg CLK_INFRA_I2C1_AXI>,
+				<&infracfg CLK_INFRA_AP_DMA>;
+		clock-names = "main", "dma";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "disabled";
+	};
+
+	i2c2: i2c at 1001e000 {
+		compatible = "mediatek,mt8512-i2c";
+		reg = <0x1001e000 0x1000>,
+			  <0x11000180 0x80>;
+		clocks = <&infracfg CLK_INFRA_I2C1_AXI>,
+					<&infracfg CLK_INFRA_AP_DMA>;
+		clock-names = "main", "dma";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "disabled";
+	};
+
 	uart0: serial at 11002000 {
 		compatible = "mediatek,hsuart";
 		reg = <0x11002000 0x1000>;
@@ -112,4 +148,4 @@
 		status = "disabled";
 	};
 
-};
\ No newline at end of file
+};
-- 
2.18.0

[PATCH v2 2/5] ARM: dts: Mediatek: add i2c node support for mt8512

[Simon Glass]

On Wed, 30 Sep 2020 at 02:22, mingming lee <mingming.lee@mediatek.com> wrote:
>
> From: Mingming Lee <Mingming.Lee@mediatek.com>
>
> add i2c dts node support for mt8512
>
> Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
> ---
>  arch/arm/dts/mt8512-bm1-emmc.dts | 12 ++++++++++
>  arch/arm/dts/mt8512.dtsi         | 38 +++++++++++++++++++++++++++++++-
>  2 files changed, 49 insertions(+), 1 deletion(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 3/5] configs: mt8512: Enable I2C related configs

[mingming lee]

From: Mingming Lee <Mingming.Lee@mediatek.com>

Enable MTK I2C
Enable I2C basic command

Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
---
 configs/mt8512_bm1_emmc_defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/configs/mt8512_bm1_emmc_defconfig b/configs/mt8512_bm1_emmc_defconfig
index 10a2083134..6b3d9a4e9b 100644
--- a/configs/mt8512_bm1_emmc_defconfig
+++ b/configs/mt8512_bm1_emmc_defconfig
@@ -33,3 +33,6 @@ CONFIG_MTK_TIMER=y
 CONFIG_WDT=y
 CONFIG_WDT_MTK=y
 CONFIG_LZO=y
+CONFIG_CMD_I2C=y
+CONFIG_DM_I2C=y
+CONFIG_SYS_I2C_MTK=y
-- 
2.18.0

[PATCH v2 3/5] configs: mt8512: Enable I2C related configs

[Simon Glass]

On Wed, 30 Sep 2020 at 02:22, mingming lee <mingming.lee@mediatek.com> wrote:
>
> From: Mingming Lee <Mingming.Lee@mediatek.com>
>
> Enable MTK I2C
> Enable I2C basic command
>
> Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
> ---
>  configs/mt8512_bm1_emmc_defconfig | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Please check that the sorting is correct.

Something like this bash function may help:

# $1 is config name
function fix_kconfig()
{
make O=/tmp/b/$1_defconfig cfg && \
make O=/tmp/b/$1 savedefconfig && \
meld /tmp/b/$1/defconfig configs/$1_defconfig
}

Regards,
Simon

[PATCH v2 3/5] configs: mt8512: Enable I2C related configs

[Mingming Lee]

hello Simon,

thank you for your review
I have re-order it and would send it in next version.
 
On Sun, 2020-10-11 at 21:34 -0600, Simon Glass wrote:
> On Wed, 30 Sep 2020 at 02:22, mingming lee <mingming.lee@mediatek.com> wrote:
> >
> > From: Mingming Lee <Mingming.Lee@mediatek.com>
> >
> > Enable MTK I2C
> > Enable I2C basic command
> >
> > Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
> > ---
> >  configs/mt8512_bm1_emmc_defconfig | 3 +++
> >  1 file changed, 3 insertions(+)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Please check that the sorting is correct.
> 
> Something like this bash function may help:
> 
> # $1 is config name
> function fix_kconfig()
> {
> make O=/tmp/b/$1_defconfig cfg && \
> make O=/tmp/b/$1 savedefconfig && \
> meld /tmp/b/$1/defconfig configs/$1_defconfig
> }
> 
> Regards,
> Simon

[PATCH v2 4/5] dt-binding: i2c: add bindings for mediatek i2c driver

[mingming lee]

From: Mingming Lee <Mingming.Lee@mediatek.com>

add bindings for mediatek i2c driver

Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
---
 doc/device-tree-bindings/i2c/i2c-mtk.txt | 39 ++++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 doc/device-tree-bindings/i2c/i2c-mtk.txt

diff --git a/doc/device-tree-bindings/i2c/i2c-mtk.txt b/doc/device-tree-bindings/i2c/i2c-mtk.txt
new file mode 100644
index 0000000000..10a3f29a1e
--- /dev/null
+++ b/doc/device-tree-bindings/i2c/i2c-mtk.txt
@@ -0,0 +1,39 @@
+I2C for Mediatek platforms
+
+Required properties :
+- compatible : Must be "mediatek,mt8512-i2c"
+- reg: physical base address of the controller and length of memory mapped
+     region.
+- #address-cells = <1>;
+- #size-cells = <0>;
+- clocks: phandles to input clocks.
+- clock-names : Contains the names of the clocks:
+   "main", the clock used for normal mode I2C.
+   "dma", the clock used for apdma mode I2C.
+- status : enable in requried dts or not.
+
+Examples :
+
+	i2c0: i2c at 11007000 {
+		compatible = "mediatek,mt8512-i2c";
+		reg = <0x11007000 0x1000>,
+			  <0x11000080 0x80>;
+		clocks = <&infracfg CLK_INFRA_I2C0_AXI>,
+					<&infracfg CLK_INFRA_AP_DMA>;
+		clock-names = "main", "dma";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "disabled";
+	};
+
+	i2c1: i2c at 10019000 {
+		compatible = "mediatek,mt8512-i2c";
+		reg = <0x10019000 0x1000>,
+			  <0x11000100 0x80>;
+		clocks = <&infracfg CLK_INFRA_I2C1_AXI>,
+				<&infracfg CLK_INFRA_AP_DMA>;
+		clock-names = "main", "dma";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "disabled";
+	};
\ No newline at end of file
-- 
2.18.0

[PATCH v2 4/5] dt-binding: i2c: add bindings for mediatek i2c driver

[Simon Glass]

On Wed, 30 Sep 2020 at 02:22, mingming lee <mingming.lee@mediatek.com> wrote:
>
> From: Mingming Lee <Mingming.Lee@mediatek.com>
>
> add bindings for mediatek i2c driver

Should indicate which version of linux this file comes from.

>
> Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
> ---
>  doc/device-tree-bindings/i2c/i2c-mtk.txt | 39 ++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 doc/device-tree-bindings/i2c/i2c-mtk.txt

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 4/5] dt-binding: i2c: add bindings for mediatek i2c driver

[Mingming Lee]

hello Simon,

 
On Sun, 2020-10-11 at 21:34 -0600, Simon Glass wrote:
> On Wed, 30 Sep 2020 at 02:22, mingming lee <mingming.lee@mediatek.com> wrote:
> >
> > From: Mingming Lee <Mingming.Lee@mediatek.com>
> >
> > add bindings for mediatek i2c driver
> 
> Should indicate which version of linux this file comes from.
> 
we just send it in u-boot, do not sync with kernel yet.

> >
> > Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
> > ---
> >  doc/device-tree-bindings/i2c/i2c-mtk.txt | 39 ++++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >  create mode 100644 doc/device-tree-bindings/i2c/i2c-mtk.txt
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 5/5] MAINTAINERS: add i2c driver to ARM MEDIATEK

[mingming lee]

From: Mingming Lee <Mingming.Lee@mediatek.com>

add Mediatek i2c controller driver to ARM MEDIATEK.

Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 889a73f15f..cc78561818 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -254,10 +254,12 @@ S:	Maintained
 F:	arch/arm/mach-mediatek/
 F:	arch/arm/include/asm/arch-mediatek/
 F:	board/mediatek/
+F:	doc/device-tree-bindings/i2c/i2c-mtk.txt
 F:	doc/device-tree-bindings/phy/phy-mtk-*
 F:	doc/device-tree-bindings/usb/mediatek,*
 F:	doc/README.mediatek
 F:	drivers/clk/mediatek/
+F:	drivers/i2c/mt_i2c.c
 F:	drivers/mmc/mtk-sd.c
 F:	drivers/phy/phy-mtk-*
 F:	drivers/pinctrl/mediatek/
-- 
2.18.0

[PATCH v2 5/5] MAINTAINERS: add i2c driver to ARM MEDIATEK

[Simon Glass]

On Wed, 30 Sep 2020 at 02:22, mingming lee <mingming.lee@mediatek.com> wrote:
>
> From: Mingming Lee <Mingming.Lee@mediatek.com>
>
> add Mediatek i2c controller driver to ARM MEDIATEK.
>
> Signed-off-by: Mingming Lee <Mingming.Lee@mediatek.com>
> ---
>  MAINTAINERS | 2 ++
>  1 file changed, 2 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>