i2c: Add support for Qcom CCI I2C controller
v3 cover i2c: Add support for Qcom CCI I2C controller
Hi,
This series adds support for Qualcomm Camera Control Interface (CCI) I2C controller. This block contains one/two I2C masters and registers these adapters based on the controller probed on.
Changes in v3:
- Update DT binding per RobH comments
- Add the subnode for bus instances
- Move all compataible data to driver data
- add num_master data and use that to register and use masters
Changes in v2:
- Add support for two adapters
Todor Tomov (2):
dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller
i2c: Add Qualcomm CCI I2C driver
.../devicetree/bindings/i2c/i2c-qcom-cci.txt | 55 ++ drivers/i2c/busses/Kconfig | 10 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-qcom-cci.c | 814 +++++++++++++++++++++ 4 files changed, 880 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt create mode 100644 drivers/i2c/busses/i2c-qcom-cci.c
Signature · 1 line
2.14.4
PATCH v3 1/2 dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller
From: Todor Tomov <[email protected]>
Add DT binding document for Qualcomm Camera Control Interface (CCI) I2C controller.
--- .../devicetree/bindings/i2c/i2c-qcom-cci.txt | 55 ++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +55 -0
@@ -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>; + }; + };
Signature · 1 line
2.14.4
Re: [PATCH v3 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller
Quoted 18 lines · Vinod Koul
From: Todor Tomov <[email protected]>
Add DT binding document for Qualcomm Camera Control Interface (CCI) I2C controller.
Signed-off-by Todor Tomov <[email protected]>Signed-off-by Vinod Koul <[email protected]>--- .../devicetree/bindings/i2c/i2c-qcom-cci.txt | 55 ++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txtDocumentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +0
-0
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 6
Quoted 6 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +6
-0@@ -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"
Quoted 6 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +6
-0+ - #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:
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 22
Quoted 10 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +10
-0+ - "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".
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 23
Quoted 1 line
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +1
-0+ - Optional properties:
"Optional properties for subnodes"
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 28
Quoted 5 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +5
-0+ - 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.
Quoted 10 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +10
-0+ +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>;
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 48
Quoted 10 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +10
-0+ 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.
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 55
Quoted 7 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +7
-0+ i2c-bus0 { + clock-frequency = <400000>; + }; + i2c-bus1 { + clock-frequency = <400000>; + }; + };
Regards,
Bjorn
Re: [PATCH v3 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller
Quoted 30 lines · Bjorn Andersson
Quoted 18 lines · Vinod Koul
From: Todor Tomov <[email protected]>
Add DT binding document for Qualcomm Camera Control Interface (CCI) I2C controller.
Signed-off-by Todor Tomov <[email protected]>Signed-off-by Vinod Koul <[email protected]>--- .../devicetree/bindings/i2c/i2c-qcom-cci.txt | 55 ++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txtDocumentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +0
-0Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 6
Quoted 6 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +6
-0@@ -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.
Quoted 4 lines
So this should either be:
- "qcom,cci-v1.0.8"
- "qcom,cci-v1.4.0"
ok will use this one..
Quoted 28 lines
or:
- "qcom,msm8916-cci"
- "qcom,msm8996-cci"Quoted 6 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +6
-0+ - #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:Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 22
Quoted 10 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +10
-0+ - "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.0Rather 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.
Quoted 6 lines
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.
Quoted 5 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 23
Quoted 2 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +1
-0+ - Optional properties:"Optional properties for subnodes"
Quoted 8 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 28
Quoted 5 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +5
-0+ - 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
Quoted 16 lines
Quoted 16 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +15
-0+ +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>;
Quoted 8 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 48
Quoted 5 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +5
-0+ 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.
Signature · 1 line
~Vinod
Rob Herring
maintainerRe: [PATCH v3 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller
Quoted 15 lines · Vinod
Quoted 14 lines · Bjorn Andersson
Quoted 12 lines · Vinod Koul
From: Todor Tomov <[email protected]>
Add DT binding document for Qualcomm Camera Control Interface (CCI) I2C controller.
Signed-off-by Todor Tomov <[email protected]>Signed-off-by Vinod Koul <[email protected]>--- .../devicetree/bindings/i2c/i2c-qcom-cci.txt | 55 ++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
Quoted 15 lines
Quoted 7 lines
Quoted 7 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +1
-0@@ -0,0 +1,55 @@ +Qualcomm Camera Control Interface (CCI) I2C controllerQuoted 8 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 6
Quoted 5 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +5
-0+ +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...
Quoted 13 lines
Quoted 5 lines
So this should either be:
- "qcom,cci-v1.0.8"
- "qcom,cci-v1.4.0"ok will use this one..
Quoted 5 lines
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
Re: [PATCH v3 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller
Quoted 32 lines · Rob Herring
Quoted 15 lines · Vinod
Quoted 14 lines · Bjorn Andersson
Quoted 12 lines · Vinod Koul
From: Todor Tomov <[email protected]>
Add DT binding document for Qualcomm Camera Control Interface (CCI) I2C controller.
Signed-off-by Todor Tomov <[email protected]>Signed-off-by Vinod Koul <[email protected]>--- .../devicetree/bindings/i2c/i2c-qcom-cci.txt | 55 ++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txtQuoted 15 lines
Quoted 7 lines
Quoted 7 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +1
-0@@ -0,0 +1,55 @@ +Qualcomm Camera Control Interface (CCI) I2C controllerQuoted 8 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 6
Quoted 5 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +5
-0+ +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.
Quoted 2 lines
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.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
So this should either be:
- "qcom,cci-v1.0.8"
This is supposed to be 1.1.0 for 8916...
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
- "qcom,cci-v1.4.0"
ok will use this one..
Quoted 1 line
Quoted 9 lines
Quoted 4 lines
Quoted 4 lines
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
Re: [PATCH v3 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller
Hi Bjorn,
Quoted 44 lines · Bjorn Andersson
Quoted 32 lines · Rob Herring
Quoted 15 lines · Vinod
Quoted 14 lines · Bjorn Andersson
Quoted 12 lines · Vinod Koul
From: Todor Tomov <[email protected]>
Add DT binding document for Qualcomm Camera Control Interface (CCI) I2C controller.
Signed-off-by Todor Tomov <[email protected]>Signed-off-by Vinod Koul <[email protected]>--- .../devicetree/bindings/i2c/i2c-qcom-cci.txt | 55 ++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txtQuoted 15 lines
Quoted 7 lines
Quoted 7 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +1
-0@@ -0,0 +1,55 @@ +Qualcomm Camera Control Interface (CCI) I2C controllerQuoted 8 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 6
Quoted 5 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +5
-0+ +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.
Quoted 2 lines
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.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
- "qcom,cci-v1.4.0"
Signature · 2 lines
Best regards, Todor Tomov
Rob Herring
maintainerRe: [PATCH v3 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller
Quoted 48 lines · Todor Tomov
Hi Bjorn,
Quoted 44 lines · Bjorn Andersson
Quoted 32 lines · Rob Herring
Quoted 15 lines · Vinod
Quoted 14 lines · Bjorn Andersson
Quoted 12 lines · Vinod Koul
From: Todor Tomov <[email protected]>
Add DT binding document for Qualcomm Camera Control Interface (CCI) I2C controller.
Signed-off-by Todor Tomov <[email protected]>Signed-off-by Vinod Koul <[email protected]>--- .../devicetree/bindings/i2c/i2c-qcom-cci.txt | 55 ++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txtQuoted 15 lines
Quoted 7 lines
Quoted 7 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +1
-0@@ -0,0 +1,55 @@ +Qualcomm Camera Control Interface (CCI) I2C controllerQuoted 8 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt line 6
Quoted 5 lines
Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt new file +5
-0+ +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.
Quoted 2 lines
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.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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
Re: [PATCH v3 1/2] dt-bindings: i2c: Add binding for Qualcomm CCI I2C controller
Quoted 7 lines · Rob Herring
Quoted 6 lines · Todor Tomov
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
So this should either be:
- "qcom,cci-v1.0.8"
This is supposed to be 1.1.0 for 8916...
Quoted 7 lines
Quoted 4 lines
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
Signature · 1 line
~Vinod
PATCH v3 2/2 i2c: Add Qualcomm CCI I2C driver
In reply to Vinod KoulFrom: Todor Tomov <[email protected]>
This commit adds I2C driver for the Camera Control Interface (CCI) I2C controller found on the Qualcomm SoC processors.
CCI versions supported:
- 1.0.8 - found on MSM8916/APQ8016
- 1.4.0 - found on MSM8996/APQ8096
--- drivers/i2c/busses/Kconfig | 10 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-qcom-cci.c | 814 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 825 insertions(+) create mode 100644 drivers/i2c/busses/i2c-qcom-cci.c
drivers/i2c/busses/Kconfig +10 -0
@@ -828,6 +828,16 @@ config I2C_PXA_SLAVE is necessary for systems where the PXA may be a target on the I2C bus. +config I2C_QCOM_CCI + tristate "Qualcomm Camera Control Interface" + depends on ARCH_QCOM + help + If you say yes to this option, support will be included for the + built-in camera control interface on the Qualcomm SoCs. + + This driver can also be built as a module. If so, the module + will be called i2c-qcom-cci. + config I2C_QUP tristate "Qualcomm QUP based I2C controller" depends on ARCH_QCOM
drivers/i2c/busses/Makefile +1 -0
@@ -83,6 +83,7 @@ obj-$(CONFIG_I2C_PNX) += i2c-pnx.o obj-$(CONFIG_I2C_PUV3) += i2c-puv3.o obj-$(CONFIG_I2C_PXA) += i2c-pxa.o obj-$(CONFIG_I2C_PXA_PCI) += i2c-pxa-pci.o +obj-$(CONFIG_I2C_QCOM_CCI) += i2c-qcom-cci.o obj-$(CONFIG_I2C_QUP) += i2c-qup.o obj-$(CONFIG_I2C_RIIC) += i2c-riic.o obj-$(CONFIG_I2C_RK3X) += i2c-rk3x.o
drivers/i2c/busses/i2c-qcom-cci.c new file +814 -0
814 lines not on this page · raw message
Signature · 1 line
2.14.4
Re: [PATCH v3 2/2] i2c: Add Qualcomm CCI I2C driver
On Mon 06 Aug 04:04 PDT 2018, Vinod Koul wrote:
[..]
Quoted 1 line
drivers/i2c/busses/i2c-qcom-cci.c +0
-0
[..]
Quoted 4 lines
drivers/i2c/busses/i2c-qcom-cci.c +4
-0+struct hw_params { +u16 thigh; +u16 tlow; +u16 tsu_sto;
drivers/i2c/busses/i2c-qcom-cci.c line 116
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-cci.c +10
-0+u16 tsu_sta; +u16 thd_dat; +u16 thd_sta; +u16 tbuf; +u8 scl_stretch_en; +u16 trdhld; +u16 tsp; +}; + +struct cci_clock {
This is now unused.
Quoted 6 lines
drivers/i2c/busses/i2c-qcom-cci.c +6
-0+struct clk *clk; +const char *name; +u32 freq; +}; + +struct cci ;
drivers/i2c/busses/i2c-qcom-cci.c line 132
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-cci.c +10
-0+ +struct cci_master { +struct i2c_adapter adap; +u16 master; +u8 mode; +int status; +bool complete_pending; +struct completion irq_complete; +struct cci *cci; +
Empty line.
Quoted 4 lines
drivers/i2c/busses/i2c-qcom-cci.c +4
-0+}; + +struct cci_data { +unsigned int num_masters;
drivers/i2c/busses/i2c-qcom-cci.c line 146
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-cci.c +10
-0+struct i2c_adapter_quirks quirks; +u16 queue_size[NUM_QUEUES ]; +struct cci_res res; +struct hw_params params[3 ]; +}; + +struct cci { +struct device *dev; +void __iomem *base; +u32 irq;
Use unsigned int rather than a type of specific size
Quoted 13 lines
drivers/i2c/busses/i2c-qcom-cci.c +13
-0+const struct cci_data *data; +struct clk_bulk_data *clock; +u32 *clock_freq; +int nclocks; +struct cci_master master[NUM_MASTERS ]; +}; + +/** +* cci_clock_set_rate() - Set clock frequency rates +* @nclocks: Number of clocks +* @clock: Clock array +* @clock_freq: Clock frequency rate array +* @dev: Device
drivers/i2c/busses/i2c-qcom-cci.c line 169
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-cci.c +10
-0+* +* Return 0 on success or a negative error code otherwise +*/ +static int cci_clock_set_rate (int nclocks,struct clk_bulk_data *clock, +u32 *clock_freq,struct device *dev) +{ +int i, ret; +long rate; + +for (i =0 ; i < nclocks; i++)
Multiline loop deserves {}
Quoted 18 lines
drivers/i2c/busses/i2c-qcom-cci.c +18
-0+if (clock_freq[i]) { + rate =clk_round_rate (clock[i].clk, clock_freq[i]); +if (rate <0 ) { +dev_err (dev,"clk round rate failed: %ld\n" , + rate); +return rate; + } + + ret =clk_set_rate (clock[i].clk, clock_freq[i]); +if (ret <0 ) { +dev_err (dev,"clk set rate failed: %d\n" , ret); +return ret; + } + } + +return 0 ; +} +
[..]
[..]
drivers/i2c/busses/i2c-qcom-cci.c line 293
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-cci.c +10
-0+static int cci_reset (struct cci *cci) +{ +unsigned long time; + + cci->master[0 ].complete_pending = true; +writel (CCI_RESET_CMD_MASK , cci->base +CCI_RESET_CMD ); + + time =wait_for_completion_timeout + (&cci->master[0 ].irq_complete, +msecs_to_jiffies (CCI_TIMEOUT_MS ));
Please rework the indentation of this.
Also CCI_TIMEOUT_MS is converted to jiffies in all the places, define it in jiffies instead.
Quoted 7 lines
drivers/i2c/busses/i2c-qcom-cci.c +7
-0+if (!time) { +dev_err (cci->dev,"CCI reset timeout\n" ); +return -ETIMEDOUT ; + } + +return 0 ; +}
[..]
drivers/i2c/busses/i2c-qcom-cci.c line 381
Quoted 12 lines
drivers/i2c/busses/i2c-qcom-cci.c +12
-0+static int cci_validate_queue (struct cci *cci,u8 master,u8 queue) +{ +int ret =0 ; +u32 val; + + val =readl (cci->base +CCI_I2C_Mm_Qn_CUR_WORD_CNT (master, queue)); + +if (val == cci->data->queue_size[queue]) +return -EINVAL ; + +if (val) { + val =CCI_I2C_REPORT |BIT (8 );
Can we get a define (or a comment) for BIT(8) as well?
drivers/i2c/busses/i2c-qcom-cci.c line 387
Quoted 6 lines
drivers/i2c/busses/i2c-qcom-cci.c +6
-0+writel (val, cci->base +CCI_I2C_Mm_Qn_LOAD_DATA (master, queue)); + + ret =cci_run_queue (cci, master, queue); + } + +return ret;
Rather than wrapping the second half of the function in a check for val, return early on !val and then you can end with return cci_run_queue() and drop the "ret".
drivers/i2c/busses/i2c-qcom-cci.c line 396
Quoted 9 lines
drivers/i2c/busses/i2c-qcom-cci.c +9
-0+} + +static int cci_i2c_read (struct cci *cci,u16 master, +u16 addr,u8 *buf,u16 len) +{ +u32 val, words_read, words_exp; +u8 queue =QUEUE_1 ; +int i, index =0 , ret; +bool first =0 ;
s/0/false/
Quoted 16 lines
drivers/i2c/busses/i2c-qcom-cci.c +16
-0+ +/* +* Call validate queue to make sure queue is empty before starting. +* This is to avoid overflow / underflow of queue. +*/ + ret =cci_validate_queue (cci, master, queue); +if (ret <0 ) +return ret; + + val =CCI_I2C_SET_PARAM | (addr &0x7f ) <<4 ; +writel (val, cci->base +CCI_I2C_Mm_Qn_LOAD_DATA (master, queue)); + + val =CCI_I2C_READ | len <<4 ; +writel (val, cci->base +CCI_I2C_Mm_Qn_LOAD_DATA (master, queue)); + + ret =cci_run_queue (cci, master, queue);
drivers/i2c/busses/i2c-qcom-cci.c line 422
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-cci.c +10
-0+if (ret <0 ) +return ret; + + words_read =readl (cci->base +CCI_I2C_Mm_READ_BUF_LEVEL (master)); + words_exp = len /4 +1 ; +if (words_read != words_exp) { +dev_err (cci->dev,"words read = %d, words expected = %d\n" , + words_read, words_exp); +return -EIO ; + }
I thought that an i2c read could return less data than was requested...
Quoted 15 lines
drivers/i2c/busses/i2c-qcom-cci.c +15
-0+ +do { + val =readl (cci->base +CCI_I2C_Mm_READ_DATA (master)); + +for (i =0 ; i <4 && index < len; i++) { +if (first) { + first = false; +continue ; + } + buf[index++] = (val >> (i *8 )) &0xff ; + } + }while (--words_read); + +return 0 ; +}
[..]
Quoted 2 lines
drivers/i2c/busses/i2c-qcom-cci.c +2
-0+static int cci_probe (struct platform_device *pdev) +{
[..]
drivers/i2c/busses/i2c-qcom-cci.c line 607
Quoted 12 lines
drivers/i2c/busses/i2c-qcom-cci.c +12
-0+/* Clocks */ + + cci->nclocks =0 ; +while (cci->data->res.clock[cci->nclocks]) + cci->nclocks++; + + cci->clock =devm_kcalloc (dev, cci->nclocks, +sizeof (*cci->clock),GFP_KERNEL ); +if (!cci->clock) +return -ENOMEM ; + + cci->clock_freq = devm_kcalloc(dev, cci->nclocks,
You're just using this temporarily to create a copy of the res.clock_rate array, how about just passing the res.clock_rate into cci_clock_set_rate() ?
Quoted 4 lines
drivers/i2c/busses/i2c-qcom-cci.c +4
-0+ sizeof(*cci->clock_freq), GFP_KERNEL); +if (!cci->clock_freq) +return -ENOMEM ; +
drivers/i2c/busses/i2c-qcom-cci.c line 621
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-cci.c +10
-0+for (i =0 ; i < cci->nclocks; i++) { +struct clk_bulk_data *clock = &cci->clock[i]; + + clock->clk =devm_clk_get (dev, cci->data->res.clock[i]); +if (IS_ERR (clock->clk)) +return PTR_ERR (clock->clk); + + clock->id = cci->data->res.clock[i]; + cci->clock_freq[i] = cci->data->res.clock_rate[i]; + }
Fill out cci->clock[*].id and call clk_bulk_get()
drivers/i2c/busses/i2c-qcom-cci.c line 628
Quoted 7 lines
drivers/i2c/busses/i2c-qcom-cci.c +7
-0+ + ret =cci_clock_set_rate (cci->nclocks, cci->clock, + cci->clock_freq, dev); +if (ret <0 ) +return ret; + + ret =clk_bulk_prepare_enable (cci->nclocks, cci->clock);
It seems a little bit excessive to keep the clocks on while the driver is probed, but this could be improved in a follow up patch...
Quoted 31 lines
drivers/i2c/busses/i2c-qcom-cci.c +31
-0+if (ret <0 ) +return ret; + + val =readl_relaxed (cci->base +CCI_HW_VERSION ); +dev_dbg (dev,"%s: CCI HW version = 0x%08x" , __func__, val); + +enable_irq (cci->irq); + + ret =cci_reset (cci); +if (ret <0 ) + goto error; + +for (i =0 ; i < cci->data->num_masters; i++) { + ret =cci_init (cci, &cci->data->params[cci->master[i].mode]); +if (ret <0 ) + goto error; + + ret =i2c_add_adapter (&cci->master[i].adap); +if (ret <0 ) + goto error_i2c; + } + +return 0 ; + +error_i2c: +for (; i >=0 ; i--) +i2c_del_adapter (&cci->master[i].adap); +error: +disable_irq (cci->irq); +clk_bulk_disable_unprepare (cci->nclocks, cci->clock); +
drivers/i2c/busses/i2c-qcom-cci.c line 669
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-cci.c +10
-0+return ret; +} + +static int cci_remove (struct platform_device *pdev) +{ +struct cci *cci =platform_get_drvdata (pdev); +int i; + +disable_irq (cci->irq); +clk_bulk_disable_unprepare (cci->nclocks, cci->clock);
i2c clients might be communicating with their clients until you call i2c_del_adapter(), so better pull the resources after you have removed the adaptor.
Maybe even a cci_halt() call before we cut the clocks?
Quoted 6 lines
drivers/i2c/busses/i2c-qcom-cci.c +6
-0+ +for (i =0 ; i < cci->data->num_masters; i++) +i2c_del_adapter (&cci->master[i].adap); + +return 0 ; +}
[..]
Quoted 1 line
+static const struct
cci_datacci_v1_4_0_data= {
[..]
Quoted 4 lines
drivers/i2c/busses/i2c-qcom-cci.c +4
-0+ { +/* I2C_MODE_FAST_PLUS */ + .thigh = 16, + .tlow = 22,
drivers/i2c/busses/i2c-qcom-cci.c line 789
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-cci.c +10
-0+ .tsu_sto = 17, + .tsu_sta = 18, + .thd_dat = 16, + .thd_sta = 15, + .tbuf = 24, + .scl_stretch_en = 0, + .trdhld = 3, + .tsp = 3 + } +
Empty line.
drivers/i2c/busses/i2c-qcom-cci.c line 792
Quoted 3 lines
drivers/i2c/busses/i2c-qcom-cci.c +3
-0+ }, + +};
The dt binding mentions that a power domain is required for v1.4, but there's no support for this in the driver.
Regards,
Bjorn
Re: [PATCH v3 2/2] i2c: Add Qualcomm CCI I2C driver
Quoted 4 lines · Bjorn Andersson
On Mon 06 Aug 04:04 PDT 2018, Vinod Koul wrote:
[..]Quoted 1 line
drivers/i2c/busses/i2c-qcom-cci.c +0
-0[..]
Quoted 16 lines
Quoted 4 lines
drivers/i2c/busses/i2c-qcom-cci.c +4
-0+struct hw_params { +u16 thigh; +u16 tlow; +u16 tsu_sto;drivers/i2c/busses/i2c-qcom-cci.c line 116
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-cci.c +10
-0+u16 tsu_sta; +u16 thd_dat; +u16 thd_sta; +u16 tbuf; +u8 scl_stretch_en; +u16 trdhld; +u16 tsp; +}; + +struct cci_clock {This is now unused.
ah yes, thanks for spotting, will remove
Quoted 11 lines
Quoted 11 lines
drivers/i2c/busses/i2c-qcom-cci.c +10
-0+struct clk *clk; +const char *name; +u32 freq; +}; + +struct cci ; + +struct cci_master { +struct i2c_adapter adap; +u16 master;
Quoted 8 lines
drivers/i2c/busses/i2c-qcom-cci.c line 132
Quoted 6 lines
drivers/i2c/busses/i2c-qcom-cci.c +6
-0+u8 mode; +int status; +bool complete_pending; +struct completion irq_complete; +struct cci *cci; +Empty line.
will fix this and other style/empty lines, thanks.
Quoted 17 lines
Quoted 5 lines
drivers/i2c/busses/i2c-qcom-cci.c +4
-0+}; + +struct cci_data { +unsigned int num_masters;drivers/i2c/busses/i2c-qcom-cci.c line 146
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-cci.c +10
-0+struct i2c_adapter_quirks quirks; +u16 queue_size[NUM_QUEUES ]; +struct cci_res res; +struct hw_params params[3 ]; +}; + +struct cci { +struct device *dev; +void __iomem *base; +u32 irq;Use unsigned int rather than a type of specific size
Any specific reason for that preference?
Quoted 18 lines
Quoted 18 lines
drivers/i2c/busses/i2c-qcom-cci.c +17
-0+const struct cci_data *data; +struct clk_bulk_data *clock; +u32 *clock_freq; +int nclocks; +struct cci_master master[NUM_MASTERS ]; +}; + +/** +* cci_clock_set_rate() - Set clock frequency rates +* @nclocks: Number of clocks +* @clock: Clock array +* @clock_freq: Clock frequency rate array +* @dev: Device +* +* Return 0 on success or a negative error code otherwise +*/ +static int cci_clock_set_rate(int nclocks, struct clk_bulk_data *clock,
Quoted 8 lines
drivers/i2c/busses/i2c-qcom-cci.c line 169
Quoted 6 lines
drivers/i2c/busses/i2c-qcom-cci.c +6
-0+ u32 *clock_freq, struct device *dev) +{ + int i, ret; +long rate; + +for (i =0 ; i < nclocks; i++)Multiline loop deserves {}
yes
Quoted 21 lines
Quoted 19 lines
drivers/i2c/busses/i2c-qcom-cci.c +18
-0+if (clock_freq[i]) { + rate =clk_round_rate (clock[i].clk, clock_freq[i]); +if (rate <0 ) { +dev_err (dev,"clk round rate failed: %ld\n" , + rate); +return rate; + } + + ret =clk_set_rate (clock[i].clk, clock_freq[i]); +if (ret <0 ) { +dev_err (dev,"clk set rate failed: %d\n" , ret); +return ret; + } + } + +return 0 ; +} +[..]
[..]
Quoted 12 lines
drivers/i2c/busses/i2c-qcom-cci.c line 293
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-cci.c +10
-0+static int cci_reset (struct cci *cci) +{ +unsigned long time; + + cci->master[0 ].complete_pending = true; +writel (CCI_RESET_CMD_MASK , cci->base +CCI_RESET_CMD ); + + time =wait_for_completion_timeout + (&cci->master[0 ].irq_complete, +msecs_to_jiffies (CCI_TIMEOUT_MS ));Please rework the indentation of this.
and I think we should pass master to this, this looks not correct to me.
Quoted 3 lines
Also
CCI_TIMEOUT_MSis converted to jiffies in all the places, define it in jiffies instead.
will do
Quoted 9 lines
Quoted 8 lines
drivers/i2c/busses/i2c-qcom-cci.c +7
-0+if (!time) { +dev_err (cci->dev,"CCI reset timeout\n" ); +return -ETIMEDOUT ; + } + +return 0 ; +}[..]
Quoted 14 lines
drivers/i2c/busses/i2c-qcom-cci.c line 381
Quoted 12 lines
drivers/i2c/busses/i2c-qcom-cci.c +12
-0+static int cci_validate_queue (struct cci *cci,u8 master,u8 queue) +{ +int ret =0 ; +u32 val; + + val =readl (cci->base +CCI_I2C_Mm_Qn_CUR_WORD_CNT (master, queue)); + +if (val == cci->data->queue_size[queue]) +return -EINVAL ; + +if (val) { + val =CCI_I2C_REPORT |BIT (8 );Can we get a define (or a comment) for BIT(8) as well?
lets define BIT(8)..
Quoted 11 lines
drivers/i2c/busses/i2c-qcom-cci.c line 387
Quoted 7 lines
drivers/i2c/busses/i2c-qcom-cci.c +6
-0+writel (val, cci->base +CCI_I2C_Mm_Qn_LOAD_DATA (master, queue)); + + ret =cci_run_queue (cci, master, queue); + } + +return ret;Rather than wrapping the second half of the function in a check for val, return early on !val and then you can end with return
cci_run_queue()and drop the "ret".
yes sounds better
Quoted 12 lines
drivers/i2c/busses/i2c-qcom-cci.c line 396
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-cci.c +9
-0+} + +static int cci_i2c_read (struct cci *cci,u16 master, +u16 addr,u8 *buf,u16 len) +{ +u32 val, words_read, words_exp; +u8 queue =QUEUE_1 ; +int i, index =0 , ret; +bool first =0 ;s/0/false/
right
Quoted 21 lines
Quoted 21 lines
drivers/i2c/busses/i2c-qcom-cci.c +20
-0+ +/* +* Call validate queue to make sure queue is empty before starting. +* This is to avoid overflow / underflow of queue. +*/ + ret =cci_validate_queue (cci, master, queue); +if (ret <0 ) +return ret; + + val =CCI_I2C_SET_PARAM | (addr &0x7f ) <<4 ; +writel (val, cci->base +CCI_I2C_Mm_Qn_LOAD_DATA (master, queue)); + + val =CCI_I2C_READ | len <<4 ; +writel (val, cci->base +CCI_I2C_Mm_Qn_LOAD_DATA (master, queue)); + + ret =cci_run_queue (cci, master, queue); +if (ret <0 ) +return ret; + + words_read =readl (cci->base +CCI_I2C_Mm_READ_BUF_LEVEL (master));
Quoted 8 lines
drivers/i2c/busses/i2c-qcom-cci.c line 422
Quoted 6 lines
drivers/i2c/busses/i2c-qcom-cci.c +6
-0+ words_exp = len /4 +1 ; +if (words_read != words_exp) { +dev_err (cci->dev,"words read = %d, words expected = %d\n" , + words_read, words_exp); +return -EIO ; + }I thought that an i2c read could return less data than was requested...
yes that is my understanding, will check with Todor and update this
Quoted 20 lines
Quoted 16 lines
drivers/i2c/busses/i2c-qcom-cci.c +15
-0+ +do { + val =readl (cci->base +CCI_I2C_Mm_READ_DATA (master)); + +for (i =0 ; i <4 && index < len; i++) { +if (first) { + first = false; +continue ; + } + buf[index++] = (val >> (i *8 )) &0xff ; + } + }while (--words_read); + +return 0 ; +}[..]
Quoted 2 lines
drivers/i2c/busses/i2c-qcom-cci.c +2
-0+static int cci_probe (struct platform_device *pdev) +{[..]
Quoted 16 lines
drivers/i2c/busses/i2c-qcom-cci.c line 607
Quoted 12 lines
drivers/i2c/busses/i2c-qcom-cci.c +12
-0+/* Clocks */ + + cci->nclocks =0 ; +while (cci->data->res.clock[cci->nclocks]) + cci->nclocks++; + + cci->clock =devm_kcalloc (dev, cci->nclocks, +sizeof (*cci->clock),GFP_KERNEL ); +if (!cci->clock) +return -ENOMEM ; + + cci->clock_freq = devm_kcalloc(dev, cci->nclocks,You're just using this temporarily to create a copy of the res.clock_rate array, how about just passing the res.clock_rate into
cci_clock_set_rate()?
that sound reasonable, will give it a shot
Quoted 17 lines
Quoted 5 lines
drivers/i2c/busses/i2c-qcom-cci.c +4
-0+ sizeof(*cci->clock_freq), GFP_KERNEL); +if (!cci->clock_freq) +return -ENOMEM ; +drivers/i2c/busses/i2c-qcom-cci.c line 621
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-cci.c +10
-0+for (i =0 ; i < cci->nclocks; i++) { +struct clk_bulk_data *clock = &cci->clock[i]; + + clock->clk =devm_clk_get (dev, cci->data->res.clock[i]); +if (IS_ERR (clock->clk)) +return PTR_ERR (clock->clk); + + clock->id = cci->data->res.clock[i]; + cci->clock_freq[i] = cci->data->res.clock_rate[i]; + }Fill out cci->clock[*].id and call
clk_bulk_get()
ok
Quoted 11 lines
drivers/i2c/busses/i2c-qcom-cci.c line 628
Quoted 8 lines
drivers/i2c/busses/i2c-qcom-cci.c +7
-0+ + ret =cci_clock_set_rate (cci->nclocks, cci->clock, + cci->clock_freq, dev); +if (ret <0 ) +return ret; + + ret =clk_bulk_prepare_enable (cci->nclocks, cci->clock);It seems a little bit excessive to keep the clocks on while the driver is probed, but this could be improved in a follow up patch...
okay but are they required for controller to be probed, I will check that part and update
Quoted 47 lines
Quoted 32 lines
drivers/i2c/busses/i2c-qcom-cci.c +31
-0+if (ret <0 ) +return ret; + + val =readl_relaxed (cci->base +CCI_HW_VERSION ); +dev_dbg (dev,"%s: CCI HW version = 0x%08x" , __func__, val); + +enable_irq (cci->irq); + + ret =cci_reset (cci); +if (ret <0 ) + goto error; + +for (i =0 ; i < cci->data->num_masters; i++) { + ret =cci_init (cci, &cci->data->params[cci->master[i].mode]); +if (ret <0 ) + goto error; + + ret =i2c_add_adapter (&cci->master[i].adap); +if (ret <0 ) + goto error_i2c; + } + +return 0 ; + +error_i2c: +for (; i >=0 ; i--) +i2c_del_adapter (&cci->master[i].adap); +error: +disable_irq (cci->irq); +clk_bulk_disable_unprepare (cci->nclocks, cci->clock); +drivers/i2c/busses/i2c-qcom-cci.c line 669
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-cci.c +10
-0+return ret; +} + +static int cci_remove (struct platform_device *pdev) +{ +struct cci *cci =platform_get_drvdata (pdev); +int i; + +disable_irq (cci->irq); +clk_bulk_disable_unprepare (cci->nclocks, cci->clock);i2c clients might be communicating with their clients until you call
i2c_del_adapter(), so better pull the resources after you have removed the adaptor.
Quoted 1 line
Maybe even a
cci_halt()call before we cut the clocks?
yes makes sense
Quoted 27 lines
Quoted 7 lines
drivers/i2c/busses/i2c-qcom-cci.c +6
-0+ +for (i =0 ; i < cci->data->num_masters; i++) +i2c_del_adapter (&cci->master[i].adap); + +return 0 ; +}[..]
Quoted 1 line
+static const struct
cci_datacci_v1_4_0_data= {[..]
Quoted 4 lines
drivers/i2c/busses/i2c-qcom-cci.c +4
-0+ { +/* I2C_MODE_FAST_PLUS */ + .thigh = 16, + .tlow = 22,drivers/i2c/busses/i2c-qcom-cci.c line 789
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-cci.c +10
-0+ .tsu_sto = 17, + .tsu_sta = 18, + .thd_dat = 16, + .thd_sta = 15, + .tbuf = 24, + .scl_stretch_en = 0, + .trdhld = 3, + .tsp = 3 + } +Empty line.
Quoted 6 lines
drivers/i2c/busses/i2c-qcom-cci.c line 792
Quoted 3 lines
drivers/i2c/busses/i2c-qcom-cci.c +3
-0+ }, + +};The dt binding mentions that a power domain is required for v1.4, but there's no support for this in the driver.
I will check that, yes it is not handled in the driver we have
Signature · 1 line
~Vinod
Re: [PATCH v3 2/2] i2c: Add Qualcomm CCI I2C driver
Quoted 5 lines · Vinod
Quoted 4 lines · Bjorn Andersson
On Mon 06 Aug 04:04 PDT 2018, Vinod Koul wrote:
[..]Quoted 1 line
drivers/i2c/busses/i2c-qcom-cci.c +0
-0[..]
[..]
Quoted 9 lines
Quoted 6 lines
drivers/i2c/busses/i2c-qcom-cci.c line 146
Quoted 4 lines
drivers/i2c/busses/i2c-qcom-cci.c +4
-0+struct cci { +struct device *dev; +void __iomem *base; +u32 irq;Use unsigned int rather than a type of specific size
Any specific reason for that preference?
Just that unsigned int is the type used in the irq api.
[..]
Quoted 12 lines
Quoted 11 lines
drivers/i2c/busses/i2c-qcom-cci.c line 628
Quoted 8 lines
drivers/i2c/busses/i2c-qcom-cci.c +7
-0+ + ret =cci_clock_set_rate (cci->nclocks, cci->clock, + cci->clock_freq, dev); +if (ret <0 ) +return ret; + + ret =clk_bulk_prepare_enable (cci->nclocks, cci->clock);It seems a little bit excessive to keep the clocks on while the driver is probed, but this could be improved in a follow up patch...
Quoted 3 lines
okay but are they required for controller to be probed, I will check that part and update
Right. So doing something similar to how we deal with clocks in i2c-qup would probably make sense; we enable them during probe and then toggle them using pm_runtime from there on.
Regards,
Bjorn
Re: [PATCH v3 2/2] i2c: Add Qualcomm CCI I2C driver
In reply to VinodQuoted 6 lines · Vinod
Quoted 2 lines · Bjorn Andersson
The dt binding mentions that a power domain is required for v1.4, but there's no support for this in the driver.
I will check that, yes it is not handled in the driver we have
So I discussed this with Todor and found that __genpd_dev_pm_attach in drivers/base/power/domain.c handles the "power-domain" binding.
I do not think driver needs to do anything here, this seems handled in core and we need to provide binding, FWIW, the 8996 doesn't work without this
Signature · 1 line
~Vinod
Re: [PATCH v3 2/2] i2c: Add Qualcomm CCI I2C driver
Quoted 6 lines · Vinod
Quoted 5 lines · Vinod
Quoted 2 lines · Bjorn Andersson
The dt binding mentions that a power domain is required for v1.4, but there's no support for this in the driver.
Quoted 9 lines
Quoted 1 line
I will check that, yes it is not handled in the driver we have
So I discussed this with Todor and found that
__genpd_dev_pm_attachindrivers/base/power/domain.chandles the "power-domain" binding.I do not think driver needs to do anything here, this seems handled in core and we need to provide binding, FWIW, the 8996 doesn't work without this
Right, the power-domain will be picked up and afaict will be turned on as it's attached to the device during probe of the platform driver. So you don't need to do anything additionally to make it work.
But I suggest that you follow up with a patch adding runtime pm support so that the associated power domain will be disabled when the bus is not in use.
Regards,
Bjorn
Re: [PATCH v3 2/2] i2c: Add Qualcomm CCI I2C driver
Quoted 18 lines · Bjorn Andersson
Quoted 6 lines · Vinod
Quoted 5 lines · Vinod
Quoted 2 lines · Bjorn Andersson
The dt binding mentions that a power domain is required for v1.4, but there's no support for this in the driver.
Quoted 9 lines
Quoted 1 line
I will check that, yes it is not handled in the driver we have
So I discussed this with Todor and found that
__genpd_dev_pm_attachindrivers/base/power/domain.chandles the "power-domain" binding.I do not think driver needs to do anything here, this seems handled in core and we need to provide binding, FWIW, the 8996 doesn't work without this
Quoted 7 lines
Right, the power-domain will be picked up and afaict will be turned on as it's attached to the device during probe of the platform driver. So you don't need to do anything additionally to make it work.
But I suggest that you follow up with a patch adding runtime pm support so that the associated power domain will be disabled when the bus is not in use.
Agreed, have already started on that.
Signature · 1 line
~Vinod