[PATCH v3 0/2] i2c: Add support for Qcom CCI I2C controller

[PATCH v3 0/2] i2c: Add support for Qcom CCI I2C controller

[Vinod Koul]

Hi,

This series adds support for Qualcomm Camera Control Interface (CCI) I2C
controller. This block contains one/two I2C masters and registers these
adapters based on the controller probed on.

Changes in v3:
 - Update DT binding per RobH comments
 - Add the subnode for bus instances
 - Move all compataible data to driver data
 - add num_master data and use that to register and use masters

Changes in v2:
 - Add support for two adapters

Todor Tomov (2):
  dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller
  i2c: Add Qualcomm CCI I2C driver

 .../devicetree/bindings/i2c/i2c-qcom-cci.txt       |  55 ++
 drivers/i2c/busses/Kconfig                         |  10 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-qcom-cci.c                  | 814 +++++++++++++++++++++
 4 files changed, 880 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
 create mode 100644 drivers/i2c/busses/i2c-qcom-cci.c

-- 
2.14.4

[PATCH v3 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller

[Vinod Koul]

From: Todor Tomov <todor.tomov@linaro.org>

Add DT binding document for Qualcomm Camera Control Interface (CCI)
I2C controller.

Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 .../devicetree/bindings/i2c/i2c-qcom-cci.txt       | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
new file mode 100644
index 000000000000..977978dd4444
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
@@ -0,0 +1,55 @@
+Qualcomm Camera Control Interface (CCI) I2C controller
+
+Required properties:
+ - compatible: Should be one of:
+   - "qcom,cci-v1.0.8" for 8916;
+   - "qcom,cci-v1.4.0" for 8996.
+ - #address-cells: Should be <1>.
+ - #size-cells: Should be <0>.
+ - reg: Base address of the I2C controller and length of memory mapped region.
+ - interrupts: Specifier for CCI I2C interrupt.
+ - clocks: List of clock specifiers, one for each entry in clock-names.
+ - clock-names: Should contain:
+   - "mmss_mmagic_ahb" - on qcom,cci-v1.4.0 only;
+   - "camss_top_ahb";
+   - "cci_ahb";
+   - "cci";
+   - "camss_ahb".
+
+Required subnodes:
+ - i2c-bus instances:
+	 Mandatory i2c-bus0 subnode
+	 Mandatory i2c-bus1 for qcom,cci-v1.4.0
+ - Optional properties:
+	- clock-frequency: Desired I2C bus clock frequency
+	  in Hz, defaults to 100 kHz if omitted.
+
+Required properties on qcom,cci-v1.4.0:
+ - power-domains: Power domain specifier.
+
+Example:
+
+                cci@a0c000 {
+                        compatible = "qcom,cci-v1.4.0";
+                        #address-cells = <1>;
+                        #size-cells = <0>;
+                        reg = <0xa0c000 0x1000>;
+                        interrupts = <GIC_SPI 295 IRQ_TYPE_EDGE_RISING>;
+                        power-domains = <&mmcc CAMSS_GDSC>;
+                        clocks = <&mmcc MMSS_MMAGIC_AHB_CLK>,
+                                        <&mmcc CAMSS_TOP_AHB_CLK>,
+                                        <&mmcc CAMSS_CCI_AHB_CLK>,
+                                        <&mmcc CAMSS_CCI_CLK>,
+                                        <&mmcc CAMSS_AHB_CLK>;
+                        clock-names = "mmss_mmagic_ahb",
+                                        "camss_top_ahb",
+                                        "cci_ahb",
+                                        "cci",
+                                        "camss_ahb";
+			i2c-bus0 {
+	                        clock-frequency = <400000>;
+			};
+			i2c-bus1 {
+				clock-frequency = <400000>;
+			};
+                };
-- 
2.14.4

[PATCH v3 2/2] i2c: Add Qualcomm CCI I2C driver

[Vinod Koul]

From: Todor Tomov <todor.tomov@linaro.org>

This commit adds I2C driver for the Camera Control Interface
(CCI) I2C controller found on the Qualcomm SoC processors.

CCI versions supported:
 - 1.0.8 - found on MSM8916/APQ8016
 - 1.4.0 - found on MSM8996/APQ8096

Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/i2c/busses/Kconfig        |  10 +
 drivers/i2c/busses/Makefile       |   1 +
 drivers/i2c/busses/i2c-qcom-cci.c | 814 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 825 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-qcom-cci.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 4f8df2ec87b1..033958bcdd4b 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -828,6 +828,16 @@ config I2C_PXA_SLAVE
 	  is necessary for systems where the PXA may be a target on the
 	  I2C bus.
 
+config I2C_QCOM_CCI
+	tristate "Qualcomm Camera Control Interface"
+	depends on ARCH_QCOM
+	help
+	  If you say yes to this option, support will be included for the
+	  built-in camera control interface on the Qualcomm SoCs.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-qcom-cci.
+
 config I2C_QUP
 	tristate "Qualcomm QUP based I2C controller"
 	depends on ARCH_QCOM
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 5a869144a0c5..d6e48ec3372a 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -83,6 +83,7 @@ obj-$(CONFIG_I2C_PNX)		+= i2c-pnx.o
 obj-$(CONFIG_I2C_PUV3)		+= i2c-puv3.o
 obj-$(CONFIG_I2C_PXA)		+= i2c-pxa.o
 obj-$(CONFIG_I2C_PXA_PCI)	+= i2c-pxa-pci.o
+obj-$(CONFIG_I2C_QCOM_CCI)	+= i2c-qcom-cci.o
 obj-$(CONFIG_I2C_QUP)		+= i2c-qup.o
 obj-$(CONFIG_I2C_RIIC)		+= i2c-riic.o
 obj-$(CONFIG_I2C_RK3X)		+= i2c-rk3x.o
diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
new file mode 100644
index 000000000000..3459acd40ab0
--- /dev/null
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -0,0 +1,814 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2012-2016, The Linux Foundation. All rights reserved.
+// Copyright (c) 2017-18 Linaro Limited.
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/i2c.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#define CCI_HW_VERSION				0x0
+#define CCI_RESET_CMD				0x004
+#define CCI_RESET_CMD_MASK			0x0f73f3f7
+#define CCI_RESET_CMD_M0_MASK			0x000003f1
+#define CCI_RESET_CMD_M1_MASK			0x0003f001
+#define CCI_QUEUE_START				0x008
+#define CCI_HALT_REQ				0x034
+#define CCI_HALT_REQ_I2C_M0_Q0Q1		BIT(0)
+#define CCI_HALT_REQ_I2C_M1_Q0Q1		BIT(1)
+
+#define CCI_I2C_Mm_SCL_CTL(m)			(0x100 + 0x100 * (m))
+#define CCI_I2C_Mm_SDA_CTL_0(m)			(0x104 + 0x100 * (m))
+#define CCI_I2C_Mm_SDA_CTL_1(m)			(0x108 + 0x100 * (m))
+#define CCI_I2C_Mm_SDA_CTL_2(m)			(0x10c + 0x100 * (m))
+#define CCI_I2C_Mm_MISC_CTL(m)			(0x110 + 0x100 * (m))
+
+#define CCI_I2C_Mm_READ_DATA(m)			(0x118 + 0x100 * (m))
+#define CCI_I2C_Mm_READ_BUF_LEVEL(m)		(0x11c + 0x100 * (m))
+#define CCI_I2C_Mm_Qn_EXEC_WORD_CNT(m, n)	(0x300 + 0x200 * (m) + 0x100 * (n))
+#define CCI_I2C_Mm_Qn_CUR_WORD_CNT(m, n)	(0x304 + 0x200 * (m) + 0x100 * (n))
+#define CCI_I2C_Mm_Qn_CUR_CMD(m, n)		(0x308 + 0x200 * (m) + 0x100 * (n))
+#define CCI_I2C_Mm_Qn_REPORT_STATUS(m, n)	(0x30c + 0x200 * (m) + 0x100 * (n))
+#define CCI_I2C_Mm_Qn_LOAD_DATA(m, n)		(0x310 + 0x200 * (m) + 0x100 * (n))
+
+#define CCI_IRQ_GLOBAL_CLEAR_CMD		0xc00
+#define CCI_IRQ_MASK_0				0xc04
+#define CCI_IRQ_MASK_0_I2C_M0_RD_DONE		BIT(0)
+#define CCI_IRQ_MASK_0_I2C_M0_Q0_REPORT		BIT(4)
+#define CCI_IRQ_MASK_0_I2C_M0_Q1_REPORT		BIT(8)
+#define CCI_IRQ_MASK_0_I2C_M1_RD_DONE		BIT(12)
+#define CCI_IRQ_MASK_0_I2C_M1_Q0_REPORT		BIT(16)
+#define CCI_IRQ_MASK_0_I2C_M1_Q1_REPORT		BIT(20)
+#define CCI_IRQ_MASK_0_RST_DONE_ACK		BIT(24)
+#define CCI_IRQ_MASK_0_I2C_M0_Q0Q1_HALT_ACK	BIT(25)
+#define CCI_IRQ_MASK_0_I2C_M1_Q0Q1_HALT_ACK	BIT(26)
+#define CCI_IRQ_MASK_0_I2C_M0_ERROR		0x18000ee6
+#define CCI_IRQ_MASK_0_I2C_M1_ERROR		0x60ee6000
+#define CCI_IRQ_CLEAR_0				0xc08
+#define CCI_IRQ_STATUS_0			0xc0c
+#define CCI_IRQ_STATUS_0_I2C_M0_RD_DONE		BIT(0)
+#define CCI_IRQ_STATUS_0_I2C_M0_Q0_REPORT	BIT(4)
+#define CCI_IRQ_STATUS_0_I2C_M0_Q1_REPORT	BIT(8)
+#define CCI_IRQ_STATUS_0_I2C_M1_RD_DONE		BIT(12)
+#define CCI_IRQ_STATUS_0_I2C_M1_Q0_REPORT	BIT(16)
+#define CCI_IRQ_STATUS_0_I2C_M1_Q1_REPORT	BIT(20)
+#define CCI_IRQ_STATUS_0_RST_DONE_ACK		BIT(24)
+#define CCI_IRQ_STATUS_0_I2C_M0_Q0Q1_HALT_ACK	BIT(25)
+#define CCI_IRQ_STATUS_0_I2C_M1_Q0Q1_HALT_ACK	BIT(26)
+#define CCI_IRQ_STATUS_0_I2C_M0_ERROR		0x18000ee6
+#define CCI_IRQ_STATUS_0_I2C_M1_ERROR		0x60ee6000
+
+#define CCI_TIMEOUT_MS 100
+#define NUM_MASTERS 2
+#define NUM_QUEUES 2
+
+/* Max number of resources + 1 for a NULL terminator */
+#define CCI_RES_MAX 6
+
+enum cci_i2c_cmd {
+	CCI_I2C_SET_PARAM = 1,
+	CCI_I2C_WAIT,
+	CCI_I2C_WAIT_SYNC,
+	CCI_I2C_WAIT_GPIO_EVENT,
+	CCI_I2C_TRIG_I2C_EVENT,
+	CCI_I2C_LOCK,
+	CCI_I2C_UNLOCK,
+	CCI_I2C_REPORT,
+	CCI_I2C_WRITE,
+	CCI_I2C_READ,
+	CCI_I2C_WRITE_DISABLE_P,
+	CCI_I2C_READ_DISABLE_P,
+};
+
+enum {
+	I2C_MODE_STANDARD,
+	I2C_MODE_FAST,
+	I2C_MODE_FAST_PLUS,
+};
+
+enum cci_i2c_queue_t {
+	QUEUE_0,
+	QUEUE_1
+};
+
+struct cci_res {
+	char *clock[CCI_RES_MAX];
+	u32 clock_rate[CCI_RES_MAX];
+};
+
+struct hw_params {
+	u16 thigh;
+	u16 tlow;
+	u16 tsu_sto;
+	u16 tsu_sta;
+	u16 thd_dat;
+	u16 thd_sta;
+	u16 tbuf;
+	u8 scl_stretch_en;
+	u16 trdhld;
+	u16 tsp;
+};
+
+struct cci_clock {
+	struct clk *clk;
+	const char *name;
+	u32 freq;
+};
+
+struct cci;
+
+struct cci_master {
+	struct i2c_adapter adap;
+	u16 master;
+	u8 mode;
+	int status;
+	bool complete_pending;
+	struct completion irq_complete;
+	struct cci *cci;
+
+};
+
+struct cci_data {
+	unsigned int num_masters;
+	struct i2c_adapter_quirks quirks;
+	u16 queue_size[NUM_QUEUES];
+	struct cci_res res;
+	struct hw_params params[3];
+};
+
+struct cci {
+	struct device *dev;
+	void __iomem *base;
+	u32 irq;
+	const struct cci_data *data;
+	struct clk_bulk_data *clock;
+	u32 *clock_freq;
+	int nclocks;
+	struct cci_master master[NUM_MASTERS];
+};
+
+/**
+ * cci_clock_set_rate() - Set clock frequency rates
+ * @nclocks: Number of clocks
+ * @clock: Clock array
+ * @clock_freq: Clock frequency rate array
+ * @dev: Device
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+static int cci_clock_set_rate(int nclocks, struct clk_bulk_data *clock,
+			      u32 *clock_freq, struct device *dev)
+{
+	int i, ret;
+	long rate;
+
+	for (i = 0; i < nclocks; i++)
+		if (clock_freq[i]) {
+			rate = clk_round_rate(clock[i].clk, clock_freq[i]);
+			if (rate < 0) {
+				dev_err(dev, "clk round rate failed: %ld\n",
+					rate);
+				return rate;
+			}
+
+			ret = clk_set_rate(clock[i].clk, clock_freq[i]);
+			if (ret < 0) {
+				dev_err(dev, "clk set rate failed: %d\n", ret);
+				return ret;
+			}
+		}
+
+	return 0;
+}
+
+static irqreturn_t cci_isr(int irq, void *dev)
+{
+	struct cci *cci = dev;
+	u32 val, reset = 0;
+
+	val = readl(cci->base + CCI_IRQ_STATUS_0);
+	writel(val, cci->base + CCI_IRQ_CLEAR_0);
+	writel(0x1, cci->base + CCI_IRQ_GLOBAL_CLEAR_CMD);
+
+	if (val & CCI_IRQ_STATUS_0_RST_DONE_ACK) {
+		if (cci->master[0].complete_pending) {
+			cci->master[0].complete_pending = false;
+			complete(&cci->master[0].irq_complete);
+		}
+
+		if (cci->master[1].complete_pending) {
+			cci->master[1].complete_pending = false;
+			complete(&cci->master[1].irq_complete);
+		}
+	}
+
+	if (val & CCI_IRQ_STATUS_0_I2C_M0_RD_DONE ||
+			val & CCI_IRQ_STATUS_0_I2C_M0_Q0_REPORT ||
+			val & CCI_IRQ_STATUS_0_I2C_M0_Q1_REPORT) {
+		cci->master[0].status = 0;
+		complete(&cci->master[0].irq_complete);
+	}
+
+	if (val & CCI_IRQ_STATUS_0_I2C_M1_RD_DONE ||
+			val & CCI_IRQ_STATUS_0_I2C_M1_Q0_REPORT ||
+			val & CCI_IRQ_STATUS_0_I2C_M1_Q1_REPORT) {
+		cci->master[1].status = 0;
+		complete(&cci->master[1].irq_complete);
+	}
+
+	if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M0_Q0Q1_HALT_ACK)) {
+		cci->master[0].complete_pending = true;
+		reset = CCI_RESET_CMD_M0_MASK;
+	}
+
+	if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M1_Q0Q1_HALT_ACK)) {
+		cci->master[1].complete_pending = true;
+		reset = CCI_RESET_CMD_M1_MASK;
+	}
+
+	if (unlikely(reset))
+		writel(reset, cci->base + CCI_RESET_CMD);
+
+	if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M0_ERROR)) {
+		dev_err_ratelimited(cci->dev, "Master 0 error 0x%08x\n", val);
+		cci->master[0].status = -EIO;
+		writel(CCI_HALT_REQ_I2C_M0_Q0Q1, cci->base + CCI_HALT_REQ);
+	}
+
+	if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M1_ERROR)) {
+		dev_err_ratelimited(cci->dev, "Master 1 error 0x%08x\n", val);
+		cci->master[1].status = -EIO;
+		writel(CCI_HALT_REQ_I2C_M1_Q0Q1, cci->base + CCI_HALT_REQ);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int cci_halt(struct cci *cci, u8 master_num)
+{
+	struct cci_master *master;
+	unsigned long time;
+	u32 val;
+
+	switch (master_num) {
+	case 0:
+		val = CCI_HALT_REQ_I2C_M0_Q0Q1;
+		master = &cci->master[0];
+		break;
+	case 1:
+		val = CCI_HALT_REQ_I2C_M1_Q0Q1;
+		master = &cci->master[1];
+		break;
+	default:
+		dev_err(cci->dev, "Invalid master:%d\n", master_num);
+		return -EINVAL;
+	}
+
+	master->complete_pending = true;
+	writel(val, cci->base + CCI_HALT_REQ);
+
+	time = wait_for_completion_timeout(&master->irq_complete,
+					   msecs_to_jiffies(CCI_TIMEOUT_MS));
+	if (!time) {
+		dev_err(cci->dev, "CCI halt timeout\n");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static int cci_reset(struct cci *cci)
+{
+	unsigned long time;
+
+	cci->master[0].complete_pending = true;
+	writel(CCI_RESET_CMD_MASK, cci->base + CCI_RESET_CMD);
+
+	time = wait_for_completion_timeout
+		(&cci->master[0].irq_complete,
+		 msecs_to_jiffies(CCI_TIMEOUT_MS));
+	if (!time) {
+		dev_err(cci->dev, "CCI reset timeout\n");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static int cci_init(struct cci *cci, const struct hw_params *hw)
+{
+	u32 val = CCI_IRQ_MASK_0_I2C_M0_RD_DONE |
+			CCI_IRQ_MASK_0_I2C_M0_Q0_REPORT |
+			CCI_IRQ_MASK_0_I2C_M0_Q1_REPORT |
+			CCI_IRQ_MASK_0_I2C_M1_RD_DONE |
+			CCI_IRQ_MASK_0_I2C_M1_Q0_REPORT |
+			CCI_IRQ_MASK_0_I2C_M1_Q1_REPORT |
+			CCI_IRQ_MASK_0_RST_DONE_ACK |
+			CCI_IRQ_MASK_0_I2C_M0_Q0Q1_HALT_ACK |
+			CCI_IRQ_MASK_0_I2C_M1_Q0Q1_HALT_ACK |
+			CCI_IRQ_MASK_0_I2C_M0_ERROR |
+			CCI_IRQ_MASK_0_I2C_M1_ERROR;
+	int i;
+
+	writel(val, cci->base + CCI_IRQ_MASK_0);
+
+	for (i = 0; i < cci->data->num_masters; i++) {
+		val = hw->thigh << 16 | hw->tlow;
+		writel(val, cci->base + CCI_I2C_Mm_SCL_CTL(i));
+
+		val = hw->tsu_sto << 16 | hw->tsu_sta;
+		writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_0(i));
+
+		val = hw->thd_dat << 16 | hw->thd_sta;
+		writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_1(i));
+
+		val = hw->tbuf;
+		writel(val, cci->base + CCI_I2C_Mm_SDA_CTL_2(i));
+
+		val = hw->scl_stretch_en << 8 | hw->trdhld << 4 | hw->tsp;
+		writel(val, cci->base + CCI_I2C_Mm_MISC_CTL(i));
+	}
+
+	return 0;
+}
+
+static int cci_run_queue(struct cci *cci, u8 master, u8 queue)
+{
+	unsigned long time;
+	u32 val;
+	int ret;
+
+	val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue));
+	writel(val, cci->base + CCI_I2C_Mm_Qn_EXEC_WORD_CNT(master, queue));
+
+	val = BIT(master * 2 + queue);
+	writel(val, cci->base + CCI_QUEUE_START);
+
+	time = wait_for_completion_timeout(&cci->master[master].irq_complete,
+					   msecs_to_jiffies(CCI_TIMEOUT_MS));
+	if (!time) {
+		dev_err(cci->dev, "master %d queue %d timeout\n",
+			master, queue);
+
+		cci_halt(cci, master);
+
+		return -ETIMEDOUT;
+	}
+
+	ret = cci->master[master].status;
+	if (ret < 0)
+		dev_err(cci->dev, "master %d queue %d error %d\n",
+			master, queue, ret);
+
+	return ret;
+}
+
+static int cci_validate_queue(struct cci *cci, u8 master, u8 queue)
+{
+	int ret = 0;
+	u32 val;
+
+	val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue));
+
+	if (val == cci->data->queue_size[queue])
+		return -EINVAL;
+
+	if (val) {
+		val = CCI_I2C_REPORT | BIT(8);
+		writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
+
+		ret = cci_run_queue(cci, master, queue);
+	}
+
+	return ret;
+}
+
+static int cci_i2c_read(struct cci *cci, u16 master,
+			u16 addr, u8 *buf, u16 len)
+{
+	u32 val, words_read, words_exp;
+	u8 queue = QUEUE_1;
+	int i, index = 0, ret;
+	bool first = 0;
+
+	/*
+	 * Call validate queue to make sure queue is empty before starting.
+	 * This is to avoid overflow / underflow of queue.
+	 */
+	ret = cci_validate_queue(cci, master, queue);
+	if (ret < 0)
+		return ret;
+
+	val = CCI_I2C_SET_PARAM | (addr & 0x7f) << 4;
+	writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
+
+	val = CCI_I2C_READ | len << 4;
+	writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
+
+	ret = cci_run_queue(cci, master, queue);
+	if (ret < 0)
+		return ret;
+
+	words_read = readl(cci->base + CCI_I2C_Mm_READ_BUF_LEVEL(master));
+	words_exp = len / 4 + 1;
+	if (words_read != words_exp) {
+		dev_err(cci->dev, "words read = %d, words expected = %d\n",
+			words_read, words_exp);
+		return -EIO;
+	}
+
+	do {
+		val = readl(cci->base + CCI_I2C_Mm_READ_DATA(master));
+
+		for (i = 0; i < 4 && index < len; i++) {
+			if (first) {
+				first = false;
+				continue;
+			}
+			buf[index++] = (val >> (i * 8)) & 0xff;
+		}
+	} while (--words_read);
+
+	return 0;
+}
+
+static int cci_i2c_write(struct cci *cci, u16 master,
+			 u16 addr, u8 *buf, u16 len)
+{
+	u8 queue = QUEUE_0;
+	u8 load[12] = { 0 };
+	int i = 0, j, ret;
+	u32 val;
+
+	/*
+	 * Call validate queue to make sure queue is empty before starting.
+	 * This is to avoid overflow / underflow of queue.
+	 */
+	ret = cci_validate_queue(cci, master, queue);
+	if (ret < 0)
+		return ret;
+
+	val = CCI_I2C_SET_PARAM | (addr & 0x7f) << 4;
+	writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
+
+	load[i++] = CCI_I2C_WRITE | len << 4;
+
+	for (j = 0; j < len; j++)
+		load[i++] = buf[j];
+
+	for (j = 0; j < i; j += 4) {
+		val = load[j];
+		val |= load[j + 1] << 8;
+		val |= load[j + 2] << 16;
+		val |= load[j + 3] << 24;
+		writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
+	}
+
+	val = CCI_I2C_REPORT | BIT(8);
+	writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
+
+	return cci_run_queue(cci, master, queue);
+}
+
+static int cci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
+{
+	struct cci_master *cci_master = i2c_get_adapdata(adap);
+	struct cci *cci = cci_master->cci;
+	int i, ret = 0;
+
+	for (i = 0; i < num; i++) {
+		if (msgs[i].flags & I2C_M_RD)
+			ret = cci_i2c_read(cci, cci_master->master,
+					   msgs[i].addr, msgs[i].buf,
+					   msgs[i].len);
+		else
+			ret = cci_i2c_write(cci, msgs[i].addr,
+					    cci_master->master,
+					    msgs[i].buf, msgs[i].len);
+
+		if (ret < 0) {
+			dev_err(cci->dev, "cci i2c xfer error %d", ret);
+			break;
+		}
+	}
+
+	if (!ret)
+		ret = num;
+
+	return ret;
+}
+
+static u32 cci_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C;
+}
+
+static const struct i2c_algorithm cci_algo = {
+	.master_xfer	= cci_xfer,
+	.functionality	= cci_func,
+};
+
+static int cci_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *of_node;
+	struct resource *r;
+	struct cci *cci;
+	char name[16];
+	int ret = 0, i;
+	u32 val;
+
+	cci = devm_kzalloc(dev, sizeof(*cci), GFP_KERNEL);
+	if (!cci)
+		return -ENOMEM;
+
+	cci->dev = dev;
+	platform_set_drvdata(pdev, cci);
+	cci->data = device_get_match_data(&pdev->dev);
+	if (!cci->data) {
+		dev_err(&pdev->dev, "Driver data is null, abort\n");
+		return -EIO;
+	}
+
+	for (i = 0; i < cci->data->num_masters; i++) {
+		cci->master[i].adap.quirks = &cci->data->quirks;
+		cci->master[i].adap.algo = &cci_algo;
+		cci->master[i].adap.dev.parent = cci->dev;
+		cci->master[i].master = i;
+		cci->master[i].cci = cci;
+		i2c_set_adapdata(&cci->master[i].adap, cci);
+		snprintf(cci->master[i].adap.name,
+			 sizeof(cci->master[i].adap.name),
+			 "Qualcomm Camera Control Interface: %d", i);
+
+		/* find the child node for i2c-bus as we are on cci node */
+		snprintf(name, sizeof(name), "i2c-bus%d", i);
+		of_node = of_get_child_by_name(dev->of_node, name);
+		if (!of_node) {
+			dev_err(dev, "couldn't find i2cbus child node\n");
+			return -EINVAL;
+		}
+		cci->master[i].adap.dev.of_node = of_node;
+
+		cci->master[i].mode = I2C_MODE_STANDARD;
+		ret = of_property_read_u32(of_node, "clock-frequency", &val);
+		if (!ret) {
+			if (val == 400000)
+				cci->master[i].mode = I2C_MODE_FAST;
+			else if (val == 1000000)
+				cci->master[i].mode = I2C_MODE_FAST_PLUS;
+		}
+
+		init_completion(&cci->master[i].irq_complete);
+	}
+
+	/* Memory */
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	cci->base = devm_ioremap_resource(dev, r);
+	if (IS_ERR(cci->base)) {
+		dev_err(dev, "could not map memory\n");
+		return PTR_ERR(cci->base);
+	}
+
+	/* Interrupt */
+
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0) {
+		dev_err(dev, "missing IRQ: %d\n", ret);
+		return ret;
+	}
+	cci->irq = ret;
+
+	ret = devm_request_irq(dev, cci->irq, cci_isr,
+			       IRQF_TRIGGER_RISING, dev_name(dev), cci);
+	if (ret < 0) {
+		dev_err(dev, "request_irq failed, ret: %d\n", ret);
+		return ret;
+	}
+
+	disable_irq(cci->irq);
+
+	/* Clocks */
+
+	cci->nclocks = 0;
+	while (cci->data->res.clock[cci->nclocks])
+		cci->nclocks++;
+
+	cci->clock = devm_kcalloc(dev, cci->nclocks,
+				  sizeof(*cci->clock), GFP_KERNEL);
+	if (!cci->clock)
+		return -ENOMEM;
+
+	cci->clock_freq = devm_kcalloc(dev, cci->nclocks,
+				       sizeof(*cci->clock_freq), GFP_KERNEL);
+	if (!cci->clock_freq)
+		return -ENOMEM;
+
+	for (i = 0; i < cci->nclocks; i++) {
+		struct clk_bulk_data *clock = &cci->clock[i];
+
+		clock->clk = devm_clk_get(dev, cci->data->res.clock[i]);
+		if (IS_ERR(clock->clk))
+			return PTR_ERR(clock->clk);
+
+		clock->id = cci->data->res.clock[i];
+		cci->clock_freq[i] = cci->data->res.clock_rate[i];
+	}
+
+	ret = cci_clock_set_rate(cci->nclocks, cci->clock,
+				 cci->clock_freq, dev);
+	if (ret < 0)
+		return ret;
+
+	ret = clk_bulk_prepare_enable(cci->nclocks, cci->clock);
+	if (ret < 0)
+		return ret;
+
+	val = readl_relaxed(cci->base + CCI_HW_VERSION);
+	dev_dbg(dev, "%s: CCI HW version = 0x%08x", __func__, val);
+
+	enable_irq(cci->irq);
+
+	ret = cci_reset(cci);
+	if (ret < 0)
+		goto error;
+
+	for (i = 0; i < cci->data->num_masters; i++) {
+		ret = cci_init(cci, &cci->data->params[cci->master[i].mode]);
+		if (ret < 0)
+			goto error;
+
+		ret = i2c_add_adapter(&cci->master[i].adap);
+		if (ret < 0)
+			goto error_i2c;
+	}
+
+	return 0;
+
+error_i2c:
+	for (; i >= 0; i--)
+		i2c_del_adapter(&cci->master[i].adap);
+error:
+	disable_irq(cci->irq);
+	clk_bulk_disable_unprepare(cci->nclocks, cci->clock);
+
+	return ret;
+}
+
+static int cci_remove(struct platform_device *pdev)
+{
+	struct cci *cci = platform_get_drvdata(pdev);
+	int i;
+
+	disable_irq(cci->irq);
+	clk_bulk_disable_unprepare(cci->nclocks, cci->clock);
+
+	for (i = 0; i < cci->data->num_masters; i++)
+		i2c_del_adapter(&cci->master[i].adap);
+
+	return 0;
+}
+
+static const struct cci_data cci_v1_0_8_data = {
+	.num_masters = 1,
+	.queue_size = { 64, 16 },
+	.quirks = {
+		.max_write_len = 10,
+		.max_read_len = 12,
+	},
+	.res = {
+		.clock = {
+			"camss_top_ahb",
+			"cci_ahb",
+			"camss_ahb",
+			"cci"
+		},
+		.clock_rate = {
+			0,
+			80000000,
+			0,
+			19200000
+		},
+	},
+	.params = {
+		{
+			/* I2C_MODE_STANDARD */
+			.thigh = 78,
+			.tlow = 114,
+			.tsu_sto = 28,
+			.tsu_sta = 28,
+			.thd_dat = 10,
+			.thd_sta = 77,
+			.tbuf = 118,
+			.scl_stretch_en = 0,
+			.trdhld = 6,
+			.tsp = 1
+		},
+		{
+			/* I2C_MODE_FAST */
+			.thigh = 20,
+			.tlow = 28,
+			.tsu_sto = 21,
+			.tsu_sta = 21,
+			.thd_dat = 13,
+			.thd_sta = 18,
+			.tbuf = 32,
+			.scl_stretch_en = 0,
+			.trdhld = 6,
+			.tsp = 3
+		},
+	},
+};
+
+static const struct cci_data cci_v1_4_0_data = {
+	.num_masters = 2,
+	.queue_size = { 64, 16 },
+	.quirks = {
+		.max_write_len = 11,
+		.max_read_len = 12,
+	},
+	.res = {
+		.clock = {
+			"camss_top_ahb",
+			"cci_ahb",
+			"camss_ahb",
+			"cci"
+		},
+		.clock_rate = {
+			0,
+			0,
+			0,
+			37500000
+		},
+	},
+	.params = {
+		{
+			/* I2C_MODE_STANDARD */
+			.thigh = 201,
+			.tlow = 174,
+			.tsu_sto = 204,
+			.tsu_sta = 231,
+			.thd_dat = 22,
+			.thd_sta = 162,
+			.tbuf = 227,
+			.scl_stretch_en = 0,
+			.trdhld = 6,
+			.tsp = 3
+		},
+		{
+			/* I2C_MODE_FAST */
+			.thigh = 38,
+			.tlow = 56,
+			.tsu_sto = 40,
+			.tsu_sta = 40,
+			.thd_dat = 22,
+			.thd_sta = 35,
+			.tbuf = 62,
+			.scl_stretch_en = 0,
+			.trdhld = 6,
+			.tsp = 3
+		},
+		{
+			/* I2C_MODE_FAST_PLUS */
+			.thigh = 16,
+			.tlow = 22,
+			.tsu_sto = 17,
+			.tsu_sta = 18,
+			.thd_dat = 16,
+			.thd_sta = 15,
+			.tbuf = 24,
+			.scl_stretch_en = 0,
+			.trdhld = 3,
+			.tsp = 3
+		}
+
+	},
+
+};
+
+static const struct of_device_id cci_dt_match[] = {
+	{ .compatible = "qcom,cci-v1.0.8", .data = &cci_v1_0_8_data },
+	{ .compatible = "qcom,cci-v1.4.0", .data = &cci_v1_4_0_data },
+	{}
+};
+MODULE_DEVICE_TABLE(of, cci_dt_match);
+
+static struct platform_driver qcom_cci_driver = {
+	.probe  = cci_probe,
+	.remove = cci_remove,
+	.driver = {
+		.name = "i2c-qcom-cci",
+		.of_match_table = cci_dt_match,
+	},
+};
+
+module_platform_driver(qcom_cci_driver);
+
+MODULE_DESCRIPTION("Qualcomm Camera Control Interface driver");
+MODULE_AUTHOR("Todor Tomov <todor.tomov@linaro.org>");
+MODULE_LICENSE("GPL v2");
-- 
2.14.4

Re: [PATCH v3 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller

[Bjorn Andersson]

On Mon 06 Aug 04:04 PDT 2018, Vinod Koul wrote:

> From: Todor Tomov <todor.tomov@linaro.org>
> 
> Add DT binding document for Qualcomm Camera Control Interface (CCI)
> I2C controller.
> 
> Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  .../devicetree/bindings/i2c/i2c-qcom-cci.txt       | 55 ++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> new file mode 100644
> index 000000000000..977978dd4444
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> @@ -0,0 +1,55 @@
> +Qualcomm Camera Control Interface (CCI) I2C controller
> +
> +Required properties:
> + - compatible: Should be one of:
> +   - "qcom,cci-v1.0.8" for 8916;
> +   - "qcom,cci-v1.4.0" for 8996.

As pointed out by Rob previously we should either use version numbers or
platform numbers, not both.

So this should either be:

	- "qcom,cci-v1.0.8"
	- "qcom,cci-v1.4.0"

or:

	- "qcom,msm8916-cci"
	- "qcom,msm8996-cci"

> + - #address-cells: Should be <1>.
> + - #size-cells: Should be <0>.
> + - reg: Base address of the I2C controller and length of memory mapped region.
> + - interrupts: Specifier for CCI I2C interrupt.
> + - clocks: List of clock specifiers, one for each entry in clock-names.
> + - clock-names: Should contain:
> +   - "mmss_mmagic_ahb" - on qcom,cci-v1.4.0 only;
> +   - "camss_top_ahb";
> +   - "cci_ahb";
> +   - "cci";
> +   - "camss_ahb".
> +
> +Required subnodes:
> + - i2c-bus instances:
> +	 Mandatory i2c-bus0 subnode
> +	 Mandatory i2c-bus1 for qcom,cci-v1.4.0

Rather than a bullet list, the subnodes are probably better described in
text. I think most versions have two masters, so in order to not have to
update this every time we add a new compatible we could probably write
this in a more generic fashion.

E.g.

Required subnodes:

The CCI provides I2C masters for one or two i2c busses, described as
subdevices named "i2c-bus0" and "i2c-bus1".

> + - Optional properties:

"Optional properties for subnodes"

> +	- clock-frequency: Desired I2C bus clock frequency
> +	  in Hz, defaults to 100 kHz if omitted.
> +
> +Required properties on qcom,cci-v1.4.0:
> + - power-domains: Power domain specifier.

Properties has to come before any subnodes in the dts, so move this
above the subnode definition as well.

> +
> +Example:
> +
> +                cci@a0c000 {
> +                        compatible = "qcom,cci-v1.4.0";
> +                        #address-cells = <1>;
> +                        #size-cells = <0>;
> +                        reg = <0xa0c000 0x1000>;
> +                        interrupts = <GIC_SPI 295 IRQ_TYPE_EDGE_RISING>;
> +                        power-domains = <&mmcc CAMSS_GDSC>;
> +                        clocks = <&mmcc MMSS_MMAGIC_AHB_CLK>,
> +                                        <&mmcc CAMSS_TOP_AHB_CLK>,
> +                                        <&mmcc CAMSS_CCI_AHB_CLK>,
> +                                        <&mmcc CAMSS_CCI_CLK>,
> +                                        <&mmcc CAMSS_AHB_CLK>;
> +                        clock-names = "mmss_mmagic_ahb",
> +                                        "camss_top_ahb",
> +                                        "cci_ahb",
> +                                        "cci",
> +                                        "camss_ahb";

Seems lines above this is indented with space and the following with
tabs.

> +			i2c-bus0 {
> +	                        clock-frequency = <400000>;
> +			};
> +			i2c-bus1 {
> +				clock-frequency = <400000>;
> +			};
> +                };

Regards,
Bjorn

Re: [PATCH v3 2/2] i2c: Add Qualcomm CCI I2C driver

[Bjorn Andersson]

On Mon 06 Aug 04:04 PDT 2018, Vinod Koul wrote:
[..]
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
[..]
> +struct hw_params {
> +	u16 thigh;
> +	u16 tlow;
> +	u16 tsu_sto;
> +	u16 tsu_sta;
> +	u16 thd_dat;
> +	u16 thd_sta;
> +	u16 tbuf;
> +	u8 scl_stretch_en;
> +	u16 trdhld;
> +	u16 tsp;
> +};
> +
> +struct cci_clock {

This is now unused.

> +	struct clk *clk;
> +	const char *name;
> +	u32 freq;
> +};
> +
> +struct cci;
> +
> +struct cci_master {
> +	struct i2c_adapter adap;
> +	u16 master;
> +	u8 mode;
> +	int status;
> +	bool complete_pending;
> +	struct completion irq_complete;
> +	struct cci *cci;
> +

Empty line.

> +};
> +
> +struct cci_data {
> +	unsigned int num_masters;
> +	struct i2c_adapter_quirks quirks;
> +	u16 queue_size[NUM_QUEUES];
> +	struct cci_res res;
> +	struct hw_params params[3];
> +};
> +
> +struct cci {
> +	struct device *dev;
> +	void __iomem *base;
> +	u32 irq;

Use unsigned int rather than a type of specific size

> +	const struct cci_data *data;
> +	struct clk_bulk_data *clock;
> +	u32 *clock_freq;
> +	int nclocks;
> +	struct cci_master master[NUM_MASTERS];
> +};
> +
> +/**
> + * cci_clock_set_rate() - Set clock frequency rates
> + * @nclocks: Number of clocks
> + * @clock: Clock array
> + * @clock_freq: Clock frequency rate array
> + * @dev: Device
> + *
> + * Return 0 on success or a negative error code otherwise
> + */
> +static int cci_clock_set_rate(int nclocks, struct clk_bulk_data *clock,
> +			      u32 *clock_freq, struct device *dev)
> +{
> +	int i, ret;
> +	long rate;
> +
> +	for (i = 0; i < nclocks; i++)

Multiline loop deserves {}

> +		if (clock_freq[i]) {
> +			rate = clk_round_rate(clock[i].clk, clock_freq[i]);
> +			if (rate < 0) {
> +				dev_err(dev, "clk round rate failed: %ld\n",
> +					rate);
> +				return rate;
> +			}
> +
> +			ret = clk_set_rate(clock[i].clk, clock_freq[i]);
> +			if (ret < 0) {
> +				dev_err(dev, "clk set rate failed: %d\n", ret);
> +				return ret;
> +			}
> +		}
> +
> +	return 0;
> +}
> +
[..]
[..]
> +static int cci_reset(struct cci *cci)
> +{
> +	unsigned long time;
> +
> +	cci->master[0].complete_pending = true;
> +	writel(CCI_RESET_CMD_MASK, cci->base + CCI_RESET_CMD);
> +
> +	time = wait_for_completion_timeout
> +		(&cci->master[0].irq_complete,
> +		 msecs_to_jiffies(CCI_TIMEOUT_MS));

Please rework the indentation of this.


Also CCI_TIMEOUT_MS is converted to jiffies in all the places, define it
in jiffies instead.

> +	if (!time) {
> +		dev_err(cci->dev, "CCI reset timeout\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
[..]
> +static int cci_validate_queue(struct cci *cci, u8 master, u8 queue)
> +{
> +	int ret = 0;
> +	u32 val;
> +
> +	val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue));
> +
> +	if (val == cci->data->queue_size[queue])
> +		return -EINVAL;
> +
> +	if (val) {
> +		val = CCI_I2C_REPORT | BIT(8);

Can we get a define (or a comment) for BIT(8) as well?

> +		writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> +
> +		ret = cci_run_queue(cci, master, queue);
> +	}
> +
> +	return ret;

Rather than wrapping the second half of the function in a check for val,
return early on !val and then you can end with return cci_run_queue()
and drop the "ret".

> +}
> +
> +static int cci_i2c_read(struct cci *cci, u16 master,
> +			u16 addr, u8 *buf, u16 len)
> +{
> +	u32 val, words_read, words_exp;
> +	u8 queue = QUEUE_1;
> +	int i, index = 0, ret;
> +	bool first = 0;

s/0/false/

> +
> +	/*
> +	 * Call validate queue to make sure queue is empty before starting.
> +	 * This is to avoid overflow / underflow of queue.
> +	 */
> +	ret = cci_validate_queue(cci, master, queue);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = CCI_I2C_SET_PARAM | (addr & 0x7f) << 4;
> +	writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> +
> +	val = CCI_I2C_READ | len << 4;
> +	writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> +
> +	ret = cci_run_queue(cci, master, queue);
> +	if (ret < 0)
> +		return ret;
> +
> +	words_read = readl(cci->base + CCI_I2C_Mm_READ_BUF_LEVEL(master));
> +	words_exp = len / 4 + 1;
> +	if (words_read != words_exp) {
> +		dev_err(cci->dev, "words read = %d, words expected = %d\n",
> +			words_read, words_exp);
> +		return -EIO;
> +	}

I thought that an i2c read could return less data than was requested...

> +
> +	do {
> +		val = readl(cci->base + CCI_I2C_Mm_READ_DATA(master));
> +
> +		for (i = 0; i < 4 && index < len; i++) {
> +			if (first) {
> +				first = false;
> +				continue;
> +			}
> +			buf[index++] = (val >> (i * 8)) & 0xff;
> +		}
> +	} while (--words_read);
> +
> +	return 0;
> +}
[..]
> +static int cci_probe(struct platform_device *pdev)
> +{
[..]
> +	/* Clocks */
> +
> +	cci->nclocks = 0;
> +	while (cci->data->res.clock[cci->nclocks])
> +		cci->nclocks++;
> +
> +	cci->clock = devm_kcalloc(dev, cci->nclocks,
> +				  sizeof(*cci->clock), GFP_KERNEL);
> +	if (!cci->clock)
> +		return -ENOMEM;
> +
> +	cci->clock_freq = devm_kcalloc(dev, cci->nclocks,

You're just using this temporarily to create a copy of the
res.clock_rate array, how about just passing the res.clock_rate into
cci_clock_set_rate() ?

> +				       sizeof(*cci->clock_freq), GFP_KERNEL);
> +	if (!cci->clock_freq)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < cci->nclocks; i++) {
> +		struct clk_bulk_data *clock = &cci->clock[i];
> +
> +		clock->clk = devm_clk_get(dev, cci->data->res.clock[i]);
> +		if (IS_ERR(clock->clk))
> +			return PTR_ERR(clock->clk);
> +
> +		clock->id = cci->data->res.clock[i];
> +		cci->clock_freq[i] = cci->data->res.clock_rate[i];
> +	}

Fill out cci->clock[*].id and call clk_bulk_get()

> +
> +	ret = cci_clock_set_rate(cci->nclocks, cci->clock,
> +				 cci->clock_freq, dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = clk_bulk_prepare_enable(cci->nclocks, cci->clock);

It seems a little bit excessive to keep the clocks on while the driver
is probed, but this could be improved in a follow up patch...

> +	if (ret < 0)
> +		return ret;
> +
> +	val = readl_relaxed(cci->base + CCI_HW_VERSION);
> +	dev_dbg(dev, "%s: CCI HW version = 0x%08x", __func__, val);
> +
> +	enable_irq(cci->irq);
> +
> +	ret = cci_reset(cci);
> +	if (ret < 0)
> +		goto error;
> +
> +	for (i = 0; i < cci->data->num_masters; i++) {
> +		ret = cci_init(cci, &cci->data->params[cci->master[i].mode]);
> +		if (ret < 0)
> +			goto error;
> +
> +		ret = i2c_add_adapter(&cci->master[i].adap);
> +		if (ret < 0)
> +			goto error_i2c;
> +	}
> +
> +	return 0;
> +
> +error_i2c:
> +	for (; i >= 0; i--)
> +		i2c_del_adapter(&cci->master[i].adap);
> +error:
> +	disable_irq(cci->irq);
> +	clk_bulk_disable_unprepare(cci->nclocks, cci->clock);
> +
> +	return ret;
> +}
> +
> +static int cci_remove(struct platform_device *pdev)
> +{
> +	struct cci *cci = platform_get_drvdata(pdev);
> +	int i;
> +
> +	disable_irq(cci->irq);
> +	clk_bulk_disable_unprepare(cci->nclocks, cci->clock);

i2c clients might be communicating with their clients until you call
i2c_del_adapter(), so better pull the resources after you have removed
the adaptor.

Maybe even a cci_halt() call before we cut the clocks?

> +
> +	for (i = 0; i < cci->data->num_masters; i++)
> +		i2c_del_adapter(&cci->master[i].adap);
> +
> +	return 0;
> +}
[..]
> +static const struct cci_data cci_v1_4_0_data = {
[..]
> +		{
> +			/* I2C_MODE_FAST_PLUS */
> +			.thigh = 16,
> +			.tlow = 22,
> +			.tsu_sto = 17,
> +			.tsu_sta = 18,
> +			.thd_dat = 16,
> +			.thd_sta = 15,
> +			.tbuf = 24,
> +			.scl_stretch_en = 0,
> +			.trdhld = 3,
> +			.tsp = 3
> +		}
> +

Empty line.

> +	},
> +
> +};

The dt binding mentions that a power domain is required for v1.4, but
there's no support for this in the driver.

Regards,
Bjorn

Re: [PATCH v3 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller

[Vinod]

On 06-08-18, 11:03, Bjorn Andersson wrote:
> On Mon 06 Aug 04:04 PDT 2018, Vinod Koul wrote:
> 
> > From: Todor Tomov <todor.tomov@linaro.org>
> > 
> > Add DT binding document for Qualcomm Camera Control Interface (CCI)
> > I2C controller.
> > 
> > Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >  .../devicetree/bindings/i2c/i2c-qcom-cci.txt       | 55 ++++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> > new file mode 100644
> > index 000000000000..977978dd4444
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> > @@ -0,0 +1,55 @@
> > +Qualcomm Camera Control Interface (CCI) I2C controller
> > +
> > +Required properties:
> > + - compatible: Should be one of:
> > +   - "qcom,cci-v1.0.8" for 8916;
> > +   - "qcom,cci-v1.4.0" for 8996.
> 
> As pointed out by Rob previously we should either use version numbers or
> platform numbers, not both.
> 
> So this should either be:
> 
> 	- "qcom,cci-v1.0.8"
> 	- "qcom,cci-v1.4.0"

ok will use this one..

> 
> or:
> 
> 	- "qcom,msm8916-cci"
> 	- "qcom,msm8996-cci"
> 
> > + - #address-cells: Should be <1>.
> > + - #size-cells: Should be <0>.
> > + - reg: Base address of the I2C controller and length of memory mapped region.
> > + - interrupts: Specifier for CCI I2C interrupt.
> > + - clocks: List of clock specifiers, one for each entry in clock-names.
> > + - clock-names: Should contain:
> > +   - "mmss_mmagic_ahb" - on qcom,cci-v1.4.0 only;
> > +   - "camss_top_ahb";
> > +   - "cci_ahb";
> > +   - "cci";
> > +   - "camss_ahb".
> > +
> > +Required subnodes:
> > + - i2c-bus instances:
> > +	 Mandatory i2c-bus0 subnode
> > +	 Mandatory i2c-bus1 for qcom,cci-v1.4.0
> 
> Rather than a bullet list, the subnodes are probably better described in
> text. I think most versions have two masters, so in order to not have to
> update this every time we add a new compatible we could probably write
> this in a more generic fashion.
> 
> E.g.
> 
> Required subnodes:
> 
> The CCI provides I2C masters for one or two i2c busses, described as
> subdevices named "i2c-bus0" and "i2c-bus1".

okay but the v1-0-8 doesn't (8916) seem to have two busses, later ones yes.

> 
> > + - Optional properties:
> 
> "Optional properties for subnodes"
> 
> > +	- clock-frequency: Desired I2C bus clock frequency
> > +	  in Hz, defaults to 100 kHz if omitted.
> > +
> > +Required properties on qcom,cci-v1.4.0:
> > + - power-domains: Power domain specifier.
> 
> Properties has to come before any subnodes in the dts, so move this
> above the subnode definition as well.

yes will do

> 
> > +
> > +Example:
> > +
> > +                cci@a0c000 {
> > +                        compatible = "qcom,cci-v1.4.0";
> > +                        #address-cells = <1>;
> > +                        #size-cells = <0>;
> > +                        reg = <0xa0c000 0x1000>;
> > +                        interrupts = <GIC_SPI 295 IRQ_TYPE_EDGE_RISING>;
> > +                        power-domains = <&mmcc CAMSS_GDSC>;
> > +                        clocks = <&mmcc MMSS_MMAGIC_AHB_CLK>,
> > +                                        <&mmcc CAMSS_TOP_AHB_CLK>,
> > +                                        <&mmcc CAMSS_CCI_AHB_CLK>,
> > +                                        <&mmcc CAMSS_CCI_CLK>,
> > +                                        <&mmcc CAMSS_AHB_CLK>;
> > +                        clock-names = "mmss_mmagic_ahb",
> > +                                        "camss_top_ahb",
> > +                                        "cci_ahb",
> > +                                        "cci",
> > +                                        "camss_ahb";
> 
> Seems lines above this is indented with space and the following with
> tabs.

will fix

Thanks for the review.
-- 
~Vinod

Re: [PATCH v3 2/2] i2c: Add Qualcomm CCI I2C driver

[Vinod]

On 06-08-18, 11:45, Bjorn Andersson wrote:
> On Mon 06 Aug 04:04 PDT 2018, Vinod Koul wrote:
> [..]
> > diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> [..]
> > +struct hw_params {
> > +	u16 thigh;
> > +	u16 tlow;
> > +	u16 tsu_sto;
> > +	u16 tsu_sta;
> > +	u16 thd_dat;
> > +	u16 thd_sta;
> > +	u16 tbuf;
> > +	u8 scl_stretch_en;
> > +	u16 trdhld;
> > +	u16 tsp;
> > +};
> > +
> > +struct cci_clock {
> 
> This is now unused.

ah yes, thanks for spotting, will remove

> 
> > +	struct clk *clk;
> > +	const char *name;
> > +	u32 freq;
> > +};
> > +
> > +struct cci;
> > +
> > +struct cci_master {
> > +	struct i2c_adapter adap;
> > +	u16 master;
> > +	u8 mode;
> > +	int status;
> > +	bool complete_pending;
> > +	struct completion irq_complete;
> > +	struct cci *cci;
> > +
> 
> Empty line.

will fix this and other style/empty lines, thanks.

> 
> > +};
> > +
> > +struct cci_data {
> > +	unsigned int num_masters;
> > +	struct i2c_adapter_quirks quirks;
> > +	u16 queue_size[NUM_QUEUES];
> > +	struct cci_res res;
> > +	struct hw_params params[3];
> > +};
> > +
> > +struct cci {
> > +	struct device *dev;
> > +	void __iomem *base;
> > +	u32 irq;
> 
> Use unsigned int rather than a type of specific size

Any specific reason for that preference?

> 
> > +	const struct cci_data *data;
> > +	struct clk_bulk_data *clock;
> > +	u32 *clock_freq;
> > +	int nclocks;
> > +	struct cci_master master[NUM_MASTERS];
> > +};
> > +
> > +/**
> > + * cci_clock_set_rate() - Set clock frequency rates
> > + * @nclocks: Number of clocks
> > + * @clock: Clock array
> > + * @clock_freq: Clock frequency rate array
> > + * @dev: Device
> > + *
> > + * Return 0 on success or a negative error code otherwise
> > + */
> > +static int cci_clock_set_rate(int nclocks, struct clk_bulk_data *clock,
> > +			      u32 *clock_freq, struct device *dev)
> > +{
> > +	int i, ret;
> > +	long rate;
> > +
> > +	for (i = 0; i < nclocks; i++)
> 
> Multiline loop deserves {}

yes

> 
> > +		if (clock_freq[i]) {
> > +			rate = clk_round_rate(clock[i].clk, clock_freq[i]);
> > +			if (rate < 0) {
> > +				dev_err(dev, "clk round rate failed: %ld\n",
> > +					rate);
> > +				return rate;
> > +			}
> > +
> > +			ret = clk_set_rate(clock[i].clk, clock_freq[i]);
> > +			if (ret < 0) {
> > +				dev_err(dev, "clk set rate failed: %d\n", ret);
> > +				return ret;
> > +			}
> > +		}
> > +
> > +	return 0;
> > +}
> > +
> [..]
> [..]
> > +static int cci_reset(struct cci *cci)
> > +{
> > +	unsigned long time;
> > +
> > +	cci->master[0].complete_pending = true;
> > +	writel(CCI_RESET_CMD_MASK, cci->base + CCI_RESET_CMD);
> > +
> > +	time = wait_for_completion_timeout
> > +		(&cci->master[0].irq_complete,
> > +		 msecs_to_jiffies(CCI_TIMEOUT_MS));
> 
> Please rework the indentation of this.

and I think we should pass master to this, this looks not correct to me.

> 
> Also CCI_TIMEOUT_MS is converted to jiffies in all the places, define it
> in jiffies instead.

will do

> 
> > +	if (!time) {
> > +		dev_err(cci->dev, "CCI reset timeout\n");
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	return 0;
> > +}
> [..]
> > +static int cci_validate_queue(struct cci *cci, u8 master, u8 queue)
> > +{
> > +	int ret = 0;
> > +	u32 val;
> > +
> > +	val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue));
> > +
> > +	if (val == cci->data->queue_size[queue])
> > +		return -EINVAL;
> > +
> > +	if (val) {
> > +		val = CCI_I2C_REPORT | BIT(8);
> 
> Can we get a define (or a comment) for BIT(8) as well?

lets define BIT(8)..

> 
> > +		writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> > +
> > +		ret = cci_run_queue(cci, master, queue);
> > +	}
> > +
> > +	return ret;
> 
> Rather than wrapping the second half of the function in a check for val,
> return early on !val and then you can end with return cci_run_queue()
> and drop the "ret".

yes sounds better


> 
> > +}
> > +
> > +static int cci_i2c_read(struct cci *cci, u16 master,
> > +			u16 addr, u8 *buf, u16 len)
> > +{
> > +	u32 val, words_read, words_exp;
> > +	u8 queue = QUEUE_1;
> > +	int i, index = 0, ret;
> > +	bool first = 0;
> 
> s/0/false/

right

> 
> > +
> > +	/*
> > +	 * Call validate queue to make sure queue is empty before starting.
> > +	 * This is to avoid overflow / underflow of queue.
> > +	 */
> > +	ret = cci_validate_queue(cci, master, queue);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	val = CCI_I2C_SET_PARAM | (addr & 0x7f) << 4;
> > +	writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> > +
> > +	val = CCI_I2C_READ | len << 4;
> > +	writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
> > +
> > +	ret = cci_run_queue(cci, master, queue);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	words_read = readl(cci->base + CCI_I2C_Mm_READ_BUF_LEVEL(master));
> > +	words_exp = len / 4 + 1;
> > +	if (words_read != words_exp) {
> > +		dev_err(cci->dev, "words read = %d, words expected = %d\n",
> > +			words_read, words_exp);
> > +		return -EIO;
> > +	}
> 
> I thought that an i2c read could return less data than was requested...

yes that is my understanding, will check with Todor and update this

> 
> > +
> > +	do {
> > +		val = readl(cci->base + CCI_I2C_Mm_READ_DATA(master));
> > +
> > +		for (i = 0; i < 4 && index < len; i++) {
> > +			if (first) {
> > +				first = false;
> > +				continue;
> > +			}
> > +			buf[index++] = (val >> (i * 8)) & 0xff;
> > +		}
> > +	} while (--words_read);
> > +
> > +	return 0;
> > +}
> [..]
> > +static int cci_probe(struct platform_device *pdev)
> > +{
> [..]
> > +	/* Clocks */
> > +
> > +	cci->nclocks = 0;
> > +	while (cci->data->res.clock[cci->nclocks])
> > +		cci->nclocks++;
> > +
> > +	cci->clock = devm_kcalloc(dev, cci->nclocks,
> > +				  sizeof(*cci->clock), GFP_KERNEL);
> > +	if (!cci->clock)
> > +		return -ENOMEM;
> > +
> > +	cci->clock_freq = devm_kcalloc(dev, cci->nclocks,
> 
> You're just using this temporarily to create a copy of the
> res.clock_rate array, how about just passing the res.clock_rate into
> cci_clock_set_rate() ?

that sound reasonable, will give it a shot

> 
> > +				       sizeof(*cci->clock_freq), GFP_KERNEL);
> > +	if (!cci->clock_freq)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < cci->nclocks; i++) {
> > +		struct clk_bulk_data *clock = &cci->clock[i];
> > +
> > +		clock->clk = devm_clk_get(dev, cci->data->res.clock[i]);
> > +		if (IS_ERR(clock->clk))
> > +			return PTR_ERR(clock->clk);
> > +
> > +		clock->id = cci->data->res.clock[i];
> > +		cci->clock_freq[i] = cci->data->res.clock_rate[i];
> > +	}
> 
> Fill out cci->clock[*].id and call clk_bulk_get()

ok

> 
> > +
> > +	ret = cci_clock_set_rate(cci->nclocks, cci->clock,
> > +				 cci->clock_freq, dev);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = clk_bulk_prepare_enable(cci->nclocks, cci->clock);
> 
> It seems a little bit excessive to keep the clocks on while the driver
> is probed, but this could be improved in a follow up patch...

okay but are they required for controller to be probed, I will check
that part and update

> 
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	val = readl_relaxed(cci->base + CCI_HW_VERSION);
> > +	dev_dbg(dev, "%s: CCI HW version = 0x%08x", __func__, val);
> > +
> > +	enable_irq(cci->irq);
> > +
> > +	ret = cci_reset(cci);
> > +	if (ret < 0)
> > +		goto error;
> > +
> > +	for (i = 0; i < cci->data->num_masters; i++) {
> > +		ret = cci_init(cci, &cci->data->params[cci->master[i].mode]);
> > +		if (ret < 0)
> > +			goto error;
> > +
> > +		ret = i2c_add_adapter(&cci->master[i].adap);
> > +		if (ret < 0)
> > +			goto error_i2c;
> > +	}
> > +
> > +	return 0;
> > +
> > +error_i2c:
> > +	for (; i >= 0; i--)
> > +		i2c_del_adapter(&cci->master[i].adap);
> > +error:
> > +	disable_irq(cci->irq);
> > +	clk_bulk_disable_unprepare(cci->nclocks, cci->clock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int cci_remove(struct platform_device *pdev)
> > +{
> > +	struct cci *cci = platform_get_drvdata(pdev);
> > +	int i;
> > +
> > +	disable_irq(cci->irq);
> > +	clk_bulk_disable_unprepare(cci->nclocks, cci->clock);
> 
> i2c clients might be communicating with their clients until you call
> i2c_del_adapter(), so better pull the resources after you have removed
> the adaptor.
> 
> Maybe even a cci_halt() call before we cut the clocks?

yes makes sense

> 
> > +
> > +	for (i = 0; i < cci->data->num_masters; i++)
> > +		i2c_del_adapter(&cci->master[i].adap);
> > +
> > +	return 0;
> > +}
> [..]
> > +static const struct cci_data cci_v1_4_0_data = {
> [..]
> > +		{
> > +			/* I2C_MODE_FAST_PLUS */
> > +			.thigh = 16,
> > +			.tlow = 22,
> > +			.tsu_sto = 17,
> > +			.tsu_sta = 18,
> > +			.thd_dat = 16,
> > +			.thd_sta = 15,
> > +			.tbuf = 24,
> > +			.scl_stretch_en = 0,
> > +			.trdhld = 3,
> > +			.tsp = 3
> > +		}
> > +
> 
> Empty line.
> 
> > +	},
> > +
> > +};
> 
> The dt binding mentions that a power domain is required for v1.4, but
> there's no support for this in the driver.

I will check that, yes it is not handled in the driver we have

-- 
~Vinod

Re: [PATCH v3 2/2] i2c: Add Qualcomm CCI I2C driver

[Bjorn Andersson]

On Mon 06 Aug 21:30 PDT 2018, Vinod wrote:

> On 06-08-18, 11:45, Bjorn Andersson wrote:
> > On Mon 06 Aug 04:04 PDT 2018, Vinod Koul wrote:
> > [..]
> > > diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> > [..]
[..]
> > > +struct cci {
> > > +	struct device *dev;
> > > +	void __iomem *base;
> > > +	u32 irq;
> > 
> > Use unsigned int rather than a type of specific size
> 
> Any specific reason for that preference?
> 

Just that unsigned int is the type used in the irq api.

[..]
> > 
> > > +
> > > +	ret = cci_clock_set_rate(cci->nclocks, cci->clock,
> > > +				 cci->clock_freq, dev);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ret = clk_bulk_prepare_enable(cci->nclocks, cci->clock);
> > 
> > It seems a little bit excessive to keep the clocks on while the driver
> > is probed, but this could be improved in a follow up patch...
> 
> okay but are they required for controller to be probed, I will check
> that part and update
> 

Right. So doing something similar to how we deal with clocks in i2c-qup
would probably make sense; we enable them during probe and then toggle
them using pm_runtime from there on.

Regards,
Bjorn

Re: [PATCH v3 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller

[Rob Herring]

On Tue, Aug 07, 2018 at 09:48:26AM +0530, Vinod wrote:
> On 06-08-18, 11:03, Bjorn Andersson wrote:
> > On Mon 06 Aug 04:04 PDT 2018, Vinod Koul wrote:
> > 
> > > From: Todor Tomov <todor.tomov@linaro.org>
> > > 
> > > Add DT binding document for Qualcomm Camera Control Interface (CCI)
> > > I2C controller.
> > > 
> > > Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
> > > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > > ---
> > >  .../devicetree/bindings/i2c/i2c-qcom-cci.txt       | 55 ++++++++++++++++++++++
> > >  1 file changed, 55 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> > > new file mode 100644
> > > index 000000000000..977978dd4444
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> > > @@ -0,0 +1,55 @@
> > > +Qualcomm Camera Control Interface (CCI) I2C controller
> > > +
> > > +Required properties:
> > > + - compatible: Should be one of:
> > > +   - "qcom,cci-v1.0.8" for 8916;
> > > +   - "qcom,cci-v1.4.0" for 8996.
> > 
> > As pointed out by Rob previously we should either use version numbers or
> > platform numbers, not both.

Not really what I said...

> > 
> > So this should either be:
> > 
> > 	- "qcom,cci-v1.0.8"
> > 	- "qcom,cci-v1.4.0"
> 
> ok will use this one..
> 
> > 
> > or:
> > 
> > 	- "qcom,msm8916-cci"
> > 	- "qcom,msm8996-cci"

I strongly prefer this one as I've yet to be convinced there is a strong 
benefit of version numbers and every other binding follows this 
convention.

Rob

Re: [PATCH v3 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller

[Bjorn Andersson]

On Tue 07 Aug 10:39 PDT 2018, Rob Herring wrote:

> On Tue, Aug 07, 2018 at 09:48:26AM +0530, Vinod wrote:
> > On 06-08-18, 11:03, Bjorn Andersson wrote:
> > > On Mon 06 Aug 04:04 PDT 2018, Vinod Koul wrote:
> > > 
> > > > From: Todor Tomov <todor.tomov@linaro.org>
> > > > 
> > > > Add DT binding document for Qualcomm Camera Control Interface (CCI)
> > > > I2C controller.
> > > > 
> > > > Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
> > > > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > > > ---
> > > >  .../devicetree/bindings/i2c/i2c-qcom-cci.txt       | 55 ++++++++++++++++++++++
> > > >  1 file changed, 55 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> > > > new file mode 100644
> > > > index 000000000000..977978dd4444
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> > > > @@ -0,0 +1,55 @@
> > > > +Qualcomm Camera Control Interface (CCI) I2C controller
> > > > +
> > > > +Required properties:
> > > > + - compatible: Should be one of:
> > > > +   - "qcom,cci-v1.0.8" for 8916;
> > > > +   - "qcom,cci-v1.4.0" for 8996.
> > > 
> > > As pointed out by Rob previously we should either use version numbers or
> > > platform numbers, not both.
> 
> Not really what I said...
> 

Sorry, I thought this was part of your concern.

Then I'll revise my statement to: if we're going to list the platforms
here anyways I think we should just use the platform numbering as
compatible directly.

> > > 
> > > So this should either be:
> > > 
> > > 	- "qcom,cci-v1.0.8"

This is supposed to be 1.1.0 for 8916...

> > > 	- "qcom,cci-v1.4.0"
> > 
> > ok will use this one..
> > 
> > > 
> > > or:
> > > 
> > > 	- "qcom,msm8916-cci"
> > > 	- "qcom,msm8996-cci"
> 
> I strongly prefer this one as I've yet to be convinced there is a strong 
> benefit of version numbers and every other binding follows this 
> convention.
> 

Looking through the list of versions for this block and the platforms
that there's any upstream interest in we have 6 versions covering 7
platforms.

So I agree with you.

Regards,
Bjorn

Re: [PATCH v3 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller

[Todor Tomov]

Hi Bjorn,

On  7.08.2018 21:07, Bjorn Andersson wrote:
> On Tue 07 Aug 10:39 PDT 2018, Rob Herring wrote:
> 
>> On Tue, Aug 07, 2018 at 09:48:26AM +0530, Vinod wrote:
>>> On 06-08-18, 11:03, Bjorn Andersson wrote:
>>>> On Mon 06 Aug 04:04 PDT 2018, Vinod Koul wrote:
>>>>
>>>>> From: Todor Tomov <todor.tomov@linaro.org>
>>>>>
>>>>> Add DT binding document for Qualcomm Camera Control Interface (CCI)
>>>>> I2C controller.
>>>>>
>>>>> Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
>>>>> Signed-off-by: Vinod Koul <vkoul@kernel.org>
>>>>> ---
>>>>>  .../devicetree/bindings/i2c/i2c-qcom-cci.txt       | 55 ++++++++++++++++++++++
>>>>>  1 file changed, 55 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
>>>>> new file mode 100644
>>>>> index 000000000000..977978dd4444
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
>>>>> @@ -0,0 +1,55 @@
>>>>> +Qualcomm Camera Control Interface (CCI) I2C controller
>>>>> +
>>>>> +Required properties:
>>>>> + - compatible: Should be one of:
>>>>> +   - "qcom,cci-v1.0.8" for 8916;
>>>>> +   - "qcom,cci-v1.4.0" for 8996.
>>>>
>>>> As pointed out by Rob previously we should either use version numbers or
>>>> platform numbers, not both.
>>
>> Not really what I said...
>>
> 
> Sorry, I thought this was part of your concern.
> 
> Then I'll revise my statement to: if we're going to list the platforms
> here anyways I think we should just use the platform numbering as
> compatible directly.
> 
>>>>
>>>> So this should either be:
>>>>
>>>> 	- "qcom,cci-v1.0.8"
> 
> This is supposed to be 1.1.0 for 8916...

There is conflicting information about this in the documentation. However the version read from the hw version register is 1.0.8 and this matches the value listed in the SWI too so I believe 1.0.8 is correct for 8916.

> 
>>>> 	- "qcom,cci-v1.4.0"


-- 
Best regards,
Todor Tomov

Re: [PATCH v3 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller

[Rob Herring]

On Wed, Aug 8, 2018 at 8:03 AM Todor Tomov <todor.tomov@linaro.org> wrote:
>
> Hi Bjorn,
>
> On  7.08.2018 21:07, Bjorn Andersson wrote:
> > On Tue 07 Aug 10:39 PDT 2018, Rob Herring wrote:
> >
> >> On Tue, Aug 07, 2018 at 09:48:26AM +0530, Vinod wrote:
> >>> On 06-08-18, 11:03, Bjorn Andersson wrote:
> >>>> On Mon 06 Aug 04:04 PDT 2018, Vinod Koul wrote:
> >>>>
> >>>>> From: Todor Tomov <todor.tomov@linaro.org>
> >>>>>
> >>>>> Add DT binding document for Qualcomm Camera Control Interface (CCI)
> >>>>> I2C controller.
> >>>>>
> >>>>> Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
> >>>>> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> >>>>> ---
> >>>>>  .../devicetree/bindings/i2c/i2c-qcom-cci.txt       | 55 ++++++++++++++++++++++
> >>>>>  1 file changed, 55 insertions(+)
> >>>>>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> >>>>> new file mode 100644
> >>>>> index 000000000000..977978dd4444
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> >>>>> @@ -0,0 +1,55 @@
> >>>>> +Qualcomm Camera Control Interface (CCI) I2C controller
> >>>>> +
> >>>>> +Required properties:
> >>>>> + - compatible: Should be one of:
> >>>>> +   - "qcom,cci-v1.0.8" for 8916;
> >>>>> +   - "qcom,cci-v1.4.0" for 8996.
> >>>>
> >>>> As pointed out by Rob previously we should either use version numbers or
> >>>> platform numbers, not both.
> >>
> >> Not really what I said...
> >>
> >
> > Sorry, I thought this was part of your concern.
> >
> > Then I'll revise my statement to: if we're going to list the platforms
> > here anyways I think we should just use the platform numbering as
> > compatible directly.
> >
> >>>>
> >>>> So this should either be:
> >>>>
> >>>>    - "qcom,cci-v1.0.8"
> >
> > This is supposed to be 1.1.0 for 8916...
>
> There is conflicting information about this in the documentation. However the version read from the hw version register is 1.0.8 and this matches the value listed in the SWI too so I believe 1.0.8 is correct for 8916.

And this is why we don't use version numbers. Unless you have access
to the RTL repositories with version tags then it is just error prone.

Rob

Re: [PATCH v3 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller

[Vinod]

On 08-08-18, 08:48, Rob Herring wrote:
> On Wed, Aug 8, 2018 at 8:03 AM Todor Tomov <todor.tomov@linaro.org> wrote:
> > >>>> So this should either be:
> > >>>>
> > >>>>    - "qcom,cci-v1.0.8"
> > >
> > > This is supposed to be 1.1.0 for 8916...
> >
> > There is conflicting information about this in the documentation.
> > However the version read from the hw version register is 1.0.8 and
> > this matches the value listed in the SWI too so I believe 1.0.8 is
> > correct for 8916.
> 
> And this is why we don't use version numbers. Unless you have access
> to the RTL repositories with version tags then it is just error prone.

Yeah we have a HW_VERSION register too, but someone might forget to
populate it, so lets go ahead and use the soc numbers as discussed.

Thanks
-- 
~Vinod

Re: [PATCH v3 2/2] i2c: Add Qualcomm CCI I2C driver

[Vinod]

On 07-08-18, 10:00, Vinod wrote:
> On 06-08-18, 11:45, Bjorn Andersson wrote:

> > The dt binding mentions that a power domain is required for v1.4, but
> > there's no support for this in the driver.
> 
> I will check that, yes it is not handled in the driver we have

So I discussed this with Todor and found that __genpd_dev_pm_attach in
drivers/base/power/domain.c handles the "power-domain" binding.

I do not think driver needs to do anything here, this seems handled in
core and we need to provide binding, FWIW, the 8996 doesn't work without
this

-- 
~Vinod

Re: [PATCH v3 2/2] i2c: Add Qualcomm CCI I2C driver

[Bjorn Andersson]

On Fri 17 Aug 05:41 PDT 2018, Vinod wrote:

> On 07-08-18, 10:00, Vinod wrote:
> > On 06-08-18, 11:45, Bjorn Andersson wrote:
> 
> > > The dt binding mentions that a power domain is required for v1.4, but
> > > there's no support for this in the driver.
> > 
> > I will check that, yes it is not handled in the driver we have
> 
> So I discussed this with Todor and found that __genpd_dev_pm_attach in
> drivers/base/power/domain.c handles the "power-domain" binding.
> 
> I do not think driver needs to do anything here, this seems handled in
> core and we need to provide binding, FWIW, the 8996 doesn't work without
> this
> 

Right, the power-domain will be picked up and afaict will be turned on
as it's attached to the device during probe of the platform driver. So
you don't need to do anything additionally to make it work.

But I suggest that you follow up with a patch adding runtime pm support
so that the associated power domain will be disabled when the bus is not
in use.

Regards,
Bjorn

Re: [PATCH v3 2/2] i2c: Add Qualcomm CCI I2C driver

[Vinod]

On 17-08-18, 11:33, Bjorn Andersson wrote:
> On Fri 17 Aug 05:41 PDT 2018, Vinod wrote:
> 
> > On 07-08-18, 10:00, Vinod wrote:
> > > On 06-08-18, 11:45, Bjorn Andersson wrote:
> > 
> > > > The dt binding mentions that a power domain is required for v1.4, but
> > > > there's no support for this in the driver.
> > > 
> > > I will check that, yes it is not handled in the driver we have
> > 
> > So I discussed this with Todor and found that __genpd_dev_pm_attach in
> > drivers/base/power/domain.c handles the "power-domain" binding.
> > 
> > I do not think driver needs to do anything here, this seems handled in
> > core and we need to provide binding, FWIW, the 8996 doesn't work without
> > this
> > 
> 
> Right, the power-domain will be picked up and afaict will be turned on
> as it's attached to the device during probe of the platform driver. So
> you don't need to do anything additionally to make it work.
> 
> But I suggest that you follow up with a patch adding runtime pm support
> so that the associated power domain will be disabled when the bus is not
> in use.

Agreed, have already started on that.

-- 
~Vinod