Re: [PATCH 1/3] dt-bindings: media: i2c: Add OV8865 bindings documentation

[Sakari Ailus]

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:
> > Hi Paul,
> > 
> > On Thu, Nov 05, 2020 at 04:35:34PM +0100, Paul Kocialkowski wrote:
> > > Hi Sakari,
> > > 
> > > On Thu 05 Nov 20, 10:19, Sakari Ailus wrote:
> > > > 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:
> > > > > > On Fri, Oct 23, 2020 at 07:54:04PM +0200, Paul Kocialkowski wrote:
> > > > > > > This introduces YAML bindings documentation for the OV8865
> > > > > > > image sensor.
> > > > > > > 
> > > > > > > Co-developed-by: Kévin L'hôpital <kevin.lhopital@xxxxxxxxxxx>
> > > > > > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@xxxxxxxxxxx>
> > > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> > > > > > > ---
> > > > > > >  .../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@xxxxxxxxxxx>
> > > > > > > +
> > > > > > > +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.
> > 
> > > 
> > > > > > 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@xxxxxxxxxxx/
> > 
> > Yes, there has been more discussion on the topic, most recently in this
> > thread:
> > 
> > <URL:https://lore.kernel.org/linux-arm-kernel/20201102150547.GY26150@xxxxxxxxxxxxxxxxxxxxxx/>
> > 
> > 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.
> 
> > > > 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