Hi Sakari and thanks for the review!

On Tue 03 Nov 20, 01:24, Sakari Ailus wrote:

On Fri, Oct 23, 2020 at 07:54:04PM +0200, Paul Kocialkowski wrote:

quoted

This introduces YAML bindings documentation for the OV8865
image sensor.

Co-developed-by: Kévin L'hôpital <redacted>
Signed-off-by: Kévin L'hôpital <redacted>
Signed-off-by: Paul Kocialkowski <redacted>
---
 .../bindings/media/i2c/ovti,ov8865.yaml       | 124 ++++++++++++++++++
 1 file changed, 124 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml

diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml
new file mode 100644
index 000000000000..807f1a94afae
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml

@@ -0,0 +1,124 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/ovti,ov8865.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: OmniVision OV8865 Image Sensor Device Tree Bindings
+
+maintainers:
+  - Paul Kocialkowski <paul.kocialkowski@bootlin.com>
+
+properties:
+  compatible:
+    const: ovti,ov8865
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: EXTCLK Clock
+
+  clock-names:
+    items:
+      - const: extclk

Is this needed with a single clock?

Yes I think so: we grab the clock with devm_clk_get which takes a name string
that matches the clock-names property.

And... shouldn't this also come with assigned-clock-rates etc., to set the
clock frequency?

I'm a bit confused why we would need to do that in the device-tree rather than
setting the clock rate with clk_set_rate in the driver, like any other driver
does. I think this was discussed before (on the initial ov8865 series) and the
conclusion was that there is no particular reason for media i2c drivers to
behave differently. So I believe this is the correct approach.

quoted

+
+  dvdd-supply:
+    description: Digital Domain Power Supply
+
+  avdd-supply:
+    description: Analog Domain Power Supply (internal AVDD is used if missing)
+
+  dovdd-supply:
+    description: I/O Domain Power Supply
+
+  powerdown-gpios:
+    maxItems: 1
+    description: Power Down Pin GPIO Control (active low)
+
+  reset-gpios:
+    maxItems: 1
+    description: Reset Pin GPIO Control (active low)
+
+  port:
+    type: object
+    description: Input port, connect to a MIPI CSI-2 receiver
+
+    properties:
+      endpoint:
+        type: object
+
+        properties:
+          remote-endpoint: true
+
+          bus-type:
+            const: 4
+
+          clock-lanes:
+            maxItems: 1

I believe you can drop clock-lanes and bus-type; these are both constants.

I don't understand why bus-type should be dropped because it is constant:
if bus-type is set to something else, the driver will definitely not probe
since we're requesting V4L2_MBUS_CSI2_DPHY for v4l2_fwnode_endpoint_parse.
So I think it's quite important for the bindings to reflect this.

I presume the device does not support lane remapping?

That's correct so this is indeed not something we can configure.
But shouldn't we instead specift clock-lanes = <0> as a const rather than
getting rid of it?

Could you also add link-frequencies, to list which frequencies are known to
be good?

Ah right, I had missed it. I'm a bit unsure about what I should do with the
information from the driver though: should I refuse to use link frequencies that
are not in the list?

Cheers,

Paul

Same comments on the other OV sensor bindings.

quoted

+
+          data-lanes:
+            minItems: 1
+            maxItems: 4
+
+        required:
+          - bus-type
+          - data-lanes
+          - remote-endpoint
+
+        additionalProperties: false
+
+    required:
+      - endpoint
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - dvdd-supply
+  - dovdd-supply
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/sun8i-a83t-ccu.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c2 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ov8865: camera@36 {
+            compatible = "ovti,ov8865";
+            reg = <0x36>;
+
+            pinctrl-names = "default";
+            pinctrl-0 = <&csi_mclk_pin>;
+
+            clocks = <&ccu CLK_CSI_MCLK>;
+            clock-names = "extclk";
+
+            avdd-supply = <&reg_ov8865_avdd>;
+            dovdd-supply = <&reg_ov8865_dovdd>;
+            dvdd-supply = <&reg_ov8865_dvdd>;
+
+            powerdown-gpios = <&pio 4 17 GPIO_ACTIVE_LOW>; /* PE17 */
+            reset-gpios = <&pio 4 16 GPIO_ACTIVE_LOW>; /* PE16 */
+
+            port {
+                ov8865_out_mipi_csi2: endpoint {
+                    bus-type = <4>; /* MIPI CSI-2 D-PHY */
+                    clock-lanes = <0>;
+                    data-lanes = <1 2 3 4>;
+
+                    remote-endpoint = <&mipi_csi2_in_ov8865>;
+                };
+            };
+        };
+    };
+
+...

-- 
Regards,

Sakari Ailus

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com