| Message ID | 20180806110416.4288-2-vkoul@kernel.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | i2c: Add support for Qcom CCI I2C controller | expand |
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
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.
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... > > > > 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" 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. 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
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
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. > >>>> - "qcom,cci-v1.4.0"
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
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
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>; + }; + };