[PATCH 0/4] Qualcomm Camera Control Interface driver

[PATCH 0/4] Qualcomm Camera Control Interface driver

[Todor Tomov]

This patchset adds basic support for the Qualcomm Camera Control Interface
(CCI) controller found on Qualcomm MSM8916/APQ8016 and MSM8996/APQ8996 SoC.

CCI has one I2C bus on MSM8916/APQ8016 and two I2C busses on MSM8996/APQ8096.
The present driver supports the first I2C bus only.

The driver is implemented using as a reference the Qualcomm CCI driver
for Android as found in Code Aurora [1] and [2].

The driver is tested on Dragonboard 410c (APQ8016) and Dragonboard 820c 
(APQ8096) with one and two OV5645 camera sensors.


[1] https://source.codeaurora.org/quic/la/kernel/msm-3.10/
[2] https://source.codeaurora.org/quic/la/kernel/msm-3.18/


Todor Tomov (4):
  dt-bindings: media: Binding document for Qualcomm Camera Control
    Interface driver
  i2c: Add Qualcomm Camera Control Interface driver
  MAINTAINERS: Add Qualcomm Camera Control Interface driver
  arm64: dts: qcom: Add Camera Control Interface support

 .../devicetree/bindings/i2c/i2c-qcom-cci.txt       |  55 ++
 MAINTAINERS                                        |   7 +
 arch/arm64/boot/dts/qcom/msm8916-pins.dtsi         |  14 +
 arch/arm64/boot/dts/qcom/msm8916.dtsi              |  21 +
 arch/arm64/boot/dts/qcom/msm8996-pins.dtsi         |  14 +
 arch/arm64/boot/dts/qcom/msm8996.dtsi              |  24 +
 drivers/i2c/busses/Kconfig                         |  10 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-qcom-cci.c                  | 793 +++++++++++++++++++++
 9 files changed, 939 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
 create mode 100644 drivers/i2c/busses/i2c-qcom-cci.c

-- 
2.7.4

[PATCH 1/4] dt-bindings: media: Binding document for Qualcomm Camera Control Interface driver

[Todor Tomov]

Add DT binding document for Qualcomm Camera Control Interface driver

CC: Rob Herring <robh+dt@kernel.org>
CC: Mark Rutland <mark.rutland@arm.com>
CC: devicetree@vger.kernel.org
Signed-off-by: Todor Tomov <todor.tomov@linaro.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 0000000..f4c5338
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
@@ -0,0 +1,55 @@
+Qualcomm Camera Control Interface 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 controller and length of memory mapped region.
+ - reg-names: Should be "cci".
+ - interrupts: Specifier for CCI interrupt.
+ - interrupt-names: Should be "cci".
+ - clocks: List of clock specifiers, one for each entry in clock-names.
+ - clock-names: Should contain:
+   - "mmss_mmagic_ahb" - on 8996 only;
+   - "camss_top_ahb";
+   - "cci_ahb";
+   - "cci";
+   - "camss_ahb".
+ - pinctrl-names: Should contain only one value - "default".
+ - pinctrl-0: Pin control group to be used for this controller.
+
+
+Required properties on 8996:
+ - power-domains: Power domain specifier.
+
+Optional:
+ - clock-frequency: Desired I2C bus clock frequency in Hz, defaults to 100 kHz
+   if omitted.
+
+Example:
+
+                cci: qcom,cci@a0c000 {
+                        compatible = "qcom,cci-v1.4.0";
+                        #address-cells = <1>;
+                        #size-cells = <0>;
+                        reg = <0xa0c000 0x1000>;
+                        reg-names = "cci";
+                        interrupts = <GIC_SPI 295 IRQ_TYPE_EDGE_RISING>;
+                        interrupt-names = "cci";
+                        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";
+                        pinctrl-names = "default";
+                        pinctrl-0 = <&cci0_default>;
+                        clock-frequency = <400000>;
+                };
-- 
2.7.4

Re: [PATCH 1/4] dt-bindings: media: Binding document for Qualcomm Camera Control Interface driver

[Bjorn Andersson]

On Mon 02 Oct 07:13 PDT 2017, Todor Tomov wrote:
> 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 0000000..f4c5338
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> @@ -0,0 +1,55 @@
> +Qualcomm Camera Control Interface I2C controller

Well, it's not a I2C controller, it is a MIPI CCI 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 controller and length of memory mapped region.
> + - reg-names: Should be "cci".

No need for reg-names, as we only have one.

> + - interrupts: Specifier for CCI interrupt.
> + - interrupt-names: Should be "cci".

No need for interrupt-names, as we only have one.

> + - clocks: List of clock specifiers, one for each entry in clock-names.
> + - clock-names: Should contain:
> +   - "mmss_mmagic_ahb" - on 8996 only;
> +   - "camss_top_ahb";
> +   - "cci_ahb";
> +   - "cci";
> +   - "camss_ahb".
> + - pinctrl-names: Should contain only one value - "default".
> + - pinctrl-0: Pin control group to be used for this controller.

No need to document that the node can have the default pinctrl
properties.

> +
> +
> +Required properties on 8996:
> + - power-domains: Power domain specifier.
> +
> +Optional:
> + - clock-frequency: Desired I2C bus clock frequency in Hz, defaults to 100 kHz
> +   if omitted.
> +
> +Example:
> +
> +                cci: qcom,cci@a0c000 {

Don't do qcom, in the node name, and there's probably no reason to have
a label for this node either. So just make it

		   cci@a0c000 {

> +                        compatible = "qcom,cci-v1.4.0";
> +                        #address-cells = <1>;
> +                        #size-cells = <0>;
> +                        reg = <0xa0c000 0x1000>;
> +                        reg-names = "cci";
> +                        interrupts = <GIC_SPI 295 IRQ_TYPE_EDGE_RISING>;
> +                        interrupt-names = "cci";
> +                        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";
> +                        pinctrl-names = "default";
> +                        pinctrl-0 = <&cci0_default>;
> +                        clock-frequency = <400000>;
> +                };

In the case of this single cci block having two masters we need a way to
describe the two sets of clients. I think the two options we have are:

cci {
	client {
		reg = <0 0x2c>;
	};
};

The reg here being <master address>

or:

cci {
	master0 {
		client {
			reg = <0x2c>;
		};
	};
};

The name "master0" could be made significant and picked up by name, and
in the case of single-master this level could be omitted.


I like the first one, but it looks really hard to get implemented in
Linux, based on the i2c core's expectation that #address-cells is 1. So
I think the latter is the favourable option.

Regards,
Bjorn

Re: [PATCH 1/4] dt-bindings: media: Binding document for Qualcomm Camera Control Interface driver

[Todor Tomov]

Hi Bjorn,

Thank you for the review.

On  6.10.2017 08:29, Bjorn Andersson wrote:
> On Mon 02 Oct 07:13 PDT 2017, Todor Tomov wrote:
>> 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 0000000..f4c5338
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
>> @@ -0,0 +1,55 @@
>> +Qualcomm Camera Control Interface I2C controller
> 
> Well, it's not a I2C controller, it is a MIPI CCI 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 controller and length of memory mapped region.
>> + - reg-names: Should be "cci".
> 
> No need for reg-names, as we only have one.
> 
>> + - interrupts: Specifier for CCI interrupt.
>> + - interrupt-names: Should be "cci".
> 
> No need for interrupt-names, as we only have one.
> 
>> + - clocks: List of clock specifiers, one for each entry in clock-names.
>> + - clock-names: Should contain:
>> +   - "mmss_mmagic_ahb" - on 8996 only;
>> +   - "camss_top_ahb";
>> +   - "cci_ahb";
>> +   - "cci";
>> +   - "camss_ahb".
>> + - pinctrl-names: Should contain only one value - "default".
>> + - pinctrl-0: Pin control group to be used for this controller.
> 
> No need to document that the node can have the default pinctrl
> properties.
> 
>> +
>> +
>> +Required properties on 8996:
>> + - power-domains: Power domain specifier.
>> +
>> +Optional:
>> + - clock-frequency: Desired I2C bus clock frequency in Hz, defaults to 100 kHz
>> +   if omitted.
>> +
>> +Example:
>> +
>> +                cci: qcom,cci@a0c000 {
> 
> Don't do qcom, in the node name, and there's probably no reason to have
> a label for this node either. So just make it
> 
> 		   cci@a0c000 {
> 
>> +                        compatible = "qcom,cci-v1.4.0";
>> +                        #address-cells = <1>;
>> +                        #size-cells = <0>;
>> +                        reg = <0xa0c000 0x1000>;
>> +                        reg-names = "cci";
>> +                        interrupts = <GIC_SPI 295 IRQ_TYPE_EDGE_RISING>;
>> +                        interrupt-names = "cci";
>> +                        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";
>> +                        pinctrl-names = "default";
>> +                        pinctrl-0 = <&cci0_default>;
>> +                        clock-frequency = <400000>;
>> +                };
> 
> In the case of this single cci block having two masters we need a way to
> describe the two sets of clients. I think the two options we have are:
> 
> cci {
> 	client {
> 		reg = <0 0x2c>;
> 	};
> };
> 
> The reg here being <master address>
> 
> or:
> 
> cci {
> 	master0 {
> 		client {
> 			reg = <0x2c>;
> 		};
> 	};
> };
> 
> The name "master0" could be made significant and picked up by name, and
> in the case of single-master this level could be omitted.
> 
> 
> I like the first one, but it looks really hard to get implemented in
> Linux, based on the i2c core's expectation that #address-cells is 1. So
> I think the latter is the favourable option.

Yes, I'll try this for the version which supports the second bus. Thank
you for this suggestion.

The rest of the points I'll fix now and send a v2.

> 
> Regards,
> Bjorn
> 

-- 
Best regards,
Todor Tomov

[PATCH 2/4] i2c: Add Qualcomm Camera Control Interface driver

[Todor Tomov]

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

CCI versions supported:
- 1.0.8 - found on MSM8916/APQ8016 - support for the only CCI I2C bus;
- 1.4.0 - found on MSM8996/APQ8096 - support for first (out of two)
  I2C bus.

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

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index c06dce2..5841094 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -832,6 +832,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 47f3ac9..c55e57c 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 0000000..080f0ca
--- /dev/null
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -0,0 +1,793 @@
+/* Copyright (c) 2012-2016, The Linux Foundation. All rights reserved.
+ * Copyright (C) 2017 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/i2c.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of.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	(1 << 0)
+#define CCI_HALT_REQ_I2C_M1_Q0Q1	(1 << 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		(1 << 0)
+#define CCI_IRQ_MASK_0_I2C_M0_Q0_REPORT		(1 << 4)
+#define CCI_IRQ_MASK_0_I2C_M0_Q1_REPORT		(1 << 8)
+#define CCI_IRQ_MASK_0_I2C_M1_RD_DONE		(1 << 12)
+#define CCI_IRQ_MASK_0_I2C_M1_Q0_REPORT		(1 << 16)
+#define CCI_IRQ_MASK_0_I2C_M1_Q1_REPORT		(1 << 20)
+#define CCI_IRQ_MASK_0_RST_DONE_ACK		(1 << 24)
+#define CCI_IRQ_MASK_0_I2C_M0_Q0Q1_HALT_ACK	(1 << 25)
+#define CCI_IRQ_MASK_0_I2C_M1_Q0Q1_HALT_ACK	(1 << 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		(1 << 0)
+#define CCI_IRQ_STATUS_0_I2C_M0_Q0_REPORT	(1 << 4)
+#define CCI_IRQ_STATUS_0_I2C_M0_Q1_REPORT	(1 << 8)
+#define CCI_IRQ_STATUS_0_I2C_M1_RD_DONE		(1 << 12)
+#define CCI_IRQ_STATUS_0_I2C_M1_Q0_REPORT	(1 << 16)
+#define CCI_IRQ_STATUS_0_I2C_M1_Q1_REPORT	(1 << 20)
+#define CCI_IRQ_STATUS_0_RST_DONE_ACK		(1 << 24)
+#define CCI_IRQ_STATUS_0_I2C_M0_Q0Q1_HALT_ACK	(1 << 25)
+#define CCI_IRQ_STATUS_0_I2C_M1_Q0Q1_HALT_ACK	(1 << 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 1
+#define NUM_QUEUES 2
+
+#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
+};
+
+enum cci_i2c_master_t {
+	MASTER_0,
+	MASTER_1
+};
+
+struct resources {
+	char *clock[CCI_RES_MAX];
+	u32 clock_rate[CCI_RES_MAX];
+	char *reg[CCI_RES_MAX];
+	char *interrupt[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_master {
+	u32 status;
+	u8 complete_pending;
+	struct completion irq_complete;
+};
+
+struct cci {
+	struct device *dev;
+	struct i2c_adapter adap;
+	void __iomem *base;
+	u32 irq;
+	char irq_name[30];
+	struct cci_clock *clock;
+	int nclocks;
+	u8 mode;
+	u16 queue_size[NUM_QUEUES];
+	struct cci_master master[NUM_MASTERS];
+};
+
+static const struct resources res_v1_0_8 = {
+	.clock = { "camss_top_ahb",
+		   "cci_ahb",
+		   "camss_ahb",
+		   "cci" },
+	.clock_rate = { 0,
+			80000000,
+			0,
+			19200000 },
+	.reg = { "cci" },
+	.interrupt = { "cci" }
+};
+
+static const struct resources res_v1_4_0 = {
+	.clock = { "mmss_mmagic_ahb",
+		   "camss_top_ahb",
+		   "cci_ahb",
+		   "camss_ahb",
+		   "cci" },
+	.clock_rate = { 0,
+			0,
+			0,
+			0,
+			37500000 },
+	.reg = { "cci" },
+	.interrupt = { "cci" }
+};
+
+static const struct hw_params hw_params_v1_0_8[3] = {
+	{	/* 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 hw_params hw_params_v1_4_0[3] = {
+	{	/* 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 u16 queue_0_size_v1_0_8 = 64;
+static const u16 queue_1_size_v1_0_8 = 16;
+
+static const u16 queue_0_size_v1_4_0 = 64;
+static const u16 queue_1_size_v1_4_0 = 16;
+
+/*
+ * cci_enable_clocks - Enable multiple clocks
+ * @nclocks: Number of clocks in clock array
+ * @clock: Clock array
+ * @dev: Device
+ *
+ * Return 0 on success or a negative error code otherwise
+ */
+int cci_enable_clocks(int nclocks, struct cci_clock *clock, struct device *dev)
+{
+	int ret;
+	int i;
+
+	for (i = 0; i < nclocks; i++) {
+		if (clock[i].freq) {
+			long rate;
+
+			rate = clk_round_rate(clock[i].clk, clock[i].freq);
+			if (rate < 0) {
+				dev_err(dev, "clk round rate failed: %ld\n",
+					rate);
+				goto error;
+			}
+
+			ret = clk_set_rate(clock[i].clk, clock[i].freq);
+			if (ret < 0) {
+				dev_err(dev, "clk set rate failed: %d\n", ret);
+				goto error;
+			}
+		}
+
+		ret = clk_prepare_enable(clock[i].clk);
+		if (ret) {
+			dev_err(dev, "clock enable failed, ret: %d\n", ret);
+			goto error;
+		}
+	}
+
+	return 0;
+
+error:
+	for (i--; i >= 0; i--)
+		clk_disable_unprepare(clock[i].clk);
+
+	return ret;
+}
+
+/*
+ * cci_disable_clocks - Disable multiple clocks
+ * @nclocks: Number of clocks in clock array
+ * @clock: Clock array
+ */
+void cci_disable_clocks(int nclocks, struct cci_clock *clock)
+{
+	int i;
+
+	for (i = nclocks - 1; i >= 0; i--)
+		clk_disable_unprepare(clock[i].clk);
+}
+
+static irqreturn_t cci_isr(int irq, void *dev)
+{
+	struct cci *cci = dev;
+	u32 val;
+
+	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[MASTER_0].complete_pending) {
+			cci->master[MASTER_0].complete_pending = 0;
+			complete(&cci->master[MASTER_0].irq_complete);
+		}
+
+		if (cci->master[MASTER_1].complete_pending) {
+			cci->master[MASTER_1].complete_pending = 0;
+			complete(&cci->master[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[MASTER_0].status = 0;
+		complete(&cci->master[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[MASTER_1].status = 0;
+		complete(&cci->master[MASTER_1].irq_complete);
+	}
+
+	if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M0_Q0Q1_HALT_ACK)) {
+		cci->master[MASTER_0].complete_pending = 1;
+		writel(CCI_RESET_CMD_M0_MASK, cci->base + CCI_RESET_CMD);
+	}
+
+	if (unlikely(val & CCI_IRQ_STATUS_0_I2C_M1_Q0Q1_HALT_ACK)) {
+		cci->master[MASTER_1].complete_pending = 1;
+		writel(CCI_RESET_CMD_M1_MASK, 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[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[MASTER_1].status = -EIO;
+		writel(CCI_HALT_REQ_I2C_M1_Q0Q1, cci->base + CCI_HALT_REQ);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int cci_reset(struct cci *cci)
+{
+	unsigned long time;
+
+	cci->master[MASTER_0].complete_pending = 1;
+	writel(CCI_RESET_CMD_MASK, cci->base + CCI_RESET_CMD);
+	time = wait_for_completion_timeout(
+				&cci->master[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 < 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));
+
+		cci->master[i].status = 0;
+	}
+
+	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 = 1 << ((master * 2) + queue);
+	writel(val, cci->base + CCI_QUEUE_START);
+
+	time = wait_for_completion_timeout(&cci->master[master].irq_complete,
+					   CCI_TIMEOUT_MS);
+	if (!time) {
+		dev_err(cci->dev, "master %d queue %d timeout\n",
+			master, queue);
+		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, u32 len, u8 master, u8 queue)
+{
+	int ret = 0;
+	u32 val;
+
+	val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue));
+
+	if (val + len + 1 > cci->queue_size[queue]) {
+		val = CCI_I2C_REPORT | (1 << 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 addr, u8 *buf, u16 len)
+{
+	u8 master = MASTER_0;
+	u8 queue = QUEUE_1;
+	u32 val;
+	u32 words_read, words_exp;
+	int i, index, first;
+	int ret;
+
+	if (len > cci->adap.quirks->max_read_len)
+		return -EOPNOTSUPP;
+
+	/*
+	 * Call validate queue to make sure queue is empty before starting.
+	 * This is to avoid overflow / underflow of queue.
+	 */
+	ret = cci_validate_queue(cci, cci->queue_size[queue], master, queue);
+	if (ret < 0)
+		return ret;
+
+	val = CCI_I2C_SET_PARAM | ((addr >> 1) & 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;
+	}
+
+	index = 0;
+	first = 1;
+	do {
+		val = readl(cci->base + CCI_I2C_Mm_READ_DATA(master));
+
+		for (i = 0; i < 4 && index < len; i++) {
+			if (first) {
+				first = 0;
+				continue;
+			}
+			buf[index++] = (val >> (i * 8)) & 0xff;
+		}
+	} while (--words_read);
+
+	return 0;
+}
+
+static int cci_i2c_write(struct cci *cci, u16 addr, u8 *buf, u16 len)
+{
+	u8 master = MASTER_0;
+	u8 queue = QUEUE_0;
+	u8 load[12] = { 0 };
+	u8 i, j;
+	u32 val;
+	int ret;
+
+	if (len > cci->adap.quirks->max_write_len)
+		return -EOPNOTSUPP;
+
+	/*
+	 * Call validate queue to make sure queue is empty before starting.
+	 * This is to avoid overflow / underflow of queue.
+	 */
+	ret = cci_validate_queue(cci, cci->queue_size[queue], master, queue);
+	if (ret < 0)
+		return ret;
+
+	val = CCI_I2C_SET_PARAM | ((addr >> 1) & 0x7f) << 4;
+	writel(val, cci->base + CCI_I2C_Mm_Qn_LOAD_DATA(master, queue));
+
+	i = 0;
+	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 | 1 << 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 *cci = i2c_get_adapdata(adap);
+	int i;
+	int ret = 0;
+
+	if (!num)
+		return -EOPNOTSUPP;
+
+	for (i = 0; i < num; i++) {
+		if (msgs[i].flags & I2C_M_RD)
+			ret = cci_i2c_read(cci, msgs[i].addr, msgs[i].buf,
+					   msgs[i].len);
+		else
+			ret = cci_i2c_write(cci, msgs[i].addr, msgs[i].buf,
+					    msgs[i].len);
+
+		if (ret < 0) {
+			dev_err(cci->dev, "cci i2c xfer error %d", ret);
+			break;
+		}
+	}
+
+	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 const struct i2c_adapter_quirks cci_quirks_v1_0_8 = {
+	.max_write_len = 10,
+	.max_read_len = 12,
+};
+
+static const struct i2c_adapter_quirks cci_quirks_v1_4_0 = {
+	.max_write_len = 11,
+	.max_read_len = 12,
+};
+
+/*
+ * cci_probe - Probe CCI platform device
+ * @pdev: Pointer to CCI platform device
+ *
+ * Return 0 on success or a negative error code on failure
+ */
+static int cci_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct resources *res;
+	const struct hw_params *hw;
+	struct cci *cci;
+	struct resource *r;
+	int ret = 0;
+	u32 val;
+	int i;
+
+	cci = devm_kzalloc(&pdev->dev, sizeof(*cci), GFP_KERNEL);
+	if (!cci)
+		return -ENOMEM;
+
+	cci->dev = dev;
+	platform_set_drvdata(pdev, cci);
+
+	if (of_device_is_compatible(dev->of_node, "qcom,cci-v1.0.8")) {
+		res = &res_v1_0_8;
+		hw = hw_params_v1_0_8;
+		cci->queue_size[0] = queue_0_size_v1_0_8;
+		cci->queue_size[1] = queue_1_size_v1_0_8;
+		cci->adap.quirks = &cci_quirks_v1_0_8;
+	} else if (of_device_is_compatible(dev->of_node, "qcom,cci-v1.4.0")) {
+		res = &res_v1_4_0;
+		hw = hw_params_v1_4_0;
+		cci->queue_size[0] = queue_0_size_v1_4_0;
+		cci->queue_size[1] = queue_1_size_v1_4_0;
+		cci->adap.quirks = &cci_quirks_v1_4_0;
+	} else {
+		return -EINVAL;
+	}
+
+	cci->adap.algo = &cci_algo;
+	cci->adap.dev.parent = cci->dev;
+	cci->adap.dev.of_node = dev->of_node;
+	i2c_set_adapdata(&cci->adap, cci);
+
+	strlcpy(cci->adap.name, "Qualcomm Camera Control Interface",
+		sizeof(cci->adap.name));
+
+	cci->mode = I2C_MODE_STANDARD;
+	ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &val);
+	if (!ret) {
+		if (val == 400000)
+			cci->mode = I2C_MODE_FAST;
+		else if (val == 1000000)
+			cci->mode = I2C_MODE_FAST_PLUS;
+	}
+
+	/* Memory */
+
+	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, res->reg[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 */
+
+	r = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
+					 res->interrupt[0]);
+	if (!r) {
+		dev_err(dev, "missing IRQ\n");
+		return -EINVAL;
+	}
+
+	cci->irq = r->start;
+	snprintf(cci->irq_name, sizeof(cci->irq_name), "%s", dev_name(dev));
+	ret = devm_request_irq(dev, cci->irq, cci_isr,
+			       IRQF_TRIGGER_RISING, cci->irq_name, 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 (res->clock[cci->nclocks])
+		cci->nclocks++;
+
+	cci->clock = devm_kzalloc(dev, cci->nclocks *
+				  sizeof(*cci->clock), GFP_KERNEL);
+	if (!cci->clock)
+		return -ENOMEM;
+
+	for (i = 0; i < cci->nclocks; i++) {
+		struct cci_clock *clock = &cci->clock[i];
+
+		clock->clk = devm_clk_get(dev, res->clock[i]);
+		if (IS_ERR(clock->clk))
+			return PTR_ERR(clock->clk);
+
+		clock->name = res->clock[i];
+		clock->freq = res->clock_rate[i];
+	}
+
+	ret = cci_enable_clocks(cci->nclocks, cci->clock, dev);
+	if (ret < 0)
+		return ret;
+
+	val = readl_relaxed(cci->base + CCI_HW_VERSION);
+	dev_info(dev, "%s: CCI HW version = 0x%08x", __func__, val);
+
+	init_completion(&cci->master[0].irq_complete);
+	init_completion(&cci->master[1].irq_complete);
+
+	enable_irq(cci->irq);
+
+	ret = cci_reset(cci);
+	if (ret < 0)
+		return ret;
+
+	ret = cci_init(cci, &hw[cci->mode]);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_add_adapter(&cci->adap);
+
+	return ret;
+}
+
+/*
+ * cci_remove - Remove CCI platform device
+ * @pdev: Pointer to CCI platform device
+ *
+ * Always returns 0.
+ */
+static int cci_remove(struct platform_device *pdev)
+{
+	struct cci *cci = platform_get_drvdata(pdev);
+
+	disable_irq(cci->irq);
+	cci_disable_clocks(cci->nclocks, cci->clock);
+
+	i2c_del_adapter(&cci->adap);
+
+	return 0;
+}
+
+static const struct of_device_id cci_dt_match[] = {
+	{ .compatible = "qcom,cci-v1.0.8" },
+	{ .compatible = "qcom,cci-v1.4.0" },
+	{}
+};
+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_ALIAS("platform:i2c-qcom-cci");
+MODULE_DESCRIPTION("Qualcomm Camera Control Interface driver");
+MODULE_AUTHOR("Todor Tomov <todor.tomov@linaro.org>");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH 2/4] i2c: Add Qualcomm Camera Control Interface driver

[Bjorn Andersson]

On Mon 02 Oct 07:13 PDT 2017, Todor Tomov wrote:
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
[..]
> +#define NUM_MASTERS 1

So you will only register one of the masters?

[..]
> +enum cci_i2c_queue_t {
> +	QUEUE_0,
> +	QUEUE_1

Could these be called QUEUE_READ and QUEUE_WRITE instead?

> +};
> +
> +enum cci_i2c_master_t {
> +	MASTER_0,
> +	MASTER_1

Just use 0 and 1 when denoting the two masters.

> +};
> +
> +struct resources {

This struct name is not awesome, name it something like cci_res instead.

> +	char *clock[CCI_RES_MAX];
> +	u32 clock_rate[CCI_RES_MAX];
> +	char *reg[CCI_RES_MAX];
> +	char *interrupt[CCI_RES_MAX];
> +};
[..]
> +
> +struct cci_master {
> +	u32 status;

You use this to pass 0 or negative errno between functions, so make it
int.

> +	u8 complete_pending;

bool

> +	struct completion irq_complete;
> +};
> +
> +struct cci {
> +	struct device *dev;
> +	struct i2c_adapter adap;
> +	void __iomem *base;
> +	u32 irq;

"irq" is generally supposed to be a "int".

> +	char irq_name[30];
> +	struct cci_clock *clock;

If you set the rate of your clocks initially you can replace this array
with clk_bulk_data and use the clk_bulk_prepare_enable() and
clk_bulk_disable_unprepare().

> +	int nclocks;
> +	u8 mode;
> +	u16 queue_size[NUM_QUEUES];
> +	struct cci_master master[NUM_MASTERS];
> +};
> +
> +static const struct resources res_v1_0_8 = {
> +	.clock = { "camss_top_ahb",
> +		   "cci_ahb",
> +		   "camss_ahb",
> +		   "cci" },

The code consuming this will read until NULL, so please add terminator.
It happens to work as the first clock_rate is 0 in both cases.

> +	.clock_rate = { 0,
> +			80000000,
> +			0,
> +			19200000 },
> +	.reg = { "cci" },
> +	.interrupt = { "cci" }

No need to specify reg and interrupt here until they actually need to be
configured; just put the strings in the code.

> +};
> +
[..]
> +
> +/*
> + * cci_enable_clocks - Enable multiple clocks

Correct kerneldoc would be:

/**
 * cci_enable_clocks() - Enable multiple clocks

> + * @nclocks: Number of clocks in clock array
> + * @clock: Clock array
> + * @dev: Device
> + *
> + * Return 0 on success or a negative error code otherwise
> + */
> +int cci_enable_clocks(int nclocks, struct cci_clock *clock, struct device *dev)

Overall this is clk_bulk_prepare_enable(), please use that.

[..]
> +/*
> + * cci_disable_clocks - Disable multiple clocks
> + * @nclocks: Number of clocks in clock array
> + * @clock: Clock array
> + */
> +void cci_disable_clocks(int nclocks, struct cci_clock *clock)

Overall this is clk_bulk_disable_unprepare(), so please use that.

> +{
> +	int i;
> +
> +	for (i = nclocks - 1; i >= 0; i--)
> +		clk_disable_unprepare(clock[i].clk);
> +}
> +
[..]
> +
> +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 < 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));
> +
> +		cci->master[i].status = 0;

I don't think you need to initialize status here, it's always written
before read in the rest of the driver.

> +	}
> +
> +	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 = 1 << ((master * 2) + queue);

BIT(master * 2 + queue)

> +	writel(val, cci->base + CCI_QUEUE_START);
> +
> +	time = wait_for_completion_timeout(&cci->master[master].irq_complete,
> +					   CCI_TIMEOUT_MS);
> +	if (!time) {
> +		dev_err(cci->dev, "master %d queue %d timeout\n",
> +			master, queue);
> +		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, u32 len, u8 master, u8 queue)
> +{
> +	int ret = 0;
> +	u32 val;
> +
> +	val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue));
> +
> +	if (val + len + 1 > cci->queue_size[queue]) {

	if (val + len >= cci->queue_size[queue])


Or even better, flip this around and do:

	if (val + len < cci->queue_size[queue])
		return 0;

	val = ..
	writel()

	return cci_run_queue();


That said, this function is always called with len ==
cci->queue_size[queue]. So this will always "run the queue" unless val
== 0. So you can simplify this function slightly by dropping the "len".

> +		val = CCI_I2C_REPORT | (1 << 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 addr, u8 *buf, u16 len)
> +{
> +	u8 master = MASTER_0;
> +	u8 queue = QUEUE_1;
> +	u32 val;
> +	u32 words_read, words_exp;
> +	int i, index, first;
> +	int ret;
> +
> +	if (len > cci->adap.quirks->max_read_len)
> +		return -EOPNOTSUPP;
> +

The core already checks each entry against the max length quirks.

But are these actually the max amount of data you can read in one
operation? Generally one has to drain the fifo but the actual transfers
can be larger...

[..]
> +}
> +
> +static int cci_i2c_write(struct cci *cci, u16 addr, u8 *buf, u16 len)
> +{
> +	u8 master = MASTER_0;
> +	u8 queue = QUEUE_0;
> +	u8 load[12] = { 0 };
> +	u8 i, j;

Use int for counters.

> +	u32 val;
> +	int ret;
> +
> +	if (len > cci->adap.quirks->max_write_len)
> +		return -EOPNOTSUPP;
> +

This is taken care of by the core.

[..]
> +}
> +
> +static int cci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> +{
> +	struct cci *cci = i2c_get_adapdata(adap);
> +	int i;
> +	int ret = 0;
> +
> +	if (!num)
> +		return -EOPNOTSUPP;

I don't think you need to handle this case explicitly.

> +
> +	for (i = 0; i < num; i++) {
> +		if (msgs[i].flags & I2C_M_RD)
> +			ret = cci_i2c_read(cci, msgs[i].addr, msgs[i].buf,
> +					   msgs[i].len);
> +		else
> +			ret = cci_i2c_write(cci, msgs[i].addr, msgs[i].buf,
> +					    msgs[i].len);
> +
> +		if (ret < 0) {
> +			dev_err(cci->dev, "cci i2c xfer error %d", ret);
> +			break;
> +		}
> +	}
> +
> +	return ret;

This should return num on success.

[..]
> +static int cci_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
[..]
> +	cci = devm_kzalloc(&pdev->dev, sizeof(*cci), GFP_KERNEL);

Use dev instead &pdev->dev here, of just use pdev->dev throughout the
function.

[..]
> +	cci->mode = I2C_MODE_STANDARD;

cci->mode is a local variable, please don't put it in the struct.

> +	ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &val);
> +	if (!ret) {
> +		if (val == 400000)
> +			cci->mode = I2C_MODE_FAST;
> +		else if (val == 1000000)
> +			cci->mode = I2C_MODE_FAST_PLUS;
> +	}
> +
> +	/* Memory */
> +
> +	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, res->reg[0]);

You only have a single reg, so please just use

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 */
> +
> +	r = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
> +					 res->interrupt[0]);
> +	if (!r) {
> +		dev_err(dev, "missing IRQ\n");
> +		return -EINVAL;
> +	}
> +
> +	cci->irq = r->start;

Please replace this with:

cci->irq = platform_get_irq(pdev, 0);

> +	snprintf(cci->irq_name, sizeof(cci->irq_name), "%s", dev_name(dev));

And just hard code this in the function call.

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

The time between devm_request_irq() and disable_irq() is still long
enough for cci_isr() to be called, before irq_complete is initialized.

Don't request irqs until you're ready to receive the interrupts.

> +
> +	/* Clocks */
> +
> +	cci->nclocks = 0;
> +	while (res->clock[cci->nclocks])
> +		cci->nclocks++;
> +
> +	cci->clock = devm_kzalloc(dev, cci->nclocks *
> +				  sizeof(*cci->clock), GFP_KERNEL);

This can max be CCI_RES_MAX (i.e. 6) so just make the array fixed size
and skip the kzalloc().

> +	if (!cci->clock)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < cci->nclocks; i++) {
> +		struct cci_clock *clock = &cci->clock[i];
> +
> +		clock->clk = devm_clk_get(dev, res->clock[i]);
> +		if (IS_ERR(clock->clk))
> +			return PTR_ERR(clock->clk);
> +
> +		clock->name = res->clock[i];
> +		clock->freq = res->clock_rate[i];
> +	}
> +
> +	ret = cci_enable_clocks(cci->nclocks, cci->clock, dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = readl_relaxed(cci->base + CCI_HW_VERSION);
> +	dev_info(dev, "%s: CCI HW version = 0x%08x", __func__, val);

Please don't "spam" the kernel log by default, if this is useful make it
dev_dbg() and people can easily enable the print.

> +
> +	init_completion(&cci->master[0].irq_complete);
> +	init_completion(&cci->master[1].irq_complete);
> +
> +	enable_irq(cci->irq);
> +
> +	ret = cci_reset(cci);
> +	if (ret < 0)

Clocks needs to be disabled if this fails.

> +		return ret;
> +
> +	ret = cci_init(cci, &hw[cci->mode]);
> +	if (ret < 0)

Dito

> +		return ret;
> +
> +	ret = i2c_add_adapter(&cci->adap);

Dito

> +
> +	return ret;
> +}
[..]
> +MODULE_ALIAS("platform:i2c-qcom-cci");

You don't need to specify this alias.

Regards,
Bjorn

Re: [PATCH 2/4] i2c: Add Qualcomm Camera Control Interface driver

[Todor Tomov]

Hi Bjorn,

Thank you for the review. There are a lot of important suggestions.

On  6.10.2017 08:20, Bjorn Andersson wrote:
> On Mon 02 Oct 07:13 PDT 2017, Todor Tomov wrote:
>> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> [..]
>> +#define NUM_MASTERS 1
> 
> So you will only register one of the masters?

Yes, in this version of the driver - only one. Probably I will extend the driver later
to support also the second master on 8096.

> 
> [..]
>> +enum cci_i2c_queue_t {
>> +	QUEUE_0,
>> +	QUEUE_1
> 
> Could these be called QUEUE_READ and QUEUE_WRITE instead?

There isn't anything read or write specific in the queues, this is just how we assign them.
Both of them can do both operations. So I think that we should not narrow the names to read
and write.

> 
>> +};
>> +
>> +enum cci_i2c_master_t {
>> +	MASTER_0,
>> +	MASTER_1
> 
> Just use 0 and 1 when denoting the two masters.

Ok.

> 
>> +};
>> +
>> +struct resources {
> 
> This struct name is not awesome, name it something like cci_res instead.

Ok.

> 
>> +	char *clock[CCI_RES_MAX];
>> +	u32 clock_rate[CCI_RES_MAX];
>> +	char *reg[CCI_RES_MAX];
>> +	char *interrupt[CCI_RES_MAX];
>> +};
> [..]
>> +
>> +struct cci_master {
>> +	u32 status;
> 
> You use this to pass 0 or negative errno between functions, so make it
> int.

Ops.

> 
>> +	u8 complete_pending;
> 
> bool

Ok.

> 
>> +	struct completion irq_complete;
>> +};
>> +
>> +struct cci {
>> +	struct device *dev;
>> +	struct i2c_adapter adap;
>> +	void __iomem *base;
>> +	u32 irq;
> 
> "irq" is generally supposed to be a "int".

Ok.

> 
>> +	char irq_name[30];
>> +	struct cci_clock *clock;
> 
> If you set the rate of your clocks initially you can replace this array
> with clk_bulk_data and use the clk_bulk_prepare_enable() and
> clk_bulk_disable_unprepare().

Ok, I'll switch to the bulk API.

> 
>> +	int nclocks;
>> +	u8 mode;
>> +	u16 queue_size[NUM_QUEUES];
>> +	struct cci_master master[NUM_MASTERS];
>> +};
>> +
>> +static const struct resources res_v1_0_8 = {
>> +	.clock = { "camss_top_ahb",
>> +		   "cci_ahb",
>> +		   "camss_ahb",
>> +		   "cci" },
> 
> The code consuming this will read until NULL, so please add terminator.

The fields which are not explicitely initialized are set to NULL, right?
I may add a comment that a space in the array for the terminator must be left.

> It happens to work as the first clock_rate is 0 in both cases.

I think that it works because it reaches the terminator.

> 
>> +	.clock_rate = { 0,
>> +			80000000,
>> +			0,
>> +			19200000 },
>> +	.reg = { "cci" },
>> +	.interrupt = { "cci" }
> 
> No need to specify reg and interrupt here until they actually need to be
> configured; just put the strings in the code.
> 

Ok.

>> +};
>> +
> [..]
>> +
>> +/*
>> + * cci_enable_clocks - Enable multiple clocks
> 
> Correct kerneldoc would be:
> 
> /**
>  * cci_enable_clocks() - Enable multiple clocks

Ok.

> 
>> + * @nclocks: Number of clocks in clock array
>> + * @clock: Clock array
>> + * @dev: Device
>> + *
>> + * Return 0 on success or a negative error code otherwise
>> + */
>> +int cci_enable_clocks(int nclocks, struct cci_clock *clock, struct device *dev)
> 
> Overall this is clk_bulk_prepare_enable(), please use that.

Yes, I will.

> 
> [..]
>> +/*
>> + * cci_disable_clocks - Disable multiple clocks
>> + * @nclocks: Number of clocks in clock array
>> + * @clock: Clock array
>> + */
>> +void cci_disable_clocks(int nclocks, struct cci_clock *clock)
> 
> Overall this is clk_bulk_disable_unprepare(), so please use that.

Yes.

> 
>> +{
>> +	int i;
>> +
>> +	for (i = nclocks - 1; i >= 0; i--)
>> +		clk_disable_unprepare(clock[i].clk);
>> +}
>> +
> [..]
>> +
>> +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 < 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));
>> +
>> +		cci->master[i].status = 0;
> 
> I don't think you need to initialize status here, it's always written
> before read in the rest of the driver.
> 

Ok.

>> +	}
>> +
>> +	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 = 1 << ((master * 2) + queue);
> 
> BIT(master * 2 + queue)

Ok. Will use BIT() also on the rest of the places where it can be used.

> 
>> +	writel(val, cci->base + CCI_QUEUE_START);
>> +
>> +	time = wait_for_completion_timeout(&cci->master[master].irq_complete,
>> +					   CCI_TIMEOUT_MS);
>> +	if (!time) {
>> +		dev_err(cci->dev, "master %d queue %d timeout\n",
>> +			master, queue);
>> +		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, u32 len, u8 master, u8 queue)
>> +{
>> +	int ret = 0;
>> +	u32 val;
>> +
>> +	val = readl(cci->base + CCI_I2C_Mm_Qn_CUR_WORD_CNT(master, queue));
>> +
>> +	if (val + len + 1 > cci->queue_size[queue]) {
> 
> 	if (val + len >= cci->queue_size[queue])
> 
> 
> Or even better, flip this around and do:
> 
> 	if (val + len < cci->queue_size[queue])
> 		return 0;
> 
> 	val = ..
> 	writel()
> 
> 	return cci_run_queue();
> 
> 
> That said, this function is always called with len ==
> cci->queue_size[queue]. So this will always "run the queue" unless val
> == 0. So you can simplify this function slightly by dropping the "len".

Yes, I'll rework this and remove "len".

> 
>> +		val = CCI_I2C_REPORT | (1 << 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 addr, u8 *buf, u16 len)
>> +{
>> +	u8 master = MASTER_0;
>> +	u8 queue = QUEUE_1;
>> +	u32 val;
>> +	u32 words_read, words_exp;
>> +	int i, index, first;
>> +	int ret;
>> +
>> +	if (len > cci->adap.quirks->max_read_len)
>> +		return -EOPNOTSUPP;
>> +
> 
> The core already checks each entry against the max length quirks.
> 
> But are these actually the max amount of data you can read in one
> operation? Generally one has to drain the fifo but the actual transfers
> can be larger...

Yes, the maximum data which you can read in one operation. And this is
the meaning of quirks->max_read_len, right? Not the i2c transfer size.
So the check is correct but I'll remove it as it is already done in
i2c_check_for_quirks(), as you have pointed out.

> 
> [..]
>> +}
>> +
>> +static int cci_i2c_write(struct cci *cci, u16 addr, u8 *buf, u16 len)
>> +{
>> +	u8 master = MASTER_0;
>> +	u8 queue = QUEUE_0;
>> +	u8 load[12] = { 0 };
>> +	u8 i, j;
> 
> Use int for counters.

Ok.

> 
>> +	u32 val;
>> +	int ret;
>> +
>> +	if (len > cci->adap.quirks->max_write_len)
>> +		return -EOPNOTSUPP;
>> +
> 
> This is taken care of by the core.

Yes, I'll remove it.

> 
> [..]
>> +}
>> +
>> +static int cci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>> +{
>> +	struct cci *cci = i2c_get_adapdata(adap);
>> +	int i;
>> +	int ret = 0;
>> +
>> +	if (!num)
>> +		return -EOPNOTSUPP;
> 
> I don't think you need to handle this case explicitly.

Ok.

> 
>> +
>> +	for (i = 0; i < num; i++) {
>> +		if (msgs[i].flags & I2C_M_RD)
>> +			ret = cci_i2c_read(cci, msgs[i].addr, msgs[i].buf,
>> +					   msgs[i].len);
>> +		else
>> +			ret = cci_i2c_write(cci, msgs[i].addr, msgs[i].buf,
>> +					    msgs[i].len);
>> +
>> +		if (ret < 0) {
>> +			dev_err(cci->dev, "cci i2c xfer error %d", ret);
>> +			break;
>> +		}
>> +	}
>> +
>> +	return ret;
> 
> This should return num on success.

Yes.

> 
> [..]
>> +static int cci_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
> [..]
>> +	cci = devm_kzalloc(&pdev->dev, sizeof(*cci), GFP_KERNEL);
> 
> Use dev instead &pdev->dev here, of just use pdev->dev throughout the
> function.

Ok.

> 
> [..]
>> +	cci->mode = I2C_MODE_STANDARD;
> 
> cci->mode is a local variable, please don't put it in the struct.

Ok.

> 
>> +	ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency", &val);
>> +	if (!ret) {
>> +		if (val == 400000)
>> +			cci->mode = I2C_MODE_FAST;
>> +		else if (val == 1000000)
>> +			cci->mode = I2C_MODE_FAST_PLUS;
>> +	}
>> +
>> +	/* Memory */
>> +
>> +	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, res->reg[0]);
> 
> You only have a single reg, so please just use
> 
> r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 

Ok.

> 
>> +	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 */
>> +
>> +	r = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
>> +					 res->interrupt[0]);
>> +	if (!r) {
>> +		dev_err(dev, "missing IRQ\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	cci->irq = r->start;
> 
> Please replace this with:
> 
> cci->irq = platform_get_irq(pdev, 0);

Ok.

> 
>> +	snprintf(cci->irq_name, sizeof(cci->irq_name), "%s", dev_name(dev));
> 
> And just hard code this in the function call.

Ok.

> 
>> +	ret = devm_request_irq(dev, cci->irq, cci_isr,
>> +			       IRQF_TRIGGER_RISING, cci->irq_name, cci);
>> +	if (ret < 0) {
>> +		dev_err(dev, "request_irq failed, ret: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	disable_irq(cci->irq);
> 
> The time between devm_request_irq() and disable_irq() is still long
> enough for cci_isr() to be called, before irq_complete is initialized.
> 
> Don't request irqs until you're ready to receive the interrupts.

This makes sense however at this point the clocks to the device are
still not enabled, doesn't this avoid any problems?

> 
>> +
>> +	/* Clocks */
>> +
>> +	cci->nclocks = 0;
>> +	while (res->clock[cci->nclocks])
>> +		cci->nclocks++;
>> +
>> +	cci->clock = devm_kzalloc(dev, cci->nclocks *
>> +				  sizeof(*cci->clock), GFP_KERNEL);
> 
> This can max be CCI_RES_MAX (i.e. 6) so just make the array fixed size
> and skip the kzalloc().

Does it matter?

> 
>> +	if (!cci->clock)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < cci->nclocks; i++) {
>> +		struct cci_clock *clock = &cci->clock[i];
>> +
>> +		clock->clk = devm_clk_get(dev, res->clock[i]);
>> +		if (IS_ERR(clock->clk))
>> +			return PTR_ERR(clock->clk);
>> +
>> +		clock->name = res->clock[i];
>> +		clock->freq = res->clock_rate[i];
>> +	}
>> +
>> +	ret = cci_enable_clocks(cci->nclocks, cci->clock, dev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	val = readl_relaxed(cci->base + CCI_HW_VERSION);
>> +	dev_info(dev, "%s: CCI HW version = 0x%08x", __func__, val);
> 
> Please don't "spam" the kernel log by default, if this is useful make it
> dev_dbg() and people can easily enable the print.

Ok.

> 
>> +
>> +	init_completion(&cci->master[0].irq_complete);
>> +	init_completion(&cci->master[1].irq_complete);
>> +
>> +	enable_irq(cci->irq);
>> +
>> +	ret = cci_reset(cci);
>> +	if (ret < 0)
> 
> Clocks needs to be disabled if this fails.

Ok.

> 
>> +		return ret;
>> +
>> +	ret = cci_init(cci, &hw[cci->mode]);
>> +	if (ret < 0)
> 
> Dito

Ok.

> 
>> +		return ret;
>> +
>> +	ret = i2c_add_adapter(&cci->adap);
> 
> Dito

Ok.

> 
>> +
>> +	return ret;
>> +}
> [..]
>> +MODULE_ALIAS("platform:i2c-qcom-cci");
> 
> You don't need to specify this alias.

Ok.

> 
> Regards,
> Bjorn
> 

-- 
Best regards,
Todor Tomov

Re: [PATCH 2/4] i2c: Add Qualcomm Camera Control Interface driver

[Bjorn Andersson]

On Thu 12 Oct 07:47 PDT 2017, Todor Tomov wrote:

> Hi Bjorn,
> 
> Thank you for the review. There are a lot of important suggestions.
> 
> On  6.10.2017 08:20, Bjorn Andersson wrote:
> > On Mon 02 Oct 07:13 PDT 2017, Todor Tomov wrote:
> >> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> > [..]
> >> +#define NUM_MASTERS 1
> > 
> > So you will only register one of the masters?
> 
> Yes, in this version of the driver - only one. Probably I will extend the driver later
> to support also the second master on 8096.
> 

That's perfectly fine, as long as there's a plan of how to add the
second master that is backwards compatible (DT wise).

[..]
> >> +static const struct resources res_v1_0_8 = {
> >> +	.clock = { "camss_top_ahb",
> >> +		   "cci_ahb",
> >> +		   "camss_ahb",
> >> +		   "cci" },
> > 
> > The code consuming this will read until NULL, so please add terminator.
> 
> The fields which are not explicitely initialized are set to NULL, right?
> I may add a comment that a space in the array for the terminator must be left.
> 

Right, I didn't think of this list being a statically sized array, then
there are implicit NULLs here.

> > It happens to work as the first clock_rate is 0 in both cases.
> 
> I think that it works because it reaches the terminator.
> 

Adding a NULL after the list will make this more obvious and continue to
work as intended.

[..]
> >> +	if (len > cci->adap.quirks->max_read_len)
> >> +		return -EOPNOTSUPP;
> >> +
> > 
> > The core already checks each entry against the max length quirks.
> > 
> > But are these actually the max amount of data you can read in one
> > operation? Generally one has to drain the fifo but the actual transfers
> > can be larger...
> 
> Yes, the maximum data which you can read in one operation. And this is
> the meaning of quirks->max_read_len, right? Not the i2c transfer size.
> So the check is correct but I'll remove it as it is already done in
> i2c_check_for_quirks(), as you have pointed out.
> 

AFAICT you're doing it correct. I just find it surprising that these
limits are so low (in the hardware) - in the qup driver people
complained that the max is 255.

[..]
> >> +	ret = devm_request_irq(dev, cci->irq, cci_isr,
> >> +			       IRQF_TRIGGER_RISING, cci->irq_name, cci);
> >> +	if (ret < 0) {
> >> +		dev_err(dev, "request_irq failed, ret: %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	disable_irq(cci->irq);
> > 
> > The time between devm_request_irq() and disable_irq() is still long
> > enough for cci_isr() to be called, before irq_complete is initialized.
> > 
> > Don't request irqs until you're ready to receive the interrupts.
> 
> This makes sense however at this point the clocks to the device are
> still not enabled, doesn't this avoid any problems?
> 

You're probably right, but unless it's too much hassle it's better to
be on the safe side - if nothing else for the times when someone
copy-paste your code for a new driver that doesn't follow this premise.

> > 
> >> +
> >> +	/* Clocks */
> >> +
> >> +	cci->nclocks = 0;
> >> +	while (res->clock[cci->nclocks])
> >> +		cci->nclocks++;
> >> +
> >> +	cci->clock = devm_kzalloc(dev, cci->nclocks *
> >> +				  sizeof(*cci->clock), GFP_KERNEL);
> > 
> > This can max be CCI_RES_MAX (i.e. 6) so just make the array fixed size
> > and skip the kzalloc().
> 
> Does it matter?
> 

At the cost of making the list statically sized in the struct you can
drop this call and error check. But other than that, no.

> > 
> >> +	if (!cci->clock)
> >> +		return -ENOMEM;
> >> +

Regards,
Bjorn

[PATCH 3/4] MAINTAINERS: Add Qualcomm Camera Control Interface driver

[Todor Tomov]

Add an entry for Qualcomm Camera Control Interface driver.

Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6671f37..aa3484f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11158,6 +11158,13 @@ W:	http://wireless.kernel.org/en/users/Drivers/ath9k
 S:	Supported
 F:	drivers/net/wireless/ath/ath9k/
 
+QUALCOMM CAMERA CONTROL INTERFACE DRIVER
+M:	Todor Tomov <todor.tomov@linaro.org>
+L:	linux-i2c@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
+F:	drivers/i2c/busses/i2c-qcom-cci.c
+
 QUALCOMM CAMERA SUBSYSTEM DRIVER
 M:	Todor Tomov <todor.tomov@linaro.org>
 L:	linux-media@vger.kernel.org
-- 
2.7.4

[PATCH 4/4] arm64: dts: qcom: Add Camera Control Interface support

[Todor Tomov]

This commit adds the CCI node for the CCI controller that resides on
the Qualcomm MSM8916 and MSM8996 platforms.

CC: Andy Gross <andy.gross@linaro.org>
CC: Rob Herring <robh+dt@kernel.org>
CC: Mark Rutland <mark.rutland@arm.com>
CC: devicetree@vger.kernel.org
CC: linux-soc@vger.kernel.org
CC: linux-arm-kernel@lists.infradead.org
Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8916-pins.dtsi | 14 ++++++++++++++
 arch/arm64/boot/dts/qcom/msm8916.dtsi      | 21 +++++++++++++++++++++
 arch/arm64/boot/dts/qcom/msm8996-pins.dtsi | 14 ++++++++++++++
 arch/arm64/boot/dts/qcom/msm8996.dtsi      | 24 ++++++++++++++++++++++++
 4 files changed, 73 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi
index 4cb0b58..4fa6a72 100644
--- a/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi
@@ -733,4 +733,18 @@
 			bias-pull-up;
 		};
 	};
+
+	cci_lines {
+		cci0_default: cci0_default {
+			pinmux {
+				function = "cci_i2c";
+				pins = "gpio29", "gpio30";
+			};
+			pinconf {
+				pins = "gpio29", "gpio30";
+				drive-strength = <16>;
+				bias-disable;
+			};
+		};
+	};
 };
diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index dc38175..4885360 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -1325,6 +1325,27 @@
 				compatible = "venus-encoder";
 			};
 		};
+
+		cci: qcom,cci@1b0c000 {
+			compatible = "qcom,cci-v1.0.8";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x1b0c000 0x1000>;
+			reg-names = "cci";
+			interrupts = <GIC_SPI 50 IRQ_TYPE_EDGE_RISING>;
+			interrupt-names = "cci";
+			clocks = <&gcc GCC_CAMSS_TOP_AHB_CLK>,
+				<&gcc GCC_CAMSS_CCI_AHB_CLK>,
+				<&gcc GCC_CAMSS_CCI_CLK>,
+				<&gcc GCC_CAMSS_AHB_CLK>;
+			clock-names = "camss_top_ahb",
+				"cci_ahb",
+				"cci",
+				"camss_ahb";
+			pinctrl-names = "default";
+			pinctrl-0 = <&cci0_default>;
+			status = "disabled";
+		};
 	};
 
 	smd {
diff --git a/arch/arm64/boot/dts/qcom/msm8996-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8996-pins.dtsi
index 6599404..3faaa4c 100644
--- a/arch/arm64/boot/dts/qcom/msm8996-pins.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996-pins.dtsi
@@ -300,4 +300,18 @@
 			drive-strength = <2>;	/* 2 MA */
 		};
 	};
+
+	cci_lines {
+		cci0_default: cci0_default {
+			pinmux {
+				function = "cci_i2c";
+				pins = "gpio17", "gpio18";
+			};
+			pinconf {
+				pins = "gpio17", "gpio18";
+				drive-strength = <16>;
+				bias-disable;
+			};
+		};
+	};
 };
diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 887b61c..118722f 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -819,6 +819,30 @@
 				phy-names = "usb2-phy", "usb3-phy";
 			};
 		};
+
+		cci: qcom,cci@a0c000 {
+			compatible = "qcom,cci-v1.4.0";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0xa0c000 0x1000>;
+			reg-names = "cci";
+			interrupts = <GIC_SPI 295 IRQ_TYPE_EDGE_RISING>;
+			interrupt-names = "cci";
+			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";
+			pinctrl-names = "default";
+			pinctrl-0 = <&cci0_default>;
+			status = "disabled";
+		};
 	};
 
 	adsp-pil {
-- 
2.7.4