Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema
@ 2020-07-17 13:28 Jacopo Mondi
  2020-07-17 13:28 ` [PATCH 01/13] " Jacopo Mondi
                   ` (13 more replies)
  0 siblings, 14 replies; 46+ messages in thread
From: Jacopo Mondi @ 2020-07-17 13:28 UTC (permalink / raw)
  To: robh+dt, devicetree
  Cc: Jacopo Mondi, slongerbeam, linux-media, laurent.pinchart,
	linux-renesas-soc

Hello,
   this series has been broken out from
" [PATCH 0/8] dt-bindings: media: i2c: Convert to json-schema"
which I sent out a few days ago.

It was a single patch, now it's 13. Not sure it's a good sign :)

Anyway, in the version sent as part of the mentioned series I just converted
the existing bindings to the json-schema format, without putting to much
attention to what was there.

As suggested by Laurent I moved to specify properties as dt-schema, to
allow their validation, and that sprinkled to a lot of fun.

For each patch in the series, the DTS and bindings maintainers are CC-ed.
The whole series has been sent to a limited audience only.

The series starts with a plain conversion of the existing bindings to
dt-schema. It then fixes a potential issue on a dts user of the sensor,
and then move to remove or demote endpoint properties that were marked as
required in the original text bindings. This of course causes a log of churn
in the existing DTS which use the sensor, and opens some questions there from
which I expect the individual maintainers to provide directions on.

Finally, the small patch for the driver to initialize DVP default signal
values to what the sensor manual and the bindings report.

The series ends with the update of the MAINTAINERS file with the new
.yaml binding file and the addition of myself as co-maintainer for the driver.
Steve if you intended to remove yourself please send a patch, I didn't dare to
do it myself :)

A final note for Rob and his bot: some patches breaks validation of existing
dts, which are then fixed by following patches. Hope this is ok, but it I
have to keep validation of all dts successful, patches would likely be huge
and hinder changes which I preferred to make explicitly in single patches.

Jacopo Mondi (13):
  dt-bindings: media: ov5640: Convert to json-schema
  arm64: dts: qcom: apq8016-sbc: Fix CSI-2 lanes routing
  dt-bindings: media: ov5640: Do not limit rotation
  dt-bindings: media: ti,cal: Remove sensor from example
  dt-bindings: media: ov5640: Make bus-type mandatory
  dt-bindings: media: ov5640: Require ep properties
  dt-bindings: media: ov5640: Remove clock-lanes
  dt-bindings: media: ov5640: Remove data-shift
  dt-bindings: media: ov5640: Add default for synch signals
  dt-bindings: media: ov5640: Demote DVP required properties
  media: i2c: ov5640: Initialize DVP polarities as default
  MAINTAINERS: ov5640: Add bindings file
  dt-bindings: media: ov5640: Add myself as co-maintainer

 .../devicetree/bindings/media/i2c/ov5640.txt  |  92 --------
 .../devicetree/bindings/media/i2c/ov5640.yaml | 208 ++++++++++++++++++
 .../devicetree/bindings/media/ti,cal.yaml     |  22 --
 MAINTAINERS                                   |   2 +
 arch/arm/boot/dts/dra72-evm-common.dtsi       |   2 +-
 arch/arm/boot/dts/dra76-evm.dts               |   2 +-
 arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi |   2 +-
 arch/arm/boot/dts/imx6qdl-icore.dtsi          |   2 +-
 arch/arm/boot/dts/imx6qdl-sabrelite.dtsi      |   2 +-
 arch/arm/boot/dts/imx6qdl-sabresd.dtsi        |   2 +-
 arch/arm/boot/dts/stm32mp157c-ev1.dts         |   2 +-
 .../allwinner/sun50i-a64-amarula-relic.dts    |   1 +
 .../boot/dts/allwinner/sun50i-a64-pinetab.dts |   1 +
 arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi     |   8 +-
 .../arm64/boot/dts/ti/k3-am654-base-board.dts |   2 +-
 drivers/media/i2c/ov5640.c                    |  16 +-
 16 files changed, 232 insertions(+), 134 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5640.txt
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5640.yaml

--
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH 01/13] dt-bindings: media: ov5640: Convert to json-schema
  2020-07-17 13:28 [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema Jacopo Mondi
@ 2020-07-17 13:28 ` Jacopo Mondi
  2020-07-17 19:28   ` Laurent Pinchart
  2020-07-20 16:32   ` Rob Herring
  2020-07-17 13:28 ` [PATCH 02/13] arm64: dts: qcom: apq8016-sbc: Fix CSI-2 lanes routing Jacopo Mondi
                   ` (12 subsequent siblings)
  13 siblings, 2 replies; 46+ messages in thread
From: Jacopo Mondi @ 2020-07-17 13:28 UTC (permalink / raw)
  To: robh+dt, devicetree
  Cc: Jacopo Mondi, slongerbeam, linux-media, laurent.pinchart,
	linux-renesas-soc

Convert the ov5640 bindings document to json-schema.

This commit ports the existing bindings, clean up patches
will follow.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../devicetree/bindings/media/i2c/ov5640.txt  |  92 ---------
 .../devicetree/bindings/media/i2c/ov5640.yaml | 181 ++++++++++++++++++
 2 files changed, 181 insertions(+), 92 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5640.txt
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5640.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
deleted file mode 100644
index c97c2f2da12d..000000000000
--- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt
+++ /dev/null
@@ -1,92 +0,0 @@
-* Omnivision OV5640 MIPI CSI-2 / parallel sensor
-
-Required Properties:
-- compatible: should be "ovti,ov5640"
-- clocks: reference to the xclk input clock.
-- clock-names: should be "xclk".
-- DOVDD-supply: Digital I/O voltage supply, 1.8 volts
-- AVDD-supply: Analog voltage supply, 2.8 volts
-- DVDD-supply: Digital core voltage supply, 1.5 volts
-
-Optional Properties:
-- reset-gpios: reference to the GPIO connected to the reset pin, if any.
-	       This is an active low signal to the OV5640.
-- powerdown-gpios: reference to the GPIO connected to the powerdown pin,
-		   if any. This is an active high signal to the OV5640.
-- rotation: as defined in
-	    Documentation/devicetree/bindings/media/video-interfaces.txt,
-	    valid values are 0 (sensor mounted upright) and 180 (sensor
-	    mounted upside down).
-
-The device node must contain one 'port' child node for its digital output
-video port, in accordance with the video interface bindings defined in
-Documentation/devicetree/bindings/media/video-interfaces.txt.
-
-OV5640 can be connected to a MIPI CSI-2 bus or a parallel bus endpoint.
-
-Endpoint node required properties for CSI-2 connection are:
-- remote-endpoint: a phandle to the bus receiver's endpoint node.
-- clock-lanes: should be set to <0> (clock lane on hardware lane 0)
-- data-lanes: should be set to <1> or <1 2> (one or two CSI-2 lanes supported)
-
-Endpoint node required properties for parallel connection are:
-- remote-endpoint: a phandle to the bus receiver's endpoint node.
-- bus-width: shall be set to <8> for 8 bits parallel bus
-	     or <10> for 10 bits parallel bus
-- data-shift: shall be set to <2> for 8 bits parallel bus
-	      (lines 9:2 are used) or <0> for 10 bits parallel bus
-- hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH respectively.
-- vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH respectively.
-- pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock
-	       signal.
-
-Examples:
-
-&i2c1 {
-	ov5640: camera@3c {
-		compatible = "ovti,ov5640";
-		pinctrl-names = "default";
-		pinctrl-0 = <&pinctrl_ov5640>;
-		reg = <0x3c>;
-		clocks = <&clks IMX6QDL_CLK_CKO>;
-		clock-names = "xclk";
-		DOVDD-supply = <&vgen4_reg>; /* 1.8v */
-		AVDD-supply = <&vgen3_reg>;  /* 2.8v */
-		DVDD-supply = <&vgen2_reg>;  /* 1.5v */
-		powerdown-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
-		reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
-		rotation = <180>;
-
-		port {
-			/* MIPI CSI-2 bus endpoint */
-			ov5640_to_mipi_csi2: endpoint {
-				remote-endpoint = <&mipi_csi2_from_ov5640>;
-				clock-lanes = <0>;
-				data-lanes = <1 2>;
-			};
-		};
-	};
-};
-
-&i2c1 {
-	ov5640: camera@3c {
-		compatible = "ovti,ov5640";
-		pinctrl-names = "default";
-		pinctrl-0 = <&pinctrl_ov5640>;
-		reg = <0x3c>;
-		clocks = <&clk_ext_camera>;
-		clock-names = "xclk";
-
-		port {
-			/* Parallel bus endpoint */
-			ov5640_to_parallel: endpoint {
-				remote-endpoint = <&parallel_from_ov5640>;
-				bus-width = <8>;
-				data-shift = <2>; /* lines 9:2 are used */
-				hsync-active = <0>;
-				vsync-active = <0>;
-				pclk-sample = <1>;
-			};
-		};
-	};
-};
diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
new file mode 100644
index 000000000000..ceeacc91c801
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
@@ -0,0 +1,181 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ov5640.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Omnivision OV5640 MIPI CSI-2 / parallel sensor
+
+maintainers:
+  - Steve Longerbeam <slongerbeam@gmail.com>
+
+description: -|
+  The OV5640 is a 5 megapixels image sensor capable of producing images in RGB,
+  RAW, YUV and compressed formats. It features a MIPI CSI-2 and a parallel data
+  interface and an I2C-compatible (CCI) control interface.
+
+properties:
+  compatible:
+    const: ovti,ov5640
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: xclk
+
+  DOVDD-supply:
+    description: Digital I/O voltage supply, 1.8 volts.
+    maxItems: 1
+
+  AVDD-supply:
+    description: Analog voltage supply, 2.8 volts.
+    maxItems: 1
+
+  DVDD-supply:
+    description: Digital core voltage supply.
+    maxItems: 1
+
+  reset-gpios:
+    description: |
+      Reference to the GPIO connected to the reset pin, if any. This is an
+      active low signal to the OV5640.
+    maxItems: 1
+
+  powerdown-gpios:
+    description: |
+      Reference tot he GPIO connected to the powerdown pin, if any. This is an
+      active high signal to the OV5640.
+    maxItems: 1
+
+  rotation:
+    description: |
+      As defined in Documentation/devicetree/bindings/media/video-interfaces.txt
+    enum: [0, 180]
+
+  port:
+    type: object
+    description: |
+      The device node must contain one 'port' child node for its digital output
+      video port, in accordance with the video interface bindings defined in
+      Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+      OV5640 can be connected to a MIPI CSI-2 bus or a parallel bus endpoint.
+
+    properties:
+      endpoint:
+        type: object
+        properties:
+          remote-endpoint:
+            description: A phandle to the bus receiver's endpoint node.
+
+          clock-lanes:
+            const: 0
+
+          data-lanes:
+            description: |
+              Should be set to <1> or <1 2> (one or two CSI-2 lanes supported).
+            oneOf:
+              - items:
+                - const: 1
+              - items:
+                - const: 1
+                - const: 2
+
+          bus-width:
+            description: |
+              Shall be set to <8> for 8 bits parallel bus or <10> for 10 bits
+              parallel bus.
+            enum: [8, 10]
+
+          data-shift:
+            description: |
+              Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or
+              <0> for 10 bits parallel bus.
+            enum: [0, 2]
+
+          hsync-active:
+            enum: [0, 1]
+
+          vsync-active:
+            enum: [0, 1]
+
+          pclk-sample:
+            enum: [0, 1]
+
+        required:
+          - remote-endpoint
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - DOVDD-supply
+  - AVDD-supply
+  - DVDD-supply
+  - port
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/clock/imx6qdl-clock.h>
+
+    i2c0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        camera@3c {
+            compatible = "ovti,ov5640";
+            reg = <0x3c>;
+            clocks = <&clks IMX6QDL_CLK_CKO>;
+            clock-names = "xclk";
+            DOVDD-supply = <&vgen4_reg>; /* 1.8v */
+            AVDD-supply = <&vgen3_reg>;  /* 2.8v */
+            DVDD-supply = <&vgen2_reg>;  /* 1.5v */
+            powerdown-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
+            reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
+            rotation = <180>;
+
+            port {
+                ov5640_to_mipi_csi2: endpoint {
+                    remote-endpoint = <&mipi_csi2_from_ov5640>;
+                    clock-lanes = <0>;
+                    data-lanes = <1 2>;
+                };
+            };
+        };
+    };
+
+    i2c1 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        camera@3c {
+            compatible = "ovti,ov5640";
+            reg = <0x3c>;
+            clocks = <&clks IMX6QDL_CLK_CKO>;
+            clock-names = "xclk";
+            DOVDD-supply = <&vgen4_reg>; /* 1.8v */
+            AVDD-supply = <&vgen3_reg>;  /* 2.8v */
+            DVDD-supply = <&vgen2_reg>;  /* 1.5v */
+            powerdown-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
+            reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
+            rotation = <180>;
+
+            port {
+                ov5640_to_parallel: endpoint {
+                    remote-endpoint = <&parallel_from_ov5640>;
+                    bus-width = <10>;
+                };
+            };
+        };
+    };
+
+...
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH 02/13] arm64: dts: qcom: apq8016-sbc: Fix CSI-2 lanes routing
  2020-07-17 13:28 [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema Jacopo Mondi
  2020-07-17 13:28 ` [PATCH 01/13] " Jacopo Mondi
@ 2020-07-17 13:28 ` Jacopo Mondi
  2020-07-17 19:35   ` Laurent Pinchart
  2020-07-17 13:28 ` [PATCH 03/13] dt-bindings: media: ov5640: Do not limit rotation Jacopo Mondi
                   ` (11 subsequent siblings)
  13 siblings, 1 reply; 46+ messages in thread
From: Jacopo Mondi @ 2020-07-17 13:28 UTC (permalink / raw)
  To: robh+dt, devicetree
  Cc: Jacopo Mondi, slongerbeam, linux-media, laurent.pinchart,
	linux-renesas-soc, Andy Gross, Bjorn Andersson, Loic Poulain

The ov5640 sensor does not support lanes reconfiguration according
to version of the datasheet I have (version 2.03) and the driver
does not parse the properties to try to reconfigure them.

Fix the properties values in the camera and cci node.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
Loic, I see you added the camera nodes in
39e0ce6cd1bf ("arm64: dts: qcom: apq8016-sbc: Add CCI/Sensor nodes")

Do you have any idea how lanes could be swapped if, from my understanding,
nor the sensor nor the driver supports that ?

Thanks
  j
---
 arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
index 8a4b790aa7ff..fe6613676e45 100644
--- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
@@ -591,8 +591,8 @@ ports {
 		port@0 {
 			reg = <0>;
 			csiphy0_ep: endpoint {
-				clock-lanes = <1>;
-				data-lanes = <0 2>;
+				clock-lanes = <0>;
+				data-lanes = <1 2>;
 				remote-endpoint = <&ov5640_ep>;
 				status = "okay";
 			};
@@ -627,8 +627,8 @@ camera_rear@3b {

 		port {
 			ov5640_ep: endpoint {
-				clock-lanes = <1>;
-				data-lanes = <0 2>;
+				clock-lanes = <0>;
+				data-lanes = <1 2>;
 				remote-endpoint = <&csiphy0_ep>;
 			};
 		};
--
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH 03/13] dt-bindings: media: ov5640: Do not limit rotation
  2020-07-17 13:28 [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema Jacopo Mondi
  2020-07-17 13:28 ` [PATCH 01/13] " Jacopo Mondi
  2020-07-17 13:28 ` [PATCH 02/13] arm64: dts: qcom: apq8016-sbc: Fix CSI-2 lanes routing Jacopo Mondi
@ 2020-07-17 13:28 ` Jacopo Mondi
  2020-07-17 19:38   ` Laurent Pinchart
  2020-07-17 13:28 ` [PATCH 04/13] dt-bindings: media: ti,cal: Remove sensor from example Jacopo Mondi
                   ` (10 subsequent siblings)
  13 siblings, 1 reply; 46+ messages in thread
From: Jacopo Mondi @ 2020-07-17 13:28 UTC (permalink / raw)
  To: robh+dt, devicetree
  Cc: Jacopo Mondi, slongerbeam, linux-media, laurent.pinchart,
	linux-renesas-soc

The 'rotation' property should not be limited to only support 0 and
180 degrees, as that limitation comes from the driver implementation
and not from any device specific constraint.

Remove the enumeration of supported values from 'rotation'.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
index ceeacc91c801..503f8b78615c 100644
--- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
@@ -55,7 +55,7 @@ properties:
   rotation:
     description: |
       As defined in Documentation/devicetree/bindings/media/video-interfaces.txt
-    enum: [0, 180]
+    maxItems: 1
 
   port:
     type: object
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH 04/13] dt-bindings: media: ti,cal: Remove sensor from example
  2020-07-17 13:28 [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema Jacopo Mondi
                   ` (2 preceding siblings ...)
  2020-07-17 13:28 ` [PATCH 03/13] dt-bindings: media: ov5640: Do not limit rotation Jacopo Mondi
@ 2020-07-17 13:28 ` Jacopo Mondi
  2020-07-17 19:39   ` Laurent Pinchart
  2020-07-23 20:57   ` Rob Herring
  2020-07-17 13:28 ` [PATCH 05/13] dt-bindings: media: ov5640: Make bus-type mandatory Jacopo Mondi
                   ` (9 subsequent siblings)
  13 siblings, 2 replies; 46+ messages in thread
From: Jacopo Mondi @ 2020-07-17 13:28 UTC (permalink / raw)
  To: robh+dt, devicetree
  Cc: Jacopo Mondi, slongerbeam, linux-media, laurent.pinchart,
	linux-renesas-soc, bparrot

The newly introduced dt-schema bindings for the ov5640 image
sensor triggered a warning in the ti,cal.yaml bindings file, which
has a camera sensor node marked compatible with the sensor.

As in the example is enough to report a node for the device that
the bindings is about, and maintaining the sensor node up-to-date
is a non-necessary burden, remove the image sensor node from the
ti,cal bindings file.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../devicetree/bindings/media/ti,cal.yaml     | 22 -------------------
 1 file changed, 22 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/ti,cal.yaml b/Documentation/devicetree/bindings/media/ti,cal.yaml
index 5e066629287d..fe5c74dd246e 100644
--- a/Documentation/devicetree/bindings/media/ti,cal.yaml
+++ b/Documentation/devicetree/bindings/media/ti,cal.yaml
@@ -177,26 +177,4 @@ examples:
         };
     };

-    i2c {
-        clock-frequency = <400000>;
-        #address-cells = <1>;
-        #size-cells = <0>;
-
-        camera-sensor@3c {
-               compatible = "ovti,ov5640";
-               reg = <0x3c>;
-
-               clocks = <&clk_ov5640_fixed>;
-               clock-names = "xclk";
-
-               port {
-                    csi2_cam0: endpoint {
-                            remote-endpoint = <&csi2_phy0>;
-                            clock-lanes = <0>;
-                            data-lanes = <1 2>;
-                    };
-               };
-        };
-    };
-
 ...
--
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH 05/13] dt-bindings: media: ov5640: Make bus-type mandatory
  2020-07-17 13:28 [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema Jacopo Mondi
                   ` (3 preceding siblings ...)
  2020-07-17 13:28 ` [PATCH 04/13] dt-bindings: media: ti,cal: Remove sensor from example Jacopo Mondi
@ 2020-07-17 13:28 ` Jacopo Mondi
  2020-07-17 15:48   ` Fabio Estevam
                     ` (2 more replies)
  2020-07-17 13:28 ` [PATCH 06/13] dt-bindings: media: ov5640: Require ep properties Jacopo Mondi
                   ` (8 subsequent siblings)
  13 siblings, 3 replies; 46+ messages in thread
From: Jacopo Mondi @ 2020-07-17 13:28 UTC (permalink / raw)
  To: robh+dt, devicetree
  Cc: Jacopo Mondi, slongerbeam, linux-media, laurent.pinchart,
	linux-renesas-soc, Benoît Cousson, Tony Lindgren,
	linux-omap, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	Fabio Estevam, NXP Linux Team, Maxime Coquelin, Alexandre Torgue,
	moderated list:ARM/STM32 ARCHITECTURE, Maxime Ripard,
	Chen-Yu Tsai, Andy Gross, Bjorn Andersson, Tero Kristo,
	Nishanth Menon

The ov5640 driver supports both a parallel data interface and a
CSI-2 serial data interface.

Depending on which interface is in use, the required endpoint
properties are different. In order to be able to validate if a
device node is compliant with the dt-schema bindings, start by
making the bus-type a mandatory property, and add it to all the
existing users of ov5640 in mainline DTS.

On top of this, endpoint properties validation will be implemented,
conditionally to the reported bus type.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 Documentation/devicetree/bindings/media/i2c/ov5640.yaml    | 6 ++++++
 arch/arm/boot/dts/dra72-evm-common.dtsi                    | 1 +
 arch/arm/boot/dts/dra76-evm.dts                            | 1 +
 arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi              | 1 +
 arch/arm/boot/dts/imx6qdl-icore.dtsi                       | 1 +
 arch/arm/boot/dts/imx6qdl-sabrelite.dtsi                   | 1 +
 arch/arm/boot/dts/imx6qdl-sabresd.dtsi                     | 1 +
 arch/arm/boot/dts/stm32mp157c-ev1.dts                      | 1 +
 arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts | 1 +
 arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts       | 1 +
 arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi                  | 1 +
 arch/arm64/boot/dts/ti/k3-am654-base-board.dts             | 1 +
 12 files changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
index 503f8b78615c..16e6c2dc629a 100644
--- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
@@ -73,6 +73,9 @@ properties:
           remote-endpoint:
             description: A phandle to the bus receiver's endpoint node.

+          bus-type:
+            enum: [4, 5]
+
           clock-lanes:
             const: 0

@@ -109,6 +112,7 @@ properties:

         required:
           - remote-endpoint
+          - bus-type

     additionalProperties: false

@@ -146,6 +150,7 @@ examples:
             port {
                 ov5640_to_mipi_csi2: endpoint {
                     remote-endpoint = <&mipi_csi2_from_ov5640>;
+                    bus-type = <4>;
                     clock-lanes = <0>;
                     data-lanes = <1 2>;
                 };
@@ -172,6 +177,7 @@ examples:
             port {
                 ov5640_to_parallel: endpoint {
                     remote-endpoint = <&parallel_from_ov5640>;
+                    bus-type = <5>;
                     bus-width = <10>;
                 };
             };
diff --git a/arch/arm/boot/dts/dra72-evm-common.dtsi b/arch/arm/boot/dts/dra72-evm-common.dtsi
index c84b63bf0fc8..aba7eb55525d 100644
--- a/arch/arm/boot/dts/dra72-evm-common.dtsi
+++ b/arch/arm/boot/dts/dra72-evm-common.dtsi
@@ -287,6 +287,7 @@ ov5640@3c {
 		port {
 			csi2_cam0: endpoint {
 				remote-endpoint = <&csi2_phy0>;
+				bus-type = <4>;
 				clock-lanes = <0>;
 				data-lanes = <1 2>;
 			};
diff --git a/arch/arm/boot/dts/dra76-evm.dts b/arch/arm/boot/dts/dra76-evm.dts
index 820a0ece20d4..ddf16e65ec7f 100644
--- a/arch/arm/boot/dts/dra76-evm.dts
+++ b/arch/arm/boot/dts/dra76-evm.dts
@@ -420,6 +420,7 @@ ov5640@3c {
 		port {
 			csi2_cam0: endpoint {
 				remote-endpoint = <&csi2_phy0>;
+				bus-type = <4>;
 				clock-lanes = <0>;
 				data-lanes = <1 2>;
 			};
diff --git a/arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi b/arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi
index 9e027b9a5f91..dd5765778e87 100644
--- a/arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi
+++ b/arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi
@@ -274,6 +274,7 @@ ov5640: camera@10 {
 		port {
 			ov5640_to_mipi_csi2: endpoint {
 				remote-endpoint = <&mipi_csi2_in>;
+				bus-type = <4>;
 				clock-lanes = <0>;
 				data-lanes = <1 2>;
 			};
diff --git a/arch/arm/boot/dts/imx6qdl-icore.dtsi b/arch/arm/boot/dts/imx6qdl-icore.dtsi
index 756f3a9f1b4f..2b1f275b8cc4 100644
--- a/arch/arm/boot/dts/imx6qdl-icore.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-icore.dtsi
@@ -213,6 +213,7 @@ ov5640: camera@3c {
 		port {
 			ov5640_to_mipi_csi2: endpoint {
 				remote-endpoint = <&mipi_csi2_in>;
+				bus-type = <4>;
 				clock-lanes = <0>;
 				data-lanes = <1 2>;
 			};
diff --git a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
index 8468216dae9b..5752abb48e68 100644
--- a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
@@ -382,6 +382,7 @@ ov5640: camera@40 {
 		port {
 			ov5640_to_mipi_csi2: endpoint {
 				remote-endpoint = <&mipi_csi2_in>;
+				bus-type = <4>;
 				clock-lanes = <0>;
 				data-lanes = <1 2>;
 			};
diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
index 28b35ccb3757..990f4cc345da 100644
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -313,6 +313,7 @@ rev B board is VGEN5 */
 		port {
 			ov5640_to_mipi_csi2: endpoint {
 				remote-endpoint = <&mipi_csi2_in>;
+				bus-type = <4>;
 				clock-lanes = <0>;
 				data-lanes = <1 2>;
 			};
diff --git a/arch/arm/boot/dts/stm32mp157c-ev1.dts b/arch/arm/boot/dts/stm32mp157c-ev1.dts
index b19056557ef0..613ede73b65b 100644
--- a/arch/arm/boot/dts/stm32mp157c-ev1.dts
+++ b/arch/arm/boot/dts/stm32mp157c-ev1.dts
@@ -190,6 +190,7 @@ ov5640: camera@3c {
 		port {
 			ov5640_0: endpoint {
 				remote-endpoint = <&dcmi_0>;
+				bus-type = <5>;
 				bus-width = <8>;
 				data-shift = <2>; /* lines 9:2 are used */
 				hsync-active = <0>;
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
index c7bd73f35ed8..6ef528bb5564 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
@@ -46,6 +46,7 @@ ov5640: camera@3c {
 			port {
 				ov5640_ep: endpoint {
 					remote-endpoint = <&csi_ep>;
+					bus-type = <5>;
 					bus-width = <8>;
 					hsync-active = <1>; /* Active high */
 					vsync-active = <0>; /* Active low */
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts
index dc4ab6b434f9..8ebd6547786e 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts
@@ -61,6 +61,7 @@ ov5640: camera@3c {
 			port {
 				ov5640_ep: endpoint {
 					remote-endpoint = <&csi_ep>;
+					bus-type = <5>;
 					bus-width = <8>;
 					hsync-active = <1>; /* Active high */
 					vsync-active = <0>; /* Active low */
diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
index fe6613676e45..5c79ad5e7b5d 100644
--- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
@@ -627,6 +627,7 @@ camera_rear@3b {

 		port {
 			ov5640_ep: endpoint {
+				bus-type = <4>;
 				clock-lanes = <0>;
 				data-lanes = <1 2>;
 				remote-endpoint = <&csiphy0_ep>;
diff --git a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
index 2f3d3316a1cf..33f4eceffbe5 100644
--- a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
+++ b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
@@ -254,6 +254,7 @@ ov5640@3c {
 		port {
 			csi2_cam0: endpoint {
 				remote-endpoint = <&csi2_phy0>;
+				bus-type = <4>;
 				clock-lanes = <0>;
 				data-lanes = <1 2>;
 			};
--
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH 06/13] dt-bindings: media: ov5640: Require ep properties
  2020-07-17 13:28 [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema Jacopo Mondi
                   ` (4 preceding siblings ...)
  2020-07-17 13:28 ` [PATCH 05/13] dt-bindings: media: ov5640: Make bus-type mandatory Jacopo Mondi
@ 2020-07-17 13:28 ` Jacopo Mondi
  2020-07-17 19:50   ` Laurent Pinchart
  2020-07-17 13:28 ` [PATCH 07/13] dt-bindings: media: ov5640: Remove clock-lanes Jacopo Mondi
                   ` (7 subsequent siblings)
  13 siblings, 1 reply; 46+ messages in thread
From: Jacopo Mondi @ 2020-07-17 13:28 UTC (permalink / raw)
  To: robh+dt, devicetree
  Cc: Jacopo Mondi, slongerbeam, linux-media, laurent.pinchart,
	linux-renesas-soc

Now that the bus-type property has been made mandatory for
the ov5640 bindings, use the property value to mark endpoint
properties as required depending on the data interface in use.

Mark as 'required' all the properties that were defined as required
in the original bindings in textual form. Adjust the example to
comply with validation requirement but do not update the existing
users as most of the required properties will be demoted in the
next patches.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../devicetree/bindings/media/i2c/ov5640.yaml | 41 +++++++++++++++++--
 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
index 16e6c2dc629a..8fd54575a1db 100644
--- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
@@ -110,9 +110,40 @@ properties:
           pclk-sample:
             enum: [0, 1]
 
-        required:
-          - remote-endpoint
-          - bus-type
+        allOf:
+          - if:
+              properties:
+                bus-type:
+                  const: 4
+            then:
+                properties:
+                  bus-width: false
+                  data-shift: false
+                  hsync-active: false
+                  vsync-active: false
+                  pclk-sample: false
+                required:
+                  - remote-endpoint
+                  - bus-type
+                  - clock-lanes
+                  - data-lanes
+
+          - if:
+              properties:
+                bus-type:
+                  const: 5
+            then:
+              properties:
+                clock-lanes: false
+                data-lanes: false
+              required:
+                - remote-endpoint
+                - bus-type
+                - bus-width
+                - data-shift
+                - hsync-active
+                - vsync-active
+                - pclk-sample
 
     additionalProperties: false
 
@@ -179,6 +210,10 @@ examples:
                     remote-endpoint = <&parallel_from_ov5640>;
                     bus-type = <5>;
                     bus-width = <10>;
+                    data-shift = <0>;
+                    hsync-active = <1>;
+                    vsync-active = <1>;
+                    pclk-sample = <1>;
                 };
             };
         };
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH 07/13] dt-bindings: media: ov5640: Remove clock-lanes
  2020-07-17 13:28 [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema Jacopo Mondi
                   ` (5 preceding siblings ...)
  2020-07-17 13:28 ` [PATCH 06/13] dt-bindings: media: ov5640: Require ep properties Jacopo Mondi
@ 2020-07-17 13:28 ` Jacopo Mondi
  2020-07-17 20:48   ` Laurent Pinchart
  2020-07-20 19:47   ` Rob Herring
  2020-07-17 13:28 ` [PATCH 08/13] dt-bindings: media: ov5640: Remove data-shift Jacopo Mondi
                   ` (6 subsequent siblings)
  13 siblings, 2 replies; 46+ messages in thread
From: Jacopo Mondi @ 2020-07-17 13:28 UTC (permalink / raw)
  To: robh+dt, devicetree
  Cc: Jacopo Mondi, slongerbeam, linux-media, laurent.pinchart,
	linux-renesas-soc, Benoît Cousson, Tony Lindgren,
	linux-omap, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	Fabio Estevam, NXP Linux Team, Maxime Coquelin, Alexandre Torgue,
	moderated list:ARM/STM32 ARCHITECTURE, Maxime Ripard,
	Chen-Yu Tsai, Andy Gross, Bjorn Andersson, Tero Kristo,
	Nishanth Menon

The ov5640 does not support routing the clock signal to a different
lane, and the property has value fixed to 0.

Remove the property from the bindings and update its users
accordingly.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 6 ------
 arch/arm/boot/dts/dra72-evm-common.dtsi                 | 1 -
 arch/arm/boot/dts/dra76-evm.dts                         | 1 -
 arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi           | 1 -
 arch/arm/boot/dts/imx6qdl-icore.dtsi                    | 1 -
 arch/arm/boot/dts/imx6qdl-sabrelite.dtsi                | 1 -
 arch/arm/boot/dts/imx6qdl-sabresd.dtsi                  | 1 -
 arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi               | 1 -
 arch/arm64/boot/dts/ti/k3-am654-base-board.dts          | 1 -
 9 files changed, 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
index 8fd54575a1db..5e1662e848bd 100644
--- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
@@ -76,9 +76,6 @@ properties:
           bus-type:
             enum: [4, 5]

-          clock-lanes:
-            const: 0
-
           data-lanes:
             description: |
               Should be set to <1> or <1 2> (one or two CSI-2 lanes supported).
@@ -125,7 +122,6 @@ properties:
                 required:
                   - remote-endpoint
                   - bus-type
-                  - clock-lanes
                   - data-lanes

           - if:
@@ -134,7 +130,6 @@ properties:
                   const: 5
             then:
               properties:
-                clock-lanes: false
                 data-lanes: false
               required:
                 - remote-endpoint
@@ -182,7 +177,6 @@ examples:
                 ov5640_to_mipi_csi2: endpoint {
                     remote-endpoint = <&mipi_csi2_from_ov5640>;
                     bus-type = <4>;
-                    clock-lanes = <0>;
                     data-lanes = <1 2>;
                 };
             };
diff --git a/arch/arm/boot/dts/dra72-evm-common.dtsi b/arch/arm/boot/dts/dra72-evm-common.dtsi
index aba7eb55525d..b5b31dc927d4 100644
--- a/arch/arm/boot/dts/dra72-evm-common.dtsi
+++ b/arch/arm/boot/dts/dra72-evm-common.dtsi
@@ -288,7 +288,6 @@ port {
 			csi2_cam0: endpoint {
 				remote-endpoint = <&csi2_phy0>;
 				bus-type = <4>;
-				clock-lanes = <0>;
 				data-lanes = <1 2>;
 			};
 		};
diff --git a/arch/arm/boot/dts/dra76-evm.dts b/arch/arm/boot/dts/dra76-evm.dts
index ddf16e65ec7f..6fcb96abc41d 100644
--- a/arch/arm/boot/dts/dra76-evm.dts
+++ b/arch/arm/boot/dts/dra76-evm.dts
@@ -421,7 +421,6 @@ port {
 			csi2_cam0: endpoint {
 				remote-endpoint = <&csi2_phy0>;
 				bus-type = <4>;
-				clock-lanes = <0>;
 				data-lanes = <1 2>;
 			};
 		};
diff --git a/arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi b/arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi
index dd5765778e87..a12d80a2403e 100644
--- a/arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi
+++ b/arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi
@@ -275,7 +275,6 @@ port {
 			ov5640_to_mipi_csi2: endpoint {
 				remote-endpoint = <&mipi_csi2_in>;
 				bus-type = <4>;
-				clock-lanes = <0>;
 				data-lanes = <1 2>;
 			};
 		};
diff --git a/arch/arm/boot/dts/imx6qdl-icore.dtsi b/arch/arm/boot/dts/imx6qdl-icore.dtsi
index 2b1f275b8cc4..05bd332f527b 100644
--- a/arch/arm/boot/dts/imx6qdl-icore.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-icore.dtsi
@@ -214,7 +214,6 @@ port {
 			ov5640_to_mipi_csi2: endpoint {
 				remote-endpoint = <&mipi_csi2_in>;
 				bus-type = <4>;
-				clock-lanes = <0>;
 				data-lanes = <1 2>;
 			};
 		};
diff --git a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
index 5752abb48e68..5eb7fbf33069 100644
--- a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
@@ -383,7 +383,6 @@ port {
 			ov5640_to_mipi_csi2: endpoint {
 				remote-endpoint = <&mipi_csi2_in>;
 				bus-type = <4>;
-				clock-lanes = <0>;
 				data-lanes = <1 2>;
 			};
 		};
diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
index 990f4cc345da..02f78b46a8fd 100644
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -314,7 +314,6 @@ port {
 			ov5640_to_mipi_csi2: endpoint {
 				remote-endpoint = <&mipi_csi2_in>;
 				bus-type = <4>;
-				clock-lanes = <0>;
 				data-lanes = <1 2>;
 			};
 		};
diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
index 5c79ad5e7b5d..8c24fd468a22 100644
--- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
@@ -628,7 +628,6 @@ camera_rear@3b {
 		port {
 			ov5640_ep: endpoint {
 				bus-type = <4>;
-				clock-lanes = <0>;
 				data-lanes = <1 2>;
 				remote-endpoint = <&csiphy0_ep>;
 			};
diff --git a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
index 33f4eceffbe5..dd6a636ba3af 100644
--- a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
+++ b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
@@ -255,7 +255,6 @@ port {
 			csi2_cam0: endpoint {
 				remote-endpoint = <&csi2_phy0>;
 				bus-type = <4>;
-				clock-lanes = <0>;
 				data-lanes = <1 2>;
 			};
 		};
--
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH 08/13] dt-bindings: media: ov5640: Remove data-shift
  2020-07-17 13:28 [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema Jacopo Mondi
                   ` (6 preceding siblings ...)
  2020-07-17 13:28 ` [PATCH 07/13] dt-bindings: media: ov5640: Remove clock-lanes Jacopo Mondi
@ 2020-07-17 13:28 ` Jacopo Mondi
  2020-07-17 20:57   ` Laurent Pinchart
  2020-07-17 13:28 ` [PATCH 09/13] dt-bindings: media: ov5640: Add default for synch signals Jacopo Mondi
                   ` (5 subsequent siblings)
  13 siblings, 1 reply; 46+ messages in thread
From: Jacopo Mondi @ 2020-07-17 13:28 UTC (permalink / raw)
  To: robh+dt, devicetree
  Cc: Jacopo Mondi, slongerbeam, linux-media, laurent.pinchart,
	linux-renesas-soc, Maxime Coquelin, Alexandre Torgue,
	moderated list:ARM/STM32 ARCHITECTURE

The value of the data-shift property solely depend on the selected
bus width and it's not freely configurable.

Remove it from the bindings document and update its users accordingly.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 ---------
 arch/arm/boot/dts/stm32mp157c-ev1.dts                   | 1 -
 2 files changed, 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
index 5e1662e848bd..ab700a1830aa 100644
--- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
@@ -92,12 +92,6 @@ properties:
               parallel bus.
             enum: [8, 10]

-          data-shift:
-            description: |
-              Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or
-              <0> for 10 bits parallel bus.
-            enum: [0, 2]
-
           hsync-active:
             enum: [0, 1]

@@ -115,7 +109,6 @@ properties:
             then:
                 properties:
                   bus-width: false
-                  data-shift: false
                   hsync-active: false
                   vsync-active: false
                   pclk-sample: false
@@ -135,7 +128,6 @@ properties:
                 - remote-endpoint
                 - bus-type
                 - bus-width
-                - data-shift
                 - hsync-active
                 - vsync-active
                 - pclk-sample
@@ -204,7 +196,6 @@ examples:
                     remote-endpoint = <&parallel_from_ov5640>;
                     bus-type = <5>;
                     bus-width = <10>;
-                    data-shift = <0>;
                     hsync-active = <1>;
                     vsync-active = <1>;
                     pclk-sample = <1>;
diff --git a/arch/arm/boot/dts/stm32mp157c-ev1.dts b/arch/arm/boot/dts/stm32mp157c-ev1.dts
index 613ede73b65b..96f96202ca63 100644
--- a/arch/arm/boot/dts/stm32mp157c-ev1.dts
+++ b/arch/arm/boot/dts/stm32mp157c-ev1.dts
@@ -192,7 +192,6 @@ ov5640_0: endpoint {
 				remote-endpoint = <&dcmi_0>;
 				bus-type = <5>;
 				bus-width = <8>;
-				data-shift = <2>; /* lines 9:2 are used */
 				hsync-active = <0>;
 				vsync-active = <0>;
 				pclk-sample = <1>;
--
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH 09/13] dt-bindings: media: ov5640: Add default for synch signals
  2020-07-17 13:28 [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema Jacopo Mondi
                   ` (7 preceding siblings ...)
  2020-07-17 13:28 ` [PATCH 08/13] dt-bindings: media: ov5640: Remove data-shift Jacopo Mondi
@ 2020-07-17 13:28 ` Jacopo Mondi
  2020-07-17 20:59   ` Laurent Pinchart
  2020-07-17 13:28 ` [PATCH 10/13] dt-bindings: media: ov5640: Demote DVP required properties Jacopo Mondi
                   ` (4 subsequent siblings)
  13 siblings, 1 reply; 46+ messages in thread
From: Jacopo Mondi @ 2020-07-17 13:28 UTC (permalink / raw)
  To: robh+dt, devicetree
  Cc: Jacopo Mondi, slongerbeam, linux-media, laurent.pinchart,
	linux-renesas-soc

Add defalt value for the DVP interface synchronism signals.

The default values have been derived from register 0x4740
documentation (datasheet version 2.03)

0x4740 POLARITY CTRL00 default = 0x20
bit 5: pclk polarity = 1 active high
bit 1: HREF polarity = 0 active low
bit 0: VSYNC polarity = 0 active low

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

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
index ab700a1830aa..3c20cdd02f76 100644
--- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
@@ -94,12 +94,15 @@ properties:
 
           hsync-active:
             enum: [0, 1]
+            default: 0
 
           vsync-active:
             enum: [0, 1]
+            default: 0
 
           pclk-sample:
             enum: [0, 1]
+            default: 1
 
         allOf:
           - if:
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH 10/13] dt-bindings: media: ov5640: Demote DVP required properties
  2020-07-17 13:28 [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema Jacopo Mondi
                   ` (8 preceding siblings ...)
  2020-07-17 13:28 ` [PATCH 09/13] dt-bindings: media: ov5640: Add default for synch signals Jacopo Mondi
@ 2020-07-17 13:28 ` Jacopo Mondi
  2020-07-17 21:00   ` Laurent Pinchart
  2020-07-17 13:28 ` [PATCH 11/13] media: i2c: ov5640: Initialize DVP polarities as default Jacopo Mondi
                   ` (3 subsequent siblings)
  13 siblings, 1 reply; 46+ messages in thread
From: Jacopo Mondi @ 2020-07-17 13:28 UTC (permalink / raw)
  To: robh+dt, devicetree
  Cc: Jacopo Mondi, slongerbeam, linux-media, laurent.pinchart,
	linux-renesas-soc

Now that properties have a default value assigned, do not mark them
as required anymore in the bindings documentation.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
index 3c20cdd02f76..514f8150b52e 100644
--- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
@@ -131,9 +131,6 @@ properties:
                 - remote-endpoint
                 - bus-type
                 - bus-width
-                - hsync-active
-                - vsync-active
-                - pclk-sample
 
     additionalProperties: false
 
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH 11/13] media: i2c: ov5640: Initialize DVP polarities as default
  2020-07-17 13:28 [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema Jacopo Mondi
                   ` (9 preceding siblings ...)
  2020-07-17 13:28 ` [PATCH 10/13] dt-bindings: media: ov5640: Demote DVP required properties Jacopo Mondi
@ 2020-07-17 13:28 ` Jacopo Mondi
  2020-07-17 21:02   ` Laurent Pinchart
  2020-07-17 13:28 ` [PATCH 12/13] MAINTAINERS: ov5640: Add bindings file Jacopo Mondi
                   ` (2 subsequent siblings)
  13 siblings, 1 reply; 46+ messages in thread
From: Jacopo Mondi @ 2020-07-17 13:28 UTC (permalink / raw)
  To: robh+dt, devicetree
  Cc: Jacopo Mondi, slongerbeam, linux-media, laurent.pinchart,
	linux-renesas-soc

With the bindings now updated to initialize the DVP synchronism
signals to the values reported by the datasheet, update the driver
to respect the same default values.

The driver works-around a documentation bug and reports the VSYNC
polarity control bit to be inverted. Regardless of the actual
value written to the hardware register, initialize the DVP configuration
to the defaults reported by the chip manual and the bindings document:

- VSYNC: active low
- HREF:	active low
- PCLK:	active high

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/ov5640.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 2fe4a7ac0592..012ef1df59aa 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1212,9 +1212,9 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
 {
 	int ret;
 	unsigned int flags = sensor->ep.bus.parallel.flags;
-	u8 pclk_pol = 0;
+	u8 pclk_pol = 1;
 	u8 hsync_pol = 0;
-	u8 vsync_pol = 0;
+	u8 vsync_pol = 1;
 
 	/*
 	 * Note about parallel port configuration.
@@ -1229,9 +1229,9 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
 	 * devicetree endpoint control lines properties.
 	 * If no endpoint control lines properties are set,
 	 * polarity will be as below:
-	 * - VSYNC:	active high
+	 * - VSYNC:	active low
 	 * - HREF:	active low
-	 * - PCLK:	active low
+	 * - PCLK:	active high
 	 */
 
 	if (on) {
@@ -1245,12 +1245,12 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
 		 *		datasheet and hardware, 0 is active high
 		 *		and 1 is active low...)
 		 */
-		if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
-			pclk_pol = 1;
+		if (flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
+			pclk_pol = 0;
 		if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
 			hsync_pol = 1;
-		if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
-			vsync_pol = 1;
+		if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
+			vsync_pol = 0;
 
 		ret = ov5640_write_reg(sensor,
 				       OV5640_REG_POLARITY_CTRL00,
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH 12/13] MAINTAINERS: ov5640: Add bindings file
  2020-07-17 13:28 [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema Jacopo Mondi
                   ` (10 preceding siblings ...)
  2020-07-17 13:28 ` [PATCH 11/13] media: i2c: ov5640: Initialize DVP polarities as default Jacopo Mondi
@ 2020-07-17 13:28 ` Jacopo Mondi
  2020-07-17 21:03   ` Laurent Pinchart
  2020-07-17 13:28 ` [PATCH 13/13] dt-bindings: media: ov5640: Add myself as co-maintainer Jacopo Mondi
  2020-07-17 19:31 ` [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema Laurent Pinchart
  13 siblings, 1 reply; 46+ messages in thread
From: Jacopo Mondi @ 2020-07-17 13:28 UTC (permalink / raw)
  To: robh+dt, devicetree
  Cc: Jacopo Mondi, slongerbeam, linux-media, laurent.pinchart,
	linux-renesas-soc

Add the newly introduced bindings file to the list of files
associated with the ov5640 driver in the MAINTAINERS file.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2730af1265ea..fc2a2129d18e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12604,6 +12604,7 @@ M:	Steve Longerbeam <slongerbeam@gmail.com>
 L:	linux-media@vger.kernel.org
 S:	Maintained
 T:	git git://linuxtv.org/media_tree.git
+F:	Documentation/devicetree/bindings/media/i2c/ov5640.yaml
 F:	drivers/media/i2c/ov5640.c
 
 OMNIVISION OV5647 SENSOR DRIVER
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* [PATCH 13/13] dt-bindings: media: ov5640: Add myself as co-maintainer
  2020-07-17 13:28 [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema Jacopo Mondi
                   ` (11 preceding siblings ...)
  2020-07-17 13:28 ` [PATCH 12/13] MAINTAINERS: ov5640: Add bindings file Jacopo Mondi
@ 2020-07-17 13:28 ` Jacopo Mondi
  2020-07-17 21:03   ` Laurent Pinchart
  2020-07-17 19:31 ` [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema Laurent Pinchart
  13 siblings, 1 reply; 46+ messages in thread
From: Jacopo Mondi @ 2020-07-17 13:28 UTC (permalink / raw)
  To: robh+dt, devicetree
  Cc: Jacopo Mondi, slongerbeam, linux-media, laurent.pinchart,
	linux-renesas-soc

Add myself as co-maintainer of the driver and the bindings
file.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 1 +
 MAINTAINERS                                             | 1 +
 2 files changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
index 514f8150b52e..e28ea471bc00 100644
--- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
@@ -8,6 +8,7 @@ title: Omnivision OV5640 MIPI CSI-2 / parallel sensor
 
 maintainers:
   - Steve Longerbeam <slongerbeam@gmail.com>
+  - Jacopo Mondi <jacopo@jmondi.org>
 
 description: -|
   The OV5640 is a 5 megapixels image sensor capable of producing images in RGB,
diff --git a/MAINTAINERS b/MAINTAINERS
index fc2a2129d18e..b5dd706d072b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12601,6 +12601,7 @@ F:	drivers/media/i2c/ov2740.c
 
 OMNIVISION OV5640 SENSOR DRIVER
 M:	Steve Longerbeam <slongerbeam@gmail.com>
+M:	Jacopo Mondi <jacopo@jmondi.org>
 L:	linux-media@vger.kernel.org
 S:	Maintained
 T:	git git://linuxtv.org/media_tree.git
-- 
2.27.0


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 05/13] dt-bindings: media: ov5640: Make bus-type mandatory
  2020-07-17 13:28 ` [PATCH 05/13] dt-bindings: media: ov5640: Make bus-type mandatory Jacopo Mondi
@ 2020-07-17 15:48   ` Fabio Estevam
  2020-07-17 19:48   ` Laurent Pinchart
  2020-07-20 19:45   ` Rob Herring
  2 siblings, 0 replies; 46+ messages in thread
From: Fabio Estevam @ 2020-07-17 15:48 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: Rob Herring,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Steve Longerbeam, linux-media, Laurent Pinchart, Linux-Renesas,
	Benoît Cousson, Tony Lindgren, linux-omap, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, NXP Linux Team,
	Maxime Coquelin, Alexandre Torgue,
	moderated list:ARM/STM32 ARCHITECTURE, Maxime Ripard,
	Chen-Yu Tsai, Andy Gross, Bjorn Andersson, Tero Kristo,
	Nishanth Menon

Hi Jacopo,

On Fri, Jul 17, 2020 at 10:25 AM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:

> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> index 503f8b78615c..16e6c2dc629a 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> @@ -73,6 +73,9 @@ properties:
>            remote-endpoint:
>              description: A phandle to the bus receiver's endpoint node.
>
> +          bus-type:
> +            enum: [4, 5]

Can we have an explanation for what the values 4 and 5 mean in the yaml doc?

Looking at the series I see that 4 means MIPI-CSI2 and 5 means
parallel interface, but this is not too obvious (at least for me).

Or maybe we could use a string definition instead of hard coding 4 and 5?

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 01/13] dt-bindings: media: ov5640: Convert to json-schema
  2020-07-17 13:28 ` [PATCH 01/13] " Jacopo Mondi
@ 2020-07-17 19:28   ` Laurent Pinchart
  2020-07-20 16:32   ` Rob Herring
  1 sibling, 0 replies; 46+ messages in thread
From: Laurent Pinchart @ 2020-07-17 19:28 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: robh+dt, devicetree, slongerbeam, linux-media, linux-renesas-soc

Hi Jacopo,

Thank you for the patch.

On Fri, Jul 17, 2020 at 03:28:47PM +0200, Jacopo Mondi wrote:
> Convert the ov5640 bindings document to json-schema.
> 
> This commit ports the existing bindings, clean up patches
> will follow.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../devicetree/bindings/media/i2c/ov5640.txt  |  92 ---------
>  .../devicetree/bindings/media/i2c/ov5640.yaml | 181 ++++++++++++++++++
>  2 files changed, 181 insertions(+), 92 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5640.txt
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
> deleted file mode 100644
> index c97c2f2da12d..000000000000
> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt
> +++ /dev/null
> @@ -1,92 +0,0 @@
> -* Omnivision OV5640 MIPI CSI-2 / parallel sensor
> -
> -Required Properties:
> -- compatible: should be "ovti,ov5640"
> -- clocks: reference to the xclk input clock.
> -- clock-names: should be "xclk".
> -- DOVDD-supply: Digital I/O voltage supply, 1.8 volts
> -- AVDD-supply: Analog voltage supply, 2.8 volts
> -- DVDD-supply: Digital core voltage supply, 1.5 volts
> -
> -Optional Properties:
> -- reset-gpios: reference to the GPIO connected to the reset pin, if any.
> -	       This is an active low signal to the OV5640.
> -- powerdown-gpios: reference to the GPIO connected to the powerdown pin,
> -		   if any. This is an active high signal to the OV5640.
> -- rotation: as defined in
> -	    Documentation/devicetree/bindings/media/video-interfaces.txt,
> -	    valid values are 0 (sensor mounted upright) and 180 (sensor
> -	    mounted upside down).
> -
> -The device node must contain one 'port' child node for its digital output
> -video port, in accordance with the video interface bindings defined in
> -Documentation/devicetree/bindings/media/video-interfaces.txt.
> -
> -OV5640 can be connected to a MIPI CSI-2 bus or a parallel bus endpoint.
> -
> -Endpoint node required properties for CSI-2 connection are:
> -- remote-endpoint: a phandle to the bus receiver's endpoint node.
> -- clock-lanes: should be set to <0> (clock lane on hardware lane 0)
> -- data-lanes: should be set to <1> or <1 2> (one or two CSI-2 lanes supported)
> -
> -Endpoint node required properties for parallel connection are:
> -- remote-endpoint: a phandle to the bus receiver's endpoint node.
> -- bus-width: shall be set to <8> for 8 bits parallel bus
> -	     or <10> for 10 bits parallel bus
> -- data-shift: shall be set to <2> for 8 bits parallel bus
> -	      (lines 9:2 are used) or <0> for 10 bits parallel bus
> -- hsync-active: active state of the HSYNC signal, 0/1 for LOW/HIGH respectively.
> -- vsync-active: active state of the VSYNC signal, 0/1 for LOW/HIGH respectively.
> -- pclk-sample: sample data on rising (1) or falling (0) edge of the pixel clock
> -	       signal.
> -
> -Examples:
> -
> -&i2c1 {
> -	ov5640: camera@3c {
> -		compatible = "ovti,ov5640";
> -		pinctrl-names = "default";
> -		pinctrl-0 = <&pinctrl_ov5640>;
> -		reg = <0x3c>;
> -		clocks = <&clks IMX6QDL_CLK_CKO>;
> -		clock-names = "xclk";
> -		DOVDD-supply = <&vgen4_reg>; /* 1.8v */
> -		AVDD-supply = <&vgen3_reg>;  /* 2.8v */
> -		DVDD-supply = <&vgen2_reg>;  /* 1.5v */
> -		powerdown-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
> -		reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
> -		rotation = <180>;
> -
> -		port {
> -			/* MIPI CSI-2 bus endpoint */
> -			ov5640_to_mipi_csi2: endpoint {
> -				remote-endpoint = <&mipi_csi2_from_ov5640>;
> -				clock-lanes = <0>;
> -				data-lanes = <1 2>;
> -			};
> -		};
> -	};
> -};
> -
> -&i2c1 {
> -	ov5640: camera@3c {
> -		compatible = "ovti,ov5640";
> -		pinctrl-names = "default";
> -		pinctrl-0 = <&pinctrl_ov5640>;
> -		reg = <0x3c>;
> -		clocks = <&clk_ext_camera>;
> -		clock-names = "xclk";
> -
> -		port {
> -			/* Parallel bus endpoint */
> -			ov5640_to_parallel: endpoint {
> -				remote-endpoint = <&parallel_from_ov5640>;
> -				bus-width = <8>;
> -				data-shift = <2>; /* lines 9:2 are used */
> -				hsync-active = <0>;
> -				vsync-active = <0>;
> -				pclk-sample = <1>;
> -			};
> -		};
> -	};
> -};
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> new file mode 100644
> index 000000000000..ceeacc91c801
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> @@ -0,0 +1,181 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ov5640.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Omnivision OV5640 MIPI CSI-2 / parallel sensor
> +
> +maintainers:
> +  - Steve Longerbeam <slongerbeam@gmail.com>
> +
> +description: -|
> +  The OV5640 is a 5 megapixels image sensor capable of producing images in RGB,
> +  RAW, YUV and compressed formats. It features a MIPI CSI-2 and a parallel data
> +  interface and an I2C-compatible (CCI) control interface.
> +
> +properties:
> +  compatible:
> +    const: ovti,ov5640
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: xclk
> +
> +  DOVDD-supply:
> +    description: Digital I/O voltage supply, 1.8 volts.
> +    maxItems: 1
> +
> +  AVDD-supply:
> +    description: Analog voltage supply, 2.8 volts.
> +    maxItems: 1
> +
> +  DVDD-supply:
> +    description: Digital core voltage supply.
> +    maxItems: 1
> +
> +  reset-gpios:
> +    description: |
> +      Reference to the GPIO connected to the reset pin, if any. This is an
> +      active low signal to the OV5640.
> +    maxItems: 1
> +
> +  powerdown-gpios:
> +    description: |
> +      Reference tot he GPIO connected to the powerdown pin, if any. This is an
> +      active high signal to the OV5640.
> +    maxItems: 1
> +
> +  rotation:
> +    description: |
> +      As defined in Documentation/devicetree/bindings/media/video-interfaces.txt
> +    enum: [0, 180]
> +
> +  port:
> +    type: object
> +    description: |
> +      The device node must contain one 'port' child node for its digital output
> +      video port, in accordance with the video interface bindings defined in
> +      Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +      OV5640 can be connected to a MIPI CSI-2 bus or a parallel bus endpoint.
> +
> +    properties:
> +      endpoint:
> +        type: object
> +        properties:
> +          remote-endpoint:
> +            description: A phandle to the bus receiver's endpoint node.
> +
> +          clock-lanes:
> +            const: 0
> +
> +          data-lanes:
> +            description: |
> +              Should be set to <1> or <1 2> (one or two CSI-2 lanes supported).

I think you can drop the description, it just duplicates the values
below.

> +            oneOf:
> +              - items:
> +                - const: 1
> +              - items:
> +                - const: 1
> +                - const: 2

This could also be written

            minItems: 1
            items:
              - const: 1
              - const: 2

> +
> +          bus-width:
> +            description: |
> +              Shall be set to <8> for 8 bits parallel bus or <10> for 10 bits
> +              parallel bus.

I think I'd drop the description here too. We need to eventually convert
Documentation/devicetree/bindings/media/video-interfaces.txt to YAML,
and descriptions will be stored there.

> +            enum: [8, 10]
> +
> +          data-shift:
> +            description: |
> +              Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or
> +              <0> for 10 bits parallel bus.
> +            enum: [0, 2]
> +
> +          hsync-active:
> +            enum: [0, 1]
> +
> +          vsync-active:
> +            enum: [0, 1]
> +
> +          pclk-sample:
> +            enum: [0, 1]
> +
> +        required:
> +          - remote-endpoint

You could add

        allOf:
          - if:
              properties:
                bus-width:
                  const: 8
            then:
              properties:
                data-shift:
                  const: 2
              required:
                - data-shift
          - if:
              properties:
                bus-width:
                  const: 10
            then:
              properties:
                data-shift:
                  const: 0
              required:
                - data-shift

To document the relationship between bus-width and data-shift in rules
instead of in the data-shift description.

Could you please take these comments into account for other patches in
the series, where applicable ?

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

> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - DOVDD-supply
> +  - AVDD-supply
> +  - DVDD-supply
> +  - port
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/clock/imx6qdl-clock.h>
> +
> +    i2c0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        camera@3c {
> +            compatible = "ovti,ov5640";
> +            reg = <0x3c>;
> +            clocks = <&clks IMX6QDL_CLK_CKO>;
> +            clock-names = "xclk";
> +            DOVDD-supply = <&vgen4_reg>; /* 1.8v */
> +            AVDD-supply = <&vgen3_reg>;  /* 2.8v */
> +            DVDD-supply = <&vgen2_reg>;  /* 1.5v */
> +            powerdown-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
> +            reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
> +            rotation = <180>;
> +
> +            port {
> +                ov5640_to_mipi_csi2: endpoint {
> +                    remote-endpoint = <&mipi_csi2_from_ov5640>;
> +                    clock-lanes = <0>;
> +                    data-lanes = <1 2>;
> +                };
> +            };
> +        };
> +    };
> +
> +    i2c1 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        camera@3c {
> +            compatible = "ovti,ov5640";
> +            reg = <0x3c>;
> +            clocks = <&clks IMX6QDL_CLK_CKO>;
> +            clock-names = "xclk";
> +            DOVDD-supply = <&vgen4_reg>; /* 1.8v */
> +            AVDD-supply = <&vgen3_reg>;  /* 2.8v */
> +            DVDD-supply = <&vgen2_reg>;  /* 1.5v */
> +            powerdown-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
> +            reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
> +            rotation = <180>;
> +
> +            port {
> +                ov5640_to_parallel: endpoint {
> +                    remote-endpoint = <&parallel_from_ov5640>;
> +                    bus-width = <10>;
> +                };
> +            };
> +        };
> +    };
> +
> +...

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema
  2020-07-17 13:28 [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema Jacopo Mondi
                   ` (12 preceding siblings ...)
  2020-07-17 13:28 ` [PATCH 13/13] dt-bindings: media: ov5640: Add myself as co-maintainer Jacopo Mondi
@ 2020-07-17 19:31 ` Laurent Pinchart
  13 siblings, 0 replies; 46+ messages in thread
From: Laurent Pinchart @ 2020-07-17 19:31 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: robh+dt, devicetree, slongerbeam, linux-media, linux-renesas-soc

Hi Jacopo,

On Fri, Jul 17, 2020 at 03:28:46PM +0200, Jacopo Mondi wrote:
> Hello,
>    this series has been broken out from
> " [PATCH 0/8] dt-bindings: media: i2c: Convert to json-schema"
> which I sent out a few days ago.
> 
> It was a single patch, now it's 13. Not sure it's a good sign :)
> 
> Anyway, in the version sent as part of the mentioned series I just converted
> the existing bindings to the json-schema format, without putting to much
> attention to what was there.
> 
> As suggested by Laurent I moved to specify properties as dt-schema, to
> allow their validation, and that sprinkled to a lot of fun.

I found it "fun" too when I started converting bindings to YAML schemas,
albeit a quite painful fun at times. It was time consuming for me, as I
found out a new mindset was needed to really understand what I was
doing. I wonder if it's easier for people with prior exposure to formal
languages.

> For each patch in the series, the DTS and bindings maintainers are CC-ed.
> The whole series has been sent to a limited audience only.
> 
> The series starts with a plain conversion of the existing bindings to
> dt-schema. It then fixes a potential issue on a dts user of the sensor,
> and then move to remove or demote endpoint properties that were marked as
> required in the original text bindings. This of course causes a log of churn
> in the existing DTS which use the sensor, and opens some questions there from
> which I expect the individual maintainers to provide directions on.
> 
> Finally, the small patch for the driver to initialize DVP default signal
> values to what the sensor manual and the bindings report.
> 
> The series ends with the update of the MAINTAINERS file with the new
> .yaml binding file and the addition of myself as co-maintainer for the driver.
> Steve if you intended to remove yourself please send a patch, I didn't dare to
> do it myself :)
> 
> A final note for Rob and his bot: some patches breaks validation of existing
> dts, which are then fixed by following patches. Hope this is ok, but it I
> have to keep validation of all dts successful, patches would likely be huge
> and hinder changes which I preferred to make explicitly in single patches.
> 
> Jacopo Mondi (13):
>   dt-bindings: media: ov5640: Convert to json-schema
>   arm64: dts: qcom: apq8016-sbc: Fix CSI-2 lanes routing
>   dt-bindings: media: ov5640: Do not limit rotation
>   dt-bindings: media: ti,cal: Remove sensor from example
>   dt-bindings: media: ov5640: Make bus-type mandatory
>   dt-bindings: media: ov5640: Require ep properties
>   dt-bindings: media: ov5640: Remove clock-lanes
>   dt-bindings: media: ov5640: Remove data-shift
>   dt-bindings: media: ov5640: Add default for synch signals
>   dt-bindings: media: ov5640: Demote DVP required properties
>   media: i2c: ov5640: Initialize DVP polarities as default
>   MAINTAINERS: ov5640: Add bindings file
>   dt-bindings: media: ov5640: Add myself as co-maintainer
> 
>  .../devicetree/bindings/media/i2c/ov5640.txt  |  92 --------
>  .../devicetree/bindings/media/i2c/ov5640.yaml | 208 ++++++++++++++++++
>  .../devicetree/bindings/media/ti,cal.yaml     |  22 --
>  MAINTAINERS                                   |   2 +
>  arch/arm/boot/dts/dra72-evm-common.dtsi       |   2 +-
>  arch/arm/boot/dts/dra76-evm.dts               |   2 +-
>  arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi |   2 +-
>  arch/arm/boot/dts/imx6qdl-icore.dtsi          |   2 +-
>  arch/arm/boot/dts/imx6qdl-sabrelite.dtsi      |   2 +-
>  arch/arm/boot/dts/imx6qdl-sabresd.dtsi        |   2 +-
>  arch/arm/boot/dts/stm32mp157c-ev1.dts         |   2 +-
>  .../allwinner/sun50i-a64-amarula-relic.dts    |   1 +
>  .../boot/dts/allwinner/sun50i-a64-pinetab.dts |   1 +
>  arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi     |   8 +-
>  .../arm64/boot/dts/ti/k3-am654-base-board.dts |   2 +-
>  drivers/media/i2c/ov5640.c                    |  16 +-
>  16 files changed, 232 insertions(+), 134 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5640.txt
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5640.yaml

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 02/13] arm64: dts: qcom: apq8016-sbc: Fix CSI-2 lanes routing
  2020-07-17 13:28 ` [PATCH 02/13] arm64: dts: qcom: apq8016-sbc: Fix CSI-2 lanes routing Jacopo Mondi
@ 2020-07-17 19:35   ` Laurent Pinchart
  2020-07-22  8:14     ` Loic Poulain
  0 siblings, 1 reply; 46+ messages in thread
From: Laurent Pinchart @ 2020-07-17 19:35 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: robh+dt, devicetree, slongerbeam, linux-media, linux-renesas-soc,
	Andy Gross, Bjorn Andersson, Loic Poulain

Hi Jacopo,

On Fri, Jul 17, 2020 at 03:28:48PM +0200, Jacopo Mondi wrote:
> The ov5640 sensor does not support lanes reconfiguration according
> to version of the datasheet I have (version 2.03) and the driver
> does not parse the properties to try to reconfigure them.
> 
> Fix the properties values in the camera and cci node.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> Loic, I see you added the camera nodes in
> 39e0ce6cd1bf ("arm64: dts: qcom: apq8016-sbc: Add CCI/Sensor nodes")
> 
> Do you have any idea how lanes could be swapped if, from my understanding,
> nor the sensor nor the driver supports that ?

It's not supported on the OV5640 side, so I think the second hunk of
this patch is correct, but I believe that the CAMSS supports lane
reordering, so the first hunk is likely incorrect and should be dropped.

> ---
>  arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> index 8a4b790aa7ff..fe6613676e45 100644
> --- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> +++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> @@ -591,8 +591,8 @@ ports {
>  		port@0 {
>  			reg = <0>;
>  			csiphy0_ep: endpoint {
> -				clock-lanes = <1>;
> -				data-lanes = <0 2>;
> +				clock-lanes = <0>;
> +				data-lanes = <1 2>;
>  				remote-endpoint = <&ov5640_ep>;
>  				status = "okay";
>  			};
> @@ -627,8 +627,8 @@ camera_rear@3b {
> 
>  		port {
>  			ov5640_ep: endpoint {
> -				clock-lanes = <1>;
> -				data-lanes = <0 2>;
> +				clock-lanes = <0>;
> +				data-lanes = <1 2>;
>  				remote-endpoint = <&csiphy0_ep>;
>  			};
>  		};

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 03/13] dt-bindings: media: ov5640: Do not limit rotation
  2020-07-17 13:28 ` [PATCH 03/13] dt-bindings: media: ov5640: Do not limit rotation Jacopo Mondi
@ 2020-07-17 19:38   ` Laurent Pinchart
  2020-07-20 19:36     ` Rob Herring
  0 siblings, 1 reply; 46+ messages in thread
From: Laurent Pinchart @ 2020-07-17 19:38 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: robh+dt, devicetree, slongerbeam, linux-media, linux-renesas-soc

Hi Jacopo,

Thank you for the patch.

On Fri, Jul 17, 2020 at 03:28:49PM +0200, Jacopo Mondi wrote:
> The 'rotation' property should not be limited to only support 0 and
> 180 degrees, as that limitation comes from the driver implementation
> and not from any device specific constraint.
> 
> Remove the enumeration of supported values from 'rotation'.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> index ceeacc91c801..503f8b78615c 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> @@ -55,7 +55,7 @@ properties:
>    rotation:
>      description: |
>        As defined in Documentation/devicetree/bindings/media/video-interfaces.txt
> -    enum: [0, 180]
> +    maxItems: 1

Maybe

    $ref: /schemas/types.yaml#/definitions/uint32
    minimum: 0
    maximum: 359

?

>  
>    port:
>      type: object

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 04/13] dt-bindings: media: ti,cal: Remove sensor from example
  2020-07-17 13:28 ` [PATCH 04/13] dt-bindings: media: ti,cal: Remove sensor from example Jacopo Mondi
@ 2020-07-17 19:39   ` Laurent Pinchart
  2020-07-23 20:57   ` Rob Herring
  1 sibling, 0 replies; 46+ messages in thread
From: Laurent Pinchart @ 2020-07-17 19:39 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: robh+dt, devicetree, slongerbeam, linux-media, linux-renesas-soc,
	bparrot

Hi Jacopo,

Thank you for the patch.

On Fri, Jul 17, 2020 at 03:28:50PM +0200, Jacopo Mondi wrote:
> The newly introduced dt-schema bindings for the ov5640 image
> sensor triggered a warning in the ti,cal.yaml bindings file, which
> has a camera sensor node marked compatible with the sensor.
> 
> As in the example is enough to report a node for the device that
> the bindings is about, and maintaining the sensor node up-to-date
> is a non-necessary burden, remove the image sensor node from the
> ti,cal bindings file.

The sensor node in the example gives a bit more clarity, but it's also
not super important. I'll let Tomi decide if he wants to keep it (and
thus get it fixed), or if he's fine dropping it.

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

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../devicetree/bindings/media/ti,cal.yaml     | 22 -------------------
>  1 file changed, 22 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/ti,cal.yaml b/Documentation/devicetree/bindings/media/ti,cal.yaml
> index 5e066629287d..fe5c74dd246e 100644
> --- a/Documentation/devicetree/bindings/media/ti,cal.yaml
> +++ b/Documentation/devicetree/bindings/media/ti,cal.yaml
> @@ -177,26 +177,4 @@ examples:
>          };
>      };
> 
> -    i2c {
> -        clock-frequency = <400000>;
> -        #address-cells = <1>;
> -        #size-cells = <0>;
> -
> -        camera-sensor@3c {
> -               compatible = "ovti,ov5640";
> -               reg = <0x3c>;
> -
> -               clocks = <&clk_ov5640_fixed>;
> -               clock-names = "xclk";
> -
> -               port {
> -                    csi2_cam0: endpoint {
> -                            remote-endpoint = <&csi2_phy0>;
> -                            clock-lanes = <0>;
> -                            data-lanes = <1 2>;
> -                    };
> -               };
> -        };
> -    };
> -
>  ...

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 05/13] dt-bindings: media: ov5640: Make bus-type mandatory
  2020-07-17 13:28 ` [PATCH 05/13] dt-bindings: media: ov5640: Make bus-type mandatory Jacopo Mondi
  2020-07-17 15:48   ` Fabio Estevam
@ 2020-07-17 19:48   ` Laurent Pinchart
  2020-07-20 19:45   ` Rob Herring
  2 siblings, 0 replies; 46+ messages in thread
From: Laurent Pinchart @ 2020-07-17 19:48 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: robh+dt, devicetree, slongerbeam, linux-media, linux-renesas-soc,
	Benoît Cousson, Tony Lindgren, linux-omap, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Maxime Coquelin, Alexandre Torgue,
	moderated list:ARM/STM32 ARCHITECTURE, Maxime Ripard,
	Chen-Yu Tsai, Andy Gross, Bjorn Andersson, Tero Kristo,
	Nishanth Menon

Hi Jacopo,

Thank you for the patch.

On Fri, Jul 17, 2020 at 03:28:51PM +0200, Jacopo Mondi wrote:
> The ov5640 driver supports both a parallel data interface and a
> CSI-2 serial data interface.
> 
> Depending on which interface is in use, the required endpoint
> properties are different. In order to be able to validate if a
> device node is compliant with the dt-schema bindings, start by
> making the bus-type a mandatory property, and add it to all the
> existing users of ov5640 in mainline DTS.
> 
> On top of this, endpoint properties validation will be implemented,
> conditionally to the reported bus type.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Rob may request this to be split in two, one patch for the binding, one
part for the DT sources. I think it's fine to have a temporary
validation breakage between the two patches, but if you want to avoid
that, it would also be possible to first add the property to the text
bindings, update the DT sources, and then convert to YAML. Up to you.

> ---
>  Documentation/devicetree/bindings/media/i2c/ov5640.yaml    | 6 ++++++
>  arch/arm/boot/dts/dra72-evm-common.dtsi                    | 1 +
>  arch/arm/boot/dts/dra76-evm.dts                            | 1 +
>  arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi              | 1 +
>  arch/arm/boot/dts/imx6qdl-icore.dtsi                       | 1 +
>  arch/arm/boot/dts/imx6qdl-sabrelite.dtsi                   | 1 +
>  arch/arm/boot/dts/imx6qdl-sabresd.dtsi                     | 1 +
>  arch/arm/boot/dts/stm32mp157c-ev1.dts                      | 1 +
>  arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts | 1 +
>  arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts       | 1 +
>  arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi                  | 1 +
>  arch/arm64/boot/dts/ti/k3-am654-base-board.dts             | 1 +
>  12 files changed, 17 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> index 503f8b78615c..16e6c2dc629a 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> @@ -73,6 +73,9 @@ properties:
>            remote-endpoint:
>              description: A phandle to the bus receiver's endpoint node.
> 
> +          bus-type:

You may want to add a comment referring to video-interfaces.txt. We
should really have a header file defining macros for the different bus
types. I wonder how that would work with YAML schemas though.

> +            enum: [4, 5]
> +
>            clock-lanes:
>              const: 0
> 
> @@ -109,6 +112,7 @@ properties:
> 
>          required:
>            - remote-endpoint
> +          - bus-type
> 
>      additionalProperties: false
> 

allOf:
  - if:
      properties:
        bus-type:
          const: 4
      then:
        ...
  - if:
      properties:
        bus-type:
          const: 5
      then:
        ...

It's really time to convert video-interfaces.txt... As that's a big
task, how about doing so piece by piece, moving the bus-type rules
first, and leaving the ports and endpoints (and other pieces) for later
? I think nobody wants to touch video-interfaces.txt because it's lots
of work, starting with a smaller piece may help setting the train in
motion.

I'm also fine doing this on top of this series, yak shaving is not
required.

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

> @@ -146,6 +150,7 @@ examples:
>              port {
>                  ov5640_to_mipi_csi2: endpoint {
>                      remote-endpoint = <&mipi_csi2_from_ov5640>;
> +                    bus-type = <4>;
>                      clock-lanes = <0>;
>                      data-lanes = <1 2>;
>                  };
> @@ -172,6 +177,7 @@ examples:
>              port {
>                  ov5640_to_parallel: endpoint {
>                      remote-endpoint = <&parallel_from_ov5640>;
> +                    bus-type = <5>;
>                      bus-width = <10>;
>                  };
>              };
> diff --git a/arch/arm/boot/dts/dra72-evm-common.dtsi b/arch/arm/boot/dts/dra72-evm-common.dtsi
> index c84b63bf0fc8..aba7eb55525d 100644
> --- a/arch/arm/boot/dts/dra72-evm-common.dtsi
> +++ b/arch/arm/boot/dts/dra72-evm-common.dtsi
> @@ -287,6 +287,7 @@ ov5640@3c {
>  		port {
>  			csi2_cam0: endpoint {
>  				remote-endpoint = <&csi2_phy0>;
> +				bus-type = <4>;
>  				clock-lanes = <0>;
>  				data-lanes = <1 2>;
>  			};
> diff --git a/arch/arm/boot/dts/dra76-evm.dts b/arch/arm/boot/dts/dra76-evm.dts
> index 820a0ece20d4..ddf16e65ec7f 100644
> --- a/arch/arm/boot/dts/dra76-evm.dts
> +++ b/arch/arm/boot/dts/dra76-evm.dts
> @@ -420,6 +420,7 @@ ov5640@3c {
>  		port {
>  			csi2_cam0: endpoint {
>  				remote-endpoint = <&csi2_phy0>;
> +				bus-type = <4>;
>  				clock-lanes = <0>;
>  				data-lanes = <1 2>;
>  			};
> diff --git a/arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi b/arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi
> index 9e027b9a5f91..dd5765778e87 100644
> --- a/arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi
> +++ b/arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi
> @@ -274,6 +274,7 @@ ov5640: camera@10 {
>  		port {
>  			ov5640_to_mipi_csi2: endpoint {
>  				remote-endpoint = <&mipi_csi2_in>;
> +				bus-type = <4>;
>  				clock-lanes = <0>;
>  				data-lanes = <1 2>;
>  			};
> diff --git a/arch/arm/boot/dts/imx6qdl-icore.dtsi b/arch/arm/boot/dts/imx6qdl-icore.dtsi
> index 756f3a9f1b4f..2b1f275b8cc4 100644
> --- a/arch/arm/boot/dts/imx6qdl-icore.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-icore.dtsi
> @@ -213,6 +213,7 @@ ov5640: camera@3c {
>  		port {
>  			ov5640_to_mipi_csi2: endpoint {
>  				remote-endpoint = <&mipi_csi2_in>;
> +				bus-type = <4>;
>  				clock-lanes = <0>;
>  				data-lanes = <1 2>;
>  			};
> diff --git a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
> index 8468216dae9b..5752abb48e68 100644
> --- a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
> @@ -382,6 +382,7 @@ ov5640: camera@40 {
>  		port {
>  			ov5640_to_mipi_csi2: endpoint {
>  				remote-endpoint = <&mipi_csi2_in>;
> +				bus-type = <4>;
>  				clock-lanes = <0>;
>  				data-lanes = <1 2>;
>  			};
> diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> index 28b35ccb3757..990f4cc345da 100644
> --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> @@ -313,6 +313,7 @@ rev B board is VGEN5 */
>  		port {
>  			ov5640_to_mipi_csi2: endpoint {
>  				remote-endpoint = <&mipi_csi2_in>;
> +				bus-type = <4>;
>  				clock-lanes = <0>;
>  				data-lanes = <1 2>;
>  			};
> diff --git a/arch/arm/boot/dts/stm32mp157c-ev1.dts b/arch/arm/boot/dts/stm32mp157c-ev1.dts
> index b19056557ef0..613ede73b65b 100644
> --- a/arch/arm/boot/dts/stm32mp157c-ev1.dts
> +++ b/arch/arm/boot/dts/stm32mp157c-ev1.dts
> @@ -190,6 +190,7 @@ ov5640: camera@3c {
>  		port {
>  			ov5640_0: endpoint {
>  				remote-endpoint = <&dcmi_0>;
> +				bus-type = <5>;
>  				bus-width = <8>;
>  				data-shift = <2>; /* lines 9:2 are used */
>  				hsync-active = <0>;
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
> index c7bd73f35ed8..6ef528bb5564 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
> @@ -46,6 +46,7 @@ ov5640: camera@3c {
>  			port {
>  				ov5640_ep: endpoint {
>  					remote-endpoint = <&csi_ep>;
> +					bus-type = <5>;
>  					bus-width = <8>;
>  					hsync-active = <1>; /* Active high */
>  					vsync-active = <0>; /* Active low */
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts
> index dc4ab6b434f9..8ebd6547786e 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts
> @@ -61,6 +61,7 @@ ov5640: camera@3c {
>  			port {
>  				ov5640_ep: endpoint {
>  					remote-endpoint = <&csi_ep>;
> +					bus-type = <5>;
>  					bus-width = <8>;
>  					hsync-active = <1>; /* Active high */
>  					vsync-active = <0>; /* Active low */
> diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> index fe6613676e45..5c79ad5e7b5d 100644
> --- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> +++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> @@ -627,6 +627,7 @@ camera_rear@3b {
> 
>  		port {
>  			ov5640_ep: endpoint {
> +				bus-type = <4>;
>  				clock-lanes = <0>;
>  				data-lanes = <1 2>;
>  				remote-endpoint = <&csiphy0_ep>;
> diff --git a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
> index 2f3d3316a1cf..33f4eceffbe5 100644
> --- a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
> +++ b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
> @@ -254,6 +254,7 @@ ov5640@3c {
>  		port {
>  			csi2_cam0: endpoint {
>  				remote-endpoint = <&csi2_phy0>;
> +				bus-type = <4>;
>  				clock-lanes = <0>;
>  				data-lanes = <1 2>;
>  			};

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 06/13] dt-bindings: media: ov5640: Require ep properties
  2020-07-17 13:28 ` [PATCH 06/13] dt-bindings: media: ov5640: Require ep properties Jacopo Mondi
@ 2020-07-17 19:50   ` Laurent Pinchart
  0 siblings, 0 replies; 46+ messages in thread
From: Laurent Pinchart @ 2020-07-17 19:50 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: robh+dt, devicetree, slongerbeam, linux-media, linux-renesas-soc

Hi Jacopo,

Thank you for the patch.

On Fri, Jul 17, 2020 at 03:28:52PM +0200, Jacopo Mondi wrote:
> Now that the bus-type property has been made mandatory for
> the ov5640 bindings, use the property value to mark endpoint
> properties as required depending on the data interface in use.

Ah, please disregard my comment on 05/13 that asked for this :-)

> Mark as 'required' all the properties that were defined as required
> in the original bindings in textual form. Adjust the example to
> comply with validation requirement but do not update the existing
> users as most of the required properties will be demoted in the
> next patches.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../devicetree/bindings/media/i2c/ov5640.yaml | 41 +++++++++++++++++--
>  1 file changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> index 16e6c2dc629a..8fd54575a1db 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> @@ -110,9 +110,40 @@ properties:
>            pclk-sample:
>              enum: [0, 1]
>  
> -        required:
> -          - remote-endpoint
> -          - bus-type
> +        allOf:
> +          - if:
> +              properties:
> +                bus-type:
> +                  const: 4
> +            then:
> +                properties:

I think there's one indentation level too much here.

This should really be moved to video-interfaces.yaml, I'll let you
decide if you want to bite the bullet now or if it should be done on
top.

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

> +                  bus-width: false
> +                  data-shift: false
> +                  hsync-active: false
> +                  vsync-active: false
> +                  pclk-sample: false
> +                required:
> +                  - remote-endpoint
> +                  - bus-type
> +                  - clock-lanes
> +                  - data-lanes
> +
> +          - if:
> +              properties:
> +                bus-type:
> +                  const: 5
> +            then:
> +              properties:
> +                clock-lanes: false
> +                data-lanes: false
> +              required:
> +                - remote-endpoint
> +                - bus-type
> +                - bus-width
> +                - data-shift
> +                - hsync-active
> +                - vsync-active
> +                - pclk-sample
>  
>      additionalProperties: false
>  
> @@ -179,6 +210,10 @@ examples:
>                      remote-endpoint = <&parallel_from_ov5640>;
>                      bus-type = <5>;
>                      bus-width = <10>;
> +                    data-shift = <0>;
> +                    hsync-active = <1>;
> +                    vsync-active = <1>;
> +                    pclk-sample = <1>;
>                  };
>              };
>          };

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 07/13] dt-bindings: media: ov5640: Remove clock-lanes
  2020-07-17 13:28 ` [PATCH 07/13] dt-bindings: media: ov5640: Remove clock-lanes Jacopo Mondi
@ 2020-07-17 20:48   ` Laurent Pinchart
  2020-07-20 19:47   ` Rob Herring
  1 sibling, 0 replies; 46+ messages in thread
From: Laurent Pinchart @ 2020-07-17 20:48 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: robh+dt, devicetree, slongerbeam, linux-media, linux-renesas-soc,
	Benoît Cousson, Tony Lindgren, linux-omap, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	NXP Linux Team, Maxime Coquelin, Alexandre Torgue,
	moderated list:ARM/STM32 ARCHITECTURE, Maxime Ripard,
	Chen-Yu Tsai, Andy Gross, Bjorn Andersson, Tero Kristo,
	Nishanth Menon

Hi Jacopo,

Thank you for the patch.

On Fri, Jul 17, 2020 at 03:28:53PM +0200, Jacopo Mondi wrote:
> The ov5640 does not support routing the clock signal to a different
> lane, and the property has value fixed to 0.
> 
> Remove the property from the bindings and update its users
> accordingly.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

When we'll convert video-interface.txt to YAML, let's state clearly that
properties should not be specified when the hardware is configurable.

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

> ---
>  Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 6 ------
>  arch/arm/boot/dts/dra72-evm-common.dtsi                 | 1 -
>  arch/arm/boot/dts/dra76-evm.dts                         | 1 -
>  arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi           | 1 -
>  arch/arm/boot/dts/imx6qdl-icore.dtsi                    | 1 -
>  arch/arm/boot/dts/imx6qdl-sabrelite.dtsi                | 1 -
>  arch/arm/boot/dts/imx6qdl-sabresd.dtsi                  | 1 -
>  arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi               | 1 -
>  arch/arm64/boot/dts/ti/k3-am654-base-board.dts          | 1 -
>  9 files changed, 14 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> index 8fd54575a1db..5e1662e848bd 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> @@ -76,9 +76,6 @@ properties:
>            bus-type:
>              enum: [4, 5]
> 
> -          clock-lanes:
> -            const: 0
> -
>            data-lanes:
>              description: |
>                Should be set to <1> or <1 2> (one or two CSI-2 lanes supported).
> @@ -125,7 +122,6 @@ properties:
>                  required:
>                    - remote-endpoint
>                    - bus-type
> -                  - clock-lanes
>                    - data-lanes
> 
>            - if:
> @@ -134,7 +130,6 @@ properties:
>                    const: 5
>              then:
>                properties:
> -                clock-lanes: false
>                  data-lanes: false
>                required:
>                  - remote-endpoint
> @@ -182,7 +177,6 @@ examples:
>                  ov5640_to_mipi_csi2: endpoint {
>                      remote-endpoint = <&mipi_csi2_from_ov5640>;
>                      bus-type = <4>;
> -                    clock-lanes = <0>;
>                      data-lanes = <1 2>;
>                  };
>              };
> diff --git a/arch/arm/boot/dts/dra72-evm-common.dtsi b/arch/arm/boot/dts/dra72-evm-common.dtsi
> index aba7eb55525d..b5b31dc927d4 100644
> --- a/arch/arm/boot/dts/dra72-evm-common.dtsi
> +++ b/arch/arm/boot/dts/dra72-evm-common.dtsi
> @@ -288,7 +288,6 @@ port {
>  			csi2_cam0: endpoint {
>  				remote-endpoint = <&csi2_phy0>;
>  				bus-type = <4>;
> -				clock-lanes = <0>;
>  				data-lanes = <1 2>;
>  			};
>  		};
> diff --git a/arch/arm/boot/dts/dra76-evm.dts b/arch/arm/boot/dts/dra76-evm.dts
> index ddf16e65ec7f..6fcb96abc41d 100644
> --- a/arch/arm/boot/dts/dra76-evm.dts
> +++ b/arch/arm/boot/dts/dra76-evm.dts
> @@ -421,7 +421,6 @@ port {
>  			csi2_cam0: endpoint {
>  				remote-endpoint = <&csi2_phy0>;
>  				bus-type = <4>;
> -				clock-lanes = <0>;
>  				data-lanes = <1 2>;
>  			};
>  		};
> diff --git a/arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi b/arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi
> index dd5765778e87..a12d80a2403e 100644
> --- a/arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi
> +++ b/arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi
> @@ -275,7 +275,6 @@ port {
>  			ov5640_to_mipi_csi2: endpoint {
>  				remote-endpoint = <&mipi_csi2_in>;
>  				bus-type = <4>;
> -				clock-lanes = <0>;
>  				data-lanes = <1 2>;
>  			};
>  		};
> diff --git a/arch/arm/boot/dts/imx6qdl-icore.dtsi b/arch/arm/boot/dts/imx6qdl-icore.dtsi
> index 2b1f275b8cc4..05bd332f527b 100644
> --- a/arch/arm/boot/dts/imx6qdl-icore.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-icore.dtsi
> @@ -214,7 +214,6 @@ port {
>  			ov5640_to_mipi_csi2: endpoint {
>  				remote-endpoint = <&mipi_csi2_in>;
>  				bus-type = <4>;
> -				clock-lanes = <0>;
>  				data-lanes = <1 2>;
>  			};
>  		};
> diff --git a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
> index 5752abb48e68..5eb7fbf33069 100644
> --- a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
> @@ -383,7 +383,6 @@ port {
>  			ov5640_to_mipi_csi2: endpoint {
>  				remote-endpoint = <&mipi_csi2_in>;
>  				bus-type = <4>;
> -				clock-lanes = <0>;
>  				data-lanes = <1 2>;
>  			};
>  		};
> diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> index 990f4cc345da..02f78b46a8fd 100644
> --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> @@ -314,7 +314,6 @@ port {
>  			ov5640_to_mipi_csi2: endpoint {
>  				remote-endpoint = <&mipi_csi2_in>;
>  				bus-type = <4>;
> -				clock-lanes = <0>;
>  				data-lanes = <1 2>;
>  			};
>  		};
> diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> index 5c79ad5e7b5d..8c24fd468a22 100644
> --- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> +++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> @@ -628,7 +628,6 @@ camera_rear@3b {
>  		port {
>  			ov5640_ep: endpoint {
>  				bus-type = <4>;
> -				clock-lanes = <0>;
>  				data-lanes = <1 2>;
>  				remote-endpoint = <&csiphy0_ep>;
>  			};
> diff --git a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
> index 33f4eceffbe5..dd6a636ba3af 100644
> --- a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
> +++ b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts
> @@ -255,7 +255,6 @@ port {
>  			csi2_cam0: endpoint {
>  				remote-endpoint = <&csi2_phy0>;
>  				bus-type = <4>;
> -				clock-lanes = <0>;
>  				data-lanes = <1 2>;
>  			};
>  		};

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 08/13] dt-bindings: media: ov5640: Remove data-shift
  2020-07-17 13:28 ` [PATCH 08/13] dt-bindings: media: ov5640: Remove data-shift Jacopo Mondi
@ 2020-07-17 20:57   ` Laurent Pinchart
  2020-07-23 22:22     ` Sakari Ailus
  0 siblings, 1 reply; 46+ messages in thread
From: Laurent Pinchart @ 2020-07-17 20:57 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: robh+dt, devicetree, slongerbeam, linux-media, linux-renesas-soc,
	Maxime Coquelin, Alexandre Torgue,
	moderated list:ARM/STM32 ARCHITECTURE, Sakari Ailus

Hi Jacopo,

(CC'ing Sakari)

Thank you for the patch.

On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote:
> The value of the data-shift property solely depend on the selected
> bus width and it's not freely configurable.
> 
> Remove it from the bindings document and update its users accordingly.

Hmmmm that's an interesting one. Sakari, what do you think ?

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 ---------
>  arch/arm/boot/dts/stm32mp157c-ev1.dts                   | 1 -
>  2 files changed, 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> index 5e1662e848bd..ab700a1830aa 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> @@ -92,12 +92,6 @@ properties:
>                parallel bus.
>              enum: [8, 10]
> 
> -          data-shift:
> -            description: |
> -              Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or
> -              <0> for 10 bits parallel bus.
> -            enum: [0, 2]

Should you document in the description of bus-width that data-shift is
implied ?

> -
>            hsync-active:
>              enum: [0, 1]
> 
> @@ -115,7 +109,6 @@ properties:
>              then:
>                  properties:
>                    bus-width: false
> -                  data-shift: false
>                    hsync-active: false
>                    vsync-active: false
>                    pclk-sample: false
> @@ -135,7 +128,6 @@ properties:
>                  - remote-endpoint
>                  - bus-type
>                  - bus-width
> -                - data-shift
>                  - hsync-active
>                  - vsync-active
>                  - pclk-sample
> @@ -204,7 +196,6 @@ examples:
>                      remote-endpoint = <&parallel_from_ov5640>;
>                      bus-type = <5>;
>                      bus-width = <10>;
> -                    data-shift = <0>;
>                      hsync-active = <1>;
>                      vsync-active = <1>;
>                      pclk-sample = <1>;
> diff --git a/arch/arm/boot/dts/stm32mp157c-ev1.dts b/arch/arm/boot/dts/stm32mp157c-ev1.dts
> index 613ede73b65b..96f96202ca63 100644
> --- a/arch/arm/boot/dts/stm32mp157c-ev1.dts
> +++ b/arch/arm/boot/dts/stm32mp157c-ev1.dts
> @@ -192,7 +192,6 @@ ov5640_0: endpoint {
>  				remote-endpoint = <&dcmi_0>;
>  				bus-type = <5>;
>  				bus-width = <8>;
> -				data-shift = <2>; /* lines 9:2 are used */
>  				hsync-active = <0>;
>  				vsync-active = <0>;
>  				pclk-sample = <1>;

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 09/13] dt-bindings: media: ov5640: Add default for synch signals
  2020-07-17 13:28 ` [PATCH 09/13] dt-bindings: media: ov5640: Add default for synch signals Jacopo Mondi
@ 2020-07-17 20:59   ` Laurent Pinchart
  2020-07-20 19:50     ` Rob Herring
  0 siblings, 1 reply; 46+ messages in thread
From: Laurent Pinchart @ 2020-07-17 20:59 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: robh+dt, devicetree, slongerbeam, linux-media, linux-renesas-soc

Hi Jacopo,

Thank you for the patch.

On Fri, Jul 17, 2020 at 03:28:55PM +0200, Jacopo Mondi wrote:
> Add defalt value for the DVP interface synchronism signals.

s/synchronism/synchronization/

> The default values have been derived from register 0x4740
> documentation (datasheet version 2.03)
> 
> 0x4740 POLARITY CTRL00 default = 0x20
> bit 5: pclk polarity = 1 active high
> bit 1: HREF polarity = 0 active low
> bit 0: VSYNC polarity = 0 active low

Do we need default values ? That's only for the case where the
properties are not specified, and I think they are mandatory when the
bus-type is set to parallel, aren't they ? I think stating this
explicitly will be less error-prone.

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> index ab700a1830aa..3c20cdd02f76 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> @@ -94,12 +94,15 @@ properties:
>  
>            hsync-active:
>              enum: [0, 1]
> +            default: 0
>  
>            vsync-active:
>              enum: [0, 1]
> +            default: 0
>  
>            pclk-sample:
>              enum: [0, 1]
> +            default: 1
>  
>          allOf:
>            - if:

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 10/13] dt-bindings: media: ov5640: Demote DVP required properties
  2020-07-17 13:28 ` [PATCH 10/13] dt-bindings: media: ov5640: Demote DVP required properties Jacopo Mondi
@ 2020-07-17 21:00   ` Laurent Pinchart
  0 siblings, 0 replies; 46+ messages in thread
From: Laurent Pinchart @ 2020-07-17 21:00 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: robh+dt, devicetree, slongerbeam, linux-media, linux-renesas-soc

Hi Jacopo,

Thank you for the patch.

On Fri, Jul 17, 2020 at 03:28:56PM +0200, Jacopo Mondi wrote:
> Now that properties have a default value assigned, do not mark them
> as required anymore in the bindings documentation.

I would squash this with 09/13 (depending on the outcome of the
discussion about 09/13).

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> index 3c20cdd02f76..514f8150b52e 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> @@ -131,9 +131,6 @@ properties:
>                  - remote-endpoint
>                  - bus-type
>                  - bus-width
> -                - hsync-active
> -                - vsync-active
> -                - pclk-sample
>  
>      additionalProperties: false
>  

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 11/13] media: i2c: ov5640: Initialize DVP polarities as default
  2020-07-17 13:28 ` [PATCH 11/13] media: i2c: ov5640: Initialize DVP polarities as default Jacopo Mondi
@ 2020-07-17 21:02   ` Laurent Pinchart
  0 siblings, 0 replies; 46+ messages in thread
From: Laurent Pinchart @ 2020-07-17 21:02 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: robh+dt, devicetree, slongerbeam, linux-media, linux-renesas-soc

Hi Jacopo,

Thank you for the patch.

On Fri, Jul 17, 2020 at 03:28:57PM +0200, Jacopo Mondi wrote:
> With the bindings now updated to initialize the DVP synchronism

s/synchronism/synchronization/

> signals to the values reported by the datasheet, update the driver
> to respect the same default values.
> 
> The driver works-around a documentation bug and reports the VSYNC
> polarity control bit to be inverted. Regardless of the actual
> value written to the hardware register, initialize the DVP configuration
> to the defaults reported by the chip manual and the bindings document:
> 
> - VSYNC: active low
> - HREF:	active low
> - PCLK:	active high
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/ov5640.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 2fe4a7ac0592..012ef1df59aa 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -1212,9 +1212,9 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>  {
>  	int ret;
>  	unsigned int flags = sensor->ep.bus.parallel.flags;
> -	u8 pclk_pol = 0;
> +	u8 pclk_pol = 1;
>  	u8 hsync_pol = 0;
> -	u8 vsync_pol = 0;
> +	u8 vsync_pol = 1;
>  
>  	/*
>  	 * Note about parallel port configuration.
> @@ -1229,9 +1229,9 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>  	 * devicetree endpoint control lines properties.
>  	 * If no endpoint control lines properties are set,
>  	 * polarity will be as below:
> -	 * - VSYNC:	active high
> +	 * - VSYNC:	active low
>  	 * - HREF:	active low
> -	 * - PCLK:	active low
> +	 * - PCLK:	active high

This changes the existing behaviour, doesn't it break DT backward
compatibility ?

>  	 */
>  
>  	if (on) {
> @@ -1245,12 +1245,12 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>  		 *		datasheet and hardware, 0 is active high
>  		 *		and 1 is active low...)
>  		 */
> -		if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> -			pclk_pol = 1;
> +		if (flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> +			pclk_pol = 0;
>  		if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
>  			hsync_pol = 1;
> -		if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> -			vsync_pol = 1;
> +		if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> +			vsync_pol = 0;
>  
>  		ret = ov5640_write_reg(sensor,
>  				       OV5640_REG_POLARITY_CTRL00,

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 12/13] MAINTAINERS: ov5640: Add bindings file
  2020-07-17 13:28 ` [PATCH 12/13] MAINTAINERS: ov5640: Add bindings file Jacopo Mondi
@ 2020-07-17 21:03   ` Laurent Pinchart
  0 siblings, 0 replies; 46+ messages in thread
From: Laurent Pinchart @ 2020-07-17 21:03 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: robh+dt, devicetree, slongerbeam, linux-media, linux-renesas-soc

Hi Jacopo,

Thank you for the patch.

On Fri, Jul 17, 2020 at 03:28:58PM +0200, Jacopo Mondi wrote:
> Add the newly introduced bindings file to the list of files
> associated with the ov5640 driver in the MAINTAINERS file.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

I'd squash this with patch 01/13.

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

> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2730af1265ea..fc2a2129d18e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12604,6 +12604,7 @@ M:	Steve Longerbeam <slongerbeam@gmail.com>
>  L:	linux-media@vger.kernel.org
>  S:	Maintained
>  T:	git git://linuxtv.org/media_tree.git
> +F:	Documentation/devicetree/bindings/media/i2c/ov5640.yaml
>  F:	drivers/media/i2c/ov5640.c
>  
>  OMNIVISION OV5647 SENSOR DRIVER

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 13/13] dt-bindings: media: ov5640: Add myself as co-maintainer
  2020-07-17 13:28 ` [PATCH 13/13] dt-bindings: media: ov5640: Add myself as co-maintainer Jacopo Mondi
@ 2020-07-17 21:03   ` Laurent Pinchart
  0 siblings, 0 replies; 46+ messages in thread
From: Laurent Pinchart @ 2020-07-17 21:03 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: robh+dt, devicetree, slongerbeam, linux-media, linux-renesas-soc

Hi Jacopo,

Thank you for the patch.

On Fri, Jul 17, 2020 at 03:28:59PM +0200, Jacopo Mondi wrote:
> Add myself as co-maintainer of the driver and the bindings
> file.

I'd squash this with 01/13 too.

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

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

> ---
>  Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 1 +
>  MAINTAINERS                                             | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> index 514f8150b52e..e28ea471bc00 100644
> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> @@ -8,6 +8,7 @@ title: Omnivision OV5640 MIPI CSI-2 / parallel sensor
>  
>  maintainers:
>    - Steve Longerbeam <slongerbeam@gmail.com>
> +  - Jacopo Mondi <jacopo@jmondi.org>
>  
>  description: -|
>    The OV5640 is a 5 megapixels image sensor capable of producing images in RGB,
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fc2a2129d18e..b5dd706d072b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12601,6 +12601,7 @@ F:	drivers/media/i2c/ov2740.c
>  
>  OMNIVISION OV5640 SENSOR DRIVER
>  M:	Steve Longerbeam <slongerbeam@gmail.com>
> +M:	Jacopo Mondi <jacopo@jmondi.org>
>  L:	linux-media@vger.kernel.org
>  S:	Maintained
>  T:	git git://linuxtv.org/media_tree.git

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 01/13] dt-bindings: media: ov5640: Convert to json-schema
  2020-07-17 13:28 ` [PATCH 01/13] " Jacopo Mondi
  2020-07-17 19:28   ` Laurent Pinchart
@ 2020-07-20 16:32   ` Rob Herring
  1 sibling, 0 replies; 46+ messages in thread
From: Rob Herring @ 2020-07-20 16:32 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: robh+dt, devicetree, linux-renesas-soc, linux-media,
	laurent.pinchart, slongerbeam

On Fri, 17 Jul 2020 15:28:47 +0200, Jacopo Mondi wrote:
> Convert the ov5640 bindings document to json-schema.
> 
> This commit ports the existing bindings, clean up patches
> will follow.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../devicetree/bindings/media/i2c/ov5640.txt  |  92 ---------
>  .../devicetree/bindings/media/i2c/ov5640.yaml | 181 ++++++++++++++++++
>  2 files changed, 181 insertions(+), 92 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5640.txt
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> 


My bot found errors running 'make dt_binding_check' on your patch:

/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/media/ti,cal.example.dt.yaml: camera-sensor@3c: 'DOVDD-supply' is a required property
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/media/ti,cal.example.dt.yaml: camera-sensor@3c: 'AVDD-supply' is a required property
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/media/ti,cal.example.dt.yaml: camera-sensor@3c: 'DVDD-supply' is a required property


See https://patchwork.ozlabs.org/patch/1331124

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.


^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 03/13] dt-bindings: media: ov5640: Do not limit rotation
  2020-07-17 19:38   ` Laurent Pinchart
@ 2020-07-20 19:36     ` Rob Herring
  0 siblings, 0 replies; 46+ messages in thread
From: Rob Herring @ 2020-07-20 19:36 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Jacopo Mondi, devicetree, slongerbeam, linux-media, linux-renesas-soc

On Fri, Jul 17, 2020 at 10:38:07PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Fri, Jul 17, 2020 at 03:28:49PM +0200, Jacopo Mondi wrote:
> > The 'rotation' property should not be limited to only support 0 and
> > 180 degrees, as that limitation comes from the driver implementation
> > and not from any device specific constraint.
> > 
> > Remove the enumeration of supported values from 'rotation'.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > index ceeacc91c801..503f8b78615c 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > @@ -55,7 +55,7 @@ properties:
> >    rotation:
> >      description: |
> >        As defined in Documentation/devicetree/bindings/media/video-interfaces.txt
> > -    enum: [0, 180]
> > +    maxItems: 1

Not an array, so 'maxItems' doesn't make sense here.

> 
> Maybe
> 
>     $ref: /schemas/types.yaml#/definitions/uint32
>     minimum: 0
>     maximum: 359
> 
> ?

We should have a common definition and this binding just needs to define 
the specific constraints. If none, then just 'rotation: true'.

Rob

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 05/13] dt-bindings: media: ov5640: Make bus-type mandatory
  2020-07-17 13:28 ` [PATCH 05/13] dt-bindings: media: ov5640: Make bus-type mandatory Jacopo Mondi
  2020-07-17 15:48   ` Fabio Estevam
  2020-07-17 19:48   ` Laurent Pinchart
@ 2020-07-20 19:45   ` Rob Herring
  2 siblings, 0 replies; 46+ messages in thread
From: Rob Herring @ 2020-07-20 19:45 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: devicetree, slongerbeam, linux-media, laurent.pinchart,
	linux-renesas-soc, Benoît Cousson, Tony Lindgren,
	linux-omap, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
	Fabio Estevam, NXP Linux Team, Maxime Coquelin, Alexandre Torgue,
	moderated list:ARM/STM32 ARCHITECTURE, Maxime Ripard,
	Chen-Yu Tsai, Andy Gross, Bjorn Andersson, Tero Kristo,
	Nishanth Menon

On Fri, Jul 17, 2020 at 03:28:51PM +0200, Jacopo Mondi wrote:
> The ov5640 driver supports both a parallel data interface and a
> CSI-2 serial data interface.
> 
> Depending on which interface is in use, the required endpoint
> properties are different. In order to be able to validate if a
> device node is compliant with the dt-schema bindings, start by
> making the bus-type a mandatory property, and add it to all the
> existing users of ov5640 in mainline DTS.

You can't really make a property required on an existing binding. That 
breaks compatibility. Are all the users of the below boards okay with 
that?

There should be a default defined if 'bus-type' is not present.

> 
> On top of this, endpoint properties validation will be implemented,
> conditionally to the reported bus type.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  Documentation/devicetree/bindings/media/i2c/ov5640.yaml    | 6 ++++++
>  arch/arm/boot/dts/dra72-evm-common.dtsi                    | 1 +
>  arch/arm/boot/dts/dra76-evm.dts                            | 1 +
>  arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi              | 1 +
>  arch/arm/boot/dts/imx6qdl-icore.dtsi                       | 1 +
>  arch/arm/boot/dts/imx6qdl-sabrelite.dtsi                   | 1 +
>  arch/arm/boot/dts/imx6qdl-sabresd.dtsi                     | 1 +
>  arch/arm/boot/dts/stm32mp157c-ev1.dts                      | 1 +
>  arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts | 1 +
>  arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts       | 1 +
>  arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi                  | 1 +
>  arch/arm64/boot/dts/ti/k3-am654-base-board.dts             | 1 +
>  12 files changed, 17 insertions(+)

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 07/13] dt-bindings: media: ov5640: Remove clock-lanes
  2020-07-17 13:28 ` [PATCH 07/13] dt-bindings: media: ov5640: Remove clock-lanes Jacopo Mondi
  2020-07-17 20:48   ` Laurent Pinchart
@ 2020-07-20 19:47   ` Rob Herring
  1 sibling, 0 replies; 46+ messages in thread
From: Rob Herring @ 2020-07-20 19:47 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: Pengutronix Kernel Team, Chen-Yu Tsai, Alexandre Torgue,
	Maxime Coquelin, Tero Kristo, Fabio Estevam, Bjorn Andersson,
	linux-omap, devicetree, linux-stm32, Andy Gross, Shawn Guo,
	robh+dt, Tony Lindgren, slongerbeam, linux-media, Sascha Hauer,
	NXP Linux Team, Benoît Cousson, Maxime Ripard,
	Nishanth Menon, linux-renesas-soc, laurent.pinchart

On Fri, 17 Jul 2020 15:28:53 +0200, Jacopo Mondi wrote:
> The ov5640 does not support routing the clock signal to a different
> lane, and the property has value fixed to 0.
> 
> Remove the property from the bindings and update its users
> accordingly.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 6 ------
>  arch/arm/boot/dts/dra72-evm-common.dtsi                 | 1 -
>  arch/arm/boot/dts/dra76-evm.dts                         | 1 -
>  arch/arm/boot/dts/imx6-logicpd-baseboard.dtsi           | 1 -
>  arch/arm/boot/dts/imx6qdl-icore.dtsi                    | 1 -
>  arch/arm/boot/dts/imx6qdl-sabrelite.dtsi                | 1 -
>  arch/arm/boot/dts/imx6qdl-sabresd.dtsi                  | 1 -
>  arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi               | 1 -
>  arch/arm64/boot/dts/ti/k3-am654-base-board.dts          | 1 -
>  9 files changed, 14 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 09/13] dt-bindings: media: ov5640: Add default for synch signals
  2020-07-17 20:59   ` Laurent Pinchart
@ 2020-07-20 19:50     ` Rob Herring
  0 siblings, 0 replies; 46+ messages in thread
From: Rob Herring @ 2020-07-20 19:50 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Jacopo Mondi, devicetree, slongerbeam, linux-media, linux-renesas-soc

On Fri, Jul 17, 2020 at 11:59:31PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Fri, Jul 17, 2020 at 03:28:55PM +0200, Jacopo Mondi wrote:
> > Add defalt value for the DVP interface synchronism signals.
> 
> s/synchronism/synchronization/
> 
> > The default values have been derived from register 0x4740
> > documentation (datasheet version 2.03)
> > 
> > 0x4740 POLARITY CTRL00 default = 0x20
> > bit 5: pclk polarity = 1 active high
> > bit 1: HREF polarity = 0 active low
> > bit 0: VSYNC polarity = 0 active low
> 
> Do we need default values ? That's only for the case where the
> properties are not specified, and I think they are mandatory when the
> bus-type is set to parallel, aren't they ? I think stating this
> explicitly will be less error-prone.

In this case, I think default should be fixed or not configurable.

> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > index ab700a1830aa..3c20cdd02f76 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > @@ -94,12 +94,15 @@ properties:
> >  
> >            hsync-active:
> >              enum: [0, 1]
> > +            default: 0
> >  
> >            vsync-active:
> >              enum: [0, 1]
> > +            default: 0
> >  
> >            pclk-sample:
> >              enum: [0, 1]
> > +            default: 1
> >  
> >          allOf:
> >            - if:
> 
> -- 
> Regards,
> 
> Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 02/13] arm64: dts: qcom: apq8016-sbc: Fix CSI-2 lanes routing
  2020-07-17 19:35   ` Laurent Pinchart
@ 2020-07-22  8:14     ` Loic Poulain
  2020-07-22  8:34       ` Jacopo Mondi
  0 siblings, 1 reply; 46+ messages in thread
From: Loic Poulain @ 2020-07-22  8:14 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Jacopo Mondi, Rob Herring, devicetree, Steve Longerbeam,
	linux-media, linux-renesas-soc, Andy Gross, Bjorn Andersson

On Fri, 17 Jul 2020 at 21:35, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Jacopo,
>
> On Fri, Jul 17, 2020 at 03:28:48PM +0200, Jacopo Mondi wrote:
> > The ov5640 sensor does not support lanes reconfiguration according
> > to version of the datasheet I have (version 2.03) and the driver
> > does not parse the properties to try to reconfigure them.
> >
> > Fix the properties values in the camera and cci node.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> > Loic, I see you added the camera nodes in
> > 39e0ce6cd1bf ("arm64: dts: qcom: apq8016-sbc: Add CCI/Sensor nodes")
> >
> > Do you have any idea how lanes could be swapped if, from my understanding,
> > nor the sensor nor the driver supports that ?
>
> It's not supported on the OV5640 side, so I think the second hunk of
> this patch is correct, but I believe that the CAMSS supports lane
> reordering, so the first hunk is likely incorrect and should be dropped.

Indeed, camss supports lane configuration (cf camss_of_parse_endpoint_node).
The sensor doesn't, so that can be removed on its side.

Regards,
Loic

>
> > ---
> >  arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> > index 8a4b790aa7ff..fe6613676e45 100644
> > --- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> > @@ -591,8 +591,8 @@ ports {
> >               port@0 {
> >                       reg = <0>;
> >                       csiphy0_ep: endpoint {
> > -                             clock-lanes = <1>;
> > -                             data-lanes = <0 2>;
> > +                             clock-lanes = <0>;
> > +                             data-lanes = <1 2>;
> >                               remote-endpoint = <&ov5640_ep>;
> >                               status = "okay";
> >                       };
> > @@ -627,8 +627,8 @@ camera_rear@3b {
> >
> >               port {
> >                       ov5640_ep: endpoint {
> > -                             clock-lanes = <1>;
> > -                             data-lanes = <0 2>;
> > +                             clock-lanes = <0>;
> > +                             data-lanes = <1 2>;
> >                               remote-endpoint = <&csiphy0_ep>;
> >                       };
> >               };
>
> --
> Regards,
>
> Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 02/13] arm64: dts: qcom: apq8016-sbc: Fix CSI-2 lanes routing
  2020-07-22  8:14     ` Loic Poulain
@ 2020-07-22  8:34       ` Jacopo Mondi
  0 siblings, 0 replies; 46+ messages in thread
From: Jacopo Mondi @ 2020-07-22  8:34 UTC (permalink / raw)
  To: Loic Poulain
  Cc: Laurent Pinchart, Jacopo Mondi, Rob Herring, devicetree,
	Steve Longerbeam, linux-media, linux-renesas-soc, Andy Gross,
	Bjorn Andersson

Hi Loic,

On Wed, Jul 22, 2020 at 10:14:52AM +0200, Loic Poulain wrote:
> On Fri, 17 Jul 2020 at 21:35, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Jacopo,
> >
> > On Fri, Jul 17, 2020 at 03:28:48PM +0200, Jacopo Mondi wrote:
> > > The ov5640 sensor does not support lanes reconfiguration according
> > > to version of the datasheet I have (version 2.03) and the driver
> > > does not parse the properties to try to reconfigure them.
> > >
> > > Fix the properties values in the camera and cci node.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > > Loic, I see you added the camera nodes in
> > > 39e0ce6cd1bf ("arm64: dts: qcom: apq8016-sbc: Add CCI/Sensor nodes")
> > >
> > > Do you have any idea how lanes could be swapped if, from my understanding,
> > > nor the sensor nor the driver supports that ?
> >
> > It's not supported on the OV5640 side, so I think the second hunk of
> > this patch is correct, but I believe that the CAMSS supports lane
> > reordering, so the first hunk is likely incorrect and should be dropped.
>
> Indeed, camss supports lane configuration (cf camss_of_parse_endpoint_node).
> The sensor doesn't, so that can be removed on its side.

I removed both as I assumed otherwise lanes assignement doesn't match,
unless there's some lanes re-routing happening in between the two.

I'll drop the property from ov5640 node only.

Thanks
  j

>
> Regards,
> Loic
>
> >
> > > ---
> > >  arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> > > index 8a4b790aa7ff..fe6613676e45 100644
> > > --- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> > > @@ -591,8 +591,8 @@ ports {
> > >               port@0 {
> > >                       reg = <0>;
> > >                       csiphy0_ep: endpoint {
> > > -                             clock-lanes = <1>;
> > > -                             data-lanes = <0 2>;
> > > +                             clock-lanes = <0>;
> > > +                             data-lanes = <1 2>;
> > >                               remote-endpoint = <&ov5640_ep>;
> > >                               status = "okay";
> > >                       };
> > > @@ -627,8 +627,8 @@ camera_rear@3b {
> > >
> > >               port {
> > >                       ov5640_ep: endpoint {
> > > -                             clock-lanes = <1>;
> > > -                             data-lanes = <0 2>;
> > > +                             clock-lanes = <0>;
> > > +                             data-lanes = <1 2>;
> > >                               remote-endpoint = <&csiphy0_ep>;
> > >                       };
> > >               };
> >
> > --
> > Regards,
> >
> > Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 04/13] dt-bindings: media: ti,cal: Remove sensor from example
  2020-07-17 13:28 ` [PATCH 04/13] dt-bindings: media: ti,cal: Remove sensor from example Jacopo Mondi
  2020-07-17 19:39   ` Laurent Pinchart
@ 2020-07-23 20:57   ` Rob Herring
  1 sibling, 0 replies; 46+ messages in thread
From: Rob Herring @ 2020-07-23 20:57 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: robh+dt, bparrot, laurent.pinchart, devicetree, linux-media,
	slongerbeam, linux-renesas-soc

On Fri, 17 Jul 2020 15:28:50 +0200, Jacopo Mondi wrote:
> The newly introduced dt-schema bindings for the ov5640 image
> sensor triggered a warning in the ti,cal.yaml bindings file, which
> has a camera sensor node marked compatible with the sensor.
> 
> As in the example is enough to report a node for the device that
> the bindings is about, and maintaining the sensor node up-to-date
> is a non-necessary burden, remove the image sensor node from the
> ti,cal bindings file.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../devicetree/bindings/media/ti,cal.yaml     | 22 -------------------
>  1 file changed, 22 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 08/13] dt-bindings: media: ov5640: Remove data-shift
  2020-07-17 20:57   ` Laurent Pinchart
@ 2020-07-23 22:22     ` Sakari Ailus
  2020-07-23 23:15       ` Laurent Pinchart
  0 siblings, 1 reply; 46+ messages in thread
From: Sakari Ailus @ 2020-07-23 22:22 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Jacopo Mondi, robh+dt, devicetree, slongerbeam, linux-media,
	linux-renesas-soc, Maxime Coquelin, Alexandre Torgue,
	moderated list:ARM/STM32 ARCHITECTURE

Hi Laurent,

On Fri, Jul 17, 2020 at 11:57:22PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> (CC'ing Sakari)
> 
> Thank you for the patch.
> 
> On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote:
> > The value of the data-shift property solely depend on the selected
> > bus width and it's not freely configurable.
> > 
> > Remove it from the bindings document and update its users accordingly.
> 
> Hmmmm that's an interesting one. Sakari, what do you think ?
> 
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 ---------
> >  arch/arm/boot/dts/stm32mp157c-ev1.dts                   | 1 -
> >  2 files changed, 10 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > index 5e1662e848bd..ab700a1830aa 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > @@ -92,12 +92,6 @@ properties:
> >                parallel bus.
> >              enum: [8, 10]
> > 
> > -          data-shift:
> > -            description: |
> > -              Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or
> > -              <0> for 10 bits parallel bus.
> > -            enum: [0, 2]
> 
> Should you document in the description of bus-width that data-shift is
> implied ?

The purpose of the datas-shift property is to convey how the parallel bus
lines are connected for a given bus width for devices where it is
configurable. As this device does not not support that, then indeed this
property is not relevant for the device IMO.

> 
> > -
> >            hsync-active:
> >              enum: [0, 1]
> > 
> > @@ -115,7 +109,6 @@ properties:
> >              then:
> >                  properties:
> >                    bus-width: false
> > -                  data-shift: false
> >                    hsync-active: false
> >                    vsync-active: false
> >                    pclk-sample: false
> > @@ -135,7 +128,6 @@ properties:
> >                  - remote-endpoint
> >                  - bus-type
> >                  - bus-width
> > -                - data-shift
> >                  - hsync-active
> >                  - vsync-active
> >                  - pclk-sample
> > @@ -204,7 +196,6 @@ examples:
> >                      remote-endpoint = <&parallel_from_ov5640>;
> >                      bus-type = <5>;
> >                      bus-width = <10>;
> > -                    data-shift = <0>;
> >                      hsync-active = <1>;
> >                      vsync-active = <1>;
> >                      pclk-sample = <1>;
> > diff --git a/arch/arm/boot/dts/stm32mp157c-ev1.dts b/arch/arm/boot/dts/stm32mp157c-ev1.dts
> > index 613ede73b65b..96f96202ca63 100644
> > --- a/arch/arm/boot/dts/stm32mp157c-ev1.dts
> > +++ b/arch/arm/boot/dts/stm32mp157c-ev1.dts
> > @@ -192,7 +192,6 @@ ov5640_0: endpoint {
> >  				remote-endpoint = <&dcmi_0>;
> >  				bus-type = <5>;
> >  				bus-width = <8>;
> > -				data-shift = <2>; /* lines 9:2 are used */
> >  				hsync-active = <0>;
> >  				vsync-active = <0>;
> >  				pclk-sample = <1>;
> 

-- 
Kind regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 08/13] dt-bindings: media: ov5640: Remove data-shift
  2020-07-23 22:22     ` Sakari Ailus
@ 2020-07-23 23:15       ` Laurent Pinchart
  2020-07-25 21:18         ` Sakari Ailus
  0 siblings, 1 reply; 46+ messages in thread
From: Laurent Pinchart @ 2020-07-23 23:15 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Jacopo Mondi, robh+dt, devicetree, slongerbeam, linux-media,
	linux-renesas-soc, Maxime Coquelin, Alexandre Torgue,
	moderated list:ARM/STM32 ARCHITECTURE

Hi Sakari,

On Fri, Jul 24, 2020 at 01:22:59AM +0300, Sakari Ailus wrote:
> On Fri, Jul 17, 2020 at 11:57:22PM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> > 
> > (CC'ing Sakari)
> > 
> > Thank you for the patch.
> > 
> > On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote:
> > > The value of the data-shift property solely depend on the selected
> > > bus width and it's not freely configurable.
> > > 
> > > Remove it from the bindings document and update its users accordingly.
> > 
> > Hmmmm that's an interesting one. Sakari, what do you think ?
> > 
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > >  Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 ---------
> > >  arch/arm/boot/dts/stm32mp157c-ev1.dts                   | 1 -
> > >  2 files changed, 10 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > index 5e1662e848bd..ab700a1830aa 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > @@ -92,12 +92,6 @@ properties:
> > >                parallel bus.
> > >              enum: [8, 10]
> > > 
> > > -          data-shift:
> > > -            description: |
> > > -              Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or
> > > -              <0> for 10 bits parallel bus.
> > > -            enum: [0, 2]
> > 
> > Should you document in the description of bus-width that data-shift is
> > implied ?
> 
> The purpose of the datas-shift property is to convey how the parallel bus
> lines are connected for a given bus width for devices where it is
> configurable. As this device does not not support that, then indeed this
> property is not relevant for the device IMO.

Could you elaborate on this ? I believe the case that Jacopo is
describing connects D[9:2] from the sensor to D[7:0] of the receiver
(Jacopo, could you confirm ?). Isn't that what data-shift is for ?

> > > -
> > >            hsync-active:
> > >              enum: [0, 1]
> > > 
> > > @@ -115,7 +109,6 @@ properties:
> > >              then:
> > >                  properties:
> > >                    bus-width: false
> > > -                  data-shift: false
> > >                    hsync-active: false
> > >                    vsync-active: false
> > >                    pclk-sample: false
> > > @@ -135,7 +128,6 @@ properties:
> > >                  - remote-endpoint
> > >                  - bus-type
> > >                  - bus-width
> > > -                - data-shift
> > >                  - hsync-active
> > >                  - vsync-active
> > >                  - pclk-sample
> > > @@ -204,7 +196,6 @@ examples:
> > >                      remote-endpoint = <&parallel_from_ov5640>;
> > >                      bus-type = <5>;
> > >                      bus-width = <10>;
> > > -                    data-shift = <0>;
> > >                      hsync-active = <1>;
> > >                      vsync-active = <1>;
> > >                      pclk-sample = <1>;
> > > diff --git a/arch/arm/boot/dts/stm32mp157c-ev1.dts b/arch/arm/boot/dts/stm32mp157c-ev1.dts
> > > index 613ede73b65b..96f96202ca63 100644
> > > --- a/arch/arm/boot/dts/stm32mp157c-ev1.dts
> > > +++ b/arch/arm/boot/dts/stm32mp157c-ev1.dts
> > > @@ -192,7 +192,6 @@ ov5640_0: endpoint {
> > >  				remote-endpoint = <&dcmi_0>;
> > >  				bus-type = <5>;
> > >  				bus-width = <8>;
> > > -				data-shift = <2>; /* lines 9:2 are used */
> > >  				hsync-active = <0>;
> > >  				vsync-active = <0>;
> > >  				pclk-sample = <1>;

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 08/13] dt-bindings: media: ov5640: Remove data-shift
  2020-07-23 23:15       ` Laurent Pinchart
@ 2020-07-25 21:18         ` Sakari Ailus
  2020-07-25 21:31           ` Laurent Pinchart
  0 siblings, 1 reply; 46+ messages in thread
From: Sakari Ailus @ 2020-07-25 21:18 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Jacopo Mondi, robh+dt, devicetree, slongerbeam, linux-media,
	linux-renesas-soc, Maxime Coquelin, Alexandre Torgue,
	moderated list:ARM/STM32 ARCHITECTURE

Hi Laurent,

On Fri, Jul 24, 2020 at 02:15:49AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Fri, Jul 24, 2020 at 01:22:59AM +0300, Sakari Ailus wrote:
> > On Fri, Jul 17, 2020 at 11:57:22PM +0300, Laurent Pinchart wrote:
> > > Hi Jacopo,
> > > 
> > > (CC'ing Sakari)
> > > 
> > > Thank you for the patch.
> > > 
> > > On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote:
> > > > The value of the data-shift property solely depend on the selected
> > > > bus width and it's not freely configurable.
> > > > 
> > > > Remove it from the bindings document and update its users accordingly.
> > > 
> > > Hmmmm that's an interesting one. Sakari, what do you think ?
> > > 
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 ---------
> > > >  arch/arm/boot/dts/stm32mp157c-ev1.dts                   | 1 -
> > > >  2 files changed, 10 deletions(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > index 5e1662e848bd..ab700a1830aa 100644
> > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > @@ -92,12 +92,6 @@ properties:
> > > >                parallel bus.
> > > >              enum: [8, 10]
> > > > 
> > > > -          data-shift:
> > > > -            description: |
> > > > -              Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or
> > > > -              <0> for 10 bits parallel bus.
> > > > -            enum: [0, 2]
> > > 
> > > Should you document in the description of bus-width that data-shift is
> > > implied ?
> > 
> > The purpose of the datas-shift property is to convey how the parallel bus
> > lines are connected for a given bus width for devices where it is
> > configurable. As this device does not not support that, then indeed this
> > property is not relevant for the device IMO.
> 
> Could you elaborate on this ? I believe the case that Jacopo is
> describing connects D[9:2] from the sensor to D[7:0] of the receiver
> (Jacopo, could you confirm ?). Isn't that what data-shift is for ?

Yes, it is. But in this case what data-shift configures is not configurable
as such but defined by another configuration, making the data-shift
property redundant. We generally haven't documented redundant things in DT
bindings --- for instance data-lanes is documented in bindings only if it
is configurable.

That said, it'd be nice to say which pins are used on less than full width
busses.

-- 
Regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 08/13] dt-bindings: media: ov5640: Remove data-shift
  2020-07-25 21:18         ` Sakari Ailus
@ 2020-07-25 21:31           ` Laurent Pinchart
  2020-07-29 14:29             ` Sakari Ailus
  0 siblings, 1 reply; 46+ messages in thread
From: Laurent Pinchart @ 2020-07-25 21:31 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Jacopo Mondi, robh+dt, devicetree, slongerbeam, linux-media,
	linux-renesas-soc, Maxime Coquelin, Alexandre Torgue,
	moderated list:ARM/STM32 ARCHITECTURE

Hi Sakari,

On Sun, Jul 26, 2020 at 12:18:33AM +0300, Sakari Ailus wrote:
> On Fri, Jul 24, 2020 at 02:15:49AM +0300, Laurent Pinchart wrote:
> > On Fri, Jul 24, 2020 at 01:22:59AM +0300, Sakari Ailus wrote:
> > > On Fri, Jul 17, 2020 at 11:57:22PM +0300, Laurent Pinchart wrote:
> > > > Hi Jacopo,
> > > > 
> > > > (CC'ing Sakari)
> > > > 
> > > > Thank you for the patch.
> > > > 
> > > > On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote:
> > > > > The value of the data-shift property solely depend on the selected
> > > > > bus width and it's not freely configurable.
> > > > > 
> > > > > Remove it from the bindings document and update its users accordingly.
> > > > 
> > > > Hmmmm that's an interesting one. Sakari, what do you think ?
> > > > 
> > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 ---------
> > > > >  arch/arm/boot/dts/stm32mp157c-ev1.dts                   | 1 -
> > > > >  2 files changed, 10 deletions(-)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > > index 5e1662e848bd..ab700a1830aa 100644
> > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > > @@ -92,12 +92,6 @@ properties:
> > > > >                parallel bus.
> > > > >              enum: [8, 10]
> > > > > 
> > > > > -          data-shift:
> > > > > -            description: |
> > > > > -              Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or
> > > > > -              <0> for 10 bits parallel bus.
> > > > > -            enum: [0, 2]
> > > > 
> > > > Should you document in the description of bus-width that data-shift is
> > > > implied ?
> > > 
> > > The purpose of the datas-shift property is to convey how the parallel bus
> > > lines are connected for a given bus width for devices where it is
> > > configurable. As this device does not not support that, then indeed this
> > > property is not relevant for the device IMO.
> > 
> > Could you elaborate on this ? I believe the case that Jacopo is
> > describing connects D[9:2] from the sensor to D[7:0] of the receiver
> > (Jacopo, could you confirm ?). Isn't that what data-shift is for ?
> 
> Yes, it is. But in this case what data-shift configures is not configurable
> as such but defined by another configuration, making the data-shift
> property redundant. We generally haven't documented redundant things in DT
> bindings --- for instance data-lanes is documented in bindings only if it
> is configurable.

Then I think we share the same understanding. I believe the
documentation in video-interfaces.txt needs to be expanded, as it's
quite terse and not very clear.

> That said, it'd be nice to say which pins are used on less than full width
> busses.

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 08/13] dt-bindings: media: ov5640: Remove data-shift
  2020-07-25 21:31           ` Laurent Pinchart
@ 2020-07-29 14:29             ` Sakari Ailus
  2020-07-29 14:46               ` Laurent Pinchart
  0 siblings, 1 reply; 46+ messages in thread
From: Sakari Ailus @ 2020-07-29 14:29 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Jacopo Mondi, robh+dt, devicetree, slongerbeam, linux-media,
	linux-renesas-soc, Maxime Coquelin, Alexandre Torgue,
	moderated list:ARM/STM32 ARCHITECTURE

On Sun, Jul 26, 2020 at 12:31:25AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Sun, Jul 26, 2020 at 12:18:33AM +0300, Sakari Ailus wrote:
> > On Fri, Jul 24, 2020 at 02:15:49AM +0300, Laurent Pinchart wrote:
> > > On Fri, Jul 24, 2020 at 01:22:59AM +0300, Sakari Ailus wrote:
> > > > On Fri, Jul 17, 2020 at 11:57:22PM +0300, Laurent Pinchart wrote:
> > > > > Hi Jacopo,
> > > > > 
> > > > > (CC'ing Sakari)
> > > > > 
> > > > > Thank you for the patch.
> > > > > 
> > > > > On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote:
> > > > > > The value of the data-shift property solely depend on the selected
> > > > > > bus width and it's not freely configurable.
> > > > > > 
> > > > > > Remove it from the bindings document and update its users accordingly.
> > > > > 
> > > > > Hmmmm that's an interesting one. Sakari, what do you think ?
> > > > > 
> > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 ---------
> > > > > >  arch/arm/boot/dts/stm32mp157c-ev1.dts                   | 1 -
> > > > > >  2 files changed, 10 deletions(-)
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > > > index 5e1662e848bd..ab700a1830aa 100644
> > > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > > > @@ -92,12 +92,6 @@ properties:
> > > > > >                parallel bus.
> > > > > >              enum: [8, 10]
> > > > > > 
> > > > > > -          data-shift:
> > > > > > -            description: |
> > > > > > -              Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or
> > > > > > -              <0> for 10 bits parallel bus.
> > > > > > -            enum: [0, 2]
> > > > > 
> > > > > Should you document in the description of bus-width that data-shift is
> > > > > implied ?
> > > > 
> > > > The purpose of the datas-shift property is to convey how the parallel bus
> > > > lines are connected for a given bus width for devices where it is
> > > > configurable. As this device does not not support that, then indeed this
> > > > property is not relevant for the device IMO.
> > > 
> > > Could you elaborate on this ? I believe the case that Jacopo is
> > > describing connects D[9:2] from the sensor to D[7:0] of the receiver
> > > (Jacopo, could you confirm ?). Isn't that what data-shift is for ?
> > 
> > Yes, it is. But in this case what data-shift configures is not configurable
> > as such but defined by another configuration, making the data-shift
> > property redundant. We generally haven't documented redundant things in DT
> > bindings --- for instance data-lanes is documented in bindings only if it
> > is configurable.
> 
> Then I think we share the same understanding. I believe the
> documentation in video-interfaces.txt needs to be expanded, as it's
> quite terse and not very clear.

The DT spec states that:

	A DTSpec-compliant devicetree describes device information in a
	system that cannot necessarily be dynamically detected by a client
	program. For example, the architecture of PCI enables a client to
	probe and detect attached devices, and thus devicetree nodes
	describing PCI devices might not be required. However, a device
	node is required to describe a PCI host bridge device in the system
	if it cannot be detected by probing.

I'd read that as there's no need to specify properties that do not provide
additional information to software. As some properties are dependent on
others and and this depends on hardware features, I don't think we can in
general case take this account in generic binding documentation, but device
specific ones.

Of course we could add this to data-shift documentation, but then I wonder
how many other similar cases there are where in hardware the configuration
defined by one property determines the value of another?

-- 
Regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 08/13] dt-bindings: media: ov5640: Remove data-shift
  2020-07-29 14:29             ` Sakari Ailus
@ 2020-07-29 14:46               ` Laurent Pinchart
  2020-07-30 16:22                 ` Sakari Ailus
  0 siblings, 1 reply; 46+ messages in thread
From: Laurent Pinchart @ 2020-07-29 14:46 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Jacopo Mondi, robh+dt, devicetree, slongerbeam, linux-media,
	linux-renesas-soc, Maxime Coquelin, Alexandre Torgue,
	moderated list:ARM/STM32 ARCHITECTURE

Hi Sakari,

On Wed, Jul 29, 2020 at 05:29:36PM +0300, Sakari Ailus wrote:
> On Sun, Jul 26, 2020 at 12:31:25AM +0300, Laurent Pinchart wrote:
> > On Sun, Jul 26, 2020 at 12:18:33AM +0300, Sakari Ailus wrote:
> > > On Fri, Jul 24, 2020 at 02:15:49AM +0300, Laurent Pinchart wrote:
> > > > On Fri, Jul 24, 2020 at 01:22:59AM +0300, Sakari Ailus wrote:
> > > > > On Fri, Jul 17, 2020 at 11:57:22PM +0300, Laurent Pinchart wrote:
> > > > > > Hi Jacopo,
> > > > > > 
> > > > > > (CC'ing Sakari)
> > > > > > 
> > > > > > Thank you for the patch.
> > > > > > 
> > > > > > On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote:
> > > > > > > The value of the data-shift property solely depend on the selected
> > > > > > > bus width and it's not freely configurable.
> > > > > > > 
> > > > > > > Remove it from the bindings document and update its users accordingly.
> > > > > > 
> > > > > > Hmmmm that's an interesting one. Sakari, what do you think ?
> > > > > > 
> > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > > > ---
> > > > > > >  Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 ---------
> > > > > > >  arch/arm/boot/dts/stm32mp157c-ev1.dts                   | 1 -
> > > > > > >  2 files changed, 10 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > > > > index 5e1662e848bd..ab700a1830aa 100644
> > > > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > > > > @@ -92,12 +92,6 @@ properties:
> > > > > > >                parallel bus.
> > > > > > >              enum: [8, 10]
> > > > > > > 
> > > > > > > -          data-shift:
> > > > > > > -            description: |
> > > > > > > -              Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or
> > > > > > > -              <0> for 10 bits parallel bus.
> > > > > > > -            enum: [0, 2]
> > > > > > 
> > > > > > Should you document in the description of bus-width that data-shift is
> > > > > > implied ?
> > > > > 
> > > > > The purpose of the datas-shift property is to convey how the parallel bus
> > > > > lines are connected for a given bus width for devices where it is
> > > > > configurable. As this device does not not support that, then indeed this
> > > > > property is not relevant for the device IMO.
> > > > 
> > > > Could you elaborate on this ? I believe the case that Jacopo is
> > > > describing connects D[9:2] from the sensor to D[7:0] of the receiver
> > > > (Jacopo, could you confirm ?). Isn't that what data-shift is for ?
> > > 
> > > Yes, it is. But in this case what data-shift configures is not configurable
> > > as such but defined by another configuration, making the data-shift
> > > property redundant. We generally haven't documented redundant things in DT
> > > bindings --- for instance data-lanes is documented in bindings only if it
> > > is configurable.
> > 
> > Then I think we share the same understanding. I believe the
> > documentation in video-interfaces.txt needs to be expanded, as it's
> > quite terse and not very clear.
> 
> The DT spec states that:
> 
> 	A DTSpec-compliant devicetree describes device information in a
> 	system that cannot necessarily be dynamically detected by a client
> 	program. For example, the architecture of PCI enables a client to
> 	probe and detect attached devices, and thus devicetree nodes
> 	describing PCI devices might not be required. However, a device
> 	node is required to describe a PCI host bridge device in the system
> 	if it cannot be detected by probing.
> 
> I'd read that as there's no need to specify properties that do not provide
> additional information to software.

That's a bit of a stretch interpretation :-)

> As some properties are dependent on
> others and and this depends on hardware features, I don't think we can in
> general case take this account in generic binding documentation, but device
> specific ones.
> 
> Of course we could add this to data-shift documentation, but then I wonder
> how many other similar cases there are where in hardware the configuration
> defined by one property determines the value of another?

I was mostly thinking about documenting *how* data-shift interacts with
bus-width. I think that specifying the default data-shift value based on
the bus-width value, for the case where data-shift is not specified,
would also make sense.

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 08/13] dt-bindings: media: ov5640: Remove data-shift
  2020-07-29 14:46               ` Laurent Pinchart
@ 2020-07-30 16:22                 ` Sakari Ailus
  2020-07-30 16:32                   ` Laurent Pinchart
  0 siblings, 1 reply; 46+ messages in thread
From: Sakari Ailus @ 2020-07-30 16:22 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Jacopo Mondi, robh+dt, devicetree, slongerbeam, linux-media,
	linux-renesas-soc, Maxime Coquelin, Alexandre Torgue,
	moderated list:ARM/STM32 ARCHITECTURE

Hi Laurent,

On Wed, Jul 29, 2020 at 05:46:08PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wed, Jul 29, 2020 at 05:29:36PM +0300, Sakari Ailus wrote:
> > On Sun, Jul 26, 2020 at 12:31:25AM +0300, Laurent Pinchart wrote:
> > > On Sun, Jul 26, 2020 at 12:18:33AM +0300, Sakari Ailus wrote:
> > > > On Fri, Jul 24, 2020 at 02:15:49AM +0300, Laurent Pinchart wrote:
> > > > > On Fri, Jul 24, 2020 at 01:22:59AM +0300, Sakari Ailus wrote:
> > > > > > On Fri, Jul 17, 2020 at 11:57:22PM +0300, Laurent Pinchart wrote:
> > > > > > > Hi Jacopo,
> > > > > > > 
> > > > > > > (CC'ing Sakari)
> > > > > > > 
> > > > > > > Thank you for the patch.
> > > > > > > 
> > > > > > > On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote:
> > > > > > > > The value of the data-shift property solely depend on the selected
> > > > > > > > bus width and it's not freely configurable.
> > > > > > > > 
> > > > > > > > Remove it from the bindings document and update its users accordingly.
> > > > > > > 
> > > > > > > Hmmmm that's an interesting one. Sakari, what do you think ?
> > > > > > > 
> > > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > > > > ---
> > > > > > > >  Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 ---------
> > > > > > > >  arch/arm/boot/dts/stm32mp157c-ev1.dts                   | 1 -
> > > > > > > >  2 files changed, 10 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > > > > > index 5e1662e848bd..ab700a1830aa 100644
> > > > > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > > > > > @@ -92,12 +92,6 @@ properties:
> > > > > > > >                parallel bus.
> > > > > > > >              enum: [8, 10]
> > > > > > > > 
> > > > > > > > -          data-shift:
> > > > > > > > -            description: |
> > > > > > > > -              Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or
> > > > > > > > -              <0> for 10 bits parallel bus.
> > > > > > > > -            enum: [0, 2]
> > > > > > > 
> > > > > > > Should you document in the description of bus-width that data-shift is
> > > > > > > implied ?
> > > > > > 
> > > > > > The purpose of the datas-shift property is to convey how the parallel bus
> > > > > > lines are connected for a given bus width for devices where it is
> > > > > > configurable. As this device does not not support that, then indeed this
> > > > > > property is not relevant for the device IMO.
> > > > > 
> > > > > Could you elaborate on this ? I believe the case that Jacopo is
> > > > > describing connects D[9:2] from the sensor to D[7:0] of the receiver
> > > > > (Jacopo, could you confirm ?). Isn't that what data-shift is for ?
> > > > 
> > > > Yes, it is. But in this case what data-shift configures is not configurable
> > > > as such but defined by another configuration, making the data-shift
> > > > property redundant. We generally haven't documented redundant things in DT
> > > > bindings --- for instance data-lanes is documented in bindings only if it
> > > > is configurable.
> > > 
> > > Then I think we share the same understanding. I believe the
> > > documentation in video-interfaces.txt needs to be expanded, as it's
> > > quite terse and not very clear.
> > 
> > The DT spec states that:
> > 
> > 	A DTSpec-compliant devicetree describes device information in a
> > 	system that cannot necessarily be dynamically detected by a client
> > 	program. For example, the architecture of PCI enables a client to
> > 	probe and detect attached devices, and thus devicetree nodes
> > 	describing PCI devices might not be required. However, a device
> > 	node is required to describe a PCI host bridge device in the system
> > 	if it cannot be detected by probing.
> > 
> > I'd read that as there's no need to specify properties that do not provide
> > additional information to software.
> 
> That's a bit of a stretch interpretation :-)
> 
> > As some properties are dependent on
> > others and and this depends on hardware features, I don't think we can in
> > general case take this account in generic binding documentation, but device
> > specific ones.
> > 
> > Of course we could add this to data-shift documentation, but then I wonder
> > how many other similar cases there are where in hardware the configuration
> > defined by one property determines the value of another?
> 
> I was mostly thinking about documenting *how* data-shift interacts with
> bus-width. I think that specifying the default data-shift value based on
> the bus-width value, for the case where data-shift is not specified,
> would also make sense.

Do you mean in device binding documentation or in generic documentation?
Device bindings should have this information, yes.

-- 
Regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 08/13] dt-bindings: media: ov5640: Remove data-shift
  2020-07-30 16:22                 ` Sakari Ailus
@ 2020-07-30 16:32                   ` Laurent Pinchart
  2020-07-30 16:43                     ` Sakari Ailus
  0 siblings, 1 reply; 46+ messages in thread
From: Laurent Pinchart @ 2020-07-30 16:32 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Jacopo Mondi, robh+dt, devicetree, slongerbeam, linux-media,
	linux-renesas-soc, Maxime Coquelin, Alexandre Torgue,
	moderated list:ARM/STM32 ARCHITECTURE

Hi Sakari,

On Thu, Jul 30, 2020 at 07:22:19PM +0300, Sakari Ailus wrote:
> On Wed, Jul 29, 2020 at 05:46:08PM +0300, Laurent Pinchart wrote:
> > On Wed, Jul 29, 2020 at 05:29:36PM +0300, Sakari Ailus wrote:
> > > On Sun, Jul 26, 2020 at 12:31:25AM +0300, Laurent Pinchart wrote:
> > > > On Sun, Jul 26, 2020 at 12:18:33AM +0300, Sakari Ailus wrote:
> > > > > On Fri, Jul 24, 2020 at 02:15:49AM +0300, Laurent Pinchart wrote:
> > > > > > On Fri, Jul 24, 2020 at 01:22:59AM +0300, Sakari Ailus wrote:
> > > > > > > On Fri, Jul 17, 2020 at 11:57:22PM +0300, Laurent Pinchart wrote:
> > > > > > > > Hi Jacopo,
> > > > > > > > 
> > > > > > > > (CC'ing Sakari)
> > > > > > > > 
> > > > > > > > Thank you for the patch.
> > > > > > > > 
> > > > > > > > On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote:
> > > > > > > > > The value of the data-shift property solely depend on the selected
> > > > > > > > > bus width and it's not freely configurable.
> > > > > > > > > 
> > > > > > > > > Remove it from the bindings document and update its users accordingly.
> > > > > > > > 
> > > > > > > > Hmmmm that's an interesting one. Sakari, what do you think ?
> > > > > > > > 
> > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > > > > > ---
> > > > > > > > >  Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 ---------
> > > > > > > > >  arch/arm/boot/dts/stm32mp157c-ev1.dts                   | 1 -
> > > > > > > > >  2 files changed, 10 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > > > > > > index 5e1662e848bd..ab700a1830aa 100644
> > > > > > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > > > > > > @@ -92,12 +92,6 @@ properties:
> > > > > > > > >                parallel bus.
> > > > > > > > >              enum: [8, 10]
> > > > > > > > > 
> > > > > > > > > -          data-shift:
> > > > > > > > > -            description: |
> > > > > > > > > -              Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or
> > > > > > > > > -              <0> for 10 bits parallel bus.
> > > > > > > > > -            enum: [0, 2]
> > > > > > > > 
> > > > > > > > Should you document in the description of bus-width that data-shift is
> > > > > > > > implied ?
> > > > > > > 
> > > > > > > The purpose of the datas-shift property is to convey how the parallel bus
> > > > > > > lines are connected for a given bus width for devices where it is
> > > > > > > configurable. As this device does not not support that, then indeed this
> > > > > > > property is not relevant for the device IMO.
> > > > > > 
> > > > > > Could you elaborate on this ? I believe the case that Jacopo is
> > > > > > describing connects D[9:2] from the sensor to D[7:0] of the receiver
> > > > > > (Jacopo, could you confirm ?). Isn't that what data-shift is for ?
> > > > > 
> > > > > Yes, it is. But in this case what data-shift configures is not configurable
> > > > > as such but defined by another configuration, making the data-shift
> > > > > property redundant. We generally haven't documented redundant things in DT
> > > > > bindings --- for instance data-lanes is documented in bindings only if it
> > > > > is configurable.
> > > > 
> > > > Then I think we share the same understanding. I believe the
> > > > documentation in video-interfaces.txt needs to be expanded, as it's
> > > > quite terse and not very clear.
> > > 
> > > The DT spec states that:
> > > 
> > > 	A DTSpec-compliant devicetree describes device information in a
> > > 	system that cannot necessarily be dynamically detected by a client
> > > 	program. For example, the architecture of PCI enables a client to
> > > 	probe and detect attached devices, and thus devicetree nodes
> > > 	describing PCI devices might not be required. However, a device
> > > 	node is required to describe a PCI host bridge device in the system
> > > 	if it cannot be detected by probing.
> > > 
> > > I'd read that as there's no need to specify properties that do not provide
> > > additional information to software.
> > 
> > That's a bit of a stretch interpretation :-)
> > 
> > > As some properties are dependent on
> > > others and and this depends on hardware features, I don't think we can in
> > > general case take this account in generic binding documentation, but device
> > > specific ones.
> > > 
> > > Of course we could add this to data-shift documentation, but then I wonder
> > > how many other similar cases there are where in hardware the configuration
> > > defined by one property determines the value of another?
> > 
> > I was mostly thinking about documenting *how* data-shift interacts with
> > bus-width. I think that specifying the default data-shift value based on
> > the bus-width value, for the case where data-shift is not specified,
> > would also make sense.
> 
> Do you mean in device binding documentation or in generic documentation?
> Device bindings should have this information, yes.

I mean in video-interfaces.txt (which should become
video-interfaces.yaml :-)) for the general rules, and in specific
bindings for any device-specific rule.

We will likely need a runtime API too, it's entirely conceivable that a
10-bit parallel sensor, which D[9:0] signals, could use either D[9:2] or
D[7:0] when configured to transmit 10-bit data. This isn't something
that can be encoded in DT. It's a separate topic though.

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH 08/13] dt-bindings: media: ov5640: Remove data-shift
  2020-07-30 16:32                   ` Laurent Pinchart
@ 2020-07-30 16:43                     ` Sakari Ailus
  0 siblings, 0 replies; 46+ messages in thread
From: Sakari Ailus @ 2020-07-30 16:43 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Jacopo Mondi, robh+dt, devicetree, slongerbeam, linux-media,
	linux-renesas-soc, Maxime Coquelin, Alexandre Torgue,
	moderated list:ARM/STM32 ARCHITECTURE

Hi Laurent,

On Thu, Jul 30, 2020 at 07:32:11PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Thu, Jul 30, 2020 at 07:22:19PM +0300, Sakari Ailus wrote:
> > On Wed, Jul 29, 2020 at 05:46:08PM +0300, Laurent Pinchart wrote:
> > > On Wed, Jul 29, 2020 at 05:29:36PM +0300, Sakari Ailus wrote:
> > > > On Sun, Jul 26, 2020 at 12:31:25AM +0300, Laurent Pinchart wrote:
> > > > > On Sun, Jul 26, 2020 at 12:18:33AM +0300, Sakari Ailus wrote:
> > > > > > On Fri, Jul 24, 2020 at 02:15:49AM +0300, Laurent Pinchart wrote:
> > > > > > > On Fri, Jul 24, 2020 at 01:22:59AM +0300, Sakari Ailus wrote:
> > > > > > > > On Fri, Jul 17, 2020 at 11:57:22PM +0300, Laurent Pinchart wrote:
> > > > > > > > > Hi Jacopo,
> > > > > > > > > 
> > > > > > > > > (CC'ing Sakari)
> > > > > > > > > 
> > > > > > > > > Thank you for the patch.
> > > > > > > > > 
> > > > > > > > > On Fri, Jul 17, 2020 at 03:28:54PM +0200, Jacopo Mondi wrote:
> > > > > > > > > > The value of the data-shift property solely depend on the selected
> > > > > > > > > > bus width and it's not freely configurable.
> > > > > > > > > > 
> > > > > > > > > > Remove it from the bindings document and update its users accordingly.
> > > > > > > > > 
> > > > > > > > > Hmmmm that's an interesting one. Sakari, what do you think ?
> > > > > > > > > 
> > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > > > > > > > > ---
> > > > > > > > > >  Documentation/devicetree/bindings/media/i2c/ov5640.yaml | 9 ---------
> > > > > > > > > >  arch/arm/boot/dts/stm32mp157c-ev1.dts                   | 1 -
> > > > > > > > > >  2 files changed, 10 deletions(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > > > > > > > index 5e1662e848bd..ab700a1830aa 100644
> > > > > > > > > > --- a/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.yaml
> > > > > > > > > > @@ -92,12 +92,6 @@ properties:
> > > > > > > > > >                parallel bus.
> > > > > > > > > >              enum: [8, 10]
> > > > > > > > > > 
> > > > > > > > > > -          data-shift:
> > > > > > > > > > -            description: |
> > > > > > > > > > -              Shall be set to <2> for 8 bits parallel bus (lines 9:2 are used) or
> > > > > > > > > > -              <0> for 10 bits parallel bus.
> > > > > > > > > > -            enum: [0, 2]
> > > > > > > > > 
> > > > > > > > > Should you document in the description of bus-width that data-shift is
> > > > > > > > > implied ?
> > > > > > > > 
> > > > > > > > The purpose of the datas-shift property is to convey how the parallel bus
> > > > > > > > lines are connected for a given bus width for devices where it is
> > > > > > > > configurable. As this device does not not support that, then indeed this
> > > > > > > > property is not relevant for the device IMO.
> > > > > > > 
> > > > > > > Could you elaborate on this ? I believe the case that Jacopo is
> > > > > > > describing connects D[9:2] from the sensor to D[7:0] of the receiver
> > > > > > > (Jacopo, could you confirm ?). Isn't that what data-shift is for ?
> > > > > > 
> > > > > > Yes, it is. But in this case what data-shift configures is not configurable
> > > > > > as such but defined by another configuration, making the data-shift
> > > > > > property redundant. We generally haven't documented redundant things in DT
> > > > > > bindings --- for instance data-lanes is documented in bindings only if it
> > > > > > is configurable.
> > > > > 
> > > > > Then I think we share the same understanding. I believe the
> > > > > documentation in video-interfaces.txt needs to be expanded, as it's
> > > > > quite terse and not very clear.
> > > > 
> > > > The DT spec states that:
> > > > 
> > > > 	A DTSpec-compliant devicetree describes device information in a
> > > > 	system that cannot necessarily be dynamically detected by a client
> > > > 	program. For example, the architecture of PCI enables a client to
> > > > 	probe and detect attached devices, and thus devicetree nodes
> > > > 	describing PCI devices might not be required. However, a device
> > > > 	node is required to describe a PCI host bridge device in the system
> > > > 	if it cannot be detected by probing.
> > > > 
> > > > I'd read that as there's no need to specify properties that do not provide
> > > > additional information to software.
> > > 
> > > That's a bit of a stretch interpretation :-)
> > > 
> > > > As some properties are dependent on
> > > > others and and this depends on hardware features, I don't think we can in
> > > > general case take this account in generic binding documentation, but device
> > > > specific ones.
> > > > 
> > > > Of course we could add this to data-shift documentation, but then I wonder
> > > > how many other similar cases there are where in hardware the configuration
> > > > defined by one property determines the value of another?
> > > 
> > > I was mostly thinking about documenting *how* data-shift interacts with
> > > bus-width. I think that specifying the default data-shift value based on
> > > the bus-width value, for the case where data-shift is not specified,
> > > would also make sense.
> > 
> > Do you mean in device binding documentation or in generic documentation?
> > Device bindings should have this information, yes.
> 
> I mean in video-interfaces.txt (which should become
> video-interfaces.yaml :-)) for the general rules, and in specific
> bindings for any device-specific rule.

Please send a patch. :-)

> 
> We will likely need a runtime API too, it's entirely conceivable that a
> 10-bit parallel sensor, which D[9:0] signals, could use either D[9:2] or
> D[7:0] when configured to transmit 10-bit data. This isn't something
> that can be encoded in DT. It's a separate topic though.

You can set defaults in the current API but those defaults are basically a
C struct. I don't think we should be looking into making those defaults
depend on property values, unless there's a clear reason to do so --- and
in this case there isn't one.

-- 
Kind regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 46+ messages in thread

end of thread, back to index

Thread overview: 46+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-17 13:28 [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema Jacopo Mondi
2020-07-17 13:28 ` [PATCH 01/13] " Jacopo Mondi
2020-07-17 19:28   ` Laurent Pinchart
2020-07-20 16:32   ` Rob Herring
2020-07-17 13:28 ` [PATCH 02/13] arm64: dts: qcom: apq8016-sbc: Fix CSI-2 lanes routing Jacopo Mondi
2020-07-17 19:35   ` Laurent Pinchart
2020-07-22  8:14     ` Loic Poulain
2020-07-22  8:34       ` Jacopo Mondi
2020-07-17 13:28 ` [PATCH 03/13] dt-bindings: media: ov5640: Do not limit rotation Jacopo Mondi
2020-07-17 19:38   ` Laurent Pinchart
2020-07-20 19:36     ` Rob Herring
2020-07-17 13:28 ` [PATCH 04/13] dt-bindings: media: ti,cal: Remove sensor from example Jacopo Mondi
2020-07-17 19:39   ` Laurent Pinchart
2020-07-23 20:57   ` Rob Herring
2020-07-17 13:28 ` [PATCH 05/13] dt-bindings: media: ov5640: Make bus-type mandatory Jacopo Mondi
2020-07-17 15:48   ` Fabio Estevam
2020-07-17 19:48   ` Laurent Pinchart
2020-07-20 19:45   ` Rob Herring
2020-07-17 13:28 ` [PATCH 06/13] dt-bindings: media: ov5640: Require ep properties Jacopo Mondi
2020-07-17 19:50   ` Laurent Pinchart
2020-07-17 13:28 ` [PATCH 07/13] dt-bindings: media: ov5640: Remove clock-lanes Jacopo Mondi
2020-07-17 20:48   ` Laurent Pinchart
2020-07-20 19:47   ` Rob Herring
2020-07-17 13:28 ` [PATCH 08/13] dt-bindings: media: ov5640: Remove data-shift Jacopo Mondi
2020-07-17 20:57   ` Laurent Pinchart
2020-07-23 22:22     ` Sakari Ailus
2020-07-23 23:15       ` Laurent Pinchart
2020-07-25 21:18         ` Sakari Ailus
2020-07-25 21:31           ` Laurent Pinchart
2020-07-29 14:29             ` Sakari Ailus
2020-07-29 14:46               ` Laurent Pinchart
2020-07-30 16:22                 ` Sakari Ailus
2020-07-30 16:32                   ` Laurent Pinchart
2020-07-30 16:43                     ` Sakari Ailus
2020-07-17 13:28 ` [PATCH 09/13] dt-bindings: media: ov5640: Add default for synch signals Jacopo Mondi
2020-07-17 20:59   ` Laurent Pinchart
2020-07-20 19:50     ` Rob Herring
2020-07-17 13:28 ` [PATCH 10/13] dt-bindings: media: ov5640: Demote DVP required properties Jacopo Mondi
2020-07-17 21:00   ` Laurent Pinchart
2020-07-17 13:28 ` [PATCH 11/13] media: i2c: ov5640: Initialize DVP polarities as default Jacopo Mondi
2020-07-17 21:02   ` Laurent Pinchart
2020-07-17 13:28 ` [PATCH 12/13] MAINTAINERS: ov5640: Add bindings file Jacopo Mondi
2020-07-17 21:03   ` Laurent Pinchart
2020-07-17 13:28 ` [PATCH 13/13] dt-bindings: media: ov5640: Add myself as co-maintainer Jacopo Mondi
2020-07-17 21:03   ` Laurent Pinchart
2020-07-17 19:31 ` [PATCH 00/13] dt-bindings: media: ov5640: Convert to json-schema Laurent Pinchart

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git