Re: [PATCH v19 11/12] arm64: dts: qcom: kaanapali: Add CCI controller nodes

[Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>]

On 10/9/2026 12:16 AM, Loic Poulain wrote:
> On Thu, Oct 8, 2026 at 9:14 AM Hangxiang Ma
> <hangxiang.ma@oss.qualcomm.com> wrote:
>>
>> Add the three Camera Control Interface (CCI) controllers present on the
>> Kaanapali SoC. Each controller provides two I2C hosts used for camera
>> sensor control, so define the controller nodes and their pinctrl states.
>>
>> The first bus has two CCI bus master pinouts:
>> cci_i2c_sda0 = gpio109
>> cci_i2c_scl0 = gpio110
>>
>> cci_i2c_sda1 = gpio111
>> cci_i2c_scl1 = gpio112
>>
>> The second bus has two CCI bus master pinouts:
>> cci_i2c_sda3 = gpio113
>> cci_i2c_scl3 = gpio114
>>
>> cci_i2c_sda4 = gpio107
>> cci_i2c_scl4 = gpio160
>>
>> The third bus has two CCI bus master pinouts:
>> cci_i2c_sda5 = gpio108
>> cci_i2c_scl5 = gpio149
>>
>> cci_i2c_sda6 = gpio115
>> cci_i2c_scl6 = gpio116
>>
>> Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>> Signed-off-by: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>
>> ---
>>   arch/arm64/boot/dts/qcom/kaanapali.dtsi | 303 ++++++++++++++++++++++++++++++++
>>   1 file changed, 303 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/kaanapali.dtsi b/arch/arm64/boot/dts/qcom/kaanapali.dtsi
>> index 7e5106865e7e..710af0597aaa 100644
>> --- a/arch/arm64/boot/dts/qcom/kaanapali.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/kaanapali.dtsi
>> @@ -3749,6 +3749,117 @@ port@5 {
>>                          };
>>                  };
>>
>> +               cci0: cci@941b000 {
>> +                       compatible = "qcom,kaanapali-cci", "qcom,msm8996-cci";
>> +                       reg = <0x0 0x0941b000 0x0 0x1000>;
>> +
>> +                       interrupts = <GIC_SPI 426 IRQ_TYPE_EDGE_RISING>;
>> +
>> +                       clocks = <&camcc CAM_CC_CAM_TOP_AHB_CLK>,
>> +                                <&camcc CAM_CC_CCI_0_CLK>;
>> +                       clock-names = "ahb",
>> +                                     "cci";
>> +
>> +                       power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
>> +
>> +                       pinctrl-0 = <&cci0_i2c0_default &cci0_i2c1_default>;
>> +                       pinctrl-1 = <&cci0_i2c0_sleep &cci0_i2c1_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@941c000 {
>> +                       compatible = "qcom,kaanapali-cci", "qcom,msm8996-cci";
>> +                       reg = <0x0 0x0941c000 0x0 0x1000>;
>> +
>> +                       interrupts = <GIC_SPI 427 IRQ_TYPE_EDGE_RISING>;
>> +
>> +                       clocks = <&camcc CAM_CC_CAM_TOP_AHB_CLK>,
>> +                                <&camcc CAM_CC_CCI_1_CLK>;
>> +                       clock-names = "ahb",
>> +                                     "cci";
>> +
>> +                       power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
>> +
>> +                       pinctrl-0 = <&cci1_i2c0_default &cci1_i2c1_default>;
>> +                       pinctrl-1 = <&cci1_i2c0_sleep &cci1_i2c1_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>;
>> +                       };
>> +               };
>> +
>> +               cci2: cci@941d000 {
>> +                       compatible = "qcom,kaanapali-cci", "qcom,msm8996-cci";
>> +                       reg = <0x0 0x0941d000 0x0 0x1000>;
>> +
>> +                       interrupts = <GIC_SPI 428 IRQ_TYPE_EDGE_RISING>;
>> +
>> +                       clocks = <&camcc CAM_CC_CAM_TOP_AHB_CLK>,
>> +                                <&camcc CAM_CC_CCI_2_CLK>;
>> +                       clock-names = "ahb",
>> +                                     "cci";
>> +
>> +                       power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
>> +
>> +                       pinctrl-0 = <&cci2_i2c0_default &cci2_i2c1_default>;
>> +                       pinctrl-1 = <&cci2_i2c0_sleep &cci2_i2c1_sleep>;
>> +                       pinctrl-names = "default", "sleep";
>> +
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +
>> +                       status = "disabled";
>> +
>> +                       cci2_i2c0: i2c-bus@0 {
>> +                               reg = <0>;
>> +                               clock-frequency = <1000000>;
>> +                               #address-cells = <1>;
>> +                               #size-cells = <0>;
>> +                       };
>> +
>> +                       cci2_i2c1: i2c-bus@1 {
>> +                               reg = <1>;
>> +                               clock-frequency = <1000000>;
>> +                               #address-cells = <1>;
>> +                               #size-cells = <0>;
>> +                       };
>> +               };
>> +
>>                  csiphy0: phy@9523000 {
>>                          compatible = "qcom,kaanapali-csi2-phy";
>>                          reg = <0x0 0x09523000 0x0 0x2000>;
>> @@ -4459,6 +4570,198 @@ tlmm: pinctrl@f100000 {
>>                          #interrupt-cells = <2>;
>>                          wakeup-parent = <&pdc>;
>>
>> +                       cci0_i2c0_default: cci0-i2c0-default-state {
>> +                               scl-pins {
>> +                                       pins = "gpio110";
>> +                                       function = "cci_i2c_scl";
>> +                                       drive-strength = <2>;
>> +                                       bias-pull-up;
> 
> A general comment on the CCI DTS patches: why are the pull-ups enabled
> here (for SCL/SDA)? Do we expect boards to lack the external pull-ups
> required by I²C? 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 I²C modes anyway?
> 

Thanks for pointing this out. The CCI buses are expected to have the
required external pull-up resistors on the board. In fact, the above 
properties follow the established Qualcomm CCI pinctrl configuration 
used by other chipsets and platforms, as well as the downstream 
Kaanapali DTS.

I checked the Kaanapali pinctrl driver. Its CCI GPIO groups do not 
define an i2c_pull_bit, so bias-pull-up would silently fall back to the
normal weak pull-up.

I will therefore change the CCI default states to bias-disable instead 
of relying on the internal pull-up. The required I2C pull-ups should be 
provided at board level; the existing sleep-state pull-down 
configuration will remain unchanged.

---
Best Regards,
Hangxiang