Message-ID: <20201111131857.GC26150@paasikivi.fi.intel.com>
Date:   Wed, 11 Nov 2020 15:18:57 +0200
From:   Sakari Ailus <sakari.ailus@...ux.intel.com>
To:     Paul Kocialkowski <paul.kocialkowski@...tlin.com>
Cc:     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>,
        Maxime Ripard <mripard@...nel.org>, 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 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 .

> 
> > 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.

> 
> I now understand that the clock frequency may depend on the system integration
> for this special case so we have to specify it via dt.

Correct.

> 
> > In other words, the driver may not be specific to a particular board and
> > SoC you have.
> 
> Although this is sadly more than often the case, because handling a variable
> clock rate in the driver is quite complex (and even more with static init tables
> that include PLL configuration). And sadly my driver is no exception and
> only supports 24 MHz input.

That's fine. If someone needs other frequencies, they can always add
support for those in the driver.

> 
> > Please also read Documentation/driver-api/media/camera-sensor.rst .
> 
> Thanks, I hadn't seen that document before. It's great that it exists!

You're welcome!

This was indeed written to reduce the number of patch revisions needed ot
get a driver to upstream. :-)

-- 
Kind regards,

Sakari Ailus

Powered by blists - more mailing lists