[PATCH v2 0/4] arm64: dts: renesas: Enable GMSL on R8A77970 V3M Eagle

[PATCH v2 0/4] arm64: dts: renesas: Enable GMSL on R8A77970 V3M Eagle

[Jacopo Mondi]

Hello,
   this series adds a .dtsi fragment that allow to describe and
enable GMSL cameras on the V3M Eagle board.

The .dtsi supports connecting the RDACM20 and RDACM21 cameras to the
FAKRA connectors installed on the board.

Tested on V3M Eagle with RDACM20 and RDACM21

v1->v2:
- Use a pattern property to describe the gpio-hog

Thanks
   j

Jacopo Mondi (1):
  dt-bindings: media: max9286: Describe gpio-hog

Kieran Bingham (3):
  arm64: dts: renesas: eagle: Enable MAX9286
  arm64: dts: renesas: eagle: Add GMSL .dtsi
  arm64: dts: renesas: eagle: Include eagle-gmsl

 .../bindings/media/i2c/maxim,max9286.yaml     |  16 ++
 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi   | 186 ++++++++++++++++++
 .../arm64/boot/dts/renesas/r8a77970-eagle.dts | 135 +++++++++++++
 3 files changed, 337 insertions(+)
 create mode 100644 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi

--
2.30.0

[PATCH v2 1/4] dt-bindings: media: max9286: Describe gpio-hog

[Jacopo Mondi]

The MAX9286 GMSL deserializer features gpio controller capabilities,
as it provides 2 GPIO lines.

As establishing a regulator that uses one of the GPIO lines and
enabling/disabling it at run-time in the max9286 won't work due to
a circular dependency on the gpio-controller/regulator creation, allow
the usage of a gpio-hog for that purpose.

The usage of the gpio-hog is required in designs where the MAX9286
GPIO lines control the remote cameras power.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../bindings/media/i2c/maxim,max9286.yaml        | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
index ee16102fdfe7..9038300e373c 100644
--- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
@@ -177,6 +177,22 @@ properties:
 
     additionalProperties: false
 
+patternProperties:
+  "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$":
+    type: object
+    properties:
+      gpio-hog: true
+      gpios: true
+      output-low: true
+      line-name: true
+
+    required:
+      - gpio-hog
+      - gpios
+      - output-low
+
+    additionalProperties: false
+
 required:
   - compatible
   - reg
-- 
2.30.0

[PATCH v2 2/4] arm64: dts: renesas: eagle: Enable MAX9286

[Jacopo Mondi]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Enable the MAX9286 GMSL deserializer on the Eagle-V3M board.

Connected cameras should be defined in a device-tree overlay or included
after these definitions.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../arm64/boot/dts/renesas/r8a77970-eagle.dts | 132 ++++++++++++++++++
 1 file changed, 132 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
index 874a7fc2730b..d2f63cccc238 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -6,6 +6,8 @@
  * Copyright (C) 2017 Cogent Embedded, Inc.
  */
 
+#include <dt-bindings/gpio/gpio.h>
+
 /dts-v1/;
 #include "r8a77970.dtsi"
 
@@ -188,6 +190,11 @@ i2c0_pins: i2c0 {
 		function = "i2c0";
 	};
 
+	i2c3_pins: i2c3 {
+		groups = "i2c3_a";
+		function = "i2c3";
+	};
+
 	qspi0_pins: qspi0 {
 		groups = "qspi0_ctrl", "qspi0_data4";
 		function = "qspi0";
@@ -266,6 +273,131 @@ &rwdt {
 	status = "okay";
 };
 
+&csi40 {
+	status = "okay";
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+
+			csi40_in: endpoint {
+				clock-lanes = <0>;
+				data-lanes = <1 2 3 4>;
+				remote-endpoint = <&max9286_out0>;
+			};
+		};
+	};
+};
+
+&i2c3 {
+	pinctrl-0 = <&i2c3_pins>;
+	pinctrl-names = "default";
+
+	status = "okay";
+	clock-frequency = <400000>;
+
+	gmsl: gmsl-deserializer@48 {
+		gpio-controller;
+		#gpio-cells = <2>;
+
+		compatible = "maxim,max9286";
+		reg = <0x48>;
+
+		/* eagle-pca9654-max9286-pwdn */
+		enable-gpios = <&io_expander 0 GPIO_ACTIVE_HIGH>;
+
+		/*
+		 * Workaround: Hog the CAMVDD line high as we can't establish a
+		 * regulator-fixed on the gpio_chip exposed by &gmsl due to
+		 * circular-dependency issues.
+		 */
+		camvdd-en-hog {
+			gpio-hog;
+			gpios = <0 0>;
+			output-low;
+			line-name = "CAMVDD_EN";
+		};
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				max9286_in0: endpoint {
+				};
+			};
+
+			port@1 {
+				reg = <1>;
+				max9286_in1: endpoint {
+				};
+			};
+
+			port@2 {
+				reg = <2>;
+				max9286_in2: endpoint {
+				};
+			};
+
+			port@3 {
+				reg = <3>;
+				max9286_in3: endpoint {
+				};
+			};
+
+			port@4 {
+				reg = <4>;
+				max9286_out0: endpoint {
+					clock-lanes = <0>;
+					data-lanes = <1 2 3 4>;
+					remote-endpoint = <&csi40_in>;
+				};
+			};
+		};
+
+		i2c-mux {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			i2c@0 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <0>;
+
+				status = "disabled";
+			};
+
+			i2c@1 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <1>;
+
+				status = "disabled";
+			};
+
+			i2c@2 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <2>;
+
+				status = "disabled";
+			};
+
+			i2c@3 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <3>;
+
+				status = "disabled";
+			};
+		};
+	};
+};
+
 &scif0 {
 	pinctrl-0 = <&scif0_pins>;
 	pinctrl-names = "default";
-- 
2.30.0

[PATCH v2 3/4] arm64: dts: renesas: eagle: Add GMSL .dtsi

[Jacopo Mondi]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Describe the FAKRA connector available on Eagle board that allows
connecting GMSL camera modules such as IMI RDACM20 and RDACM21.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi | 186 ++++++++++++++++++++
 1 file changed, 186 insertions(+)
 create mode 100644 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi

diff --git a/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
new file mode 100644
index 000000000000..ec3e7493aa71
--- /dev/null
+++ b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Device Tree Source (overlay) for the Eagle V3M GMSL connectors
+ *
+ * Copyright (C) 2017 Ideas on Board <kieran.bingham@ideasonboard.com>
+ * Copyright (C) 2021 Jacopo Mondi <jacopo+renesas@jmondi.org>
+ *
+ * This overlay allows you to define GMSL cameras connected to the FAKRA
+ * connectors on the Eagle-V3M (or compatible) board.
+ *
+ * The following cameras are currently supported:
+ *    "imi,rdacm20"
+ *    "imi,rdacm21"
+ */
+
+#include <dt-bindings/gpio/gpio.h>
+
+/*
+ * Select which cameras are in use:
+ * #define EAGLE_CAMERA0_RDACM20
+ * #define EAGLE_CAMERA0_RDACM21
+ *
+ * The two camera modules are configured with different image formats
+ * and cannot be mixed.
+ */
+#define EAGLE_CAMERA0_RDACM20
+#define EAGLE_CAMERA1_RDACM20
+#define EAGLE_CAMERA2_RDACM20
+#define EAGLE_CAMERA3_RDACM20
+
+/* Set the compatible string based on the camera model. */
+#if defined(EAGLE_CAMERA0_RDACM21) || defined(EAGLE_CAMERA1_RDACM21) || \
+    defined(EAGLE_CAMERA2_RDACM21) || defined(EAGLE_CAMERA3_RDACM21)
+#define EAGLE_CAMERA_MODEL	"imi,rdacm21"
+#define EAGLE_USE_RDACM21
+#elif defined(EAGLE_CAMERA0_RDACM20) || defined(EAGLE_CAMERA1_RDACM20) || \
+    defined(EAGLE_CAMERA2_RDACM20) || defined(EAGLE_CAMERA3_RDACM20)
+#define EAGLE_CAMERA_MODEL	"imi,rdacm20"
+#define EAGLE_USE_RDACM20
+#endif
+
+/* Define which cameras are available. */
+#if defined(EAGLE_CAMERA0_RDACM21) || defined(EAGLE_CAMERA0_RDACM20)
+#define EAGLE_USE_CAMERA_0
+#endif
+
+#if defined(EAGLE_CAMERA1_RDACM21) || defined(EAGLE_CAMERA1_RDACM20)
+#define EAGLE_USE_CAMERA_1
+#endif
+
+#if defined(EAGLE_CAMERA2_RDACM21) || defined(EAGLE_CAMERA2_RDACM20)
+#define EAGLE_USE_CAMERA_2
+#endif
+
+#if defined(EAGLE_CAMERA3_RDACM21) || defined(EAGLE_CAMERA3_RDACM20)
+#define EAGLE_USE_CAMERA_3
+#endif
+
+/* Define the endpoint links. */
+#ifdef EAGLE_USE_CAMERA_0
+&max9286_in0 {
+	remote-endpoint = <&fakra_con0>;
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_1
+&max9286_in1 {
+	remote-endpoint = <&fakra_con1>;
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_2
+&max9286_in2 {
+	remote-endpoint = <&fakra_con2>;
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_3
+&max9286_in3 {
+	remote-endpoint = <&fakra_con3>;
+};
+#endif
+
+/* Populate the GMSL i2c-mux bus with camera nodes. */
+#if defined(EAGLE_USE_RDACM21) || defined(EAGLE_USE_RDACM20)
+
+#ifdef EAGLE_USE_CAMERA_0
+&vin0 {
+	status = "okay";
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_1
+&vin1 {
+	status = "okay";
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_2
+&vin2 {
+	status = "okay";
+};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_3
+&vin3 {
+	status = "okay";
+};
+#endif
+
+&gmsl {
+
+	status = "okay";
+	maxim,reverse-channel-microvolt = <100000>;
+
+	i2c-mux {
+#ifdef EAGLE_USE_CAMERA_0
+		i2c@0 {
+			status = "okay";
+
+			camera@51 {
+				compatible = EAGLE_CAMERA_MODEL;
+				reg = <0x51>, <0x61>;
+
+				port {
+					fakra_con0: endpoint {
+						remote-endpoint = <&max9286_in0>;
+					};
+				};
+			};
+		};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_1
+		i2c@1 {
+			status = "okay";
+
+			camera@52 {
+				compatible = EAGLE_CAMERA_MODEL;
+				reg = <0x52>, <0x62>;
+
+				port {
+					fakra_con1: endpoint {
+						remote-endpoint = <&max9286_in1>;
+					};
+				};
+			};
+		};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_2
+		i2c@2 {
+			status = "okay";
+
+			camera@53 {
+				compatible = EAGLE_CAMERA_MODEL;
+				reg = <0x53>, <0x63>;
+
+				port {
+					fakra_con2: endpoint {
+						remote-endpoint = <&max9286_in2>;
+					};
+				};
+			};
+		};
+#endif
+
+#ifdef EAGLE_USE_CAMERA_3
+		i2c@3 {
+			status = "okay";
+
+			camera@54 {
+				compatible = EAGLE_CAMERA_MODEL;
+				reg = <0x54>, <0x64>;
+
+				port {
+					fakra_con3: endpoint {
+						remote-endpoint = <&max9286_in3>;
+					};
+				};
+			};
+		};
+#endif
+	};
+};
+#endif
-- 
2.30.0

[PATCH v2 4/4] arm64: dts: renesas: eagle: Include eagle-gmsl

[Jacopo Mondi]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Include the eagle-gmsl.dtsi to enable GMSL camera support on the
Eagle-V3M platform.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
index d2f63cccc238..555070aae03d 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -404,3 +404,6 @@ &scif0 {
 
 	status = "okay";
 };
+
+/* FAKRA Overlay */
+#include "eagle-gmsl.dtsi"
-- 
2.30.0

Re: [PATCH v2 1/4] dt-bindings: media: max9286: Describe gpio-hog

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Mar 15, 2021 at 05:30:25PM +0100, Jacopo Mondi wrote:
> The MAX9286 GMSL deserializer features gpio controller capabilities,
> as it provides 2 GPIO lines.
> 
> As establishing a regulator that uses one of the GPIO lines and
> enabling/disabling it at run-time in the max9286 won't work due to
> a circular dependency on the gpio-controller/regulator creation, allow
> the usage of a gpio-hog for that purpose.
> 
> The usage of the gpio-hog is required in designs where the MAX9286
> GPIO lines control the remote cameras power.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

That's really a workaround until we can find a good solution, do we have
to officially support it in the DT bindings ?

> ---
>  .../bindings/media/i2c/maxim,max9286.yaml        | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> index ee16102fdfe7..9038300e373c 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> @@ -177,6 +177,22 @@ properties:
>  
>      additionalProperties: false
>  
> +patternProperties:
> +  "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$":
> +    type: object
> +    properties:
> +      gpio-hog: true
> +      gpios: true
> +      output-low: true
> +      line-name: true
> +
> +    required:
> +      - gpio-hog
> +      - gpios
> +      - output-low
> +
> +    additionalProperties: false
> +
>  required:
>    - compatible
>    - reg

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/4] arm64: dts: renesas: eagle: Enable MAX9286

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Mar 15, 2021 at 05:30:26PM +0100, Jacopo Mondi wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Enable the MAX9286 GMSL deserializer on the Eagle-V3M board.
> 
> Connected cameras should be defined in a device-tree overlay or included
> after these definitions.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../arm64/boot/dts/renesas/r8a77970-eagle.dts | 132 ++++++++++++++++++
>  1 file changed, 132 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> index 874a7fc2730b..d2f63cccc238 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> @@ -6,6 +6,8 @@
>   * Copyright (C) 2017 Cogent Embedded, Inc.
>   */
>  
> +#include <dt-bindings/gpio/gpio.h>
> +
>  /dts-v1/;
>  #include "r8a77970.dtsi"
>  
> @@ -188,6 +190,11 @@ i2c0_pins: i2c0 {
>  		function = "i2c0";
>  	};
>  
> +	i2c3_pins: i2c3 {
> +		groups = "i2c3_a";
> +		function = "i2c3";
> +	};
> +
>  	qspi0_pins: qspi0 {
>  		groups = "qspi0_ctrl", "qspi0_data4";
>  		function = "qspi0";
> @@ -266,6 +273,131 @@ &rwdt {
>  	status = "okay";
>  };

Could we keep the nodes alphabetically sorted ?

> +&csi40 {
> +	status = "okay";
> +
> +	ports {
> +		#address-cells = <1>;
> +		#size-cells = <0>;

This is already present in r8a77970.dtsi, you can drop it.

> +
> +		port@0 {
> +			reg = <0>;

Similarly, should we add port@0 in r8a77970.dtsi, with its reg property
?

> +
> +			csi40_in: endpoint {
> +				clock-lanes = <0>;
> +				data-lanes = <1 2 3 4>;
> +				remote-endpoint = <&max9286_out0>;
> +			};
> +		};
> +	};
> +};
> +
> +&i2c3 {
> +	pinctrl-0 = <&i2c3_pins>;
> +	pinctrl-names = "default";
> +
> +	status = "okay";
> +	clock-frequency = <400000>;
> +
> +	gmsl: gmsl-deserializer@48 {
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +
> +		compatible = "maxim,max9286";
> +		reg = <0x48>;
> +
> +		/* eagle-pca9654-max9286-pwdn */
> +		enable-gpios = <&io_expander 0 GPIO_ACTIVE_HIGH>;
> +
> +		/*
> +		 * Workaround: Hog the CAMVDD line high as we can't establish a
> +		 * regulator-fixed on the gpio_chip exposed by &gmsl due to
> +		 * circular-dependency issues.
> +		 */
> +		camvdd-en-hog {
> +			gpio-hog;
> +			gpios = <0 0>;
> +			output-low;
> +			line-name = "CAMVDD_EN";
> +		};
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +				max9286_in0: endpoint {
> +				};

Shouldn't we leave out empty endpoints, and add them to the overlays
instead ? Endpoints describe links, so they shouldn't exist on their
own.

With these minor issues addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +			};
> +
> +			port@1 {
> +				reg = <1>;
> +				max9286_in1: endpoint {
> +				};
> +			};
> +
> +			port@2 {
> +				reg = <2>;
> +				max9286_in2: endpoint {
> +				};
> +			};
> +
> +			port@3 {
> +				reg = <3>;
> +				max9286_in3: endpoint {
> +				};
> +			};
> +
> +			port@4 {
> +				reg = <4>;
> +				max9286_out0: endpoint {
> +					clock-lanes = <0>;
> +					data-lanes = <1 2 3 4>;
> +					remote-endpoint = <&csi40_in>;
> +				};
> +			};
> +		};
> +
> +		i2c-mux {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			i2c@0 {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				reg = <0>;
> +
> +				status = "disabled";
> +			};
> +
> +			i2c@1 {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				reg = <1>;
> +
> +				status = "disabled";
> +			};
> +
> +			i2c@2 {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				reg = <2>;
> +
> +				status = "disabled";
> +			};
> +
> +			i2c@3 {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				reg = <3>;
> +
> +				status = "disabled";
> +			};
> +		};
> +	};
> +};
> +
>  &scif0 {
>  	pinctrl-0 = <&scif0_pins>;
>  	pinctrl-names = "default";

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/4] arm64: dts: renesas: eagle: Add GMSL .dtsi

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Mar 15, 2021 at 05:30:27PM +0100, Jacopo Mondi wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Describe the FAKRA connector available on Eagle board that allows
> connecting GMSL camera modules such as IMI RDACM20 and RDACM21.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi | 186 ++++++++++++++++++++
>  1 file changed, 186 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
> 
> diff --git a/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
> new file mode 100644
> index 000000000000..ec3e7493aa71
> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/eagle-gmsl.dtsi
> @@ -0,0 +1,186 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Device Tree Source (overlay) for the Eagle V3M GMSL connectors
> + *
> + * Copyright (C) 2017 Ideas on Board <kieran.bingham@ideasonboard.com>
> + * Copyright (C) 2021 Jacopo Mondi <jacopo+renesas@jmondi.org>
> + *
> + * This overlay allows you to define GMSL cameras connected to the FAKRA
> + * connectors on the Eagle-V3M (or compatible) board.
> + *
> + * The following cameras are currently supported:
> + *    "imi,rdacm20"
> + *    "imi,rdacm21"
> + */
> +
> +#include <dt-bindings/gpio/gpio.h>
> +
> +/*
> + * Select which cameras are in use:
> + * #define EAGLE_CAMERA0_RDACM20
> + * #define EAGLE_CAMERA0_RDACM21
> + *
> + * The two camera modules are configured with different image formats
> + * and cannot be mixed.
> + */
> +#define EAGLE_CAMERA0_RDACM20
> +#define EAGLE_CAMERA1_RDACM20
> +#define EAGLE_CAMERA2_RDACM20
> +#define EAGLE_CAMERA3_RDACM20

Shouldn't this be moved out of this file, and set in the file that
includes it, in order to make this .dtsi fully parametric ?

> +
> +/* Set the compatible string based on the camera model. */
> +#if defined(EAGLE_CAMERA0_RDACM21) || defined(EAGLE_CAMERA1_RDACM21) || \
> +    defined(EAGLE_CAMERA2_RDACM21) || defined(EAGLE_CAMERA3_RDACM21)
> +#define EAGLE_CAMERA_MODEL	"imi,rdacm21"
> +#define EAGLE_USE_RDACM21
> +#elif defined(EAGLE_CAMERA0_RDACM20) || defined(EAGLE_CAMERA1_RDACM20) || \
> +    defined(EAGLE_CAMERA2_RDACM20) || defined(EAGLE_CAMERA3_RDACM20)
> +#define EAGLE_CAMERA_MODEL	"imi,rdacm20"
> +#define EAGLE_USE_RDACM20
> +#endif

It could be nice to catch errors caused by mix-and-matching different
camera models. Or, possibly even better, we could have one #define to
select the camera model, and other #define to select the ports on which
cameras are connected. That would make the error impossible in the first
place, and would scale better when we'll add support for more cameras.

> +
> +/* Define which cameras are available. */
> +#if defined(EAGLE_CAMERA0_RDACM21) || defined(EAGLE_CAMERA0_RDACM20)
> +#define EAGLE_USE_CAMERA_0
> +#endif
> +
> +#if defined(EAGLE_CAMERA1_RDACM21) || defined(EAGLE_CAMERA1_RDACM20)
> +#define EAGLE_USE_CAMERA_1
> +#endif
> +
> +#if defined(EAGLE_CAMERA2_RDACM21) || defined(EAGLE_CAMERA2_RDACM20)
> +#define EAGLE_USE_CAMERA_2
> +#endif
> +
> +#if defined(EAGLE_CAMERA3_RDACM21) || defined(EAGLE_CAMERA3_RDACM20)
> +#define EAGLE_USE_CAMERA_3
> +#endif
> +
> +/* Define the endpoint links. */
> +#ifdef EAGLE_USE_CAMERA_0
> +&max9286_in0 {
> +	remote-endpoint = <&fakra_con0>;
> +};
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_1
> +&max9286_in1 {
> +	remote-endpoint = <&fakra_con1>;
> +};
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_2
> +&max9286_in2 {
> +	remote-endpoint = <&fakra_con2>;
> +};
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_3
> +&max9286_in3 {
> +	remote-endpoint = <&fakra_con3>;
> +};
> +#endif
> +
> +/* Populate the GMSL i2c-mux bus with camera nodes. */
> +#if defined(EAGLE_USE_RDACM21) || defined(EAGLE_USE_RDACM20)
> +
> +#ifdef EAGLE_USE_CAMERA_0
> +&vin0 {
> +	status = "okay";
> +};
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_1
> +&vin1 {
> +	status = "okay";
> +};
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_2
> +&vin2 {
> +	status = "okay";
> +};
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_3
> +&vin3 {
> +	status = "okay";
> +};
> +#endif

As routing is supposed to be dynamic (even if fully dynamic routing of
VCs and DTs may not be supported in the driver yet), shouldn't we enable
all VIN instances unconditionally ?

> +
> +&gmsl {
> +
> +	status = "okay";
> +	maxim,reverse-channel-microvolt = <100000>;
> +
> +	i2c-mux {
> +#ifdef EAGLE_USE_CAMERA_0
> +		i2c@0 {
> +			status = "okay";
> +
> +			camera@51 {
> +				compatible = EAGLE_CAMERA_MODEL;
> +				reg = <0x51>, <0x61>;
> +
> +				port {
> +					fakra_con0: endpoint {
> +						remote-endpoint = <&max9286_in0>;
> +					};
> +				};
> +			};
> +		};
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_1
> +		i2c@1 {
> +			status = "okay";
> +
> +			camera@52 {
> +				compatible = EAGLE_CAMERA_MODEL;
> +				reg = <0x52>, <0x62>;
> +
> +				port {
> +					fakra_con1: endpoint {
> +						remote-endpoint = <&max9286_in1>;
> +					};
> +				};
> +			};
> +		};
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_2
> +		i2c@2 {
> +			status = "okay";
> +
> +			camera@53 {
> +				compatible = EAGLE_CAMERA_MODEL;
> +				reg = <0x53>, <0x63>;
> +
> +				port {
> +					fakra_con2: endpoint {
> +						remote-endpoint = <&max9286_in2>;
> +					};
> +				};
> +			};
> +		};
> +#endif
> +
> +#ifdef EAGLE_USE_CAMERA_3
> +		i2c@3 {
> +			status = "okay";
> +
> +			camera@54 {
> +				compatible = EAGLE_CAMERA_MODEL;
> +				reg = <0x54>, <0x64>;
> +
> +				port {
> +					fakra_con3: endpoint {
> +						remote-endpoint = <&max9286_in3>;
> +					};
> +				};
> +			};
> +		};
> +#endif
> +	};
> +};
> +#endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/4] arm64: dts: renesas: eagle: Include eagle-gmsl

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Mar 15, 2021 at 05:30:28PM +0100, Jacopo Mondi wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Include the eagle-gmsl.dtsi to enable GMSL camera support on the
> Eagle-V3M platform.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> index d2f63cccc238..555070aae03d 100644
> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
> @@ -404,3 +404,6 @@ &scif0 {
>  
>  	status = "okay";
>  };
> +
> +/* FAKRA Overlay */
> +#include "eagle-gmsl.dtsi"

Not everybody has cameras connected to the Eagle board though.

Now that the kernel is gaining support for compiling DT overlays
(https://lore.kernel.org/lkml/cover.1615354376.git.viresh.kumar@linaro.org/),
I wonder if this is something that could be handled using overlays.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/4] dt-bindings: media: max9286: Describe gpio-hog

[Jacopo Mondi]

Hi Laurent,

On Tue, Mar 16, 2021 at 12:15:16AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Mar 15, 2021 at 05:30:25PM +0100, Jacopo Mondi wrote:
> > The MAX9286 GMSL deserializer features gpio controller capabilities,
> > as it provides 2 GPIO lines.
> >
> > As establishing a regulator that uses one of the GPIO lines and
> > enabling/disabling it at run-time in the max9286 won't work due to
> > a circular dependency on the gpio-controller/regulator creation, allow
> > the usage of a gpio-hog for that purpose.
> >
> > The usage of the gpio-hog is required in designs where the MAX9286
> > GPIO lines control the remote cameras power.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> That's really a workaround until we can find a good solution, do we have
> to officially support it in the DT bindings ?
>

That's an interesting question. The 'good' solution implies resolving
the circular dependency on the regulator/gpio-controller creation and
I feel like it might take a while to find a proper solution.

In the meantime, all designs like Eagle that control the camera power
through a MAX9286 gpio have to rely on this. I'll go with the majority
here: either we add this and upstream the gmsl .dtsi for eagle, or we
keep out-of-tree patches :/

> > ---
> >  .../bindings/media/i2c/maxim,max9286.yaml        | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > index ee16102fdfe7..9038300e373c 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > @@ -177,6 +177,22 @@ properties:
> >
> >      additionalProperties: false
> >
> > +patternProperties:
> > +  "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$":
> > +    type: object
> > +    properties:
> > +      gpio-hog: true
> > +      gpios: true
> > +      output-low: true
> > +      line-name: true
> > +
> > +    required:
> > +      - gpio-hog
> > +      - gpios
> > +      - output-low
> > +
> > +    additionalProperties: false
> > +
> >  required:
> >    - compatible
> >    - reg
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v2 1/4] dt-bindings: media: max9286: Describe gpio-hog

[Laurent Pinchart]

Hi Jacopo,

On Wed, Mar 17, 2021 at 11:14:12AM +0100, Jacopo Mondi wrote:
> On Tue, Mar 16, 2021 at 12:15:16AM +0200, Laurent Pinchart wrote:
> > On Mon, Mar 15, 2021 at 05:30:25PM +0100, Jacopo Mondi wrote:
> > > The MAX9286 GMSL deserializer features gpio controller capabilities,
> > > as it provides 2 GPIO lines.
> > >
> > > As establishing a regulator that uses one of the GPIO lines and
> > > enabling/disabling it at run-time in the max9286 won't work due to
> > > a circular dependency on the gpio-controller/regulator creation, allow
> > > the usage of a gpio-hog for that purpose.
> > >
> > > The usage of the gpio-hog is required in designs where the MAX9286
> > > GPIO lines control the remote cameras power.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >
> > That's really a workaround until we can find a good solution, do we have
> > to officially support it in the DT bindings ?
> >
> 
> That's an interesting question. The 'good' solution implies resolving
> the circular dependency on the regulator/gpio-controller creation and
> I feel like it might take a while to find a proper solution.

How about not using a regulator in that case ? If a MAX9286 GPIO is used
to controlled the camera power, we could describe that specifically in
DT, and handle everything within the MAX9286 driver in that case. It
could be considered as a bit of a hack, but it would be very localized,
and I think that trying to fix it in a way that would involve the
regulator framework would actually be worse as the complexity would be
bigger, for little gain.

> In the meantime, all designs like Eagle that control the camera power
> through a MAX9286 gpio have to rely on this. I'll go with the majority
> here: either we add this and upstream the gmsl .dtsi for eagle, or we
> keep out-of-tree patches :/

I don't mind having it upstream in the driver as a workaround, but I'd
like to keep it out of the DT bindings as it shouldn't be considered as
an officially supported option. Would that work for you ?

> > > ---
> > >  .../bindings/media/i2c/maxim,max9286.yaml        | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > index ee16102fdfe7..9038300e373c 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > @@ -177,6 +177,22 @@ properties:
> > >
> > >      additionalProperties: false
> > >
> > > +patternProperties:
> > > +  "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$":
> > > +    type: object
> > > +    properties:
> > > +      gpio-hog: true
> > > +      gpios: true
> > > +      output-low: true
> > > +      line-name: true
> > > +
> > > +    required:
> > > +      - gpio-hog
> > > +      - gpios
> > > +      - output-low
> > > +
> > > +    additionalProperties: false
> > > +
> > >  required:
> > >    - compatible
> > >    - reg

-- 
Regards,

Laurent Pinchart