Hi Maxime,

Sorry for the late reply.

On Fri, Nov 13, 2020 at 06:27:35PM +0100, Maxime Ripard wrote:

Hi Sakari,

On Wed, Nov 11, 2020 at 03:18:57PM +0200, Sakari Ailus wrote:

quoted

Hi Paul,

On Thu, Nov 05, 2020 at 04:35:34PM +0100, Paul Kocialkowski wrote:

quoted

Hi Sakari,

On Thu 05 Nov 20, 10:19, Sakari Ailus wrote:

quoted

Hi Paul,

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

quoted

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.

Understood, let's get rid of clock-names then. I see this is done in a few
drivers already, but many also give it a name with a single clock.

It would be nice if that was consistent across all drivers just so that the
expectation is clear (that the best way for that to happen is probably to
fix up a patch myself though).

I guess somewhat different practices exist depending on the tree albeit
it's all DT bindings. It's also not wrong to have the name of the clock
there, no, but virtually all camera sensors consume a single clock, so you
may as well omit the information.

quoted

quoted

quoted

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.

I may have jumped too far here. It's not exactly clear to me what was the
conclusion from...
https://lore.kernel.org/linux-arm-kernel/20200401080705.j4goeqcqhoswhx4u@gilmour.lan/ (local)

Yes, there has been more discussion on the topic, most recently in this
thread:

<URL:https://lore.kernel.org/linux-arm-kernel/20201102150547.GY26150@paasikivi.fi.intel.com/ (local)>

I think this deserves to be added to camera-sensor.rst .

It's not really a discussion though :)

We had back in that thread some issues with assigned-clock-rates that
don't get addressed at all.

quoted

quoted

quoted

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.

...but it looks like clock-frequency was preferred over assigned-clock-rates
and this is what the binding that was merged suggests. Is that correct?

assigned-clock-rates is fine. The assumption is that the clock frequency
does not change from the value set through DT, and the driver gets that
exact frequency.

That's two fairly big assumptions though. A clock driver is free to
round the clock to whatever rate it wants, and assigned-clock-rates
doesn't provide any guarantee on this, nor does it let the potential
user know about it.

It might work for one-off cases, but there's no guarantee that it will
in the future since this is very much dependent on the clock setup,
driver and the other devices in the system (and to some extent the
configuration as well).

And since we only rely on it, we can't fix it properly either if it ever
occurs.

And then, semantically, this assigned-clock-rates isn't about what the
devices support but what the driver supports. The clock tree of
omnivision sensors (at least, I can't comment on the imx218 linked
above) allows for a very flexible input clock, it's only the driver that
requires it because most of its configuration relies on it.

There is a semantic difference, yes. The driver in this case assumes the
frequency is the one it is expected to use. In case the frequency is
different, the most likely outcome is that the image sensor is inoperable.
But again that is fairly easy to notice.

If someone prefers to fix this and make it reliable also in principle, that
would require changing the semantics a little as well as adding a function
to the clock framework to set the assigned frequency.

It's definitely fine for the driver to do so, but it really doesn't
belong in the DT.

I thought we had an agreement on this last time we discussed it, but I'm
not really sure what made you change your mind.

Most new sensor drivers come with assigned-clock-rates etc. properties and
I'll need to explain the same over and over again in review. Then using
"clock-frequency" property requires quite a few more lines of code in the
drivers.

I also recently discussed the matter with Rob H. and his opinion was
basically that we should be using assigned-clock-rates here instead.

I wonder how this works on other places where the frequency can be one of
several options, but the correct frequency is board specific.

-- 
Kind regards,

Sakari Ailus