Re: [PATCH] arm64: dts: renesas: r8a774c0-cat874: Add support for AISTARVISION MIPI Adapter V2.1

Hi Geert,

Thank you for the review.

On Mon, Mar 2, 2020 at 3:57 PM Geert Uytterhoeven [off-list ref] wrote:

Hi Lad,

CC linux-media

On Fri, Feb 28, 2020 at 6:02 PM Lad Prabhakar
[off-list ref] wrote:

quoted

This patch adds support AISTARVISION MIPI Adapter V2.1 board connected
to G2E board. Common file aistarvision-mipi-adapter-2.1.dtsi is created
which have the camera endpoint nodes with disabled status and in
r8a774c0-ek874-mipi-2.1.dts file VIN/CSI nodes are enabled. By default
imx219 endpoint is tied with CSI2.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

quoted

Geert/Rob since the imx219 driver is yet to make into mainline
but has been merged into media-subsystem I would like to take
this patch via media-tree.

Usually DTS patches are merged through renesas-devel and arm-soc, not
through a driver's subsystems tree.  This is done to avoid merge
conflicts.  I prefer not to deviate from that, unless there is a very
good reason to do so.

Is there any dependency on the code in the media tree that I'm missing?
Once DT bindings have been accepted in a subsystem maintainer's tree,
you can start using them in DTS files.

In that case lets take this patch via renesas-devel, as the
DT-bindings patch has
been accepted.

quoted

--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/aistarvision-mipi-adapter-2.1.dtsi

@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device Tree Source for the AISTARVISION MIPI Adapter V2.1
+ *
+ * Copyright (C) 2020 Renesas Electronics Corp.
+ */
+
+/ {
+       ov5645_vdddo_1v8: 1p8v {
+               compatible = "regulator-fixed";
+               regulator-name = "camera_vdddo";
+               regulator-min-microvolt = <1800000>;
+               regulator-max-microvolt = <1800000>;
+               regulator-always-on;
+       };
+
+       ov5645_vdda_2v8: 2p8v {
+               compatible = "regulator-fixed";
+               regulator-name = "camera_vdda";
+               regulator-min-microvolt = <2800000>;
+               regulator-max-microvolt = <2800000>;
+               regulator-always-on;
+       };
+
+       ov5645_vddd_1v5: 1p5v {
+               compatible = "regulator-fixed";
+               regulator-name = "camera_vddd";
+               regulator-min-microvolt = <1500000>;
+               regulator-max-microvolt = <1500000>;
+               regulator-always-on;
+       };
+
+       imx219_vana_2v8: 2p8v {
+               compatible = "regulator-fixed";
+               regulator-name = "camera_vana";
+               regulator-min-microvolt = <2800000>;
+               regulator-max-microvolt = <2800000>;
+               regulator-always-on;
+       };
+
+       imx219_vdig_1v8: 1p8v {
+               compatible = "regulator-fixed";
+               regulator-name = "camera_vdig";
+               regulator-min-microvolt = <1500000>;
+               regulator-max-microvolt = <1500000>;
+               regulator-always-on;
+       };
+
+       imx219_vddl_1v2: 1p2v {
+               compatible = "regulator-fixed";
+               regulator-name = "camera_vddl";
+               regulator-min-microvolt = <1200000>;
+               regulator-max-microvolt = <1200000>;
+               regulator-always-on;
+       };
+
+       imx219_clk: imx219_clk {
+               compatible = "fixed-clock";
+               #clock-cells = <0>;
+               clock-frequency = <24000000>;
+       };
+};
+
+&MIPI_PARENT_I2C {
+       ov5645: ov5645@3c {
+               compatible = "ovti,ov5645";
+               reg = <0x3c>;
+               status = "disabled";

Is there any real need to disable this node here?
Do you envision anyone including this .dtsi file, and not enabling this
node?

Agreed will drop it.

quoted

+
+               clock-names = "xclk";
+
+               vdddo-supply = <&ov5645_vdddo_1v8>;
+               vdda-supply = <&ov5645_vdda_2v8>;
+               vddd-supply = <&ov5645_vddd_1v5>;
+
+               port@0 {

DT bindings say "port", without unit-address.

shall drop it.

quoted

+                       ov5645_ep: endpoint {
+                       };
+               };
+       };
+
+       rpi_v2_camera: imx219@10 {
+               compatible = "sony,imx219";
+               reg = <0x10>;
+               status = "disabled";

Likewise.

quoted

+
+               VANA-supply = <&imx219_vana_2v8>;
+               VDIG-supply = <&imx219_vdig_1v8>;
+               VDDL-supply = <&imx219_vddl_1v2>;
+               clocks = <&imx219_clk>;
+
+               port@0 {

DT bindings say "port", without unit-address...

quoted

+                       reg = <0>;

... and thus no "reg" property.

shall drop it.

quoted

+                       imx219_ep0: endpoint {
+                       };
+               };
+       };
+};

diff --git a/arch/arm64/boot/dts/renesas/r8a774c0-ek874-mipi-2.1.dts b/arch/arm64/boot/dts/renesas/r8a774c0-ek874-mipi-2.1.dts
new file mode 100644
index 000000000000..435b7f62d88d
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/r8a774c0-ek874-mipi-2.1.dts

@@ -0,0 +1,86 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device Tree Source for the Silicon Linux RZ/G2E 96board platform (CAT874)
+ * connected with aistarvision-mipi-v2-adapter board
+ *
+ * Copyright (C) 2020 Renesas Electronics Corp.
+ */
+
+/dts-v1/;
+#include "r8a774c0-ek874.dts"
+#define MIPI_PARENT_I2C i2c3
+#include "aistarvision-mipi-adapter-2.1.dtsi"
+
+/ {
+       model = "Silicon Linux RZ/G2E evaluation kit EK874 (CAT874 + CAT875) with aistarvision-mipi-v2-adapter board";
+       compatible = "si-linux,cat875", "si-linux,cat874", "renesas,r8a774c0";
+};
+
+&i2c3 {
+       status = "okay";
+};
+
+&vin4 {
+       status = "okay";
+};
+
+&vin5 {
+       status = "okay";
+};
+
+&csi40 {
+       status = "okay";
+
+       ports {
+               port@0 {
+                       reg = <0>;
+
+                       csi40_in: endpoint {
+                               clock-lanes = <0>;
+                               data-lanes = <1 2>;
+                               remote-endpoint = <&imx219_ep0>;
+                       };
+               };
+       };
+};
+
+&ov5645 {
+       /* uncomment status and remote-endpoint properties to tie ov5645
+        * to CSI2 also make sure remote-endpoint for imx219 camera is
+        * commented and remote endpoint in csi40_in is ov5645_ep
+        */
+       /* status = "okay"; */
+
+       #address-cells = <1>;
+       #size-cells = <0>;

#{address,size}-cells not needed.

agreed will drop it.

quoted

+       enable-gpios = <&gpio5 5 GPIO_ACTIVE_HIGH>;
+       reset-gpios = <&gpio5 3 GPIO_ACTIVE_LOW>;
+
+       clocks = <&cpg CPG_MOD 716>;
+       clock-frequency = <24000000>;

I know this is dictated by the DT bindings for the ov5645 camera, but
specifying a clock rate is usually done through assigned-clock-rates,
cfr.  Documentation/devicetree/bindings/clock/clock-bindings.txt.

agreed will replace it.

Cheers,
--Prabhakar

quoted

+
+       port@0 {

port {

quoted

+               ov5645_ep: endpoint {
+                       clock-lanes = <0>;
+                       data-lanes = <1 2>;
+                       /* remote-endpoint = <&csi40_in>; */
+               };
+       };
+};
+
+&rpi_v2_camera {
+       status = "okay";
+
+       #address-cells = <1>;
+       #size-cells = <0>;
+
+       port@0 {
+               reg = <0>;

port {

quoted

+               imx219_ep0: endpoint {
+                       clock-lanes = <0>;
+                       data-lanes = <1 2>;
+                       remote-endpoint = <&csi40_in>;
+                       link-frequencies = /bits/ 64 <456000000>;
+               };
+       };
+};

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds