Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Jacopo Mondi <jacopo@xxxxxxxxxx>]

Hi Krzysztof

On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote:
> On 10/03/2022 14:08, Jacopo Mondi wrote:
> > Provide the bindings documentation for Omnivision OV5670 image sensor.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
> > ---
> >  .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++
>
> Add the file to maintainers entry.
>

Right

> >  1 file changed, 93 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
> > new file mode 100644
> > index 000000000000..dc4a3297bf6f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml
>
> Missing vendor prefix in file name.
>

Right x2

> > @@ -0,0 +1,93 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Omnivision OV5670 5 Megapixels raw image sensor
> > +
> > +maintainers:
> > +  - Jacopo Mondi <jacopo@xxxxxxxxxx>
>
> Please add also driver maintainer.
>

I never got what the policy was, if the maintainer entries here only
refer to the binding file or to the driver too

> > +
> > +description: |-
> > +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
> > +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
> > +  controlled through an I2C compatible control bus.
> > +
> > +properties:
> > +  compatible:
> > +    const: ovti,ov5670
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clock-frequency:
> > +    description: Frequency of the xclk clock.
>
> Is the xclk external clock coming to the sensor? If yes, there should be
> a "clocks" property.
>

To be honest I was not sure about this, as clock-frequency is already
used by the driver for the ACPI part, but it seems to in DT bindings
it is a property meant to be specified in the clock providers, even if
Documentation/devicetree/bindings/clock/clock-bindings.txt doesn't
really clarify this

Clock consumer should rather use 'clocks' and point to the provider's
phandle if my understanding is right.

> > +
> > +  pwdn-gpios:
> > +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
>
> maxItems
>

I thought it was not necessary with a single description: entry. But
looking at the dt-schema source I fail to find any commit mentioning
that.

> > +
> > +  reset-gpios:
> > +    description:
> > +      Reference to the GPIO connected to the XSHUTDOWN pin. Active low.
>
> maxItems
>
> > +
> > +  avdd-supply:
> > +    description: Analog circuit power. Typically 2.8V.
> > +
> > +  dvdd-supply:
> > +    description: Digital circuit power. Typically 1.2V.
> > +
> > +  dovdd-supply:
> > +    description: Digital I/O circuit power. Typically 2.8V or 1.8V.
> > +
> > +  port:
> > +    $ref: /schemas/graph.yaml#/$defs/port-base
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      endpoint:
> > +        $ref: /schemas/media/video-interfaces.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          data-lanes:
> > +            description: The sensor supports 1 or 2 data lanes operations.
> > +            minItems: 1
> > +            maxItems: 2
> > +            items:
> > +              maximum: 2
>
> Is '0' also allowed? If not then maybe 'enum: [ 1, 2 ]'

No 0 is not allowed, but the data-lanes properties should accept any
of the following combinations
        <1>
        <1 2>
        <2 1>

As the chip seems to support lane re-ordering.

using enum would allow to between <1> or <2> if I got it right?

as the data-lane property is defined in video-interfaces.yaml

  data-lanes:
    $ref: /schemas/types.yaml#/definitions/uint32-array
    minItems: 1
    maxItems: 8
    items:
      # Assume up to 9 physical lane indices
      maximum: 8
    description:
      An array of physical data lane indexes. Position of an entry determines
      the logical lane number, while the value of an entry indicates physical
      lane, e.g. for 2-lane MIPI CSI-2 bus we could have "data-lanes = <1 2>;",
      assuming the clock lane is on hardware lane 0. If the hardware does not
      support lane reordering, monotonically incremented values shall be used
      from 0 or 1 onwards, depending on whether or not there is also a clock
      lane. This property is valid for serial busses only (e.g. MIPI CSI-2).

I did the same but restricted the max number of items to 2, and the
maximum value to 2 as well

>
> no clock-lanes?
>

clock lane is fixed on lane #0 afaict
`
> > +
> > +          clock-noncontinuous: true
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clock-frequency
> > +  - port
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c0 {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        ov5670: sensor@36 {
> > +            compatible = "ovti,ov5670";
> > +            reg = <0x36>;
> > +
> > +            clock-frequency=<19200000>;
>
> Missing spaces around '='.

ouch, thanks for spotting

Thanks
  j

>
>
>
> Best regards,
> Krzysztof