Re: [PATCH] arm64: dts: sun50i-a64-pinephone: Add front/back cameras

From: Andrey Skvortsov
Date: Wed Jan 24 2024 - 07:26:09 EST

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@xxxxxx>
> >
> > 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@xxxxxx>
> > Signed-off-by: Andrey Skvortsov <andrej.skvortzov@xxxxxxxxx>
> > ---
> > .../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