Re: [PATCH 15/15] arm64: dts: st: enable imx335/csi/dcmipp pipeline on stm32mp257f-ev1

[Krzysztof Kozlowski]

On Tue, Oct 08, 2024 at 01:18:17PM +0200, Alain Volmat wrote:
> Enable the camera pipeline with a imx335 sensor connected to the
> dcmipp via the csi interface.
> 
> Signed-off-by: Alain Volmat <alain.volmat@xxxxxxxxxxx>
> ---
>  arch/arm64/boot/dts/st/stm32mp257f-ev1.dts | 87 ++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts b/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts
> index 214191a8322b..599af4801d82 100644
> --- a/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts
> +++ b/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts
> @@ -27,6 +27,38 @@ chosen {
>  		stdout-path = "serial0:115200n8";
>  	};
>  
> +	clocks {
> +		clk_ext_camera: clk-ext-camera {
> +			#clock-cells = <0>;
> +			compatible = "fixed-clock";
> +			clock-frequency = <24000000>;
> +		};
> +	};
> +
> +	imx335_2v9: imx335-2v9 {

Please use name for all fixed regulators which matches current format
recommendation: 'regulator-[0-9]v[0-9]'
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml?h=v6.11-rc1#n46

> +		compatible = "regulator-fixed";
> +		regulator-name = "imx335-avdd";
> +		regulator-min-microvolt = <2900000>;
> +		regulator-max-microvolt = <2900000>;
> +		regulator-always-on;
> +	};
> +
> +	imx335_1v8: imx335-1v8 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "imx335-ovdd";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +		regulator-always-on;
> +	};
> +
> +	imx335_1v2: imx335-1v2 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "imx335-dvdd";
> +		regulator-min-microvolt = <1200000>;
> +		regulator-max-microvolt = <1200000>;
> +		regulator-always-on;
> +	};
> +
>  	memory@80000000 {
>  		device_type = "memory";
>  		reg = <0x0 0x80000000 0x1 0x0>;
> @@ -50,6 +82,40 @@ &arm_wdt {
>  	status = "okay";
>  };
>  
> +&csi {
> +	vdd-supply =  <&scmi_vddcore>;
> +	vdda18-supply = <&scmi_v1v8>;
> +	status = "okay";
> +	ports {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		port@0 {
> +			reg = <0>;
> +			csi_sink: endpoint {
> +				remote-endpoint = <&imx335_ep>;
> +				data-lanes = <0 1>;
> +				bus-type = <4>;
> +			};
> +		};
> +		port@1 {
> +			reg = <1>;
> +			csi_source: endpoint {
> +				remote-endpoint = <&dcmipp_0>;
> +			};
> +		};
> +	};
> +};
> +
> +&dcmipp {
> +	status = "okay";
> +	port {
> +		dcmipp_0: endpoint {
> +			remote-endpoint = <&csi_source>;
> +			bus-type = <4>;
> +		};
> +	};
> +};
> +
>  &ethernet2 {
>  	pinctrl-names = "default", "sleep";
>  	pinctrl-0 = <&eth2_rgmii_pins_a>;
> @@ -81,6 +147,27 @@ &i2c2 {
>  	i2c-scl-falling-time-ns = <13>;
>  	clock-frequency = <400000>;
>  	status = "okay";
> +
> +	imx335: imx335@1a {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +		compatible = "sony,imx335";
> +		reg = <0x1a>;
> +		clocks = <&clk_ext_camera>;
> +		avdd-supply = <&imx335_2v9>;
> +		ovdd-supply = <&imx335_1v8>;
> +		dvdd-supply = <&imx335_1v2>;
> +		reset-gpios = <&gpioi 7 (GPIO_ACTIVE_HIGH | GPIO_PUSH_PULL)>;
> +		powerdown-gpios = <&gpioi 0 (GPIO_ACTIVE_HIGH | GPIO_PUSH_PULL)>;
> +		status = "okay";

Why? Didi you disable it anywhere?

Best regards,
Krzysztof