Received-SPF: pass (google.com: domain of linux-kernel+bounces-36973-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 24 Jan 2024 15:25:53 +0300
From: Andrey Skvortsov <andrej.skvortzov@gmail.com>
To: =?utf-8?Q?Ond=C5=99ej?= Jirman <megi@xff.cz>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>, Chen-Yu Tsai <wens@csie.org>,
	Jernej Skrabec <jernej.skrabec@gmail.com>,
	Samuel Holland <samuel@sholland.org>, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev,
	linux-kernel@vger.kernel.org,
	Alain Volmat <alain.volmat@foss.st.com>,
	Pavel Machek <pavel@ucw.cz>,
	Arnaud Ferraris <arnaud.ferraris@collabora.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>
Subject: Re: [PATCH] arm64: dts: sun50i-a64-pinephone: Add front/back cameras
Message-ID: <ZbEB0TCTkzIYnLZD@skv.local>
Mail-Followup-To: Andrey Skvortsov <andrej.skvortzov@gmail.com>,
	=?utf-8?Q?Ond=C5=99ej?= Jirman <megi@xff.cz>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>, Chen-Yu Tsai <wens@csie.org>,
	Jernej Skrabec <jernej.skrabec@gmail.com>,
	Samuel Holland <samuel@sholland.org>, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev,
	linux-kernel@vger.kernel.org,
	Alain Volmat <alain.volmat@foss.st.com>,
	Pavel Machek <pavel@ucw.cz>,
	Arnaud Ferraris <arnaud.ferraris@collabora.com>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>
References: <20240123214729.2852346-1-andrej.skvortzov@gmail.com>
 <rkx4iuuudkk7iaaudmr3ccwpatps7kz2qyjalcfzp4ongmbaby@rcosajd6c746>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <rkx4iuuudkk7iaaudmr3ccwpatps7kz2qyjalcfzp4ongmbaby@rcosajd6c746>

Hi Ondrey,

On 24-01-23 23:01, Ondřej Jirman wrote:
> Hi Andrey,
> 
> On Wed, Jan 24, 2024 at 12:47:29AM +0300, Andrey Skvortsov wrote:
> > From: Ondřej Jirman <megi@xff.cz>
> > 
> > Pinephone has OV5640 back camera and GC2145 front camera. Add support
> > for both.
> 
> The upstream driver doesn't support multiple endpoints per port. See:
> 
> https://elixir.bootlin.com/linux/v6.8-rc1/source/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml#L43
> 
> Only one endpoint is allowed/supported. Looking throught LKML, I don't
> see the support for multiple parallel interface endpoints being added
> recently...
>
> So this patch will not work, and will cause DTS validation errors.

Thank you! I've not run dtb validation before submission, sorry for that.
I've ran 'make dtbs_check' and have several question now.

1. I don't see any complaints about multiple endpoints definitions.

IMHO, it looks okay from binding point of view according to that
video-interfaces.yaml [1].

I know that currently multiple endpoint implementation for parallel
interface is missing in sun6i_csi. Current out-of-tree implementation
doesn't require any change in bindings (Hopefully it will be upstream one
day) Or do you mean this change has to be submitted upstream only once
sun6i_csi gets fixed?


2. dtbs_check complaints about missing link-frequencies for recently
submitted gc2145. [2]
```
front-camera@3c: port:endpoint: 'link-frequencies' is a required property
```

I've looked at other drivers and link-frequencies are used only
mostly for CSI-2 endpoints. Should it be required for CPI/DVP ?

Not many mainline drivers support CSI-2 and DVP: ov5640, s5k5baf,
mt9mt114. Only mt9mt114 uses link-frequencies property for DVP and it
should match PCLK (double pixelrate). [3]

Should I define link-frequencies for DVP as a double pixelrate here as
well and handle that in the driver?

Currently gc2145 doesn't support DVP, but I have basic working support
for DVP for the upstreamed driver for a long time and waited for it to be merged
into mainline. I'd like to submit it for review. Until now I thought,
that submitted gc2145 bindings will be the same for DVP and CSI-2
support and therefore submitted this change.

Are they? Or should I introduce bus-type and conditionally
handle requirements in yaml if link-frequencies can be ignored for DVP?

Something link this
```
        properties:
          link-frequencies: true

          bus-type:
            enum:
              - 4 # CSI-2 D-PHY
              - 5 # Parallel

        required:
          - bus-type

        allOf:
          - if:
              properties:
                bus-type:
                  const: 4
            then:
              required:
                - link-frequencies
```

Should I better submit DVP support to the gc2145 driver first and only then
submit this change? 

> 
> Kind regards,
> 	o.
> 
> > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> > Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> > ---
> >  .../dts/allwinner/sun50i-a64-pinephone.dtsi   | 91 +++++++++++++++++++
> >  1 file changed, 91 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> > index 87847116ab6d..4104a136ff75 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> > @@ -36,6 +36,15 @@ chosen {
> >  		stdout-path = "serial0:115200n8";
> >  	};
> >  
> > +	i2c_csi: i2c-csi {
> > +		compatible = "i2c-gpio";
> > +		sda-gpios = <&pio 4 13 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; /* PE13 */
> > +		scl-gpios = <&pio 4 12 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; /* PE12 */
> > +		i2c-gpio,delay-us = <3>;
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +	};
> > +
> >  	leds {
> >  		compatible = "gpio-leds";
> >  
> > @@ -124,6 +133,36 @@ &cpu3 {
> >  	cpu-supply = <&reg_dcdc2>;
> >  };
> >  
> > +&csi {
> > +	pinctrl-0 = <&csi_pins>, <&csi_mclk_pin>;
> > +	status = "okay";
> > +
> > +	port {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		csi_ov5640_ep: endpoint@0 {
> > +			reg = <0>;
> > +			remote-endpoint = <&ov5640_ep>;
> > +			bus-width = <8>;
> > +			hsync-active = <1>; /* Active high */
> > +			vsync-active = <0>; /* Active low */
> > +			data-active = <1>;  /* Active high */
> > +			pclk-sample = <1>;  /* Rising */
> > +		};
> > +
> > +		csi_gc2145_ep: endpoint@1 {
> > +			reg = <1>;
> > +			remote-endpoint = <&gc2145_ep>;
> > +			bus-width = <8>;
> > +			hsync-active = <1>;
> > +			vsync-active = <1>;
> > +			data-active = <1>;
> > +			pclk-sample = <1>;
> > +		};
> > +	};
> > +};
> > +
> >  &dai {
> >  	status = "okay";
> >  };
> > @@ -158,6 +197,58 @@ &ehci1 {
> >  	status = "okay";
> >  };
> >  
> > +&i2c_csi {
> > +	gc2145: front-camera@3c {
> > +		compatible = "galaxycore,gc2145";
> > +		reg = <0x3c>;
> > +		clocks = <&ccu CLK_CSI_MCLK>;
> > +		clock-names = "xclk";

That should be removed to fix
```
front-camera@3c: 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+'
```
when running 'make dtbs_check'

> > +		avdd-supply = <&reg_dldo3>;
> > +		dvdd-supply = <&reg_aldo1>;
> > +		iovdd-supply = <&reg_eldo3>;
> > +		reset-gpios = <&pio 4 16 (GPIO_ACTIVE_LOW | GPIO_OPEN_DRAIN)>; /* PE16 */
> > +		powerdown-gpios = <&pio 4 17 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; /* PE17 */
> > +		rotation = <270>;
> > +		orientation = <0>;
> > +
> > +		port {
> > +			gc2145_ep: endpoint {
> > +				remote-endpoint = <&csi_gc2145_ep>;
> > +				bus-width = <8>;
> > +				hsync-active = <1>;
> > +				vsync-active = <1>;
> > +				data-active = <1>;
> > +				pclk-sample = <1>;
> > +			};
> > +		};
> > +	};
> > +
> > +	ov5640: rear-camera@4c {
> > +		compatible = "ovti,ov5640";
> > +		reg = <0x4c>;
> > +		clocks = <&ccu CLK_CSI_MCLK>;
> > +		clock-names = "xclk";
> > +		AVDD-supply = <&reg_dldo3>;
> > +		DOVDD-supply = <&reg_aldo1>; /* shared with AFVCC */
> > +		DVDD-supply = <&reg_eldo3>;
> > +		reset-gpios = <&pio 3 3 (GPIO_ACTIVE_LOW | GPIO_OPEN_DRAIN)>; /* PD3 */
> > +		powerdown-gpios = <&pio 2 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; /* PC0 */
> > +		rotation = <90>;
> > +		orientation = <1>;
> > +
> > +		port {
> > +			ov5640_ep: endpoint {
> > +				remote-endpoint = <&csi_ov5640_ep>;
> > +				bus-width = <8>;
> > +				hsync-active = <1>; /* Active high */
> > +				vsync-active = <0>; /* Active low */
> > +				data-active = <1>;  /* Active high */
> > +				pclk-sample = <1>;  /* Rising */
> > +			};
> > +		};
> > +	};
> > +};
> > +
> >  &i2c0 {
> >  	status = "okay";
> >  
> > -- 
> > 2.43.0
> > 


1. https://elixir.bootlin.com/linux/v6.8-rc1/source/Documentation/devicetree/bindings/media/video-interfaces.yaml#L41
2. https://elixir.bootlin.com/linux/v6.8-rc1/source/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml#L65
3. https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/media/i2c/mt9m114.c#L2256

-- 
Best regards,
Andrey Skvortsov