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

[Alain Volmat <alain.volmat@foss.st.com>]

Hi Krzysztof,

On Tue, Oct 08, 2024 at 03:42:42PM +0200, Krzysztof Kozlowski wrote:
> 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@foss.st.com>
> > ---
> >  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

Done in v2 for all 3 fixed-regulators.

> 
> > +		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

Changed to camera@1a.

> 
> > +		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?

status property dropped in v2.  powerdown-gpios property dropped as
well since not necessary nor described in the sensor yaml.
reset-gpios polarity is as well corrected in v2, following an change
within the imx335 sensor.

> 
> Best regards,
> Krzysztof
> 

Regards,
Alain