Message-ID: <YxCqZ8GhiTJlZ8MC@aptenodytes>
Date:   Thu, 1 Sep 2022 14:49:43 +0200
From:   Paul Kocialkowski <paul.kocialkowski@...tlin.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-sunxi@...ts.linux.dev, linux-kernel@...r.kernel.org,
        linux-media@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Samuel Holland <samuel@...lland.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Kévin L'hôpital <kevin.lhopital@...tlin.com>
Subject: Re: [PATCH NOT FOR MERGE v5 6/6] ARM: dts: sun8i: a83t: bananapi-m3:
 Enable MIPI CSI-2 with OV8865

Hi Laurent,

Thansk for the review,

On Fri 26 Aug 22, 22:08, Laurent Pinchart wrote:
> 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@...tlin.com>
> > 
> > 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 ?

Indeed this patch only adds the OV8865, not the OV5640 which is also present
on the same camera board extension.

A patch was submitted some time ago adding support for (only) the OV5640:
https://lore.kernel.org/lkml/20190408165744.11672-7-wens@kernel.org/

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

Do we have an upstream place to merge device-tree overlays now?

I'll check if it's possible to describe both sensors together and actually
be able to select one or the other properly from userspace. Last time I tried
this wasn't possible: there's at least the shared reset GPIO used by both
sensors, which cannot be requested by the two drivers in parallel.

To be honest this is very poor hardware design and it was almost certainly
designed with the idea that only one sensor will be configured per boot.
See https://wiki.banana-pi.org/Camera and
https://drive.google.com/file/d/0B4PAo2nW2KfnOEFTMjZ2aEVGUVU/view?usp=sharing
for the schematics pdf

> > Signed-off-by: Kévin L'hôpital <kevin.lhopital@...tlin.com>
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@...tlin.com>
> > ---
> >  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 ?

That's 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.

I2C2 is actually also exported in the PH pins, which is available on the board
header, so it's not obvious that using the PE pins should be the default.

The context is that Allwinner SoCs usually have a dedicated I2C controller for
cameras, but also route a "generic" I2C controller on the same pins.
Here that's on the PE14/15 pins. Since we don't have mainline support for this
camera-dedicated I2C controller, we end up routing the generic one to the PE
pins, which are routed to the camera connector.

In the future we could add support for this camera-dedicated controller, which
would then allow routing i2c2 to the PH pins independently. This is what the
downstream Allwinner BSP kernel does.

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

Mhh so I realize the regulators handling here is quite messy.
I guess I didn't do such a good review of this patch internally.

The situation is that the regulators on the camera board all have their
enable pin tied to the DLDO4 output, but that would be best described as
a vin-supply of the ov8865 regulators above. Their real vin supply is an
always-on board-wide power source but I don't think we can represent another
regulator acting as enable signal in a better way.

Now beware that the camera board schematics are confusing as they show resistors
(R17, R18, R19, R20, R23) connecting some power lines together, but they are not
populated on the board (I guess the point is to make a variant of the design
without regulators on the camera board and to use ones from the PMU instead).
This probably confused Kevin and I back when this patch was made.

> > +	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";AVDD-CSI
> >  	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

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists