From: Kévin L'hôpital <[email protected]>
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.
Signed-off-by: Kévin L'hôpital <[email protected]>
Signed-off-by: Paul Kocialkowski <[email protected]>
---
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>;
+ };
+
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>;
+
+ csi_in_mipi_csi2: endpoint {
+ remote-endpoint = <&mipi_csi2_out_csi>;
+ };
+ };
+ };
+};
+
&de {
status = "okay";
};
@@ -147,6 +188,36 @@ hdmi_out_con: endpoint {
};
};
+&i2c2 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&i2c2_pe_pins>;
+ 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;
+ regulator-min-microvolt = <2800000>;
+ regulator-max-microvolt = <2800000>;
+ regulator-name = "avdd-csi";
+};
+
®_drivevbus {
regulator-name = "usb0-vbus";
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>;
--
2.37.1
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 <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> 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 ?
> +
> 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.
> + 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 ?
> + 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";
> 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
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 <[email protected]>
> >
> > 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/[email protected]/
> 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 <[email protected]>
> > Signed-off-by: Paul Kocialkowski <[email protected]>
> > ---
> > 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
On Thu, Sep 01, 2022 at 02:49:43PM +0200, Paul Kocialkowski wrote:
> On Fri 26 Aug 22, 22:08, Laurent Pinchart wrote:
> > On Fri, Aug 26, 2022 at 08:28:03PM +0200, Paul Kocialkowski wrote:
> > > From: Kévin L'hôpital <[email protected]>
> > >
> > > 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/[email protected]/
OK. That's fine, let's just reword the commit message.
> > 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?
They're accepted in the upstream kernel as far as I know, a git grep for
/plugin/ in .dts files produces 18 matches.
> 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
It's not great indeed, but not that uncommon either (unfortunately). I
wonder if we need some kind of reset GPIO API, but that would be a hack
at most I think.
> > > Signed-off-by: Kévin L'hôpital <[email protected]>
> > > Signed-off-by: Paul Kocialkowski <[email protected]>
> > > ---
> > > 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.
Out of curiosity, what features do those dedicated camera I2C
controllers provide, compared to "normal" I2C controllers ?
> 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.
OK.
> 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.
That's right. I've modeled that as a parent regulator in the past, even
if it doesn't reflect the exact hardware topology, it's functionally
equivalent.
> 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
Hi Laurent,
On Thu 01 Sep 22, 16:00, Laurent Pinchart wrote:
> On Thu, Sep 01, 2022 at 02:49:43PM +0200, Paul Kocialkowski wrote:
> > On Fri 26 Aug 22, 22:08, Laurent Pinchart wrote:
> > > On Fri, Aug 26, 2022 at 08:28:03PM +0200, Paul Kocialkowski wrote:
> > > > From: Kévin L'hôpital <[email protected]>
> > > >
> > > > 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/[email protected]/
>
> OK. That's fine, let's just reword the commit message.
Sure.
> > > 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?
>
> They're accepted in the upstream kernel as far as I know, a git grep for
> /plugin/ in .dts files produces 18 matches.
Oh okay, definitely good to know thanks!
> > 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
>
> It's not great indeed, but not that uncommon either (unfortunately). I
> wonder if we need some kind of reset GPIO API, but that would be a hack
> at most I think.
Yeah I don't see any obvious clean solution here.
But that could still be two separate dt overlays for now.
> > > > Signed-off-by: Kévin L'hôpital <[email protected]>
> > > > Signed-off-by: Paul Kocialkowski <[email protected]>
> > > > ---
> > > > 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.
>
> Out of curiosity, what features do those dedicated camera I2C
> controllers provide, compared to "normal" I2C controllers ?
IIRC there's some feature to send i2c messages synced with the camera interface
vsync signal, to have some kind of hardware atomic mechanism for reconfiguring
a sensor.
Not sure that's very relevant for us and an implementation for it would probably
just support regular I2C. We can probably achieve simialr results with the
request API.
> > 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.
>
> OK.
>
> > 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.
>
> That's right. I've modeled that as a parent regulator in the past, even
> if it doesn't reflect the exact hardware topology, it's functionally
> equivalent.
Sounds good, I'll change it that way.
Cheers,
Paul
> > 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
Hi Dave,
On Thu 01 Sep 22, 14:34, Dave Stevenson wrote:
> Hi Laurent and Paul
>
> On Thu, 1 Sept 2022 at 14:23, Paul Kocialkowski
> <[email protected]> wrote:
> >
> > Hi Laurent,
> >
> > On Thu 01 Sep 22, 16:00, Laurent Pinchart wrote:
> > > On Thu, Sep 01, 2022 at 02:49:43PM +0200, Paul Kocialkowski wrote:
> > > > On Fri 26 Aug 22, 22:08, Laurent Pinchart wrote:
> > > > > On Fri, Aug 26, 2022 at 08:28:03PM +0200, Paul Kocialkowski wrote:
> > > > > > From: Kévin L'hôpital <[email protected]>
> > > > > >
> > > > > > 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/[email protected]/
> > >
> > > OK. That's fine, let's just reword the commit message.
> >
> > Sure.
> >
> > > > > 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?
> > >
> > > They're accepted in the upstream kernel as far as I know, a git grep for
> > > /plugin/ in .dts files produces 18 matches.
> >
> > Oh okay, definitely good to know thanks!
> >
> > > > 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
> > >
> > > It's not great indeed, but not that uncommon either (unfortunately). I
> > > wonder if we need some kind of reset GPIO API, but that would be a hack
> > > at most I think.
> >
> > Yeah I don't see any obvious clean solution here.
> > But that could still be two separate dt overlays for now.
>
> Just to throw the idea out there, (ab)use the regulator API so that
> they share a regulator-gpio device that claims that GPIO.
> When either sensor requests the regulator, the reset line will get
> pulled to the appropriate state.
Yeah I guess that would solve the single-gpio/multiple-users issue.
But besides abusing one of the regulators for the job, that would also make
it hard to respect the power sequence of the sensor.
I think I would be happier with a dedicated GPIO reset driver that consumes a
single GPIO and provides as many GPIOs as needed with a behavior similar to
a reset controller: go out of reset as soon as one of the provided GPIOs
requests it and get back to reset as soon as all the provided GPIOs need
reset.
It's a bit weird but maybe that's legit enough to make its way upstream.
Paul
> Dave
>
> > > > > > Signed-off-by: Kévin L'hôpital <[email protected]>
> > > > > > Signed-off-by: Paul Kocialkowski <[email protected]>
> > > > > > ---
> > > > > > 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.
> > >
> > > Out of curiosity, what features do those dedicated camera I2C
> > > controllers provide, compared to "normal" I2C controllers ?
> >
> > IIRC there's some feature to send i2c messages synced with the camera interface
> > vsync signal, to have some kind of hardware atomic mechanism for reconfiguring
> > a sensor.
> >
> > Not sure that's very relevant for us and an implementation for it would probably
> > just support regular I2C. We can probably achieve simialr results with the
> > request API.
> >
> > > > 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.
> > >
> > > OK.
> > >
> > > > 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.
> > >
> > > That's right. I've modeled that as a parent regulator in the past, even
> > > if it doesn't reflect the exact hardware topology, it's functionally
> > > equivalent.
> >
> > Sounds good, I'll change it that way.
> >
> > Cheers,
> >
> > Paul
> >
> > > > 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
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Hi Laurent and Paul
On Thu, 1 Sept 2022 at 14:23, Paul Kocialkowski
<[email protected]> wrote:
>
> Hi Laurent,
>
> On Thu 01 Sep 22, 16:00, Laurent Pinchart wrote:
> > On Thu, Sep 01, 2022 at 02:49:43PM +0200, Paul Kocialkowski wrote:
> > > On Fri 26 Aug 22, 22:08, Laurent Pinchart wrote:
> > > > On Fri, Aug 26, 2022 at 08:28:03PM +0200, Paul Kocialkowski wrote:
> > > > > From: Kévin L'hôpital <[email protected]>
> > > > >
> > > > > 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/[email protected]/
> >
> > OK. That's fine, let's just reword the commit message.
>
> Sure.
>
> > > > 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?
> >
> > They're accepted in the upstream kernel as far as I know, a git grep for
> > /plugin/ in .dts files produces 18 matches.
>
> Oh okay, definitely good to know thanks!
>
> > > 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
> >
> > It's not great indeed, but not that uncommon either (unfortunately). I
> > wonder if we need some kind of reset GPIO API, but that would be a hack
> > at most I think.
>
> Yeah I don't see any obvious clean solution here.
> But that could still be two separate dt overlays for now.
Just to throw the idea out there, (ab)use the regulator API so that
they share a regulator-gpio device that claims that GPIO.
When either sensor requests the regulator, the reset line will get
pulled to the appropriate state.
Dave
> > > > > Signed-off-by: Kévin L'hôpital <[email protected]>
> > > > > Signed-off-by: Paul Kocialkowski <[email protected]>
> > > > > ---
> > > > > 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.
> >
> > Out of curiosity, what features do those dedicated camera I2C
> > controllers provide, compared to "normal" I2C controllers ?
>
> IIRC there's some feature to send i2c messages synced with the camera interface
> vsync signal, to have some kind of hardware atomic mechanism for reconfiguring
> a sensor.
>
> Not sure that's very relevant for us and an implementation for it would probably
> just support regular I2C. We can probably achieve simialr results with the
> request API.
>
> > > 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.
> >
> > OK.
> >
> > > 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.
> >
> > That's right. I've modeled that as a parent regulator in the past, even
> > if it doesn't reflect the exact hardware topology, it's functionally
> > equivalent.
>
> Sounds good, I'll change it that way.
>
> Cheers,
>
> Paul
>
> > > 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
Hi Paul,
On Thu, Sep 01, 2022 at 03:22:34PM +0200, Paul Kocialkowski wrote:
> On Thu 01 Sep 22, 16:00, Laurent Pinchart wrote:
> > On Thu, Sep 01, 2022 at 02:49:43PM +0200, Paul Kocialkowski wrote:
> > > On Fri 26 Aug 22, 22:08, Laurent Pinchart wrote:
> > > > On Fri, Aug 26, 2022 at 08:28:03PM +0200, Paul Kocialkowski wrote:
> > > > > From: Kévin L'hôpital <[email protected]>
> > > > >
> > > > > 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/[email protected]/
> >
> > OK. That's fine, let's just reword the commit message.
>
> Sure.
>
> > > > 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?
> >
> > They're accepted in the upstream kernel as far as I know, a git grep for
> > /plugin/ in .dts files produces 18 matches.
>
> Oh okay, definitely good to know thanks!
>
> > > 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
> >
> > It's not great indeed, but not that uncommon either (unfortunately). I
> > wonder if we need some kind of reset GPIO API, but that would be a hack
> > at most I think.
>
> Yeah I don't see any obvious clean solution here.
> But that could still be two separate dt overlays for now.
>
> > > > > Signed-off-by: Kévin L'hôpital <[email protected]>
> > > > > Signed-off-by: Paul Kocialkowski <[email protected]>
> > > > > ---
> > > > > 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.
> >
> > Out of curiosity, what features do those dedicated camera I2C
> > controllers provide, compared to "normal" I2C controllers ?
>
> IIRC there's some feature to send i2c messages synced with the camera interface
> vsync signal, to have some kind of hardware atomic mechanism for reconfiguring
> a sensor.
>
> Not sure that's very relevant for us and an implementation for it would probably
> just support regular I2C. We can probably achieve simialr results with the
> request API.
This hardware feature would be very useful to improve compliance with
the real-time requirements of the camera sensor. It's something that I
would really like to be supported in Linux, but we'll need to design a
new API for it.
> > > 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.
> >
> > OK.
> >
> > > 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.
> >
> > That's right. I've modeled that as a parent regulator in the past, even
> > if it doesn't reflect the exact hardware topology, it's functionally
> > equivalent.
>
> Sounds good, I'll change it that 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