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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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