Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
To: Sakari Ailus <sakari.ailus@linux.intel.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: Thu, 5 Nov 2020 16:35:34 +0100
Message-ID: <20201105153534.GD615923@aptenodytes> (raw)
In-Reply-To: <20201105081954.GX26150@paasikivi.fi.intel.com>


[-- Attachment #1: Type: text/plain, Size: 6576 bytes --]

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

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

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

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.

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

> Please also read Documentation/driver-api/media/camera-sensor.rst .

Thanks, I hadn't seen that document before. It's great that it exists!

Paul

> > > > +
> > > > +  dvdd-supply:
> > > > +    description: Digital Domain Power Supply
> > > > +
> > > > +  avdd-supply:
> > > > +    description: Analog Domain Power Supply (internal AVDD is used if missing)
> > > > +
> > > > +  dovdd-supply:
> > > > +    description: I/O Domain Power Supply
> > > > +
> > > > +  powerdown-gpios:
> > > > +    maxItems: 1
> > > > +    description: Power Down Pin GPIO Control (active low)
> > > > +
> > > > +  reset-gpios:
> > > > +    maxItems: 1
> > > > +    description: Reset Pin GPIO Control (active low)
> > > > +
> > > > +  port:
> > > > +    type: object
> > > > +    description: Input port, connect to a MIPI CSI-2 receiver
> > > > +
> > > > +    properties:
> > > > +      endpoint:
> > > > +        type: object
> > > > +
> > > > +        properties:
> > > > +          remote-endpoint: true
> > > > +
> > > > +          bus-type:
> > > > +            const: 4
> > > > +
> > > > +          clock-lanes:
> > > > +            maxItems: 1
> > > 
> > > I believe you can drop clock-lanes and bus-type; these are both constants.
> > 
> > I don't understand why bus-type should be dropped because it is constant:
> > if bus-type is set to something else, the driver will definitely not probe
> > since we're requesting V4L2_MBUS_CSI2_DPHY for v4l2_fwnode_endpoint_parse.
> > So I think it's quite important for the bindings to reflect this.
> 
> This driver is for a particular device that has MIPI CSI-2 on D-PHY as the
> data bus. You can assume that in the driver.
> 
> > 
> > > I presume the device does not support lane remapping?
> > 
> > That's correct so this is indeed not something we can configure.
> > But shouldn't we instead specift clock-lanes = <0> as a const rather than
> > getting rid of it?
> 
> Why would you put redundant information to DT?
> 
> > 
> > > Could you also add link-frequencies, to list which frequencies are known to
> > > be good?
> > 
> > Ah right, I had missed it. I'm a bit unsure about what I should do with the
> > information from the driver though: should I refuse to use link frequencies that
> > are not in the list?
> 
> Yes, please.
> 
> -- 
> Regards,
> 
> Sakari Ailus

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  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 [this message]
2020-11-11 13:18           ` Sakari Ailus
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=20201105153534.GD615923@aptenodytes \
    --to=paul.kocialkowski@bootlin.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=robh+dt@kernel.org \
    --cc=sakari.ailus@linux.intel.com \
    --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