diff mbox series

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

Message ID 20180806110416.4288-2-vkoul@kernel.org
State New
Headers show
Series [v3,1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller | expand

Commit Message

Vinod Koul Aug. 6, 2018, 11:04 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       | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt

-- 
2.14.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Bjorn Andersson Aug. 6, 2018, 6:03 p.m. UTC | #1
On Mon 06 Aug 04:04 PDT 2018, 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       | 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 000000000000..977978dd4444

> --- /dev/null

> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt

> @@ -0,0 +1,55 @@

> +Qualcomm Camera Control Interface (CCI) I2C controller

> +

> +Required properties:

> + - compatible: Should be one of:

> +   - "qcom,cci-v1.0.8" for 8916;

> +   - "qcom,cci-v1.4.0" for 8996.


As pointed out by Rob previously we should either use version numbers or
platform numbers, not both.

So this should either be:

	- "qcom,cci-v1.0.8"
	- "qcom,cci-v1.4.0"

or:

	- "qcom,msm8916-cci"
	- "qcom,msm8996-cci"

> + - #address-cells: Should be <1>.

> + - #size-cells: Should be <0>.

> + - reg: Base address of the I2C controller and length of memory mapped region.

> + - interrupts: Specifier for CCI I2C interrupt.

> + - clocks: List of clock specifiers, one for each entry in clock-names.

> + - clock-names: Should contain:

> +   - "mmss_mmagic_ahb" - on qcom,cci-v1.4.0 only;

> +   - "camss_top_ahb";

> +   - "cci_ahb";

> +   - "cci";

> +   - "camss_ahb".

> +

> +Required subnodes:

> + - i2c-bus instances:

> +	 Mandatory i2c-bus0 subnode

> +	 Mandatory i2c-bus1 for qcom,cci-v1.4.0


Rather than a bullet list, the subnodes are probably better described in
text. I think most versions have two masters, so in order to not have to
update this every time we add a new compatible we could probably write
this in a more generic fashion.

E.g.

Required subnodes:

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

> + - Optional properties:


"Optional properties for subnodes"

> +	- clock-frequency: Desired I2C bus clock frequency

> +	  in Hz, defaults to 100 kHz if omitted.

> +

> +Required properties on qcom,cci-v1.4.0:

> + - power-domains: Power domain specifier.


Properties has to come before any subnodes in the dts, so move this
above the subnode definition as well.

> +

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


Seems lines above this is indented with space and the following with
tabs.

> +			i2c-bus0 {

> +	                        clock-frequency = <400000>;

> +			};

> +			i2c-bus1 {

> +				clock-frequency = <400000>;

> +			};

> +                };


Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vinod Koul Aug. 7, 2018, 4:18 a.m. UTC | #2
On 06-08-18, 11:03, Bjorn Andersson wrote:
> On Mon 06 Aug 04:04 PDT 2018, 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       | 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 000000000000..977978dd4444

> > --- /dev/null

> > +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt

> > @@ -0,0 +1,55 @@

> > +Qualcomm Camera Control Interface (CCI) I2C controller

> > +

> > +Required properties:

> > + - compatible: Should be one of:

> > +   - "qcom,cci-v1.0.8" for 8916;

> > +   - "qcom,cci-v1.4.0" for 8996.

> 

> As pointed out by Rob previously we should either use version numbers or

> platform numbers, not both.

> 

> So this should either be:

> 

> 	- "qcom,cci-v1.0.8"

> 	- "qcom,cci-v1.4.0"


ok will use this one..

> 

> or:

> 

> 	- "qcom,msm8916-cci"

> 	- "qcom,msm8996-cci"

> 

> > + - #address-cells: Should be <1>.

> > + - #size-cells: Should be <0>.

> > + - reg: Base address of the I2C controller and length of memory mapped region.

> > + - interrupts: Specifier for CCI I2C interrupt.

> > + - clocks: List of clock specifiers, one for each entry in clock-names.

> > + - clock-names: Should contain:

> > +   - "mmss_mmagic_ahb" - on qcom,cci-v1.4.0 only;

> > +   - "camss_top_ahb";

> > +   - "cci_ahb";

> > +   - "cci";

> > +   - "camss_ahb".

> > +

> > +Required subnodes:

> > + - i2c-bus instances:

> > +	 Mandatory i2c-bus0 subnode

> > +	 Mandatory i2c-bus1 for qcom,cci-v1.4.0

> 

> Rather than a bullet list, the subnodes are probably better described in

> text. I think most versions have two masters, so in order to not have to

> update this every time we add a new compatible we could probably write

> this in a more generic fashion.

> 

> E.g.

> 

> Required subnodes:

> 

> The CCI provides I2C masters for one or two i2c busses, described as

> subdevices named "i2c-bus0" and "i2c-bus1".


okay but the v1-0-8 doesn't (8916) seem to have two busses, later ones yes.

> 

> > + - Optional properties:

> 

> "Optional properties for subnodes"

> 

> > +	- clock-frequency: Desired I2C bus clock frequency

> > +	  in Hz, defaults to 100 kHz if omitted.

> > +

> > +Required properties on qcom,cci-v1.4.0:

> > + - power-domains: Power domain specifier.

> 

> Properties has to come before any subnodes in the dts, so move this

> above the subnode definition as well.


yes will do

> 

> > +

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

> 

> Seems lines above this is indented with space and the following with

> tabs.


will fix

Thanks for the review.
-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Andersson Aug. 7, 2018, 6:07 p.m. UTC | #3
On Tue 07 Aug 10:39 PDT 2018, Rob Herring wrote:

> On Tue, Aug 07, 2018 at 09:48:26AM +0530, Vinod wrote:

> > On 06-08-18, 11:03, Bjorn Andersson wrote:

> > > On Mon 06 Aug 04:04 PDT 2018, 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       | 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 000000000000..977978dd4444

> > > > --- /dev/null

> > > > +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt

> > > > @@ -0,0 +1,55 @@

> > > > +Qualcomm Camera Control Interface (CCI) I2C controller

> > > > +

> > > > +Required properties:

> > > > + - compatible: Should be one of:

> > > > +   - "qcom,cci-v1.0.8" for 8916;

> > > > +   - "qcom,cci-v1.4.0" for 8996.

> > > 

> > > As pointed out by Rob previously we should either use version numbers or

> > > platform numbers, not both.

> 

> Not really what I said...

> 


Sorry, I thought this was part of your concern.

Then I'll revise my statement to: if we're going to list the platforms
here anyways I think we should just use the platform numbering as
compatible directly.

> > > 

> > > So this should either be:

> > > 

> > > 	- "qcom,cci-v1.0.8"


This is supposed to be 1.1.0 for 8916...

> > > 	- "qcom,cci-v1.4.0"

> > 

> > ok will use this one..

> > 

> > > 

> > > or:

> > > 

> > > 	- "qcom,msm8916-cci"

> > > 	- "qcom,msm8996-cci"

> 

> I strongly prefer this one as I've yet to be convinced there is a strong 

> benefit of version numbers and every other binding follows this 

> convention.

> 


Looking through the list of versions for this block and the platforms
that there's any upstream interest in we have 6 versions covering 7
platforms.

So I agree with you.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring (Arm) Aug. 8, 2018, 2:48 p.m. UTC | #4
On Wed, Aug 8, 2018 at 8:03 AM Todor Tomov <todor.tomov@linaro.org> wrote:
>

> Hi Bjorn,

>

> On  7.08.2018 21:07, Bjorn Andersson wrote:

> > On Tue 07 Aug 10:39 PDT 2018, Rob Herring wrote:

> >

> >> On Tue, Aug 07, 2018 at 09:48:26AM +0530, Vinod wrote:

> >>> On 06-08-18, 11:03, Bjorn Andersson wrote:

> >>>> On Mon 06 Aug 04:04 PDT 2018, 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       | 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 000000000000..977978dd4444

> >>>>> --- /dev/null

> >>>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt

> >>>>> @@ -0,0 +1,55 @@

> >>>>> +Qualcomm Camera Control Interface (CCI) I2C controller

> >>>>> +

> >>>>> +Required properties:

> >>>>> + - compatible: Should be one of:

> >>>>> +   - "qcom,cci-v1.0.8" for 8916;

> >>>>> +   - "qcom,cci-v1.4.0" for 8996.

> >>>>

> >>>> As pointed out by Rob previously we should either use version numbers or

> >>>> platform numbers, not both.

> >>

> >> Not really what I said...

> >>

> >

> > Sorry, I thought this was part of your concern.

> >

> > Then I'll revise my statement to: if we're going to list the platforms

> > here anyways I think we should just use the platform numbering as

> > compatible directly.

> >

> >>>>

> >>>> So this should either be:

> >>>>

> >>>>    - "qcom,cci-v1.0.8"

> >

> > This is supposed to be 1.1.0 for 8916...

>

> There is conflicting information about this in the documentation. However the version read from the hw version register is 1.0.8 and this matches the value listed in the SWI too so I believe 1.0.8 is correct for 8916.


And this is why we don't use version numbers. Unless you have access
to the RTL repositories with version tags then it is just error prone.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vinod Koul Aug. 8, 2018, 4:07 p.m. UTC | #5
On 08-08-18, 08:48, Rob Herring wrote:
> On Wed, Aug 8, 2018 at 8:03 AM Todor Tomov <todor.tomov@linaro.org> wrote:

> > >>>> So this should either be:

> > >>>>

> > >>>>    - "qcom,cci-v1.0.8"

> > >

> > > This is supposed to be 1.1.0 for 8916...

> >

> > There is conflicting information about this in the documentation.

> > However the version read from the hw version register is 1.0.8 and

> > this matches the value listed in the SWI too so I believe 1.0.8 is

> > correct for 8916.

> 

> And this is why we don't use version numbers. Unless you have access

> to the RTL repositories with version tags then it is just error prone.


Yeah we have a HW_VERSION register too, but someone might forget to
populate it, so lets go ahead and use the soc numbers as discussed.

Thanks
-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
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..977978dd4444
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
@@ -0,0 +1,55 @@ 
+Qualcomm Camera Control Interface (CCI) 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 I2C controller and length of memory mapped region.
+ - interrupts: Specifier for CCI I2C interrupt.
+ - clocks: List of clock specifiers, one for each entry in clock-names.
+ - clock-names: Should contain:
+   - "mmss_mmagic_ahb" - on qcom,cci-v1.4.0 only;
+   - "camss_top_ahb";
+   - "cci_ahb";
+   - "cci";
+   - "camss_ahb".
+
+Required subnodes:
+ - i2c-bus instances:
+	 Mandatory i2c-bus0 subnode
+	 Mandatory i2c-bus1 for qcom,cci-v1.4.0
+ - Optional properties:
+	- clock-frequency: Desired I2C bus clock frequency
+	  in Hz, defaults to 100 kHz if omitted.
+
+Required properties on qcom,cci-v1.4.0:
+ - power-domains: Power domain specifier.
+
+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";
+			i2c-bus0 {
+	                        clock-frequency = <400000>;
+			};
+			i2c-bus1 {
+				clock-frequency = <400000>;
+			};
+                };