Re: [PATCH v3 3/3] arm64: dts: qcom: sm8750-lenovo-elden: describe the WL2868C regulators

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] arm64: dts: qcom: sm8750-lenovo-elden enables i2c10 with qupv3_2 disabled
- [Medium] arm64: dts: qcom: undocumented camera compatibles and incomplete sensor nodes
- [Low] arm64: dts: qcom: sm8750.dtsi CCI bus 1 disablement, missing pinctrl, and order
- [Low] arm64: dts: qcom: binding schema, SoC dtsi, and board dts mixed in one patch
--

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

arm64: dts: qcom: sm8750-lenovo-elden: describe the WL2868C regulators

This commit adds the device tree for the Lenovo Legion Y700 Gen4 (TB322FC)
tablet, describing the board clocks, WL2868C camera PMIC regulators on
i2c10, and camera sensor and lens nodes on the CCI buses, along with the
SM8750 CCI controller nodes and board compatible string.

> diff --git a/Documentation/devicetree/bindings/arm/qcom.yaml b/Documentation/devicetree/bindings/arm/qcom.yaml
> index ff8a35df7862d..41d5e65c4eea5 100644
> --- a/Documentation/devicetree/bindings/arm/qcom.yaml
> +++ b/Documentation/devicetree/bindings/arm/qcom.yaml
> @@ -1168,6 +1168,7 @@ properties:
>  
>        - items:
>            - enum:
> +              - lenovo,elden
>                - qcom,sm8750-mtp
>                - qcom,sm8750-qrd
>            - const: qcom,sm8750

[Severity: Low]
This isn't a bug, but per Documentation/devicetree/bindings/submitting-patches.rst,
should the Documentation/devicetree/bindings/arm/qcom.yaml binding change,
the arch/arm64/boot/dts/qcom/sm8750.dtsi SoC additions, and the new
arch/arm64/boot/dts/qcom/sm8750-lenovo-elden.dts board file be split into
separate patches?

> diff --git a/arch/arm64/boot/dts/qcom/sm8750-lenovo-elden.dts b/arch/arm64/boot/dts/qcom/sm8750-lenovo-elden.dts
> new file mode 100644
> index 0000000000000..7bbc1986a87e6
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sm8750-lenovo-elden.dts
> @@ -0,0 +1,202 @@

[ ... ]

> +&cci0_i2c0 {
> +	clock-frequency = <400000>;
> +
> +	rear_lens: lens@c {
> +		compatible = "giantec,gt9764";
> +		reg = <0x0c>;
> +		vddio-supply = <&camera_ldo7>;
> +		vdd-supply = <&camera_ldo5>;
> +		giantec,aac-mode = <0x40>;
> +		giantec,aac-timing = <0x7c>;
> +		settling-time-us = <7000>;
> +	};
> +
> +	rear_camera: camera@56 {
> +		compatible = "samsung,s5kjns";
> +		reg = <0x56>;
> +		clocks = <&cambistmclkcc CAM_BIST_MCLK_CC_MCLK3_CLK>;
> +		assigned-clocks = <&cambistmclkcc CAM_BIST_MCLK_CC_MCLK3_CLK>;
> +		assigned-clock-rates = <19200000>;
> +		power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
> +		reset-gpios = <&tlmm 11 GPIO_ACTIVE_LOW>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&rear_camera_default>;
> +		dovdd-supply = <&camera_ldo7>;
> +		dvdd-supply = <&camera_ldo1>;
> +		avdd-supply = <&camera_ldo4>;
> +		lens-focus = <&rear_lens>;
> +		orientation = <1>;
> +		rotation = <90>;
> +	};
> +};
> +
> +&cci1 {
> +	status = "okay";
> +};
> +
> +&cci1_i2c0 {
> +	clock-frequency = <400000>;
> +
> +	front_camera: camera@31 {
> +		compatible = "galaxycore,gc08a8";
> +		reg = <0x31>;
> +		clocks = <&cambistmclkcc CAM_BIST_MCLK_CC_MCLK4_CLK>;
> +		assigned-clocks = <&cambistmclkcc CAM_BIST_MCLK_CC_MCLK4_CLK>;
> +		assigned-clock-rates = <19200000>;
> +		power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
> +		reset-gpios = <&tlmm 7 GPIO_ACTIVE_LOW>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&front_camera_default>;
> +		dovdd-supply = <&camera_ldo7>;
> +		dvdd-supply = <&camera_ldo2>;
> +		avdd-supply = <&camera_ldo3>;
> +		orientation = <0>;
> +		rotation = <270>;
> +	};
> +};

[Severity: Medium]
Will these camera and lens nodes pass dtbs_check?

In sm8750-lenovo-elden.dts, rear_lens (lens@c) uses compatible =
"giantec,gt9764" with vddio-supply, giantec,aac-mode, giantec,aac-timing, and
settling-time-us, rear_camera (camera@56) uses compatible = "samsung,s5kjns",
and front_camera (camera@31) uses compatible = "galaxycore,gc08a8". None of
these compatible strings or vendor properties are documented under
Documentation/devicetree/bindings/, and no in-tree driver currently binds to
them to consume the WL2868C regulator supplies.

In addition, both camera@56 and camera@31 omit the required CSI port endpoint
nodes and attach the SoC internal power-domains = <&camcc
CAM_CC_TITAN_TOP_GDSC> to external I2C camera sensors. Should these nodes be
deferred until their DT bindings and CSI endpoints are documented?

> +
> +&i2c10 {
> +	status = "okay";
> +
> +	pmic@2f {
> +		compatible = "willsemi,wl2868c";
> +		reg = <0x2f>;

[Severity: High]
Can i2c10 and pmic@2f ever probe on this board when qupv3_2 and gpi_dma2 are
still disabled?

In arch/arm64/boot/dts/qcom/sm8750.dtsi, i2c10 (i2c@888000) is a child of
qupv3_2 (geniqup@8c0000) and uses DMA channels from gpi_dma2
(dma-controller@800000), both of which default to status = "disabled":

arch/arm64/boot/dts/qcom/sm8750.dtsi:
		gpi_dma2: dma-controller@800000 {
			compatible = "qcom,sm8750-gpi-dma", "qcom,sm6350-gpi-dma";
			...
			status = "disabled";
		};

		qupv3_2: geniqup@8c0000 {
			compatible = "qcom,geni-se-qup";
			...
			status = "disabled";
			...
			i2c10: i2c@888000 {
				...
				dmas = <&gpi_dma2 0 2 QCOM_GPI_I2C>,
				       <&gpi_dma2 1 2 QCOM_GPI_I2C>;
				dma-names = "tx", "rx";
				status = "disabled";
			};
		};

Child serial engines under a qcom,geni-se-qup node are only populated when
geni_se_probe() in drivers/soc/qcom/qcom-geni-se.c binds to an enabled
geniqup parent device, sets the wrapper drvdata, and calls
devm_of_platform_populate(dev):

drivers/soc/qcom/qcom-geni-se.c:geni_se_probe() {
	...
	dev_set_drvdata(dev, wrapper);
	dev_dbg(dev, "GENI SE Driver probed\n");
	return devm_of_platform_populate(dev);
}

Furthermore, geni_i2c_probe() -> setup_gpi_dma() in
drivers/i2c/busses/i2c-qcom-geni.c requires the parent wrapper drvdata and
gpi_dma2 channels when operating in GPI DMA mode. Because
sm8750-lenovo-elden.dts enables &i2c10 without enabling &qupv3_2 or
&gpi_dma2, qupv3_2 is skipped during device population, i2c10 is never
instantiated, and wl2868c_probe() never runs to register camera_ldo1 through
camera_ldo7. Should &qupv3_2 and &gpi_dma2 also be enabled in
sm8750-lenovo-elden.dts?

> diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi
> index dd738d13df8e0..60b8f2fb5a350 100644
> --- a/arch/arm64/boot/dts/qcom/sm8750.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi
> @@ -3512,6 +3512,72 @@ camcc: clock-controller@ade0000 {
>  			#power-domain-cells = <1>;
>  		};
>  
> +		cci0: cci@ac7b000 {
> +			compatible = "qcom,sm8750-cci", "qcom,msm8996-cci";
> +			reg = <0x0 0x0ac7b000 0x0 0x1000>;
> +			interrupts = <GIC_SPI 426 IRQ_TYPE_EDGE_RISING>;
> +			power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
> +			clocks = <&camcc CAM_CC_CAM_TOP_AHB_CLK>,
> +				 <&camcc CAM_CC_CCI_0_CLK>;
> +			clock-names = "ahb", "cci";
> +			assigned-clocks = <&camcc CAM_CC_CCI_0_CLK>;
> +			assigned-clock-rates = <37500000>;
> +			pinctrl-0 = <&cci0_0_default>;
> +			pinctrl-1 = <&cci0_0_sleep>;
> +			pinctrl-names = "default", "sleep";
> +			status = "disabled";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			cci0_i2c0: i2c-bus@0 {
> +				reg = <0>;
> +				clock-frequency = <400000>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +			};
> +
> +			cci0_i2c1: i2c-bus@1 {
> +				status = "disabled";
> +				reg = <1>;
> +				clock-frequency = <400000>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +			};
> +		};
> +
> +		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>;
> +			pinctrl-1 = <&cci1_0_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 {
> +				status = "disabled";
> +				reg = <1>;
> +				clock-frequency = <400000>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +			};
> +		};

[ ... ]

> @@ -4090,6 +4156,52 @@ qup_uart15_default: qup-uart15-default-state {
>  				bias-pull-up;
>  			};
>  
> +			cci0_0_default: cci0-0-default-state {
> +				sda-pins {
> +					pins = "gpio113";
> +					function = "cci_i2c_sda";
> +					drive-strength = <2>;
> +					bias-pull-up;
> +				};

[Severity: Low]
Is there a reason cci0_i2c1 and cci1_i2c1 are marked status = "disabled" in
sm8750.dtsi while cci0_i2c0 and cci1_i2c0 are left enabled, and bus 1 pinctrl
states (cci0_1 on gpio115/gpio116 and cci1_1 on gpio119/gpio120 in
drivers/pinctrl/qcom/pinctrl-sm8750.c) are omitted?

In drivers/i2c/busses/i2c-qcom-cci.c, cci_probe() iterates over child buses
with for_each_available_child_of_node_scoped(dev->of_node, child), and other
Qualcomm SoC dtsi files (such as sm8550.dtsi and sm8650.dtsi) leave both
i2c-bus@0 and i2c-bus@1 available with both bus pinctrl states defined while
the parent cci node is disabled. In addition, SM8750 has a third CCI
controller (CAM_CC_CCI_2_CLK in drivers/clk/qcom/camcc-sm8750.c and cci2 pins
in drivers/pinctrl/qcom/pinctrl-sm8750.c), whereas the commit message states
that the SoC has two CCI I2C controllers.

Also, in sm8750.dtsi, should cci0@ac7b000 and cci1@ac7c000 be placed in
unit-address order after videocc@aaf0000 rather than after mdss@ae00000,
dispcc@af00000, and camcc@ade0000, and should the cci0_0 and cci1_0 pinctrl
nodes in &tlmm be placed alphabetically before hub_i2c0_data_clk rather than
after qup_uart15_default?

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