Re: [PATCH v5 3/4] arm64: dts: qcom: sm8750: describe two of the three CCI I2C controllers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] arm64: dts: qcom: sm8750: omission of cci2 controller and pinctrl states
- [Medium] arm64: dts: qcom: sm8750: CCI sleep pinctrl states mux pins to gpio
--

commit a65c21663abcd66908e315d4a351c0c64b9975c6
Author: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>

arm64: dts: qcom: sm8750: describe two of the three CCI I2C controllers

This commit adds device tree nodes and TLMM pinctrl states for the cci0 and
cci1 Camera Control Interface (CCI) I2C controllers on the Qualcomm SM8750
SoC, leaving the third controller (cci2) undescribed.

>     arm64: dts: qcom: sm8750: describe two of the three CCI I2C controllers
>     
>     The SM8750 has three CCI I2C controllers; this dtsi describes the two
>     that the Qualcomm camera subsystem binds to on this SoC. Each controller
>     exposes two I2C buses; both buses are described and left available, as
>     on sm8550 and sm8650.
>     
>     The TLMM pad assignment is cci0 bus 0 on gpio113/114, cci0 bus 1 on
>     gpio115/116, cci1 bus 0 on gpio117/118 and cci1 bus 1 on gpio111/164,
>     the last two being the pads shared with cci_timer. The remaining CCI
>     pads, gpio112/153 and gpio119/120, belong to cci2, which is not
>     described here.
>     
>     The controller nodes are placed in unit-address order after
>     videocc@aaf0000, and the pinctrl states in alphabetical order inside
>     the TLMM node.
>     
>     Signed-off-by: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>
>
> diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi
> index dd738d13df8e0..5f64912d55eac 100644
> --- a/arch/arm64/boot/dts/qcom/sm8750.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi
> @@ -3060,6 +3060,70 @@ videocc: clock-controller@aaf0000 {

[ ... ]

> +		cci1: cci@ac7c000 {
> +			compatible = "qcom,sm8750-cci", "qcom,msm8996-cci";
> +			reg = <0x0 0x0ac7c000 0x0 0x1000>;
> +			interrupts = <GIC_SPI 427 IRQ_TYPE_EDGE_RISING>;
> +			power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
> +			clocks = <&camcc CAM_CC_CAM_TOP_AHB_CLK>,
> +				 <&camcc CAM_CC_CCI_1_CLK>;
> +			clock-names = "ahb", "cci";
> +			assigned-clocks = <&camcc CAM_CC_CCI_1_CLK>;
> +			assigned-clock-rates = <37500000>;
> +			pinctrl-0 = <&cci1_0_default &cci1_1_default>;
> +			pinctrl-1 = <&cci1_0_sleep &cci1_1_sleep>;
> +			pinctrl-names = "default", "sleep";
> +			status = "disabled";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			cci1_i2c0: i2c-bus@0 {
> +				reg = <0>;
> +				clock-frequency = <400000>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +			};
> +
> +			cci1_i2c1: i2c-bus@1 {
> +				reg = <1>;
> +				clock-frequency = <400000>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +			};
> +		};

[Severity: Medium]
Why is cci2 (along with its cci2_0 and cci2_1 pinctrl states in tlmm) omitted
from arch/arm64/boot/dts/qcom/sm8750.dtsi?

The commit message notes that this dtsi describes the two controllers that the
Qualcomm camera subsystem binds to, as on sm8550 and sm8650. However, the
camera subsystem driver (drivers/media/platform/qcom/camss/) does not bind to
CCI controllers directly; drivers/i2c/busses/i2c-qcom-cci.c binds to each CCI
node independently, and camera sensors on any CCI bus connect to CAMSS CSIPHY
ports via OF graph endpoints.

Furthermore, both arch/arm64/boot/dts/qcom/sm8550.dtsi and
arch/arm64/boot/dts/qcom/sm8650.dtsi describe all three CCI controllers
(cci0, cci1, and cci2) and their TLMM pinctrl states so that board device
trees can enable whichever buses they route.

Since include/dt-bindings/clock/qcom,sm8750-camcc.h and
drivers/clk/qcom/camcc-sm8750.c already provide CAM_CC_CCI_2_CLK, and
drivers/pinctrl/qcom/pinctrl-sm8750.c already defines the cci_i2c_sda and
cci_i2c_scl mux functions for gpio112/gpio153 and gpio119/gpio120, should cci2
and its pinctrl states also be described here?

>  		mdss: display-subsystem@ae00000 {
>  			compatible = "qcom,sm8750-mdss";
>  			reg = <0x0 0x0ae00000 0x0 0x1000>;
> @@ -3633,6 +3697,98 @@ tlmm: pinctrl@f100000 {
>  			gpio-ranges = <&tlmm 0 0 216>;
>  			wakeup-parent = <&pdc>;
>  
> +			cci0_0_default: cci0-0-default-state {
> +				sda-pins {
> +					pins = "gpio113";
> +					function = "cci_i2c_sda";
> +					drive-strength = <2>;
> +					bias-pull-up;
> +				};
> +
> +				scl-pins {
> +					pins = "gpio114";
> +					function = "cci_i2c_scl";
> +					drive-strength = <2>;
> +					bias-pull-up;
> +				};
> +			};
> +
> +			cci0_0_sleep: cci0-0-sleep-state {
> +				pins = "gpio113", "gpio114";
> +				function = "gpio";
> +				drive-strength = <2>;
> +				bias-pull-down;
> +			};
> +
> +			cci0_1_default: cci0-1-default-state {
> +				sda-pins {
> +					pins = "gpio115";
> +					function = "cci_i2c_sda";
> +					drive-strength = <2>;
> +					bias-pull-up;
> +				};
> +
> +				scl-pins {
> +					pins = "gpio116";
> +					function = "cci_i2c_scl";
> +					drive-strength = <2>;
> +					bias-pull-up;
> +				};
> +			};
> +
> +			cci0_1_sleep: cci0-1-sleep-state {
> +				pins = "gpio115", "gpio116";
> +				function = "gpio";
> +				drive-strength = <2>;
> +				bias-pull-down;
> +			};
> +
> +			cci1_0_default: cci1-0-default-state {
> +				sda-pins {
> +					pins = "gpio117";
> +					function = "cci_i2c_sda";
> +					drive-strength = <2>;
> +					bias-pull-up;
> +				};
> +
> +				scl-pins {
> +					pins = "gpio118";
> +					function = "cci_i2c_scl";
> +					drive-strength = <2>;
> +					bias-pull-up;
> +				};
> +			};
> +
> +			cci1_0_sleep: cci1-0-sleep-state {
> +				pins = "gpio117", "gpio118";
> +				function = "gpio";
> +				drive-strength = <2>;
> +				bias-pull-down;
> +			};
> +
> +			cci1_1_default: cci1-1-default-state {
> +				sda-pins {
> +					pins = "gpio111";
> +					function = "cci_i2c_sda";
> +					drive-strength = <2>;
> +					bias-pull-up;
> +				};
> +
> +				scl-pins {
> +					pins = "gpio164";
> +					function = "cci_i2c_scl";
> +					drive-strength = <2>;
> +					bias-pull-up;
> +				};
> +			};
> +
> +			cci1_1_sleep: cci1-1-sleep-state {
> +				pins = "gpio111", "gpio164";
> +				function = "gpio";
> +				drive-strength = <2>;
> +				bias-pull-down;
> +			};

[Severity: Medium]
Should the cci0_0_sleep, cci0_1_sleep, cci1_0_sleep, and cci1_1_sleep states
keep the cci_i2c_sda and cci_i2c_scl functions in separate sda-pins and
scl-pins subnodes instead of switching function to "gpio"?

In other Qualcomm SoC device trees using split cci_i2c_sda and cci_i2c_scl
pinmux functions (such as arch/arm64/boot/dts/qcom/sm8550.dtsi,
arch/arm64/boot/dts/qcom/sm8650.dtsi, arch/arm64/boot/dts/qcom/milos.dtsi,
and arch/arm64/boot/dts/qcom/monaco.dtsi), the sleep states retain the
cci_i2c_sda and cci_i2c_scl mux functions and only switch the bias from
bias-pull-up to bias-pull-down:

arch/arm64/boot/dts/qcom/sm8650.dtsi:
			cci0_0_sleep: cci0-0-sleep-state {
				sda-pins {
					pins = "gpio101";
					function = "cci_i2c_sda";
					drive-strength = <2>;
					bias-pull-down;
				};

				scl-pins {
					pins = "gpio102";
					function = "cci_i2c_scl";
					drive-strength = <2>;
					bias-pull-down;
				};
			};

Switching the pins to function = "gpio" during runtime PM or system suspend
disconnects the pads from the CCI controller hardware block.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102060458.461372-1-xiaqinghuan@mail.ustc.edu.cn?part=3