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@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 ? 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@xxxxxxxxxx/ > 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@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 = <®_dldo4>; > > + }; > > + > > + reg_ov8865_dovdd: ov8865-dovdd { > > + compatible = "regulator-fixed"; > > + regulator-name = "ov8865-dovdd"; > > + regulator-min-microvolt = <2800000>; > > + regulator-max-microvolt = <2800000>; > > + vin-supply = <®_dldo4>; > > + }; > > + > > + reg_ov8865_dvdd: ov8865-dvdd { > > + compatible = "regulator-fixed"; > > + regulator-name = "ov8865-dvdd"; > > + regulator-min-microvolt = <1200000>; > > + regulator-max-microvolt = <1200000>; > > + vin-supply = <®_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 = <®_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 = <®_ov8865_avdd>; > > + dovdd-supply = <®_ov8865_dovdd>; > > + dvdd-supply = <®_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 @@ ®_dldo3 { > > regulator-name = "vcc-pd"; > > }; > > > > +®_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. > > > +}; > > + > > ®_drivevbus { > > regulator-name = "usb0-vbus";AVDD-CSI > > status = "okay"; > > }; > > > > +®_eldo1 { > > + regulator-min-microvolt = <1200000>; > > + regulator-max-microvolt = <1200000>; > > + regulator-name = "dvdd-csi-r"; > > +}; > > + > > ®_fldo1 { > > regulator-min-microvolt = <1080000>; > > regulator-max-microvolt = <1320000>; > > -- > Regards, > > Laurent Pinchart -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com
Attachment:
signature.asc
Description: PGP signature
