diff mbox series

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

Message ID 20180820063953.6866-2-vkoul@kernel.org
State Changes Requested
Headers show
Series i2c: Add support for Qcom CCI I2C controller | expand

Commit Message

Vinod Koul Aug. 20, 2018, 6:39 a.m. UTC 
From: Todor Tomov <todor.tomov@linaro.org>

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

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

Comments

Rob Herring Aug. 20, 2018, 6:18 p.m. UTC | #1 
On Mon, Aug 20, 2018 at 12:09:52PM +0530, Vinod Koul wrote:
> From: Todor Tomov <todor.tomov@linaro.org>
> 
> Add DT binding document for Qualcomm Camera Control Interface (CCI)
> I2C controller.
> 
> Signed-off-by: Todor Tomov <todor.tomov@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  .../devicetree/bindings/i2c/i2c-qcom-cci.txt       | 83 ++++++++++++++++++++++
>  1 file changed, 83 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..b7f4240ce5c8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> @@ -0,0 +1,83 @@
> +Qualcomm Camera Control Interface (CCI) I2C controller
> +
> +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'.

> +
> +- 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";
> +
> +- power-domains
> +	Usage: required for "qcom,msm-8996-cci"
> +	Value type: <prop-encoded-array>
> +	Definition:
> +
> +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 <robh@kernel.org>

> +
> +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>;
> +		};
> +	};
> -- 
> 2.14.4
>
Vinod Koul Aug. 21, 2018, 9:28 a.m. UTC | #2 
On 20-08-18, 13:18, Rob Herring wrote:
> On Mon, Aug 20, 2018 at 12:09:52PM +0530, Vinod Koul wrote:

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

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

> With those fixed,
> 
> Reviewed-by: Rob Herring <robh@kernel.org>

Great, thanks for the review.
Rob Herring Aug. 21, 2018, 1:11 p.m. UTC | #3 
On Tue, Aug 21, 2018 at 4:28 AM Vinod <vkoul@kernel.org> wrote:
>
> On 20-08-18, 13:18, Rob Herring wrote:
> > On Mon, Aug 20, 2018 at 12:09:52PM +0530, Vinod Koul wrote:
>
> > > +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.
>
> > > +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.

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

Rob
Rob Herring Aug. 21, 2018, 1:12 p.m. UTC | #4 
Hit send too soon...

On Tue, Aug 21, 2018 at 8:11 AM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Aug 21, 2018 at 4:28 AM Vinod <vkoul@kernel.org> wrote:
> >
> > On 20-08-18, 13:18, Rob Herring wrote:
> > > On Mon, Aug 20, 2018 at 12:09:52PM +0530, Vinod Koul wrote:
> >
> > > > +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.
> >
> > > > +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.
>
> 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

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

Rob
Vinod Koul Aug. 21, 2018, 4 p.m. UTC | #5 
On 21-08-18, 08:12, Rob Herring wrote:
> Hit send too soon...
> 
> On Tue, Aug 21, 2018 at 8:11 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Aug 21, 2018 at 4:28 AM Vinod <vkoul@kernel.org> wrote:
> > >
> > > On 20-08-18, 13:18, Rob Herring wrote:
> > > > On Mon, Aug 20, 2018 at 12:09:52PM +0530, Vinod Koul wrote:
> > >
> > > > > +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.
> > >
> > > > > +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.
> >
> > 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
> 
> ...we can match i2c buses in a generic way.

I tried i2c-bus@0, but wasn't able to do it properly, let me try again
with this approach. It does sound great to me, will update..
diff mbox series

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..b7f4240ce5c8
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
@@ -0,0 +1,83 @@ 
+Qualcomm Camera Control Interface (CCI) I2C controller
+
+PROPERTIES:
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be one of:
+		"qcom,msm-8916-cci"
+		"qcom,msm-8996-cci"
+
+- 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";
+
+- power-domains
+	Usage: required for "qcom,msm-8996-cci"
+	Value type: <prop-encoded-array>
+	Definition:
+
+SUBNODES:
+
+The CCI provides I2C masters for one or two i2c busses, described as
+subdevices named "i2c-bus0" and "i2c-bus1".
+
+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>;
+		};
+	};