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

[Paul Kocialkowski <paul.kocialkowski@bootlin.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 --]