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 000..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 = ;
>> +interrupt-names = "cci";
>> +power-domains = < CAMSS_GDSC>;
>> +clocks = < MMSS_MMAGIC_AHB_CLK>,
>> +< CAMSS_TOP_AHB_CLK>,
>> +< CAMSS_CCI_AHB_CLK>,
>> +< CAMSS_CCI_CLK>,
>> +< CAMSS_AHB_CLK>;
>> +clock-names = "mmss_mmagic_ahb",
>> +"camss_top_ahb",
>> +"cci_ahb",
>> +"cci",
>> +"camss_ahb";
>> +pinctrl-names = "default";
>> +pinctrl-0 = <_default>;
>> +clock-frequency = <40>;
>> +};
> 
> 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 
> 
> 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

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 000..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 = ;
>> +interrupt-names = "cci";
>> +power-domains = < CAMSS_GDSC>;
>> +clocks = < MMSS_MMAGIC_AHB_CLK>,
>> +< CAMSS_TOP_AHB_CLK>,
>> +< CAMSS_CCI_AHB_CLK>,
>> +< CAMSS_CCI_CLK>,
>> +< CAMSS_AHB_CLK>;
>> +clock-names = "mmss_mmagic_ahb",
>> +"camss_top_ahb",
>> +"cci_ahb",
>> +"cci",
>> +"camss_ahb";
>> +pinctrl-names = "default";
>> +pinctrl-0 = <_default>;
>> +clock-frequency = <40>;
>> +};
> 
> 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 
> 
> 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

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 000..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 = ;
> +interrupt-names = "cci";
> +power-domains = < CAMSS_GDSC>;
> +clocks = < MMSS_MMAGIC_AHB_CLK>,
> +< CAMSS_TOP_AHB_CLK>,
> +< CAMSS_CCI_AHB_CLK>,
> +< CAMSS_CCI_CLK>,
> +< CAMSS_AHB_CLK>;
> +clock-names = "mmss_mmagic_ahb",
> +"camss_top_ahb",
> +"cci_ahb",
> +"cci",
> +"camss_ahb";
> +pinctrl-names = "default";
> +pinctrl-0 = <_default>;
> +clock-frequency = <40>;
> +};

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 

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

[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 000..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 = ;
> +interrupt-names = "cci";
> +power-domains = < CAMSS_GDSC>;
> +clocks = < MMSS_MMAGIC_AHB_CLK>,
> +< CAMSS_TOP_AHB_CLK>,
> +< CAMSS_CCI_AHB_CLK>,
> +< CAMSS_CCI_CLK>,
> +< CAMSS_AHB_CLK>;
> +clock-names = "mmss_mmagic_ahb",
> +"camss_top_ahb",
> +"cci_ahb",
> +"cci",
> +"camss_ahb";
> +pinctrl-names = "default";
> +pinctrl-0 = <_default>;
> +clock-frequency = <40>;
> +};

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 

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

[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 
CC: Mark Rutland 
CC: devicet...@vger.kernel.org
Signed-off-by: Todor Tomov 
---
 .../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 000..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 = ;
+interrupt-names = "cci";
+power-domains = < CAMSS_GDSC>;
+clocks = < MMSS_MMAGIC_AHB_CLK>,
+< CAMSS_TOP_AHB_CLK>,
+< CAMSS_CCI_AHB_CLK>,
+< CAMSS_CCI_CLK>,
+< CAMSS_AHB_CLK>;
+clock-names = "mmss_mmagic_ahb",
+"camss_top_ahb",
+"cci_ahb",
+"cci",
+"camss_ahb";
+pinctrl-names = "default";
+pinctrl-0 = <_default>;
+clock-frequency = <40>;
+};
-- 
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 
CC: Mark Rutland 
CC: devicet...@vger.kernel.org
Signed-off-by: Todor Tomov 
---
 .../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 000..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 = ;
+interrupt-names = "cci";
+power-domains = < CAMSS_GDSC>;
+clocks = < MMSS_MMAGIC_AHB_CLK>,
+< CAMSS_TOP_AHB_CLK>,
+< CAMSS_CCI_AHB_CLK>,
+< CAMSS_CCI_CLK>,
+< CAMSS_AHB_CLK>;
+clock-names = "mmss_mmagic_ahb",
+"camss_top_ahb",
+"cci_ahb",
+"cci",
+"camss_ahb";
+pinctrl-names = "default";
+pinctrl-0 = <_default>;
+clock-frequency = <40>;
+};
-- 
2.7.4