[PATCH v2 1/2] dt-bindings: i2c: Add Spreadtrum I2C controller documentation

[PATCH v2 1/2] dt-bindings: i2c: Add Spreadtrum I2C controller documentation

[Baolin Wang]

This patch adds the binding documentation for Spreadtrum I2C
controller device.

Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com>
---
Changes since v1:
 - No updates.
---
 Documentation/devicetree/bindings/i2c/i2c-sprd.txt |   31 ++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-sprd.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-sprd.txt b/Documentation/devicetree/bindings/i2c/i2c-sprd.txt
new file mode 100644
index 0000000..f285e5b
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-sprd.txt
@@ -0,0 +1,31 @@
+I2C for Spreadtrum platforms
+
+Required properties:
+- compatible: Should be "sprd,r8p0-i2c".
+- reg: Specify the physical base address of the controller and length
+  of memory mapped region.
+- interrupts: Should contain I2C interrupt.
+- clock-names: Should contain following entries:
+  "i2c" for I2C clock,
+  "source" for I2C source (parent) clock,
+  "enable" for I2C module enable clock.
+- clocks: Should contain a clock specifier for each entry in clock-names.
+- clock-frequency: Constains desired I2C bus clock frequency in Hz.
+- #address-cells: Should be 1 to describe address cells for I2C device address.
+- #size-cells: Should be 0 means no size cell for I2C device address.
+
+Optional properties:
+- Child nodes conforming to I2C bus binding
+
+Examples:
+i2c0: i2c@70500000 {
+	compatible = "sprd,r8p0-i2c";
+	reg = <0 0x70500000 0 0x1000>;
+	interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
+	clock-names = "i2c", "source", "enable";
+	clocks = <&clk_i2c3>, <&ext_26m>, <&clk_ap_apb_gates 11>;
+	clock-frequency = <400000>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+};
+
-- 
1.7.9.5

[PATCH v2 2/2] i2c: Add Spreadtrum I2C controller driver

[Baolin Wang]

This patch adds the I2C controller driver for Spreadtrum platform.

Signed-off-by: Baolin Wang <baolin.wang-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
---
Changes since v1:
 - Power on I2C device in probe().
 - Remove redundant macros and usb __maybe_unused.
 - Remove redundant 'of_match_ptr'.
 - Modify return values and check the return value for 'clk_prepare_enable'.
---
 drivers/i2c/busses/Kconfig    |   10 +
 drivers/i2c/busses/Makefile   |    1 +
 drivers/i2c/busses/i2c-sprd.c |  696 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 707 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-sprd.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 8adc0f1..dfbc301 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -877,6 +877,16 @@ config I2C_SIRF
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-sirf.
 
+config I2C_SPRD
+	tristate "Spreadtrum I2C interface"
+	depends on ARCH_SPRD
+	help
+	  If you say yes to this option, support will be included for the
+	  Spreadtrum I2C interface.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-sprd.
+
 config I2C_ST
 	tristate "STMicroelectronics SSC I2C support"
 	depends on ARCH_STI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 30b6085..dd46c21 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -84,6 +84,7 @@ obj-$(CONFIG_I2C_SH7760)	+= i2c-sh7760.o
 obj-$(CONFIG_I2C_SH_MOBILE)	+= i2c-sh_mobile.o
 obj-$(CONFIG_I2C_SIMTEC)	+= i2c-simtec.o
 obj-$(CONFIG_I2C_SIRF)		+= i2c-sirf.o
+obj-$(CONFIG_I2C_SPRD)		+= i2c-sprd.o
 obj-$(CONFIG_I2C_ST)		+= i2c-st.o
 obj-$(CONFIG_I2C_STM32F4)	+= i2c-stm32f4.o
 obj-$(CONFIG_I2C_STU300)	+= i2c-stu300.o
diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
new file mode 100644
index 0000000..6ae73e71
--- /dev/null
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -0,0 +1,696 @@
+/*
+ * Copyright (C) 2017 Spreadtrum Communications Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+
+#define I2C_CTL			0x0
+#define I2C_ADDR_CFG		0x4
+#define I2C_COUNT		0x8
+#define I2C_RX			0xC
+#define I2C_TX			0x10
+#define I2C_STATUS		0x14
+#define I2C_HSMODE_CFG		0x18
+#define I2C_VERSION		0x1C
+#define ADDR_DVD0		0x20
+#define ADDR_DVD1		0x24
+#define ADDR_STA0_DVD		0x28
+#define ADDR_RST		0x2C
+
+/* I2C_CTL */
+#define STP_EN			BIT(20)
+#define FIFO_AF_LVL		16
+#define FIFO_AE_LVL		12
+#define I2C_DMA_EN		BIT(11)
+#define FULL_INTEN		BIT(10)
+#define EMPTY_INTEN		BIT(9)
+#define I2C_DVD_OPT		BIT(8)
+#define I2C_OUT_OPT		BIT(7)
+#define I2C_TRIM_OPT		BIT(6)
+#define I2C_HS_MODE		BIT(4)
+#define I2C_MODE		BIT(3)
+#define I2C_EN			BIT(2)
+#define I2C_INT_EN		BIT(1)
+#define I2C_START		BIT(0)
+
+/* I2C_STATUS */
+#define SDA_IN			BIT(21)
+#define SCL_IN			BIT(20)
+#define FIFO_FULL		BIT(4)
+#define FIFO_EMPTY		BIT(3)
+#define I2C_INT			BIT(2)
+#define I2C_RX_ACK		BIT(1)
+#define I2C_BUSY		BIT(0)
+
+/* ADDR_RST */
+#define I2C_RST			BIT(0)
+
+#define I2C_FIFO_DEEP		12
+#define I2C_FIFO_FULL_THLD	15
+#define I2C_FIFO_EMPTY_THLD	4
+#define I2C_DATA_STEP		8
+#define SPRD_I2C_TIMEOUT	(msecs_to_jiffies(1000))
+
+/* timeout (ms) for pm runtime autosuspend */
+#define SPRD_I2C_PM_TIMEOUT	1000
+
+/* SPRD i2c data structure */
+struct sprd_i2c {
+	struct i2c_adapter adap;
+	struct device *dev;
+	void __iomem *base;
+	struct i2c_msg *msg;
+	struct clk *clk;
+	unsigned int src_clk;
+	unsigned int bus_freq;
+	struct completion complete;
+	u8 *buf;
+	u16 count;
+	int irq;
+	int err;
+};
+
+static void sprd_i2c_dump_reg(struct sprd_i2c *i2c_dev)
+{
+	dev_err(&i2c_dev->adap.dev, ": ======dump i2c-%d reg=======\n",
+		i2c_dev->adap.nr);
+	dev_err(&i2c_dev->adap.dev, ": I2C_CTRL:0x%x\n",
+		readl(i2c_dev->base + I2C_CTL));
+	dev_err(&i2c_dev->adap.dev, ": I2C_ADDR_CFG:0x%x\n",
+		readl(i2c_dev->base + I2C_ADDR_CFG));
+	dev_err(&i2c_dev->adap.dev, ": I2C_COUNT:0x%x\n",
+		readl(i2c_dev->base + I2C_COUNT));
+	dev_err(&i2c_dev->adap.dev, ": I2C_RX:0x%x\n",
+		readl(i2c_dev->base + I2C_RX));
+	dev_err(&i2c_dev->adap.dev, ": I2C_STATUS:0x%x\n",
+		readl(i2c_dev->base + I2C_STATUS));
+	dev_err(&i2c_dev->adap.dev, ": ADDR_DVD0:0x%x\n",
+		readl(i2c_dev->base + ADDR_DVD0));
+	dev_err(&i2c_dev->adap.dev, ": ADDR_DVD1:0x%x\n",
+		readl(i2c_dev->base + ADDR_DVD1));
+	dev_err(&i2c_dev->adap.dev, ": ADDR_STA0_DVD:0x%x\n",
+		readl(i2c_dev->base + ADDR_STA0_DVD));
+	dev_err(&i2c_dev->adap.dev, ": ADDR_RST:0x%x\n",
+		readl(i2c_dev->base + ADDR_RST));
+}
+
+static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count)
+{
+	writel(count, i2c_dev->base + I2C_COUNT);
+}
+
+static void sprd_i2c_send_stop(struct sprd_i2c *i2c_dev, int stop)
+{
+	unsigned int tmp = readl(i2c_dev->base + I2C_CTL);
+
+	if (stop)
+		writel(tmp & (~STP_EN), i2c_dev->base + I2C_CTL);
+	else
+		writel(tmp | STP_EN, i2c_dev->base + I2C_CTL);
+}
+
+static void sprd_i2c_clear_start(struct sprd_i2c *i2c_dev)
+{
+	unsigned int tmp = readl(i2c_dev->base + I2C_CTL);
+
+	writel(tmp & (~I2C_START), i2c_dev->base + I2C_CTL);
+}
+
+static void sprd_i2c_clear_ack(struct sprd_i2c *i2c_dev)
+{
+	unsigned int tmp = readl(i2c_dev->base + I2C_STATUS);
+
+	writel(tmp & (~I2C_RX_ACK), i2c_dev->base + I2C_STATUS);
+}
+
+static void sprd_i2c_clear_irq(struct sprd_i2c *i2c_dev)
+{
+	unsigned int tmp = readl(i2c_dev->base + I2C_STATUS);
+
+	writel(tmp & (~I2C_INT), i2c_dev->base + I2C_STATUS);
+}
+
+static void sprd_i2c_reset_fifo(struct sprd_i2c *i2c_dev)
+{
+	writel(I2C_RST, i2c_dev->base + ADDR_RST);
+}
+
+static void sprd_i2c_set_devaddr(struct sprd_i2c *i2c_dev, struct i2c_msg *m)
+{
+	writel(m->addr << 1, i2c_dev->base + I2C_ADDR_CFG);
+}
+
+static void sprd_i2c_write_bytes(struct sprd_i2c *i2c_dev, u8 *buf, u32 len)
+{
+	u32 i = 0;
+
+	for (i = 0; i < len; i++) {
+		writel(buf[i], i2c_dev->base + I2C_TX);
+		dev_dbg(&i2c_dev->adap.dev, "write bytes[%d] = %x\n", i, buf[i]);
+	}
+}
+
+static void sprd_i2c_read_bytes(struct sprd_i2c *i2c_dev, u8 *buf, u16 len)
+{
+	u32 i = 0;
+
+	for (i = 0; i < len; i++) {
+		buf[i] = readl(i2c_dev->base + I2C_RX);
+		dev_dbg(&i2c_dev->adap.dev, "read bytes[%d] = %x\n", i, buf[i]);
+	}
+}
+
+static void sprd_i2c_set_full_thld(struct sprd_i2c *i2c_dev, u32 full_thld)
+{
+	unsigned int tmp = readl(i2c_dev->base + I2C_CTL);
+
+	tmp &= ~(0xf << FIFO_AF_LVL);
+	tmp |= (full_thld << FIFO_AF_LVL);
+	writel(tmp, i2c_dev->base + I2C_CTL);
+};
+
+static void sprd_i2c_set_empty_thld(struct sprd_i2c *i2c_dev, u32 empty_thld)
+{
+	unsigned int tmp = readl(i2c_dev->base + I2C_CTL);
+
+	tmp &= ~(0xf << FIFO_AE_LVL);
+	tmp |= (empty_thld << FIFO_AE_LVL);
+	writel(tmp, i2c_dev->base + I2C_CTL);
+};
+
+static void sprd_i2c_set_fifo_full_int(struct sprd_i2c *i2c_dev, int enable)
+{
+	unsigned int tmp = readl(i2c_dev->base + I2C_CTL);
+
+	if (enable)
+		tmp |= FULL_INTEN;
+	else
+		tmp &= ~FULL_INTEN;
+
+	writel(tmp, i2c_dev->base + I2C_CTL);
+};
+
+static void sprd_i2c_set_fifo_empty_int(struct sprd_i2c *i2c_dev, int enable)
+{
+	unsigned int tmp = readl(i2c_dev->base + I2C_CTL);
+
+	if (enable)
+		tmp |= EMPTY_INTEN;
+	else
+		tmp &= ~EMPTY_INTEN;
+
+	writel(tmp, i2c_dev->base + I2C_CTL);
+};
+
+static void sprd_i2c_opt_start(struct sprd_i2c *i2c_dev)
+{
+	unsigned int tmp = readl(i2c_dev->base + I2C_CTL);
+
+	writel(tmp | I2C_START, i2c_dev->base + I2C_CTL);
+}
+
+static void sprd_i2c_opt_mode(struct sprd_i2c *i2c_dev, int rw)
+{
+	unsigned int cmd = readl(i2c_dev->base + I2C_CTL) & (~I2C_MODE);
+
+	writel((cmd | rw << 3), i2c_dev->base + I2C_CTL);
+}
+
+static void sprd_i2c_data_transfer(struct sprd_i2c *i2c_dev)
+{
+	u32 i2c_count = i2c_dev->count;
+	u32 need_tran = i2c_count <= I2C_FIFO_DEEP ? i2c_count : I2C_FIFO_DEEP;
+	struct i2c_msg *msg = i2c_dev->msg;
+
+	if ((msg->flags & I2C_M_RD)) {
+		sprd_i2c_read_bytes(i2c_dev, i2c_dev->buf, I2C_FIFO_FULL_THLD);
+		i2c_dev->count -= I2C_FIFO_FULL_THLD;
+		i2c_dev->buf += I2C_FIFO_FULL_THLD;
+
+		/*
+		 * If the read data count is larger than rx fifo full threshold,
+		 * we should enable the rx fifo full interrupt to read data
+		 * again.
+		 */
+		if (i2c_dev->count >= I2C_FIFO_FULL_THLD)
+			sprd_i2c_set_fifo_full_int(i2c_dev, 1);
+	} else {
+		sprd_i2c_write_bytes(i2c_dev, i2c_dev->buf, need_tran);
+		i2c_dev->buf += need_tran;
+		i2c_dev->count -= need_tran;
+
+		/*
+		 * If the write data count is arger than tx fifo depth which
+		 * means we can not write all data in one time, then we should
+		 * enable the tx fifo empty interrupt to write again.
+		 */
+		if (i2c_count > I2C_FIFO_DEEP)
+			sprd_i2c_set_fifo_empty_int(i2c_dev, 1);
+	}
+}
+
+static int sprd_i2c_handle_msg(struct i2c_adapter *i2c_adap,
+			       struct i2c_msg *msg, int is_last_msg)
+{
+	struct sprd_i2c *i2c_dev = i2c_adap->algo_data;
+
+	i2c_dev->msg = msg;
+	i2c_dev->buf = msg->buf;
+	i2c_dev->count = msg->len;
+
+	reinit_completion(&i2c_dev->complete);
+	/* Reset rx/tx fifos */
+	sprd_i2c_reset_fifo(i2c_dev);
+	/* Set device address */
+	sprd_i2c_set_devaddr(i2c_dev, msg);
+	/* Set I2C read or write bytes count */
+	sprd_i2c_set_count(i2c_dev, msg->len);
+
+	if ((msg->flags & I2C_M_RD)) {
+		/* Set read operation */
+		sprd_i2c_opt_mode(i2c_dev, 1);
+		/* Last byte transmission should excute stop operation */
+		sprd_i2c_send_stop(i2c_dev, 1);
+	} else {
+		/* Set write operation */
+		sprd_i2c_opt_mode(i2c_dev, 0);
+
+		/* Last byte transmission should excute stop operation */
+		if (is_last_msg)
+			sprd_i2c_send_stop(i2c_dev, 1);
+		else
+			sprd_i2c_send_stop(i2c_dev, 0);
+	}
+
+	/*
+	 * We should enable rx fifo full interrupt to get data when receiving
+	 * full data.
+	 */
+	if ((msg->flags & I2C_M_RD))
+		sprd_i2c_set_fifo_full_int(i2c_dev, 1);
+	else
+		sprd_i2c_data_transfer(i2c_dev);
+
+	/* Start to transmit */
+	sprd_i2c_opt_start(i2c_dev);
+
+	wait_for_completion(&i2c_dev->complete);
+
+	return i2c_dev->err;
+}
+
+static int sprd_i2c_master_xfer(struct i2c_adapter *i2c_adap,
+				struct i2c_msg *msgs, int num)
+{
+	struct sprd_i2c *i2c_dev = i2c_adap->algo_data;
+	int im, ret;
+
+	ret = pm_runtime_get_sync(i2c_dev->dev);
+	if (ret < 0)
+		return ret;
+
+	for (im = 0; im != num; im++)
+		ret = sprd_i2c_handle_msg(i2c_adap, &msgs[im], im == num - 1);
+
+	pm_runtime_mark_last_busy(i2c_dev->dev);
+	pm_runtime_put_autosuspend(i2c_dev->dev);
+
+	return (ret >= 0) ? im : ret;
+}
+
+static u32 sprd_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm sprd_i2c_algo = {
+	.master_xfer = sprd_i2c_master_xfer,
+	.functionality = sprd_i2c_func,
+};
+
+static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, unsigned int freq)
+{
+	u32 apb_clk = i2c_dev->src_clk;
+	u32 i2c_dvd = apb_clk / (4 * freq) - 1;
+	u32 high = ((i2c_dvd << 1) * 2) / 5;
+	u32 low = ((i2c_dvd << 1) * 3) / 5;
+	u32 div0 = (high & 0xffff) << 16 | (low & 0xffff);
+	u32 div1 =  (high & 0xffff0000) | ((low & 0xffff0000) >> 16);
+
+	writel(div0, i2c_dev->base + ADDR_DVD0);
+	writel(div1, i2c_dev->base + ADDR_DVD1);
+
+	if (freq == 400000)
+		writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
+	else if (freq == 100000)
+		writel((4 * apb_clk) / 1000000, i2c_dev->base + ADDR_STA0_DVD);
+}
+
+static void sprd_i2c_enable(struct sprd_i2c *i2c_dev)
+{
+	unsigned int tmp = readl(i2c_dev->base + I2C_CTL);
+
+	tmp &= ~(I2C_EN | I2C_TRIM_OPT | (0xF << FIFO_AF_LVL) |
+		 (0xF << FIFO_AE_LVL));
+	tmp |= I2C_INT_EN | I2C_DVD_OPT;
+	writel(tmp, i2c_dev->base + I2C_CTL);
+
+	/* Set rx fifo full data threshold */
+	sprd_i2c_set_full_thld(i2c_dev, I2C_FIFO_FULL_THLD);
+	/* Set tx fifo empty data threshold */
+	sprd_i2c_set_empty_thld(i2c_dev, I2C_FIFO_EMPTY_THLD);
+
+	sprd_i2c_set_clk(i2c_dev, i2c_dev->bus_freq);
+	/* Reset rx/tx fifo */
+	sprd_i2c_reset_fifo(i2c_dev);
+	sprd_i2c_clear_irq(i2c_dev);
+
+	/* Enable I2C module */
+	tmp = readl(i2c_dev->base + I2C_CTL);
+	writel(tmp | I2C_EN, i2c_dev->base + I2C_CTL);
+}
+
+static irqreturn_t sprd_i2c_isr_thread(int irq, void *dev_id)
+{
+	struct sprd_i2c *i2c_dev = dev_id;
+	struct i2c_msg *msg = i2c_dev->msg;
+	int ack = readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK;
+	u32 i2c_count = readl(i2c_dev->base + I2C_COUNT);
+	u32 i2c_tran;
+
+	if (msg->flags & I2C_M_RD)
+		i2c_tran = i2c_dev->count >= I2C_FIFO_FULL_THLD;
+	else
+		i2c_tran = i2c_count;
+
+	/*
+	 * If we got one ACK from slave when writing data, and we did not
+	 * finish this transmission (i2c_tran is not zero), then we should
+	 * continue to write data.
+	 *
+	 * For reading data, ack is always 0, if i2c_tran is not 0 which
+	 * means we still need to contine to read data from slave.
+	 */
+	if (i2c_tran && !ack) {
+		sprd_i2c_data_transfer(i2c_dev);
+		return IRQ_HANDLED;
+	}
+
+	i2c_dev->err = 0;
+
+	/*
+	 * If we did not get one ACK from slave when writing data, we should
+	 * dump all registers to check I2C status.
+	 */
+	if (ack) {
+		i2c_dev->err = -EIO;
+		sprd_i2c_dump_reg(i2c_dev);
+	} else if (msg->flags & I2C_M_RD && i2c_dev->count) {
+		sprd_i2c_read_bytes(i2c_dev, i2c_dev->buf, i2c_dev->count);
+	}
+
+	/* Transmission is done and clear ack and start operation */
+	sprd_i2c_clear_ack(i2c_dev);
+	sprd_i2c_clear_start(i2c_dev);
+	complete(&i2c_dev->complete);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t sprd_i2c_isr(int irq, void *dev_id)
+{
+	struct sprd_i2c *i2c_dev = dev_id;
+	struct i2c_msg *msg = i2c_dev->msg;
+	u32 i2c_count = readl(i2c_dev->base + I2C_COUNT);
+	int ack = readl(i2c_dev->base + I2C_STATUS) & I2C_RX_ACK;
+	u32 i2c_tran;
+
+	if (msg->flags & I2C_M_RD)
+		i2c_tran = i2c_dev->count >= I2C_FIFO_FULL_THLD;
+	else
+		i2c_tran = i2c_count;
+
+	/*
+	 * If ack == 0 means we got one ACK, if ack != 0 means we did not
+	 * get one ACK from slave when writing data, then we should finish
+	 * this transmission since we got some errors.
+	 *
+	 * When writing data, if i2c_tran == 0 which means we have writen
+	 * done all data, then we can finish this transmission.
+	 *
+	 * When reading data, if conut < rx fifo full threshold, which
+	 * means we can read all data in one time, then we can finish this
+	 * transmission too.
+	 */
+	if (!i2c_tran || ack) {
+		sprd_i2c_clear_start(i2c_dev);
+		sprd_i2c_clear_irq(i2c_dev);
+	}
+
+	/* Clear rx/tx fifo interrupts */
+	sprd_i2c_set_fifo_empty_int(i2c_dev, 0);
+	sprd_i2c_set_fifo_full_int(i2c_dev, 0);
+
+	return IRQ_WAKE_THREAD;
+}
+
+static int sprd_i2c_clk_init(struct sprd_i2c *i2c_dev)
+{
+	struct clk *clk_i2c, *clk_parent;
+	struct device_node *np = i2c_dev->adap.dev.of_node;
+
+	clk_i2c = of_clk_get_by_name(np, "i2c");
+	if (IS_ERR(clk_i2c)) {
+		dev_warn(&i2c_dev->adap.dev, "i2c%d can't get the i2c clock\n",
+			 i2c_dev->adap.nr);
+		clk_i2c = NULL;
+	}
+
+	clk_parent = of_clk_get_by_name(np, "source");
+	if (IS_ERR(clk_parent)) {
+		dev_warn(&i2c_dev->adap.dev,
+			 "i2c%d can't get the source clock\n",
+			 i2c_dev->adap.nr);
+		clk_parent = NULL;
+	}
+
+	if (clk_set_parent(clk_i2c, clk_parent))
+		i2c_dev->src_clk = clk_get_rate(clk_i2c);
+	else
+		i2c_dev->src_clk = 26000000;
+
+	dev_dbg(&i2c_dev->adap.dev, "i2c%d set source clock is %d\n",
+		i2c_dev->adap.nr, i2c_dev->src_clk);
+
+	i2c_dev->clk = of_clk_get_by_name(np, "enable");
+	if (IS_ERR(i2c_dev->clk)) {
+		dev_warn(&i2c_dev->adap.dev,
+			 "i2c%d can't get the enable clock\n",
+			 i2c_dev->adap.nr);
+		i2c_dev->clk = NULL;
+	}
+
+	return 0;
+}
+
+static int sprd_i2c_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sprd_i2c *i2c_dev;
+	struct resource *res;
+	u32 prop;
+	int ret;
+
+	pdev->id = of_alias_get_id(dev->of_node, "i2c");
+
+	i2c_dev = devm_kzalloc(dev, sizeof(struct sprd_i2c), GFP_KERNEL);
+	if (!i2c_dev)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	i2c_dev->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(i2c_dev->base))
+		return PTR_ERR(i2c_dev->base);
+
+	i2c_dev->irq = platform_get_irq(pdev, 0);
+	if (i2c_dev->irq < 0) {
+		dev_err(&pdev->dev, "failed to get irq resource\n");
+		return i2c_dev->irq;
+	}
+
+	i2c_set_adapdata(&i2c_dev->adap, i2c_dev);
+	init_completion(&i2c_dev->complete);
+	snprintf(i2c_dev->adap.name, sizeof(i2c_dev->adap.name),
+		 "%s", "sprd-i2c");
+
+	i2c_dev->bus_freq = 100000;
+	i2c_dev->adap.owner = THIS_MODULE;
+	i2c_dev->dev = dev;
+	i2c_dev->adap.retries = 3;
+	i2c_dev->adap.algo = &sprd_i2c_algo;
+	i2c_dev->adap.algo_data = i2c_dev;
+	i2c_dev->adap.dev.parent = dev;
+	i2c_dev->adap.nr = pdev->id;
+	i2c_dev->adap.dev.of_node = dev->of_node;
+
+	if (!of_property_read_u32(dev->of_node, "clock-frequency", &prop))
+		i2c_dev->bus_freq = prop;
+
+	sprd_i2c_clk_init(i2c_dev);
+	platform_set_drvdata(pdev, i2c_dev);
+
+	ret = clk_prepare_enable(i2c_dev->clk);
+	if (ret)
+		return ret;
+
+	sprd_i2c_enable(i2c_dev);
+
+	pm_runtime_set_autosuspend_delay(i2c_dev->dev, SPRD_I2C_PM_TIMEOUT);
+	pm_runtime_use_autosuspend(i2c_dev->dev);
+	pm_runtime_set_active(i2c_dev->dev);
+	pm_runtime_enable(i2c_dev->dev);
+
+	ret = pm_runtime_get_sync(i2c_dev->dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "i2c%d pm runtime resume failed!\n",
+			pdev->id);
+		goto error;
+	}
+
+	ret = devm_request_threaded_irq(dev, i2c_dev->irq,
+		sprd_i2c_isr, sprd_i2c_isr_thread,
+		IRQF_NO_SUSPEND | IRQF_ONESHOT,
+		pdev->name, i2c_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to request irq %d\n", i2c_dev->irq);
+		goto error;
+	}
+
+	ret = i2c_add_numbered_adapter(&i2c_dev->adap);
+	if (ret) {
+		dev_err(&pdev->dev, "add adapter failed\n");
+		goto error;
+	}
+
+	pm_runtime_mark_last_busy(i2c_dev->dev);
+	pm_runtime_put_autosuspend(i2c_dev->dev);
+	return 0;
+
+error:
+	pm_runtime_put_noidle(i2c_dev->dev);
+	pm_runtime_disable(i2c_dev->dev);
+	clk_disable_unprepare(i2c_dev->clk);
+	return ret;
+}
+
+static int sprd_i2c_remove(struct platform_device *pdev)
+{
+	struct sprd_i2c *i2c_dev = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = pm_runtime_get_sync(i2c_dev->dev);
+	if (ret < 0)
+		return ret;
+
+	i2c_del_adapter(&i2c_dev->adap);
+	clk_disable_unprepare(i2c_dev->clk);
+
+	pm_runtime_put_noidle(i2c_dev->dev);
+	pm_runtime_disable(i2c_dev->dev);
+
+	return 0;
+}
+
+static int __maybe_unused sprd_i2c_suspend_noirq(struct device *pdev)
+{
+	return pm_runtime_force_suspend(pdev);
+}
+
+static int __maybe_unused sprd_i2c_resume_noirq(struct device *pdev)
+{
+	return pm_runtime_force_resume(pdev);
+}
+
+static int __maybe_unused sprd_i2c_runtime_suspend(struct device *pdev)
+{
+	struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
+
+	clk_disable_unprepare(i2c_dev->clk);
+
+	return 0;
+}
+
+static int __maybe_unused sprd_i2c_runtime_resume(struct device *pdev)
+{
+	struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
+	int ret;
+
+	ret = clk_prepare_enable(i2c_dev->clk);
+	if (ret)
+		return ret;
+
+	sprd_i2c_enable(i2c_dev);
+
+	return 0;
+}
+
+static const struct dev_pm_ops sprd_i2c_pm_ops = {
+	SET_RUNTIME_PM_OPS(sprd_i2c_runtime_suspend,
+			   sprd_i2c_runtime_resume, NULL)
+
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(sprd_i2c_suspend_noirq,
+				      sprd_i2c_resume_noirq)
+};
+
+static const struct of_device_id sprd_i2c_of_match[] = {
+	{ .compatible = "sprd,r8p0-i2c", },
+};
+MODULE_DEVICE_TABLE(of, sprd_i2c_of_match);
+
+static struct platform_driver sprd_i2c_driver = {
+	.probe = sprd_i2c_probe,
+	.remove = sprd_i2c_remove,
+	.driver = {
+		   .name = "sprd-i2c",
+		   .of_match_table = sprd_i2c_of_match,
+		   .pm = &sprd_i2c_pm_ops,
+	},
+};
+
+static int sprd_i2c_init(void)
+{
+	return platform_driver_register(&sprd_i2c_driver);
+}
+arch_initcall_sync(sprd_i2c_init);
+
+static void sprd_i2c_exit(void)
+{
+	platform_driver_unregister(&sprd_i2c_driver);
+}
+module_exit(sprd_i2c_exit);
+
+MODULE_DESCRIPTION("Spreadtrum I2C bus adapter driver");
+MODULE_AUTHOR("Lanqing Liu <lanqing.liu-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5

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

Re: [PATCH v2 2/2] i2c: Add Spreadtrum I2C controller driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1861 bytes --]

Hi Baolin,

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v4.12-rc6 next-20170623]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Baolin-Wang/dt-bindings-i2c-Add-Spreadtrum-I2C-controller-documentation/20170622-111344
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: arm64-allmodconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

   drivers/i2c/busses/i2c-sprd.c:686:1: warning: data definition has no type or storage class
    arch_initcall_sync(sprd_i2c_init);
    ^~~~~~~~~~~~~~~~~~
>> drivers/i2c/busses/i2c-sprd.c:686:1: error: type defaults to 'int' in declaration of 'arch_initcall_sync' [-Werror=implicit-int]
   drivers/i2c/busses/i2c-sprd.c:686:1: warning: parameter names (without types) in function declaration
   drivers/i2c/busses/i2c-sprd.c:682:12: warning: 'sprd_i2c_init' defined but not used [-Wunused-function]
    static int sprd_i2c_init(void)
               ^~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +686 drivers/i2c/busses/i2c-sprd.c

   680	};
   681	
   682	static int sprd_i2c_init(void)
   683	{
   684		return platform_driver_register(&sprd_i2c_driver);
   685	}
 > 686	arch_initcall_sync(sprd_i2c_init);
   687	
   688	static void sprd_i2c_exit(void)
   689	{

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 55830 bytes --]

Re: [PATCH v2 2/2] i2c: Add Spreadtrum I2C controller driver

[Andy Shevchenko]

On Wed, Jun 21, 2017 at 10:23 AM, Baolin Wang
<baolin.wang@spreadtrum.com> wrote:
> This patch adds the I2C controller driver for Spreadtrum platform.

Needs more work.
See my comments below.



> +#include <linux/init.h>

> +#include <linux/module.h>

Since your answer to the comment about arch_initcall you perhaps need
to get rid of tristate (use bool) and drop module.h here and all
leftovers like MODULE_*() calls.

> +#include <linux/slab.h>

Should we include this still explicitly (after kernel.h)?

> +
> +#define I2C_CTL                        0x0
> +#define I2C_ADDR_CFG           0x4
> +#define I2C_COUNT              0x8
> +#define I2C_RX                 0xC
> +#define I2C_TX                 0x10
> +#define I2C_STATUS             0x14
> +#define I2C_HSMODE_CFG         0x18
> +#define I2C_VERSION            0x1C
> +#define ADDR_DVD0              0x20
> +#define ADDR_DVD1              0x24
> +#define ADDR_STA0_DVD          0x28
> +#define ADDR_RST               0x2C

1. Please, use fixed sized values
0x00 and so on.
2. Please, low case.

> +#define SPRD_I2C_TIMEOUT       (msecs_to_jiffies(1000))

Redundant parens.

> +static void sprd_i2c_dump_reg(struct sprd_i2c *i2c_dev)

If you switch your driver to regmap API, you may drop this function completely.

> +               writel(tmp & (~STP_EN), i2c_dev->base + I2C_CTL);

Redundant parens.
Also pay attention to other similar places.


> +static void sprd_i2c_write_bytes(struct sprd_i2c *i2c_dev, u8 *buf, u32 len)

So, isn't better to provide a writesb(), readsb() to ia64 instead of
doing below?

> +{
> +       u32 i = 0;
> +
> +       for (i = 0; i < len; i++) {
> +               writel(buf[i], i2c_dev->base + I2C_TX);

> +               dev_dbg(&i2c_dev->adap.dev, "write bytes[%d] = %x\n", i, buf[i]);

Moreover, don't waste lines in the kernel log buffer by doing this.
Choose either

%*ph (up to 64 bytes)

or

print_hex_dump()

(Same for _read_bytes() below)

> +       }
> +}

> +static void sprd_i2c_set_full_thld(struct sprd_i2c *i2c_dev, u32 full_thld)
> +{
> +       unsigned int tmp = readl(i2c_dev->base + I2C_CTL);
> +
> +       tmp &= ~(0xf << FIFO_AF_LVL);

Magic.
Define it using GENMASK()

> +       tmp |= (full_thld << FIFO_AF_LVL);

Redundant parens.

> +       tmp &= ~(0xf << FIFO_AE_LVL);
> +       tmp |= (empty_thld << FIFO_AE_LVL);

Same.

> +static void sprd_i2c_opt_mode(struct sprd_i2c *i2c_dev, int rw)
> +{
> +       unsigned int cmd = readl(i2c_dev->base + I2C_CTL) & (~I2C_MODE);

Redundant parens.

> +       writel((cmd | rw << 3), i2c_dev->base + I2C_CTL);

Ditto.

It's a C, and not a LISP :-)

> +}

> +static void sprd_i2c_data_transfer(struct sprd_i2c *i2c_dev)
> +{

> +       if ((msg->flags & I2C_M_RD)) {

Redundant parens.

> +       /* Reset rx/tx fifos */

Noise.

> +       /* Set device address */

Ditto.

> +       /* Set I2C read or write bytes count */

Ditto.

> +       if ((msg->flags & I2C_M_RD)) {
> +               /* Set read operation */
> +               sprd_i2c_opt_mode(i2c_dev, 1);
> +               /* Last byte transmission should excute stop operation */
> +               sprd_i2c_send_stop(i2c_dev, 1);

Same comments as above.

> +       } else {

> +               /* Set write operation */

Noise.

> +               /* Last byte transmission should excute stop operation */
> +               if (is_last_msg)
> +                       sprd_i2c_send_stop(i2c_dev, 1);
> +               else
> +                       sprd_i2c_send_stop(i2c_dev, 0);

    sprd_i2c_send_stop(i2c_dev, !!is_last_msg);

Though, consider to make is_last_msg boolean.

> +static int sprd_i2c_master_xfer(struct i2c_adapter *i2c_adap,
> +                               struct i2c_msg *msgs, int num)
> +{
> +       struct sprd_i2c *i2c_dev = i2c_adap->algo_data;
> +       int im, ret;
> +
> +       ret = pm_runtime_get_sync(i2c_dev->dev);
> +       if (ret < 0)
> +               return ret;
> +

> +       for (im = 0; im != num; im++)

im < num - 1, and...

        ret = sprd_i2c_handle_msg(i2c_adap, &msgs[im], 0);
... ret = sprd_i2c_handle_msg(i2c_adap, &msgs[im++], 1);

...and we clearly see that you have messed up with returned code on
each itteration here

> +       pm_runtime_mark_last_busy(i2c_dev->dev);
> +       pm_runtime_put_autosuspend(i2c_dev->dev);


> +       return (ret >= 0) ? im : ret;

Usual pattern is
ret < 0 ? ret : im;


> +static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, unsigned int freq)
> +{
> +       u32 apb_clk = i2c_dev->src_clk;
> +       u32 i2c_dvd = apb_clk / (4 * freq) - 1;
> +       u32 high = ((i2c_dvd << 1) * 2) / 5;
> +       u32 low = ((i2c_dvd << 1) * 3) / 5;

> +       u32 div0 = (high & 0xffff) << 16 | (low & 0xffff);
> +       u32 div1 =  (high & 0xffff0000) | ((low & 0xffff0000) >> 16);

Magic masks all over.

Also it needs a comment what formula is used and how.

> +
> +       writel(div0, i2c_dev->base + ADDR_DVD0);
> +       writel(div1, i2c_dev->base + ADDR_DVD1);
> +

> +       if (freq == 400000)
> +               writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
> +       else if (freq == 100000)
> +               writel((4 * apb_clk) / 1000000, i2c_dev->base + ADDR_STA0_DVD);

What about 3400000?

What about the rest of the speeds, shouldn't you return an error from here?

> +}
> +
> +static void sprd_i2c_enable(struct sprd_i2c *i2c_dev)
> +{
> +       unsigned int tmp = readl(i2c_dev->base + I2C_CTL);
> +
> +       tmp &= ~(I2C_EN | I2C_TRIM_OPT | (0xF << FIFO_AF_LVL) |
> +                (0xF << FIFO_AE_LVL));

Magic masks (I saw them already above).


> +       /* Set rx fifo full data threshold */

Drop noise comments. They don't bring any value since you have nice
function names.

> +       sprd_i2c_set_full_thld(i2c_dev, I2C_FIFO_FULL_THLD);
> +       /* Set tx fifo empty data threshold */
> +       sprd_i2c_set_empty_thld(i2c_dev, I2C_FIFO_EMPTY_THLD);
> +
> +       sprd_i2c_set_clk(i2c_dev, i2c_dev->bus_freq);
> +       /* Reset rx/tx fifo */
> +       sprd_i2c_reset_fifo(i2c_dev);
> +       sprd_i2c_clear_irq(i2c_dev);

> +static int sprd_i2c_clk_init(struct sprd_i2c *i2c_dev)
> +{
> +       struct clk *clk_i2c, *clk_parent;
> +       struct device_node *np = i2c_dev->adap.dev.of_node;
> +

> +       clk_i2c = of_clk_get_by_name(np, "i2c");

What the issue to use resource agnostic API here, i.e. devm_clk_get() ?

> +       clk_parent = of_clk_get_by_name(np, "source");

Ditto.

> +       i2c_dev->clk = of_clk_get_by_name(np, "enable");

Ditto.

> +       if (!of_property_read_u32(dev->of_node, "clock-frequency", &prop))
> +               i2c_dev->bus_freq = prop;
> +
> +       sprd_i2c_clk_init(i2c_dev);
> +       platform_set_drvdata(pdev, i2c_dev);
> +
> +       ret = clk_prepare_enable(i2c_dev->clk);
> +       if (ret)
> +               return ret;
> +
> +       sprd_i2c_enable(i2c_dev);
> +

> +error:

I would put it as

err_rpm_put:

> +       pm_runtime_put_noidle(i2c_dev->dev);
> +       pm_runtime_disable(i2c_dev->dev);
> +       clk_disable_unprepare(i2c_dev->clk);
> +       return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] i2c: Add Spreadtrum I2C controller driver

[Baolin Wang]

Hi Andy,

On 日,  6月 25, 2017 at 06:06:44下午 +0300, Andy Shevchenko wrote:
> On Wed, Jun 21, 2017 at 10:23 AM, Baolin Wang
> <baolin.wang-lxIno14LUO0EEoCn2XhGlw@public.gmane.org> wrote:
> > This patch adds the I2C controller driver for Spreadtrum platform.
> 
> Needs more work.
> See my comments below.
> 
> 
> 
> > +#include <linux/init.h>
> 
> > +#include <linux/module.h>
> 
> Since your answer to the comment about arch_initcall you perhaps need
> to get rid of tristate (use bool) and drop module.h here and all
> leftovers like MODULE_*() calls.

Will remove them in next version.

> 
> > +#include <linux/slab.h>
> 
> Should we include this still explicitly (after kernel.h)?

Will remove it.

> 
> > +
> > +#define I2C_CTL                        0x0
> > +#define I2C_ADDR_CFG           0x4
> > +#define I2C_COUNT              0x8
> > +#define I2C_RX                 0xC
> > +#define I2C_TX                 0x10
> > +#define I2C_STATUS             0x14
> > +#define I2C_HSMODE_CFG         0x18
> > +#define I2C_VERSION            0x1C
> > +#define ADDR_DVD0              0x20
> > +#define ADDR_DVD1              0x24
> > +#define ADDR_STA0_DVD          0x28
> > +#define ADDR_RST               0x2C
> 
> 1. Please, use fixed sized values
> 0x00 and so on.
> 2. Please, low case.

OK.

> 
> > +#define SPRD_I2C_TIMEOUT       (msecs_to_jiffies(1000))
> 
> Redundant parens.

OK.

> 
> > +static void sprd_i2c_dump_reg(struct sprd_i2c *i2c_dev)
> 
> If you switch your driver to regmap API, you may drop this function completely.

Now we have no use to switch i2c driver to regmap API and we really need
these registers info to debug I2C driver when we met something wrong. So
I think I should still keep this function for debugging.

> 
> > +               writel(tmp & (~STP_EN), i2c_dev->base + I2C_CTL);
> 
> Redundant parens.
> Also pay attention to other similar places.

OK, Will check the whole file.

> 
> 
> > +static void sprd_i2c_write_bytes(struct sprd_i2c *i2c_dev, u8 *buf, u32 len)
> 
> So, isn't better to provide a writesb(), readsb() to ia64 instead of
> doing below?

When I change to writesb/readsb, it will compile failed on my x86 platform.
So I guess I should still keep writeb/readb now.

> 
> > +{
> > +       u32 i = 0;
> > +
> > +       for (i = 0; i < len; i++) {
> > +               writel(buf[i], i2c_dev->base + I2C_TX);
> 
> > +               dev_dbg(&i2c_dev->adap.dev, "write bytes[%d] = %x\n", i, buf[i]);
> 
> Moreover, don't waste lines in the kernel log buffer by doing this.
> Choose either
> 
> %*ph (up to 64 bytes)
> 
> or
> 
> print_hex_dump()
> 
> (Same for _read_bytes() below)

OK.

> 
> > +       }
> > +}
> 
> > +static void sprd_i2c_set_full_thld(struct sprd_i2c *i2c_dev, u32 full_thld)
> > +{
> > +       unsigned int tmp = readl(i2c_dev->base + I2C_CTL);
> > +
> > +       tmp &= ~(0xf << FIFO_AF_LVL);
> 
> Magic.
> Define it using GENMASK()

OK. Will use GENMASK instead of magic number.

> 
> > +       tmp |= (full_thld << FIFO_AF_LVL);
> 
> Redundant parens.

OK.

> 
> > +       tmp &= ~(0xf << FIFO_AE_LVL);
> > +       tmp |= (empty_thld << FIFO_AE_LVL);
> 
> Same.

OK.

> 
> > +static void sprd_i2c_opt_mode(struct sprd_i2c *i2c_dev, int rw)
> > +{
> > +       unsigned int cmd = readl(i2c_dev->base + I2C_CTL) & (~I2C_MODE);
> 
> Redundant parens.

OK.

> 
> > +       writel((cmd | rw << 3), i2c_dev->base + I2C_CTL);
> 
> Ditto.
> 
> It's a C, and not a LISP :-)

OK.

> 
> > +}
> 
> > +static void sprd_i2c_data_transfer(struct sprd_i2c *i2c_dev)
> > +{
> 
> > +       if ((msg->flags & I2C_M_RD)) {
> 
> Redundant parens.

Will remove it.

> 
> > +       /* Reset rx/tx fifos */
> 
> Noise.
> 
> > +       /* Set device address */
> 
> Ditto.
> 
> > +       /* Set I2C read or write bytes count */
> 
> Ditto.
> 
> > +       if ((msg->flags & I2C_M_RD)) {
> > +               /* Set read operation */
> > +               sprd_i2c_opt_mode(i2c_dev, 1);
> > +               /* Last byte transmission should excute stop operation */
> > +               sprd_i2c_send_stop(i2c_dev, 1);
> 
> Same comments as above.

Will remove theese reduandant comments.

> 
> > +       } else {
> 
> > +               /* Set write operation */
> 
> Noise.
> 
> > +               /* Last byte transmission should excute stop operation */
> > +               if (is_last_msg)
> > +                       sprd_i2c_send_stop(i2c_dev, 1);
> > +               else
> > +                       sprd_i2c_send_stop(i2c_dev, 0);
> 
>     sprd_i2c_send_stop(i2c_dev, !!is_last_msg);
> 
> Though, consider to make is_last_msg boolean.

OK.

> 
> > +static int sprd_i2c_master_xfer(struct i2c_adapter *i2c_adap,
> > +                               struct i2c_msg *msgs, int num)
> > +{
> > +       struct sprd_i2c *i2c_dev = i2c_adap->algo_data;
> > +       int im, ret;
> > +
> > +       ret = pm_runtime_get_sync(i2c_dev->dev);
> > +       if (ret < 0)
> > +               return ret;
> > +
> 
> > +       for (im = 0; im != num; im++)
> 
> im < num - 1, and...
> 
>         ret = sprd_i2c_handle_msg(i2c_adap, &msgs[im], 0);
> ... ret = sprd_i2c_handle_msg(i2c_adap, &msgs[im++], 1);
> 
> ...and we clearly see that you have messed up with returned code on
> each itteration here

Yes, will re-check and modify this loop.

> 
> > +       pm_runtime_mark_last_busy(i2c_dev->dev);
> > +       pm_runtime_put_autosuspend(i2c_dev->dev);
> 
> 
> > +       return (ret >= 0) ? im : ret;
> 
> Usual pattern is
> ret < 0 ? ret : im;

OK.

> 
> 
> > +static void sprd_i2c_set_clk(struct sprd_i2c *i2c_dev, unsigned int freq)
> > +{
> > +       u32 apb_clk = i2c_dev->src_clk;
> > +       u32 i2c_dvd = apb_clk / (4 * freq) - 1;
> > +       u32 high = ((i2c_dvd << 1) * 2) / 5;
> > +       u32 low = ((i2c_dvd << 1) * 3) / 5;
> 
> > +       u32 div0 = (high & 0xffff) << 16 | (low & 0xffff);
> > +       u32 div1 =  (high & 0xffff0000) | ((low & 0xffff0000) >> 16);
> 
> Magic masks all over.
> 
> Also it needs a comment what formula is used and how.

Will add comments to explain the formula.

> 
> > +
> > +       writel(div0, i2c_dev->base + ADDR_DVD0);
> > +       writel(div1, i2c_dev->base + ADDR_DVD1);
> > +
> 
> > +       if (freq == 400000)
> > +               writel((6 * apb_clk) / 10000000, i2c_dev->base + ADDR_STA0_DVD);
> > +       else if (freq == 100000)
> > +               writel((4 * apb_clk) / 1000000, i2c_dev->base + ADDR_STA0_DVD);
> 
> What about 3400000?
> 
> What about the rest of the speeds, shouldn't you return an error from here?

Now we only support 100000 and 400000, otherwise will be error. I will check this
in probe() function.

> 
> > +}
> > +
> > +static void sprd_i2c_enable(struct sprd_i2c *i2c_dev)
> > +{
> > +       unsigned int tmp = readl(i2c_dev->base + I2C_CTL);
> > +
> > +       tmp &= ~(I2C_EN | I2C_TRIM_OPT | (0xF << FIFO_AF_LVL) |
> > +                (0xF << FIFO_AE_LVL));
> 
> Magic masks (I saw them already above).

OK.

> 
> 
> > +       /* Set rx fifo full data threshold */
> 
> Drop noise comments. They don't bring any value since you have nice
> function names.

OK.

> 
> > +       sprd_i2c_set_full_thld(i2c_dev, I2C_FIFO_FULL_THLD);
> > +       /* Set tx fifo empty data threshold */
> > +       sprd_i2c_set_empty_thld(i2c_dev, I2C_FIFO_EMPTY_THLD);
> > +
> > +       sprd_i2c_set_clk(i2c_dev, i2c_dev->bus_freq);
> > +       /* Reset rx/tx fifo */
> > +       sprd_i2c_reset_fifo(i2c_dev);
> > +       sprd_i2c_clear_irq(i2c_dev);
> 
> > +static int sprd_i2c_clk_init(struct sprd_i2c *i2c_dev)
> > +{
> > +       struct clk *clk_i2c, *clk_parent;
> > +       struct device_node *np = i2c_dev->adap.dev.of_node;
> > +
> 
> > +       clk_i2c = of_clk_get_by_name(np, "i2c");
> 
> What the issue to use resource agnostic API here, i.e. devm_clk_get() ?

You are right, and will replace with devm_clk_get().

> 
> > +       clk_parent = of_clk_get_by_name(np, "source");
> 
> Ditto.
> 
> > +       i2c_dev->clk = of_clk_get_by_name(np, "enable");
> 
> Ditto.
> 
> > +       if (!of_property_read_u32(dev->of_node, "clock-frequency", &prop))
> > +               i2c_dev->bus_freq = prop;
> > +
> > +       sprd_i2c_clk_init(i2c_dev);
> > +       platform_set_drvdata(pdev, i2c_dev);
> > +
> > +       ret = clk_prepare_enable(i2c_dev->clk);
> > +       if (ret)
> > +               return ret;
> > +
> > +       sprd_i2c_enable(i2c_dev);
> > +
> 
> > +error:
> 
> I would put it as
> 
> err_rpm_put:

OK. Very appreciate for your helpful comments. Thanks a lot.

> 
> > +       pm_runtime_put_noidle(i2c_dev->dev);
> > +       pm_runtime_disable(i2c_dev->dev);
> > +       clk_disable_unprepare(i2c_dev->clk);
> > +       return ret;
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] dt-bindings: i2c: Add Spreadtrum I2C controller documentation

[Rob Herring]

On Wed, Jun 21, 2017 at 03:23:03PM +0800, Baolin Wang wrote:
> This patch adds the binding documentation for Spreadtrum I2C
> controller device.
> 
> Signed-off-by: Baolin Wang <baolin.wang@spreadtrum.com>
> ---
> Changes since v1:
>  - No updates.
> ---
>  Documentation/devicetree/bindings/i2c/i2c-sprd.txt |   31 ++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-sprd.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-sprd.txt b/Documentation/devicetree/bindings/i2c/i2c-sprd.txt
> new file mode 100644
> index 0000000..f285e5b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-sprd.txt
> @@ -0,0 +1,31 @@
> +I2C for Spreadtrum platforms
> +
> +Required properties:
> +- compatible: Should be "sprd,r8p0-i2c".

Compatible strings should be SoC specific.

> +- reg: Specify the physical base address of the controller and length
> +  of memory mapped region.
> +- interrupts: Should contain I2C interrupt.
> +- clock-names: Should contain following entries:
> +  "i2c" for I2C clock,
> +  "source" for I2C source (parent) clock,
> +  "enable" for I2C module enable clock.
> +- clocks: Should contain a clock specifier for each entry in clock-names.
> +- clock-frequency: Constains desired I2C bus clock frequency in Hz.
> +- #address-cells: Should be 1 to describe address cells for I2C device address.
> +- #size-cells: Should be 0 means no size cell for I2C device address.
> +
> +Optional properties:
> +- Child nodes conforming to I2C bus binding
> +
> +Examples:
> +i2c0: i2c@70500000 {
> +	compatible = "sprd,r8p0-i2c";
> +	reg = <0 0x70500000 0 0x1000>;
> +	interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> +	clock-names = "i2c", "source", "enable";
> +	clocks = <&clk_i2c3>, <&ext_26m>, <&clk_ap_apb_gates 11>;
> +	clock-frequency = <400000>;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +};
> +
> -- 
> 1.7.9.5
> 

Re: [PATCH v2 1/2] dt-bindings: i2c: Add Spreadtrum I2C controller documentation

[Baolin Wang]

Hi Rob,

On 五,  6月 23, 2017 at 05:20:16下午 -0500, Rob Herring wrote:
> On Wed, Jun 21, 2017 at 03:23:03PM +0800, Baolin Wang wrote:
> > This patch adds the binding documentation for Spreadtrum I2C
> > controller device.
> > 
> > Signed-off-by: Baolin Wang <baolin.wang-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
> > ---
> > Changes since v1:
> >  - No updates.
> > ---
> >  Documentation/devicetree/bindings/i2c/i2c-sprd.txt |   31 ++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-sprd.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-sprd.txt b/Documentation/devicetree/bindings/i2c/i2c-sprd.txt
> > new file mode 100644
> > index 0000000..f285e5b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-sprd.txt
> > @@ -0,0 +1,31 @@
> > +I2C for Spreadtrum platforms
> > +
> > +Required properties:
> > +- compatible: Should be "sprd,r8p0-i2c".
> 
> Compatible strings should be SoC specific.

Will fix this in next version. Thanks.

> 
> > +- reg: Specify the physical base address of the controller and length
> > +  of memory mapped region.
> > +- interrupts: Should contain I2C interrupt.
> > +- clock-names: Should contain following entries:
> > +  "i2c" for I2C clock,
> > +  "source" for I2C source (parent) clock,
> > +  "enable" for I2C module enable clock.
> > +- clocks: Should contain a clock specifier for each entry in clock-names.
> > +- clock-frequency: Constains desired I2C bus clock frequency in Hz.
> > +- #address-cells: Should be 1 to describe address cells for I2C device address.
> > +- #size-cells: Should be 0 means no size cell for I2C device address.
> > +
> > +Optional properties:
> > +- Child nodes conforming to I2C bus binding
> > +
> > +Examples:
> > +i2c0: i2c@70500000 {
> > +	compatible = "sprd,r8p0-i2c";
> > +	reg = <0 0x70500000 0 0x1000>;
> > +	interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> > +	clock-names = "i2c", "source", "enable";
> > +	clocks = <&clk_i2c3>, <&ext_26m>, <&clk_ap_apb_gates 11>;
> > +	clock-frequency = <400000>;
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +};
> > +
> > -- 
> > 1.7.9.5
> > 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html