Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Sakari Ailus <sakari.ailus@linux.intel.com>
To: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	"Mauro Carvalho Chehab" <mchehab@kernel.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Liam Girdwood" <lgirdwood@gmail.com>,
	"Mark Brown" <broonie@kernel.org>,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	"Hans Verkuil" <hverkuil@xs4all.nl>,
	"Maxime Ripard" <mripard@kernel.org>,
	kevin.lhopital@hotmail.com,
	"Kévin L\'hôpital" <kevin.lhopital@bootlin.com>
Subject: Re: [PATCH 1/3] dt-bindings: media: i2c: Add OV8865 bindings documentation
Date: Wed, 11 Nov 2020 15:18:57 +0200
Message-ID: <20201111131857.GC26150@paasikivi.fi.intel.com> (raw)
In-Reply-To: <20201105153534.GD615923@aptenodytes>

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

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

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-23 17:54 [PATCH 0/3] media: i2c: OV8865 image sensor support Paul Kocialkowski
2020-10-23 17:54 ` [PATCH 1/3] dt-bindings: media: i2c: Add OV8865 bindings documentation Paul Kocialkowski
2020-10-30 16:39   ` Rob Herring
2020-11-02 23:24   ` Sakari Ailus
2020-11-04 10:26     ` Paul Kocialkowski
2020-11-05  8:19       ` Sakari Ailus
2020-11-05 15:35         ` Paul Kocialkowski
2020-11-11 13:18           ` Sakari Ailus [this message]
2020-11-13 17:27             ` Maxime Ripard
2020-11-18 22:38               ` Sakari Ailus
2020-10-23 17:54 ` [PATCH 2/3] media: i2c: Add support for the OV8865 image sensor Paul Kocialkowski
2020-10-23 17:54 ` [PATCH NOT FOR MERGE 3/3] ARM: dts: sun8i: a83t: bananapi-m3: Enable MIPI CSI-2 with OV8865 Paul Kocialkowski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201111131857.GC26150@paasikivi.fi.intel.com \
    --to=sakari.ailus@linux.intel.com \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=hverkuil@xs4all.nl \
    --cc=kevin.lhopital@bootlin.com \
    --cc=kevin.lhopital@hotmail.com \
    --cc=lgirdwood@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=mripard@kernel.org \
    --cc=paul.kocialkowski@bootlin.com \
    --cc=robh+dt@kernel.org \
    --cc=thomas.petazzoni@bootlin.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git