Re: [PATCH 2/3] arm64: dts: qcom: sm8350: Add CCI device tree nodes

[Vladimir Zapolskiy <vz@kernel.org>]

Hi Loic.

On 11/5/26 10:37, Loic Poulain wrote:
> On Thu, Nov 5, 2026 at 12:05 AM Vladimir Zapolskiy <vz@kernel.org> wrote:
>>
>> Add Camera Control Interface (CCI) controller device tree nodes and pin
>> configurations of the I2C busses to SM8350 SoC dtsi.
>>
>> Signed-off-by: Vladimir Zapolskiy <vz@kernel.org>
>> ---
>>   arch/arm64/boot/dts/qcom/sm8350.dtsi | 147 +++++++++++++++++++++++++++
>>   1 file changed, 147 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi b/arch/arm64/boot/dts/qcom/sm8350.dtsi
>> index a51942c99f8a..41dff3736061 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8350.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi
>> @@ -10,6 +10,7 @@
>>   #include <dt-bindings/clock/qcom,gcc-sm8350.h>
>>   #include <dt-bindings/clock/qcom,gpucc-sm8350.h>
>>   #include <dt-bindings/clock/qcom,rpmh.h>
>> +#include <dt-bindings/clock/qcom,sm8350-camcc.h>
>>   #include <dt-bindings/clock/qcom,sm8350-videocc.h>
>>   #include <dt-bindings/dma/qcom-gpi.h>
>>   #include <dt-bindings/firmware/qcom,scm.h>
>> @@ -2830,6 +2831,88 @@ videocc: clock-controller@abf0000 {
>>                          #power-domain-cells = <1>;
>>                  };
>>
>> +               cci0: cci@ac4f000 {
>> +                       compatible = "qcom,sm8350-cci", "qcom,msm8996-cci";
>> +                       reg = <0 0x0ac4f000 0 0x1000>;
>> +                       interrupts = <GIC_SPI 460 IRQ_TYPE_EDGE_RISING>;
>> +                       power-domains = <&camcc TITAN_TOP_GDSC>;
>> +
>> +                       clocks = <&camcc CAM_CC_CAMNOC_AXI_CLK>,
>> +                                <&camcc CAM_CC_SLOW_AHB_CLK_SRC>,
>> +                                <&camcc CAM_CC_CPAS_AHB_CLK>,
>> +                                <&camcc CAM_CC_CCI_0_CLK>,
>> +                                <&camcc CAM_CC_CCI_0_CLK_SRC>;
>> +                       clock-names = "camnoc_axi",
>> +                                     "slow_ahb_src",
>> +                                     "cpas_ahb",
>> +                                     "cci",
>> +                                     "cci_src";
>> +
>> +                       pinctrl-0 = <&cci0_0_default &cci0_1_default>;
>> +                       pinctrl-1 = <&cci0_0_sleep &cci0_1_sleep>;
>> +                       pinctrl-names = "default", "sleep";
>> +
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +
>> +                       status = "disabled";
>> +
>> +                       cci0_i2c0: i2c-bus@0 {
>> +                               reg = <0>;
>> +                               clock-frequency = <1000000>;
>> +                               #address-cells = <1>;
>> +                               #size-cells = <0>;
>> +                       };
>> +
>> +                       cci0_i2c1: i2c-bus@1 {
>> +                               reg = <1>;
>> +                               clock-frequency = <1000000>;
>> +                               #address-cells = <1>;
>> +                               #size-cells = <0>;
>> +                       };
>> +               };
>> +
>> +               cci1: cci@ac50000 {
>> +                       compatible = "qcom,sm8350-cci", "qcom,msm8996-cci";
>> +                       reg = <0 0x0ac50000 0 0x1000>;
>> +                       interrupts = <GIC_SPI 271 IRQ_TYPE_EDGE_RISING>;
>> +                       power-domains = <&camcc TITAN_TOP_GDSC>;
>> +
>> +                       clocks = <&camcc CAM_CC_CAMNOC_AXI_CLK>,
>> +                                <&camcc CAM_CC_SLOW_AHB_CLK_SRC>,
>> +                                <&camcc CAM_CC_CPAS_AHB_CLK>,
>> +                                <&camcc CAM_CC_CCI_1_CLK>,
>> +                                <&camcc CAM_CC_CCI_1_CLK_SRC>;
>> +                       clock-names = "camnoc_axi",
>> +                                     "slow_ahb_src",
>> +                                     "cpas_ahb",
>> +                                     "cci",
>> +                                     "cci_src";
>> +
>> +                       pinctrl-0 = <&cci1_0_default &cci1_1_default>;
>> +                       pinctrl-1 = <&cci1_0_sleep &cci1_1_sleep>;
>> +                       pinctrl-names = "default", "sleep";
>> +
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +
>> +                       status = "disabled";
>> +
>> +                       cci1_i2c0: i2c-bus@0 {
>> +                               reg = <0>;
>> +                               clock-frequency = <1000000>;
>> +                               #address-cells = <1>;
>> +                               #size-cells = <0>;
>> +                       };
>> +
>> +                       cci1_i2c1: i2c-bus@1 {
>> +                               reg = <1>;
>> +                               clock-frequency = <1000000>;
>> +                               #address-cells = <1>;
>> +                               #size-cells = <0>;
>> +                       };
>> +               };
>> +
>>                  camcc: clock-controller@ad00000 {
>>                          compatible = "qcom,sm8350-camcc";
>>                          reg = <0 0x0ad00000 0 0x10000>;
>> @@ -3348,6 +3431,70 @@ tlmm: pinctrl@f100000 {
>>                          gpio-ranges = <&tlmm 0 0 204>;
>>                          wakeup-parent = <&pdc>;
>>
>> +                       cci0_0_default: cci0-0-default-state {
>> +                               /* SDA, SCL */
>> +                               pins = "gpio107", "gpio108";
>> +                               function = "cci_i2c";
>> +                               drive-strength = <2>;
>> +                               bias-pull-up;
> 
> I had similar comments on a different platform, but why pull-up here
> (for SCL/SDA)?

Here the setting was literally copied from the downstream pad configuration
of CCI SDA/SCL lines, likely the same has been done for other platforms too.

> Do we expect boards to lack the external pull-ups
> required by I2C?

The I2C spec requires pull-ups, but I believe it does not require external
pull-ups explicitly, so there should be no contradiction with the spec.

> If not, enabling the internal pull-ups seems
> unnecessary. Also, what is their effective resistance on these
> platforms? Aren't they typically too weak to meet the rise-time
> requirements of most I2C modes anyway?

Likely you are right that the internal pull-ups alone are too weak to get
the I2C bus functional on higher bus speeds.

What's your preference on this subject, do you wish to see 'bias-disabled'
instead? I won't have objections against it.

-- 
Best wishes,
Vladimir