Message-ID: <20201113172735.mkgyrrobgxa33uo3@gilmour.lan>
Date:   Fri, 13 Nov 2020 18:27:35 +0100
From:   Maxime Ripard <maxime@...no.tech>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc:     Paul Kocialkowski <paul.kocialkowski@...tlin.com>,
        linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Hans Verkuil <hverkuil@...all.nl>, kevin.lhopital@...mail.com,
        Kévin L'hôpital <kevin.lhopital@...tlin.com>
Subject: Re: [PATCH 1/3] dt-bindings: media: i2c: Add OV8865 bindings
 documentation

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@...tlin.com>
> > > > > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@...tlin.com>
> > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@...tlin.com>
> > > > > > ---
> > > > > >  .../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@...tlin.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.
> 
> > 
> > > > > 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/
> 
> 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/>
> 
> 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.

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.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists