Re: [PATCH v7 6/7] ARM: dts: sun8i-a83t: Add BananaPi M3 OV5640 camera overlay

[Jernej Škrabec]

Hi Paul!

On Wednesday, November 22, 2023 3:14:24 PM CET Paul Kocialkowski wrote:
> Add an overlay supporting the OV5640 from the BananaPi Camera v3
> peripheral board. The board has two sensors (OV5640 and OV8865)
> which cannot be supported in parallel as they share the same reset
> pin and the kernel currently has no support for this case.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> ---
>  arch/arm/boot/dts/allwinner/Makefile          |   1 +
>  .../sun8i-a83t-bananapi-m3-camera-ov5640.dtso | 117 ++++++++++++++++++
>  2 files changed, 118 insertions(+)
>  create mode 100644 arch/arm/boot/dts/allwinner/sun8i-a83t-bananapi-m3-camera-ov5640.dtso
> 
> diff --git a/arch/arm/boot/dts/allwinner/Makefile b/arch/arm/boot/dts/allwinner/Makefile
> index eebb5a0c873a..a0a9aa6595e4 100644
> --- a/arch/arm/boot/dts/allwinner/Makefile
> +++ b/arch/arm/boot/dts/allwinner/Makefile
> @@ -277,6 +277,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
>  	sun8i-a33-sinlinx-sina33.dtb \
>  	sun8i-a83t-allwinner-h8homlet-v2.dtb \
>  	sun8i-a83t-bananapi-m3.dtb \
> +	sun8i-a83t-bananapi-m3-camera-ov5640.dtbo \
>  	sun8i-a83t-cubietruck-plus.dtb \
>  	sun8i-a83t-tbs-a711.dtb \
>  	sun8i-h2-plus-bananapi-m2-zero.dtb \
> diff --git a/arch/arm/boot/dts/allwinner/sun8i-a83t-bananapi-m3-camera-ov5640.dtso b/arch/arm/boot/dts/allwinner/sun8i-a83t-bananapi-m3-camera-ov5640.dtso
> new file mode 100644
> index 000000000000..5868ef11bdee
> --- /dev/null
> +++ b/arch/arm/boot/dts/allwinner/sun8i-a83t-bananapi-m3-camera-ov5640.dtso
> @@ -0,0 +1,117 @@
> +// SPDX-License-Identifier: GPL-2.0 OR X11
> +/*
> + * Copyright 2022 Bootlin
> + * Author: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +
> +#include <dt-bindings/clock/sun8i-a83t-ccu.h>
> +#include <dt-bindings/gpio/gpio.h>

I think you should tie this overlay to BananaPi M3 board by specifying its
compatible here, like:

/ {
	compatible = "sinovoip,bpi-m3";
};

> +
> +&{/} {
> +	/*
> +	 * These regulators actually have DLDO4 tied to their EN pin, which is
> +	 * described as input supply here for lack of a better representation.
> +	 * Their actual supply is PS, which is always-on.
> +	 */
> +
> +	ov5640_avdd: ov5640-avdd {
> +		compatible = "regulator-fixed";
> +		regulator-name = "ov5640-avdd";
> +		regulator-min-microvolt = <2800000>;
> +		regulator-max-microvolt = <2800000>;
> +		vin-supply = <&reg_dldo4>;
> +	};
> +
> +	ov5640_dovdd: ov5640-dovdd {
> +		compatible = "regulator-fixed";
> +		regulator-name = "ov5640-dovdd";
> +		regulator-min-microvolt = <2800000>;
> +		regulator-max-microvolt = <2800000>;
> +		vin-supply = <&reg_dldo4>;
> +	};
> +
> +	ov5640_dvdd: ov5640-dvdd {
> +		compatible = "regulator-fixed";
> +		regulator-name = "ov5640-dvdd";
> +		regulator-min-microvolt = <1500000>;
> +		regulator-max-microvolt = <1500000>;
> +		vin-supply = <&reg_dldo4>;
> +	};
> +};
> +
> +&csi {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&csi_8bit_parallel_pins>;
> +	status = "okay";
> +
> +	ports {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		port@0 {
> +			reg = <0>;
> +
> +			csi_in_ov5640: endpoint {
> +				remote-endpoint = <&ov5640_out_csi>;
> +				bus-width = <8>;
> +				data-shift = <2>;
> +				hsync-active = <1>;
> +				vsync-active = <1>;
> +				pclk-sample = <1>;
> +			};
> +		};
> +	};
> +};
> +
> +&i2c2 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2c2_pe_pins>;
> +	status = "okay";
> +
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	ov5640: camera@3c {
> +		compatible = "ovti,ov5640";
> +		reg = <0x3c>;
> +
> +		clocks = <&ccu CLK_CSI_MCLK>;
> +		clock-names = "xclk";
> +		assigned-clocks = <&ccu CLK_CSI_MCLK>;
> +		assigned-clock-parents = <&osc24M>;
> +		assigned-clock-rates = <24000000>;

Are those really necessary? If so, can it be moved to the driver? Assigned
clock rates have no guarantee that they will stay at that rate. Additionally,
same sensor in pinetab doesn't need that. What's the difference?

> +
> +		AVDD-supply = <&ov5640_avdd>;
> +		DOVDD-supply = <&ov5640_dovdd>;
> +		DVDD-supply = <&ov5640_dvdd>;
> +
> +		powerdown-gpios = <&pio 3 15 GPIO_ACTIVE_HIGH>; /* PD15 */
> +		reset-gpios = <&pio 4 16 GPIO_ACTIVE_LOW>; /* PE16 */
> +
> +		rotation = <180>;
> +
> +		port {
> +			ov5640_out_csi: endpoint {
> +				remote-endpoint = <&csi_in_ov5640>;
> +				bus-width = <8>;
> +				data-shift = <2>;
> +				hsync-active = <1>; 
> +				vsync-active = <1>;
> +				pclk-sample = <1>;
> +			};
> +		};
> +	};
> +};
> +
> +&pio {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&csi_mclk_pin>;

This should be moved to ov5640 node.

Best regards,
Jernej

> +};
> +
> +&reg_dldo4 {
> +	regulator-min-microvolt = <2800000>;
> +	regulator-max-microvolt = <2800000>;
> +};
>