Re: [PATCH v2 1/3] media: dt-bindings: ov5675: document YAML binding

[Quentin Schulz]

Hi Krzysztof,

On 5/4/22 16:46, Krzysztof Kozlowski wrote:
> On 04/05/2022 15:55, Quentin Schulz wrote:
> > From: Quentin Schulz <quentin.schulz@xxxxxxxxxxxxxxxxxxxxx>
> >
> > This patch adds documentation of device tree in YAML schema for the
> > OV5675 CMOS image sensor from Omnivision.
> >
> > Cc: Quentin Schulz <foss+kernel@xxxxxxxxx>
>
> Don't Cc yourself in commits. This goes to the Git history, so
> assumption is that the "other you" knows that you sent it. :)

It's a habit I've taken so that in the event I switch jobs, there's 
still an address where I can be reached.

But I forgot the kernel as a .mailmap :) so I can avoid doing it.

> > Signed-off-by: Quentin Schulz <quentin.schulz@xxxxxxxxxxxxxxxxxxxxx>
> > ---
> >
> > v2:
> >   - fixed incorrect id,
> >   - fixed device tree example by adding missing dt-bindings headers,
> >   - fixed device tree example by using vcc_1v2 for dvdd supply, as requested
> >   in datasheet,
> >
> >   .../bindings/media/i2c/ovti,ov5675.yaml       | 139 ++++++++++++++++++
> >   MAINTAINERS                                   |   1 +
> >   2 files changed, 140 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml
> > new file mode 100644
> > index 000000000000..29df2f82c631
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml
> > @@ -0,0 +1,139 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright (c) 2022 Theobroma Systems Design und Consulting GmbH
> > +%YAML 1.2
> > +---
> > +$id: https://urldefense.proofpoint.com/v2/url?u=http-3A__devicetree.org_schemas_media_i2c_ovti-2Cov5675.yaml-23&d=DwICaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=5gh6rUPawGp8Fw7sPXVsvOViSUSnEVGWWB9RK_CSQCeBjoEfunHaI0w2GiB0zjq_&s=W_iQiFCa2XM4k350eNawjzJR2ZTUuSx-VM4E1iuknz4&e=
> > +$schema: https://urldefense.proofpoint.com/v2/url?u=http-3A__devicetree.org_meta-2Dschemas_core.yaml-23&d=DwICaQ&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=5gh6rUPawGp8Fw7sPXVsvOViSUSnEVGWWB9RK_CSQCeBjoEfunHaI0w2GiB0zjq_&s=nrri3o0sr-6AopqJCaHZ94dUfOwS8r1V7ybfSwD7v2M&e=
> > +
> > +title: Omnivision OV5675 CMOS Sensor Device Tree Bindings
>
> s/Device Tree Bindings//
>
> > +
> > +maintainers:
> > +  - Quentin Schulz <quentin.schulz@xxxxxxxxxxxxxxxxxxxxx>
> > +
> > +description: |-
> > +  The Omnivision OV5675 is a high performance, 1/5-inch, 5 megapixel, CMOS
> > +  image sensor that delivers 2592x1944 at 30fps. It provides full-frame,
> > +  sub-sampled, and windowed 10-bit MIPI images in various formats via the
> > +  Serial Camera Control Bus (SCCB) interface. This chip is programmable
> > +  through I2C and two-wire SCCB. The sensor output is available via CSI-2
> > +  serial data output (up to 2-lane).
> > +
> > +properties:
> > +  compatible:
> > +    const: ovti,ov5675
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    description:
> > +      Input clock for the sensor.
> > +    items:
> > +      - const: xvclk
>
> Just "xv" is preferred.

The name of the clock in the datasheet is XVCLK though. Wouldn't it be 
confusing to describe HW by using names different from the datasheet?

> > +
> > +  clock-frequency:
> > +    description:
> > +      Frequency of the xvclk clock in Hertz.
> > +
> > +  dovdd-supply:
> > +    description:
> > +      Definition of the regulator used as interface power supply.
> > +
> > +  avdd-supply:
> > +    description:
> > +      Definition of the regulator used as analog power supply.
> > +
> > +  dvdd-supply:
> > +    description:
> > +      Definition of the regulator used as digital power supply.
> > +
> > +  reset-gpios:
> > +    description:
> > +      The phandle and specifier for the GPIO that controls sensor reset.
> > +      This corresponds to the hardware pin XSHUTDOWN which is physically
> > +      active low.
>
> Needs maxItems
>
> > +
> > +  port:
> > +    type: object
>
> Open other bindings and compare how it is done there. This looks like
> /schemas/graph.yaml#/$defs/port-base

Did that but used an old kernel as base :/

> > +    additionalProperties: false
> > +    description:
> > +      A node containing an output port node with an endpoint definition
> > +      as documented in
> > +      Documentation/devicetree/bindings/media/video-interfaces.txt
> > +
> > +    properties:
> > +      endpoint:
> > +        type: object
>
> Missing ref
>
> > +
> > +        properties:
> > +          data-lanes:
> > +            description: |-
>
> No need for "|-"
>
> > +              The driver only supports 2-lane operation.
>
> Please remove references to driver. It's not part of hardware.
>
> > +            items:
> > +              - const: 1
> > +              - const: 2
> > +
> > +          link-frequencies:
> > +            $ref: /schemas/types.yaml#/definitions/uint64-array
>
> The ref should be already provided by video-interfaces.
>
> > +            description:
> > +              Allowed data bus frequencies. 450000000Hz is supported by the driver.
>
> Again, skip driver reference. However you need to describe the number of
> items.

Currently, the driver is limited to 450 MHz link-freq and 2 data lanes, 
while the HW advertises: "The OV5675 supports a MIPI interface of up to 
2-lanes. The MIPI interface can be configured for 1/2-lane and each lane

is capable of a data transfer rate of up to 900 Mbps."

Was wondering what I am supposed to do in this situation as I see 
Documentation/devicetree/bindings/media/i2c/ov8856.yaml mentioning 
driver limitations in the dt-bindings.

Cheers,
Quentin