Re: [PATCH NOT FOR MERGE v5 6/6] ARM: dts: sun8i: a83t: bananapi-m3: Enable MIPI CSI-2 with OV8865

From: Laurent Pinchart
Date: Fri Aug 26 2022 - 15:08:23 EST


Hi Paul and Kévin,

Thank you for the patch.

On Fri, Aug 26, 2022 at 08:28:03PM +0200, Paul Kocialkowski wrote:
> From: Kévin L'hôpital <kevin.lhopital@xxxxxxxxxxx>
>
> The Bananapi M3 supports a camera module which includes an OV8865 sensor
> connected via the parallel CSI interface and an OV8865 sensor connected
> via MIPI CSI-2.
>
> The I2C2 bus is shared by the two sensors as well as the (active-low)
> reset signal, but each sensor has it own shutdown line.

I see a single sensor in this file, am I missing something ?

Sounds like a perfect candidate for an overlay, it could then be merged
upstream.

> Signed-off-by: Kévin L'hôpital <kevin.lhopital@xxxxxxxxxxx>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> ---
> arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts | 102 +++++++++++++++++++
> 1 file changed, 102 insertions(+)
>
> diff --git a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> index 5a7e1bd5f825..80fd99cf24b2 100644
> --- a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> +++ b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> @@ -85,6 +85,30 @@ led-1 {
> };
> };
>
> + reg_ov8865_avdd: ov8865-avdd {
> + compatible = "regulator-fixed";
> + regulator-name = "ov8865-avdd";
> + regulator-min-microvolt = <2800000>;
> + regulator-max-microvolt = <2800000>;
> + vin-supply = <&reg_dldo4>;
> + };
> +
> + reg_ov8865_dovdd: ov8865-dovdd {
> + compatible = "regulator-fixed";
> + regulator-name = "ov8865-dovdd";
> + regulator-min-microvolt = <2800000>;
> + regulator-max-microvolt = <2800000>;
> + vin-supply = <&reg_dldo4>;
> + };
> +
> + reg_ov8865_dvdd: ov8865-dvdd {
> + compatible = "regulator-fixed";
> + regulator-name = "ov8865-dvdd";
> + regulator-min-microvolt = <1200000>;
> + regulator-max-microvolt = <1200000>;
> + vin-supply = <&reg_eldo1>;
> + };

Are those three regulators on the Banana Pi, or on the camera module ?

> +
> reg_usb1_vbus: reg-usb1-vbus {
> compatible = "regulator-fixed";
> regulator-name = "usb1-vbus";
> @@ -115,6 +139,23 @@ &cpu100 {
> cpu-supply = <&reg_dcdc3>;
> };
>
> +&csi {
> + status = "okay";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@1 {
> + reg = <1>;

All of this (except the status = "okay") should go to sun8i-a83t.dtsi.

> +
> + csi_in_mipi_csi2: endpoint {
> + remote-endpoint = <&mipi_csi2_out_csi>;
> + };

This too actually, once the issue mentioned in patch 5/6 gets fixed.

> + };
> + };
> +};
> +
> &de {
> status = "okay";
> };
> @@ -147,6 +188,36 @@ hdmi_out_con: endpoint {
> };
> };
>
> +&i2c2 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&i2c2_pe_pins>;

This looks like a candidate for upstreaming in
sun8i-a83t-bananapi-m3.dts, it shouldn't be in the overlay.

> + status = "okay";
> +
> + ov8865: camera@36 {
> + compatible = "ovti,ov8865";
> + reg = <0x36>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&csi_mclk_pin>;
> + clocks = <&ccu CLK_CSI_MCLK>;
> + assigned-clocks = <&ccu CLK_CSI_MCLK>;
> + assigned-clock-rates = <24000000>;
> + avdd-supply = <&reg_ov8865_avdd>;
> + dovdd-supply = <&reg_ov8865_dovdd>;
> + dvdd-supply = <&reg_ov8865_dvdd>;
> + powerdown-gpios = <&pio 4 17 GPIO_ACTIVE_LOW>; /* PE17 */
> + reset-gpios = <&pio 4 16 GPIO_ACTIVE_LOW>; /* PE16 */
> +
> + port {
> + ov8865_out_mipi_csi2: endpoint {
> + data-lanes = <1 2 3 4>;
> + link-frequencies = /bits/ 64 <360000000>;
> +
> + remote-endpoint = <&mipi_csi2_in_ov8865>;
> + };
> + };
> + };
> +};
> +
> &mdio {
> rgmii_phy: ethernet-phy@1 {
> compatible = "ethernet-phy-ieee802.3-c22";
> @@ -154,6 +225,24 @@ rgmii_phy: ethernet-phy@1 {
> };
> };
>
> +&mipi_csi2 {
> + status = "okay";
> +};
> +
> +&mipi_csi2_in {
> + mipi_csi2_in_ov8865: endpoint {
> + data-lanes = <1 2 3 4>;
> +
> + remote-endpoint = <&ov8865_out_mipi_csi2>;
> + };
> +};
> +
> +&mipi_csi2_out {
> + mipi_csi2_out_csi: endpoint {
> + remote-endpoint = <&csi_in_mipi_csi2>;
> + };
> +};
> +
> &mmc0 {
> pinctrl-names = "default";
> pinctrl-0 = <&mmc0_pins>;
> @@ -327,11 +416,24 @@ &reg_dldo3 {
> regulator-name = "vcc-pd";
> };
>
> +&reg_dldo4 {
> + regulator-always-on;

Does it have to be always on ?

> + regulator-min-microvolt = <2800000>;
> + regulator-max-microvolt = <2800000>;
> + regulator-name = "avdd-csi";

Doesn't this belong to the base board .dts ? Same below.

> +};
> +
> &reg_drivevbus {
> regulator-name = "usb0-vbus";
> status = "okay";
> };
>
> +&reg_eldo1 {
> + regulator-min-microvolt = <1200000>;
> + regulator-max-microvolt = <1200000>;
> + regulator-name = "dvdd-csi-r";
> +};
> +
> &reg_fldo1 {
> regulator-min-microvolt = <1080000>;
> regulator-max-microvolt = <1320000>;

--
Regards,

Laurent Pinchart