Hi Paul,

On Wed, Nov 04, 2020 at 11:26:43AM +0100, Paul Kocialkowski wrote:

Hi Sakari and thanks for the review!

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

quoted

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.

That argument may be NULL.

quoted

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.

I'm not exactly sure about that conclusion.

You can use clk_set_rate() if you get the frequency from DT, but we
recently did conclude that camera sensor drivers can expect to get the
frequency indicated by assigned-clock-rate property.

In other words, the driver may not be specific to a particular board and
SoC you have.

Please also read Documentation/driver-api/media/camera-sensor.rst .

quoted

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.

This driver is for a particular device that has MIPI CSI-2 on D-PHY as the
data bus. You can assume that in the driver.

quoted

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?

Why would you put redundant information to DT?

quoted

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?

Yes, please.

-- 
Regards,

Sakari Ailus