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 = <®_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 = <®_dldo3>; > > + dvdd-supply = <®_aldo1>; > > + iovdd-supply = <®_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 = <®_dldo3>; > > + DOVDD-supply = <®_aldo1>; /* shared with AFVCC */ > > + DVDD-supply = <®_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
