diff mbox

[v2,1/2] dt-bindings: i2c: Add binding document for Qualcomm CCI

Message ID 20180717125729.23045-2-vkoul@kernel.org (mailing list archive)
State Not Applicable, archived
Delegated to: Andy Gross
Headers show

Commit Message

Vinod Koul July 17, 2018, 12:57 p.m. UTC
From: Todor Tomov <todor.tomov@linaro.org>

Add DT binding document for Qualcomm Camera Control Interface device

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

Comments

Rob Herring (Arm) July 20, 2018, 6:03 p.m. UTC | #1
On Tue, Jul 17, 2018 at 06:27:28PM +0530, Vinod Koul wrote:
> From: Todor Tomov <todor.tomov@linaro.org>
> 
> Add DT binding document for Qualcomm Camera Control Interface device
> 
> Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  .../devicetree/bindings/i2c/i2c-qcom-cci.txt       | 46 ++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> new file mode 100644
> index 000000000000..cb1fe158fba3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> @@ -0,0 +1,46 @@
> +Qualcomm Camera Control Interface controller
> +
> +Required properties:
> + - compatible: Should be one of:
> +   - "qcom,cci-v1.0.8" for 8916;
> +   - "qcom,cci-v1.4.0" for 8996.

It's not very convincing to use version numbers instead of normal SoC 
part# convention when there is a 1-1 mapping.

> + - #address-cells: Should be <1>.
> + - #size-cells: Should be <0>.
> + - reg: Base address of the controller and length of memory mapped region.
> + - interrupts: Specifier for CCI interrupt.
> + - 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".
> +
> +Required properties on 8996:

Rather than '8996', use the compatible string.

> + - power-domains: Power domain specifier.
> +
> +Optional:
> + - clock-frequency: Desired I2C bus clock frequency in Hz, defaults to 100 kHz
> +   if omitted.

This is the only place that says this block is or has a I2C controller. 
Please make that more explicit (including what ever else this block 
does). I'm guessing there's more given how many clocks and that it has a 
power domain.

> +
> +Example:
> +
> +                cci@a0c000 {
> +                        compatible = "qcom,cci-v1.4.0";
> +                        #address-cells = <1>;
> +                        #size-cells = <0>;
> +                        reg = <0xa0c000 0x1000>;
> +                        interrupts = <GIC_SPI 295 IRQ_TYPE_EDGE_RISING>;
> +                        power-domains = <&mmcc CAMSS_GDSC>;
> +                        clocks = <&mmcc MMSS_MMAGIC_AHB_CLK>,
> +                                        <&mmcc CAMSS_TOP_AHB_CLK>,
> +                                        <&mmcc CAMSS_CCI_AHB_CLK>,
> +                                        <&mmcc CAMSS_CCI_CLK>,
> +                                        <&mmcc CAMSS_AHB_CLK>;
> +                        clock-names = "mmss_mmagic_ahb",
> +                                        "camss_top_ahb",
> +                                        "cci_ahb",
> +                                        "cci",
> +                                        "camss_ahb";
> +                        clock-frequency = <400000>;
> +                };
> -- 
> 2.14.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vinod Koul July 23, 2018, 3:54 a.m. UTC | #2
On 20-07-18, 12:03, Rob Herring wrote:
> On Tue, Jul 17, 2018 at 06:27:28PM +0530, Vinod Koul wrote:
> > From: Todor Tomov <todor.tomov@linaro.org>

> > +Required properties:
> > + - compatible: Should be one of:
> > +   - "qcom,cci-v1.0.8" for 8916;
> > +   - "qcom,cci-v1.4.0" for 8996.
> 
> It's not very convincing to use version numbers instead of normal SoC 
> part# convention when there is a 1-1 mapping.

I was under the impression that we don't have 1-1 mapping. An IP version
is found in multiple SoCs

> > + - #address-cells: Should be <1>.
> > + - #size-cells: Should be <0>.
> > + - reg: Base address of the controller and length of memory mapped region.
> > + - interrupts: Specifier for CCI interrupt.
> > + - 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".
> > +
> > +Required properties on 8996:
> 
> Rather than '8996', use the compatible string.

Makes sense

> > + - power-domains: Power domain specifier.
> > +
> > +Optional:
> > + - clock-frequency: Desired I2C bus clock frequency in Hz, defaults to 100 kHz
> > +   if omitted.
> 
> This is the only place that says this block is or has a I2C controller. 
> Please make that more explicit (including what ever else this block 
> does). I'm guessing there's more given how many clocks and that it has a 
> power domain.

Yes this also goes GPIO which will be added after this.
Rob Herring (Arm) July 23, 2018, 1:20 p.m. UTC | #3
On Sun, Jul 22, 2018 at 9:54 PM Vinod <vkoul@kernel.org> wrote:
>
> On 20-07-18, 12:03, Rob Herring wrote:
> > On Tue, Jul 17, 2018 at 06:27:28PM +0530, Vinod Koul wrote:
> > > From: Todor Tomov <todor.tomov@linaro.org>
>
> > > +Required properties:
> > > + - compatible: Should be one of:
> > > +   - "qcom,cci-v1.0.8" for 8916;
> > > +   - "qcom,cci-v1.4.0" for 8996.
> >
> > It's not very convincing to use version numbers instead of normal SoC
> > part# convention when there is a 1-1 mapping.
>
> I was under the impression that we don't have 1-1 mapping. An IP version
> is found in multiple SoCs

Maybe so, but I'm only looking at what is getting upstream. We already
discussed this at length on the thermal binding[1].

As you've written it, we'll still have to update the doc to add other
chips for an already documented version.

Rob

[1] https://lkml.org/lkml/2018/7/5/830
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
new file mode 100644
index 000000000000..cb1fe158fba3
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
@@ -0,0 +1,46 @@ 
+Qualcomm Camera Control Interface 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.
+ - interrupts: Specifier for CCI interrupt.
+ - 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".
+
+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@a0c000 {
+                        compatible = "qcom,cci-v1.4.0";
+                        #address-cells = <1>;
+                        #size-cells = <0>;
+                        reg = <0xa0c000 0x1000>;
+                        interrupts = <GIC_SPI 295 IRQ_TYPE_EDGE_RISING>;
+                        power-domains = <&mmcc CAMSS_GDSC>;
+                        clocks = <&mmcc MMSS_MMAGIC_AHB_CLK>,
+                                        <&mmcc CAMSS_TOP_AHB_CLK>,
+                                        <&mmcc CAMSS_CCI_AHB_CLK>,
+                                        <&mmcc CAMSS_CCI_CLK>,
+                                        <&mmcc CAMSS_AHB_CLK>;
+                        clock-names = "mmss_mmagic_ahb",
+                                        "camss_top_ahb",
+                                        "cci_ahb",
+                                        "cci",
+                                        "camss_ahb";
+                        clock-frequency = <400000>;
+                };