i2c: Add support for Qcom CCI I2C controller

From: Todor Tomov <[email protected]>

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

Signed-off-by Todor Tomov <[email protected]>

Signed-off-by Vinod Koul <[email protected]>

---
 .../devicetree/bindings/i2c/i2c-qcom-cci.txt       | 83 ++++++++++++++++++++++
 1 file changed, 83 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt

Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +0 -0

new file mode 100644
index 000000000000..b7f4240ce5c8

Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +29 -0

+
+- reg
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: base address CCI I2C controller and length of memory
+		    mapped region.
+
+- interrupts:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: specifies the CCI I2C interrupt. The format of the
+		    specifier is defined by the binding document describing
+		    the node's interrupt parent.
+
+- clocks:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: a list of phandle, should contain an entry for each
+		    entries in clock-names.
+
+- clock-names
+	Usage: required
+	Value type: <string>
+	Definition: a list of clock names, must include these entries:
+		"mmss_mmagic_ahb" - on "qcom,msm-8996-cci" only;
+		"camss_top_ahb";
+		"cci_ahb";
+		"cci";
+		"camss_ahb";

Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +34 -0

+
+PROPERTIES:
+
+- clock-frequency:
+	Usage: optional
+	Value type: <u32>
+	Definition: Desired I2C bus clock frequency in Hz, defaults to 100
+		    kHz if omitted.
+
+Example:
+
+	cci@a0c000 {
+		compatible = "qcom,msm-8996-cci";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0xa0c000 0x1000>;
+		interrupts = <GIC_SPI 295 IRQ_TYPE_EDGE_RISING>;
+		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>;
+		};
+	};

Signature · 1 line

2.14.4

Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 10

Quoted 8 lines · Vinod Koul

Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +8 -0

+PROPERTIES:
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be one of:
+		"qcom,msm-8916-cci"
+		"qcom,msm-8996-cci"

I think everywhere else is 'msm8916' and 'msm8996'.

Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 49

Quoted 4 lines

Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +4 -0

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

Use a unit-address and reg property with 0 and 1 here.

With those fixed,

Reviewed-by Rob Herring <[email protected]>

Quoted 12 lines · Rob Herring

Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 10

Quoted 8 lines · Vinod Koul

Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +8 -0

+PROPERTIES:
+
+- compatible:
+   Usage: required
+   Value type: <string>
+   Definition: must be one of:
+           "qcom,msm-8916-cci"
+           "qcom,msm-8996-cci"

I think everywhere else is 'msm8916' and 'msm8996'.

Quick grep told me that is the case, so will update.

Quoted 6 lines

Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 49

Quoted 4 lines

Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +4 -0

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

Use a unit-address and reg property with 0 and 1 here.

Am not sure I understood that properly, still learning DT nuisances, care to elaborate a bit please.

Quoted 24 lines · Vinod

Quoted 12 lines · Rob Herring

Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 10

Quoted 8 lines · Vinod Koul

Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +8 -0

+PROPERTIES:
+
+- compatible:
+   Usage: required
+   Value type: <string>
+   Definition: must be one of:
+           "qcom,msm-8916-cci"
+           "qcom,msm-8996-cci"

I think everywhere else is 'msm8916' and 'msm8996'.

Quick grep told me that is the case, so will update.

Quoted 6 lines

Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 49

Quoted 4 lines

Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +4 -0

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

Use a unit-address and reg property with 0 and 1 here.

Quoted 2 lines

Am not sure I understood that properly, still learning DT nuisances, care to elaborate a bit please.

Node names are supposed to be standard (there's a list in the DT spec) and i2c-bus is for cases where the controller is not the bus parent. So you just need it to look like this:

i2c-bus@0 {
reg = <0>;
...
};

i2c-bus@1 {
reg = <1>;
...
};

It's similar to how i2c muxes are done where you have multiple downstream i2c buses. Following this will enable some i2c bus checks in dtc (current master, not kernel copy yet) as node names are the only way

Hit send too soon...

Quoted 43 lines · Rob Herring

Quoted 24 lines · Vinod

Quoted 12 lines · Rob Herring

Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 10

Quoted 8 lines · Vinod Koul

Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +8 -0

+PROPERTIES:
+
+- compatible:
+   Usage: required
+   Value type: <string>
+   Definition: must be one of:
+           "qcom,msm-8916-cci"
+           "qcom,msm-8996-cci"

I think everywhere else is 'msm8916' and 'msm8996'.

Quick grep told me that is the case, so will update.

Quoted 6 lines

Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 49

Quoted 4 lines

Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +4 -0

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

Use a unit-address and reg property with 0 and 1 here.

Quoted 2 lines

Am not sure I understood that properly, still learning DT nuisances, care to elaborate a bit please.

Node names are supposed to be standard (there's a list in the DT spec) and i2c-bus is for cases where the controller is not the bus parent. So you just need it to look like this:

i2c-bus@0 {
reg = <0>;
...
};

i2c-bus@1 {
reg = <1>;
...
};

Quoted 4 lines

It's similar to how i2c muxes are done where you have multiple downstream i2c buses. Following this will enable some i2c bus checks in dtc (current master, not kernel copy yet) as node names are the only way

...we can match i2c buses in a generic way.

drivers/i2c/busses/Kconfig +7 -0

+       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

drivers/i2c/busses/i2c-qcom-cci.c new file +63 -0

new file mode 100644
index 000000000000..6fd6cecc0ed5
@@ -0,0 +1,879 @@
+// 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>
+#include <linux/pm_runtime.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

drivers/i2c/busses/i2c-qcom-cci.c new file +33 -0

+       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,
+};
+
+#define CCI_I2C_REPORT_IRQ_EN  BIT(8)
+
+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 {

drivers/i2c/busses/i2c-qcom-cci.c new file +42 -0

+       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;
+       unsigned int irq;
+       const struct cci_data *data;
+       struct clk_bulk_data *clock;
+       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,
+                             const 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);

drivers/i2c/busses/i2c-qcom-cci.c new file +55 -0

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

drivers/i2c/busses/i2c-qcom-cci.c new file +13 -0

+}
+
+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;

drivers/i2c/busses/i2c-qcom-cci.c new file +56 -0

+        * simplicity use master[0].xxx for waiting on it
+        */
+       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, CCI_TIMEOUT);
+       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;

drivers/i2c/busses/i2c-qcom-cci.c new file +17 -0

+       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;
+}
+

drivers/i2c/busses/i2c-qcom-cci.c new file +40 -0

+
+       val = CCI_I2C_REPORT | CCI_I2C_REPORT_IRQ_EN;
+       writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
+
+       return cci_run_queue(cci, master, queue);
+}
+
+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 = 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;
+       }

drivers/i2c/busses/i2c-qcom-cci.c new file +16 -0

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

drivers/i2c/busses/i2c-qcom-cci.c new file +7 -0

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

drivers/i2c/busses/i2c-qcom-cci.c new file +10 -0

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

drivers/i2c/busses/i2c-qcom-cci.c new file +31 -0

+               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;
+               }

drivers/i2c/busses/i2c-qcom-cci.c new file +10 -0

+               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;
+       }

drivers/i2c/busses/i2c-qcom-cci.c new file +87 -0

+
+       /* 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;
+
+       for (i = 0; i < cci->nclocks; i++)
+               cci->clock[i].id = cci->data->res.clock[i];
+
+       ret = devm_clk_bulk_get(dev, cci->nclocks, cci->clock);
+       if (ret < 0)
+               return ret;
+
+       ret = cci_clock_set_rate(cci->nclocks, cci->clock,
+                                cci->data->res.clock_rate, dev);
+       if (ret < 0)
+               return ret;
+
+       ret = cci_enable_clocks(cci);
+       if (ret < 0)
+               return ret;
+
+       pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
+       pm_runtime_use_autosuspend(dev);
+       pm_runtime_set_active(dev);
+       pm_runtime_enable(dev);
+
+       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 pm_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);
+pm_error:
+       pm_runtime_disable(dev);
+       pm_runtime_set_suspended(dev);
+error:
+       disable_irq(cci->irq);
+       cci_disable_clocks(cci);
+
+       return ret;
+}
+
+static int cci_remove(struct platform_device *pdev)
+{
+       struct cci *cci = platform_get_drvdata(pdev);
+       int i;
+
+       for (i = 0; i < cci->data->num_masters; i++) {
+               i2c_del_adapter(&cci->master[i].adap);
+               cci_halt(cci, i);
+       }
+
+       disable_irq(cci->irq);
+       pm_runtime_disable(&pdev->dev);
+       pm_runtime_set_suspended(&pdev->dev);
+
+       return 0;
+}
+
+static const struct cci_data cci_8916_data = {
+       .num_masters = 1,
+       .queue_size = { 64, 16 },

drivers/i2c/busses/i2c-qcom-cci.c new file +47 -0

+               },
+               .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_8996_data = {
+       .num_masters = 2,
+       .queue_size = { 64, 16 },
+       .quirks = {
+               .max_write_len = 11,
+               .max_read_len = 12,
+       },
+       .res = {
+               .clock = {

drivers/i2c/busses/i2c-qcom-cci.c new file +21 -0

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

drivers/i2c/busses/i2c-qcom-cci.c new file +13 -0

+                       .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
+               }
+       },
+};

Quoting Vinod Koul (2018-08-19 23:39:53)

Quoted 8 lines

drivers/i2c/busses/Kconfig +0 -0

index 4f8df2ec87b1..033958bcdd4b 100644
@@ -828,6 +828,16 @@ config I2C_PXA_SLAVE
          is necessary for systems where the PXA may be a target on the
          I2C bus.
 

drivers/i2c/busses/Kconfig config I2C_PXA_SLAVE · line 833

Quoted 3 lines

drivers/i2c/busses/Kconfig +3 -0

+config I2C_QCOM_CCI
+       tristate "Qualcomm Camera Control Interface"
+       depends on ARCH_QCOM

Or COMPILE_TEST?

Quoted 86 lines

drivers/i2c/busses/Kconfig +7 -0

+       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

drivers/i2c/busses/i2c-qcom-cci.c new file +69 -0

new file mode 100644
index 000000000000..6fd6cecc0ed5
@@ -0,0 +1,879 @@
+// 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>
+#include <linux/pm_runtime.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    (msecs_to_jiffies(100))
+#define NUM_MASTERS    2
+#define NUM_QUEUES     2
+

drivers/i2c/busses/i2c-qcom-cci.c line 73

Quoted 4 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +4 -0

+/* Max number of resources + 1 for a NULL terminator */
+#define CCI_RES_MAX    6
+
+enum cci_i2c_cmd {

Make these #defines so we can easily see what the value is instead of having to count. Plus, the enum isn't helping anyone figure out that a command is being used because the enum isn't used anywhere in the code.

Quoted 38 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +38 -0

+       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,
+};
+
+#define CCI_I2C_REPORT_IRQ_EN  BIT(8)
+
+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;

drivers/i2c/busses/i2c-qcom-cci.c line 116

Quoted 5 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +5 -0

+       u16 thd_sta;
+       u16 tbuf;
+       u8 scl_stretch_en;
+       u16 trdhld;
+       u16 tsp;

Care to spell these out fully? Or at least add kernel documentation so we know what they actually mean?

drivers/i2c/busses/i2c-qcom-cci.c line 127

Quoted 12 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +11 -0

+};
+
+struct cci;
+
+struct cci_master {
+       struct i2c_adapter adap;
+       u16 master;
+       u8 mode;
+       int status;
+       bool complete_pending;
+       struct completion irq_complete;

Having a bool and a completion is very odd and probably racy. Can you get rid of the bool and just complete things when they need completing and wait for them when they need waiting?

Quoted 47 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +46 -0

+       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;
+       unsigned int irq;
+       const struct cci_data *data;
+       struct clk_bulk_data *clock;
+       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,
+                             const 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]);

drivers/i2c/busses/i2c-qcom-cci.c line 179

Quoted 6 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +6 -0

+                       if (ret < 0) {
+                               dev_err(dev, "clk set rate failed: %d\n", ret);
+                               return ret;
+                       }
+               }
+       }

Hopefully this whole function can go away.

Quoted 60 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +60 -0

+
+       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);

drivers/i2c/busses/i2c-qcom-cci.c line 244

Quoted 5 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +5 -0

+               cci->master[1].status = -EIO;
+               writel(CCI_HALT_REQ_I2C_M1_Q0Q1, cci->base + CCI_HALT_REQ);
+       }
+
+       return IRQ_HANDLED;

What if none of the bits above were set? We should return IRQ_NONE in that case so we can detect spurious interrupts.

Quoted 14 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +13 -0

+}
+
+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;

drivers/i2c/busses/i2c-qcom-cci.c line 267

Quoted 10 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +10 -0

+
+       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;
+       }

Why it this a case statement? It could be

	if (master_num > 1) {
		dev_err(...);
		return -EINVAL;
	}

	val = BIT(master_num);
	master = &cci->master[master_num];

drivers/i2c/busses/i2c-qcom-cci.c line 273

Quoted 7 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +6 -0

+
+       master->complete_pending = true;
+       writel(val, cci->base + CCI_HALT_REQ);
+
+       time = wait_for_completion_timeout(&master->irq_complete, CCI_TIMEOUT);
+       if (!time) {

Why not just

	if (!wait_for_completion_timeout(...)

? Applies throughout this patch.

drivers/i2c/busses/i2c-qcom-cci.c line 286

Quoted 13 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +13 -0

+               dev_err(cci->dev, "CCI halt timeout\n");
+               return -ETIMEDOUT;
+       }
+
+       return 0;
+}
+
+static int cci_reset(struct cci *cci)
+{
+       unsigned long time;
+
+       /*
+        * we reset the whole controller (CCI_RESET_CMD_MASK),here and for

Add a space after that close parenthesis.

Quoted 61 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +60 -0

+        * simplicity use master[0].xxx for waiting on it
+        */
+       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, CCI_TIMEOUT);
+       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));

drivers/i2c/busses/i2c-qcom-cci.c line 352

Quoted 6 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +6 -0

+
+       val = BIT(master * 2 + queue);
+       writel(val, cci->base + CCI_QUEUE_START);
+
+       time = wait_for_completion_timeout
+                       (&cci->master[master].irq_complete, CCI_TIMEOUT);

This line is really long. Make a local variable?

Quoted 22 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +21 -0

+       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)
+{
+       u32 val;
+

drivers/i2c/busses/i2c-qcom-cci.c line 379

Quoted 6 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +6 -0

+       val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue));
+       if (val == cci->data->queue_size[queue])
+               return -EINVAL;
+
+       if (!val)
+               return val;

return 0?

Quoted 43 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +43 -0

+       val = CCI_I2C_REPORT | CCI_I2C_REPORT_IRQ_EN;
+       writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
+
+       return cci_run_queue(cci, master, queue);
+}
+
+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 = 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;
+       }
+
+       do {
+               val = readl(cci->base + CCI_I2C_Mm_READ_DATA(master));
+

drivers/i2c/busses/i2c-qcom-cci.c line 429

Quoted 6 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +6 -0

+               for (i = 0; i < 4 && index < len; i++) {
+                       if (first) {
+                               first = false;
+                               continue;
+                       }
+                       buf[index++] = (val >> (i * 8)) & 0xff;

Is this some sort of readsl() but where the first byte is ignored?

Quoted 22 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +21 -0

+               }
+       } 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;

drivers/i2c/busses/i2c-qcom-cci.c line 455

Quoted 5 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +5 -0

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

Can 'len' really be 16 bits? Because this assignment truncates that very quickly.

drivers/i2c/busses/i2c-qcom-cci.c line 457

Quoted 3 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +2 -0

+
+       for (j = 0; j < len; j++)

Well I guess len can be at most '11', so maybe the type should be u8 instead?

drivers/i2c/busses/i2c-qcom-cci.c line 465

Quoted 8 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +8 -0

+               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));

Use writesl() instead?

Quoted 13 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +13 -0

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

drivers/i2c/busses/i2c-qcom-cci.c line 597

Quoted 6 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +6 -0

+
+       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");

This would never happen though.

Quoted 35 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +35 -0

+               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);
+       }
+

drivers/i2c/busses/i2c-qcom-cci.c line 638

Quoted 6 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +6 -0

+       /* 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");

devm_ioremap_resource() already prints an error. Drop this.

Quoted 14 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +14 -0

+               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);

drivers/i2c/busses/i2c-qcom-cci.c line 658

Quoted 6 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +6 -0

+       if (ret < 0) {
+               dev_err(dev, "request_irq failed, ret: %d\n", ret);
+               return ret;
+       }
+
+       disable_irq(cci->irq);

Why? Is the irq always triggering or something?

drivers/i2c/busses/i2c-qcom-cci.c line 755

Quoted 13 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +13 -0

+static const struct cci_data cci_8916_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"

I guess this is another design where you just want to "get all the clks" and not care about what they are?

Quoted 9 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +9 -0

+static const struct cci_data cci_8996_data = {
+       .num_masters = 2,
+       .queue_size = { 64, 16 },
+       .quirks = {
+               .max_write_len = 11,
+               .max_read_len = 12,
+       },
+       .res = {
+               .clock = {

drivers/i2c/busses/i2c-qcom-cci.c line 812

Quoted 10 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +10 -0

+                       "camss_top_ahb",
+                       "cci_ahb",
+                       "camss_ahb",
+                       "cci"
+               },
+               .clock_rate = {
+                       0,
+                       0,
+                       0,
+                       37500000

Use assigned clock rates from DT instead?

Quoted 21 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +21 -0

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

drivers/i2c/busses/i2c-qcom-cci.c line 843

Quoted 10 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +10 -0

+                       .tsu_sta = 40,
+                       .thd_dat = 22,
+                       .thd_sta = 35,
+                       .tbuf = 62,
+                       .scl_stretch_en = 0,
+                       .trdhld = 6,
+                       .tsp = 3
+               },
+               {
+                       /* I2C_MODE_FAST_PLUS */

Could be written like [I2C_MODE_FAST_PLUS] = { ... } and then drop the comments and positional magic.

Quoted 23 lines

Quoting Vinod Koul (2018-08-19 23:39:53)

Quoted 21 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +21 -0

+       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)
+{
+       u32 val;
+

Quoted 8 lines

drivers/i2c/busses/i2c-qcom-cci.c line 379

Quoted 6 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +6 -0

+       val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue));
+       if (val == cci->data->queue_size[queue])
+               return -EINVAL;
+
+       if (!val)
+               return val;

return 0?

do you want to make it explicit? both seem to me to do the same thing

Quoted 22 lines

Quoted 22 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +21 -0

+               }
+       } 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;

Quoted 8 lines

drivers/i2c/busses/i2c-qcom-cci.c line 455

Quoted 5 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +5 -0

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

Can 'len' really be 16 bits? Because this assignment truncates that very quickly.

Yes, this is passed from i2c_msg->len which is 16 bytes. But here it doesn't support so it is truncated here

Quoted 1 line

Quoted 5 lines

drivers/i2c/busses/i2c-qcom-cci.c line 457

Quoted 2 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +2 -0

+
+       for (j = 0; j < len; j++)

Well I guess len can be at most '11', so maybe the type should be u8 instead?

I can use a local variable and cast to it, but am not sure that helps!

Quoted 15 lines

Quoted 15 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +14 -0

+               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);

Quoted 8 lines

drivers/i2c/busses/i2c-qcom-cci.c line 658

Quoted 6 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +6 -0

+       if (ret < 0) {
+               dev_err(dev, "request_irq failed, ret: %d\n", ret);
+               return ret;
+       }
+
+       disable_irq(cci->irq);

Why? Is the irq always triggering or something?

I supposed Todor didn't want to enable IRQ until everything is set. I could move this block before adding i2c adapter.

Quoted 15 lines

Quoted 14 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +13 -0

+static const struct cci_data cci_8916_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"

Quoted 2 lines

I guess this is another design where you just want to "get all the clks" and not care about what they are?

Yes that is how this seems to be

Quoting Vinod (2018-08-24 03:16:35)

Quoted 34 lines · Stephen Boyd

Quoted 17 lines

Quoting Vinod Koul (2018-08-19 23:39:53)

Quoted 15 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +15 -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;

Quoted 8 lines

drivers/i2c/busses/i2c-qcom-cci.c line 455

Quoted 5 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +5 -0

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

Can 'len' really be 16 bits? Because this assignment truncates that very quickly.

Yes, this is passed from i2c_msg->len which is 16 bytes. But here it doesn't support so it is truncated here

Quoted 4 lines

Quoted 3 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +2 -0

+
+       for (j = 0; j < len; j++)

Quoted 4 lines

Quoted 2 lines

Well I guess len can be at most '11', so maybe the type should be u8 instead?

I can use a local variable and cast to it, but am not sure that helps!

Maybe just a comment indicating that max len is 11?

Quoted 23 lines

Quoted 22 lines

Quoted 10 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +10 -0

+               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;
+       }

drivers/i2c/busses/i2c-qcom-cci.c line 658

Quoted 10 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +10 -0

+       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);

Why? Is the irq always triggering or something?

Quoted 2 lines

I supposed Todor didn't want to enable IRQ until everything is set. I could move this block before adding i2c adapter.

But does it actually matter? The interrupt is "enabled" from request to this function call so it's still possible to trigger. I'd just remove the irq disabling unless it actually matters.

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +13 -0

+static const struct cci_data cci_8916_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"

Quoted 4 lines

Quoted 2 lines

I guess this is another design where you just want to "get all the clks" and not care about what they are?

Yes that is how this seems to be

Ok. I'm going to merge the "get all the clks" API soon. Maybe you can use that here.

Quoted 35 lines · Stephen Boyd

Quoting Vinod (2018-08-24 03:16:35)

Quoted 34 lines · Stephen Boyd

Quoted 17 lines

Quoting Vinod Koul (2018-08-19 23:39:53)

Quoted 15 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +15 -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;

Quoted 8 lines

drivers/i2c/busses/i2c-qcom-cci.c line 455

Quoted 5 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +5 -0

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

Can 'len' really be 16 bits? Because this assignment truncates that very quickly.

Yes, this is passed from i2c_msg->len which is 16 bytes. But here it doesn't support so it is truncated here

Quoted 4 lines

Quoted 3 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +2 -0

+
+       for (j = 0; j < len; j++)

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Well I guess len can be at most '11', so maybe the type should be u8 instead?

I can use a local variable and cast to it, but am not sure that helps!

Maybe just a comment indicating that max len is 11?

Sure, will add

Quoted 23 lines

Quoted 23 lines

Quoted 22 lines

Quoted 10 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +10 -0

+               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;
+       }

drivers/i2c/busses/i2c-qcom-cci.c line 658

Quoted 10 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +10 -0

+       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);

Why? Is the irq always triggering or something?

Quoted 6 lines

Quoted 2 lines

I supposed Todor didn't want to enable IRQ until everything is set. I could move this block before adding i2c adapter.

But does it actually matter? The interrupt is "enabled" from request to this function call so it's still possible to trigger. I'd just remove the irq disabling unless it actually matters.

Agreed, will do that

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

drivers/i2c/busses/i2c-qcom-cci.c new file +13 -0

+static const struct cci_data cci_8916_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"

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

I guess this is another design where you just want to "get all the clks" and not care about what they are?

Yes that is how this seems to be

Ok. I'm going to merge the "get all the clks" API soon. Maybe you can use that here.

Sure, once merged I will send a patch for this driver, which I guess would be after next merge window..

--
~Vinod

Sorry to enter the discussion this late.

I have been using this driver with release/qcomlt-4.14 on a db820c and if I try to use the i2c bus before camss is running I get a Timeout error and the bus is unusable after that. I have sent a patch that fixes this issue, in case you want to merge it/add it before this driver goes upstream.

Is there a way to use the i2c bus without using the camera?

Hi Ricardo,

Quoted 7 lines · Ricardo Ribalda Delgado

Sorry to enter the discussion this late.

I have been using this driver with release/qcomlt-4.14 on a db820c and if I try to use the i2c bus before camss is running I get a Timeout error and the bus is unusable after that. I have sent a patch that fixes this issue, in case you want to merge it/add it before this driver goes upstream.

Thanks for this, I will test and port the fix to upstream and send it along.

Quoted 1 line

Is there a way to use the i2c bus without using the camera?

This is Camera specific bus but it is an i2c bus, so theoretically it should be possible to do so, Todor do you know if that is doable/done?

Hi Vinod,

Quoted 14 lines · Vinod

Hi Ricardo,

Quoted 7 lines · Ricardo Ribalda Delgado

Sorry to enter the discussion this late.

I have been using this driver with release/qcomlt-4.14 on a db820c and if I try to use the i2c bus before camss is running I get a Timeout error and the bus is unusable after that. I have sent a patch that fixes this issue, in case you want to merge it/add it before this driver goes upstream.

Thanks for this, I will test and port the fix to upstream and send it along.

Quoted 4 lines

Quoted 1 line

Is there a way to use the i2c bus without using the camera?

This is Camera specific bus but it is an i2c bus, so theoretically it should be possible to do so, Todor do you know if that is doable/done?

I'm not aware of such experiments but yes, if you manage to connect another i2c hardware it should be possible to control it.

--
Best regards,
Todor Tomov