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

[Jacopo Mondi <jacopo@jmondi.org>]

Hi Krzysztof

On Fri, Mar 11, 2022 at 05:11:47PM +0100, Krzysztof Kozlowski wrote:
> On 11/03/2022 17:05, Jacopo Mondi wrote:
> > Hi Krzysztof,
> >
> > On Thu, Mar 10, 2022 at 06:26:02PM +0100, Krzysztof Kozlowski wrote:
> >> On 10/03/2022 18:16, Jacopo Mondi wrote:
> >>> 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@jmondi.org>
> >>>>> ---
> >>>>>  .../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@jmondi.org>
> >>>>
> >>>> 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
> >>
> >> It is a person responsible for the bindings, so indeed it might not feed
> >> existing maintainer.
> >>
> >>>
> >>>>> +
> >>>>> +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.
> >>
> >> This is a clock-frequency, not clock reference. For external clocks, a
> >
> > Yes, I was suggesting to replace clock-frequency with clocks, that
> > accepts a phandle.
> >
> > The thing is, the driver parses 'clock-frequency'
> > 	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
> >
> > which I assume comes from ACPI (as the driver was developed for an
> > ACPI platform).
> >
> > If in DTS we don't use it, I then need to
> >
> > #ifdef CONFIG_ACPI
> >
> > #elif defined CONFIG_OF
> >
> > #endif
> >
> > Which I would really like to avoid.
> >
> > Anyone with ACPI experience that knows where clock-frequency comes
> > from ?
>
> I would assume that ACPI simply does not support common clock framework,
> so it had to use clock-frequency. Several of such drivers were added by
> folks from Intel which use ACPI, not Devicetree.
>
> >
> >> clock phandles + assigned-clock-rates should be rather used. However for
> >> internal clocks, this is a perfectly valid property.
> >>
> >> Therefore the question is - what is the "xclk"?
> >
> > xclk is the clock fed to the sensor, which which all its internal
> > clocks are generated, so it's indeed an 'external' clock. As I've
> > said, clock-frequency seems to be meant for clock providers, and
> > the image sensor is a clock consumer.
>
> Regardless whether clock-frequency stays or not, you need the clocks
> property in such case.
>

Yes, I will have to ifdef in the driver if no better alternatives

> >
> >>
> >>>
> >>>>> +
> >>>>> +  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.
> >>
> >> The purpose of maxItems is to constrain the number of GPIOs, so two
> >> would be incorrect.
> >>
> >
> > I recall that with a single description entry then maxItems: 1 was
> > assumed by the dt-schema validator, but I cannot find references to
> > any commit, so I'll add it.
> >
> >>>
> >>>>> +
> >>>>> +  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?
> >>
> >> Yeah, enum would be equivalent. I find it more readable, than min+max,
> >> but it's not a strong preference.
> >>
> >
> > I don't think enum is equivalent, as it specifies a set of valid values
> > a property can assume, but it does not support arrays.
>
> It is equivalent, just has to be used in equivalent way.
>
> >
> > https://json-schema.org/draft/2020-12/json-schema-validation.html#rfc.section.6.16.1.2.
> >
> > enum
> >   The value of this keyword MUST be an array. This array SHOULD have
> >   at least one element. Elements in the array SHOULD be unique.
> >
> >   An instance validates successfully against this keyword if its value
> >   is equal to one of the elements in this keyword's array value.
> > > In facts:
>
> That's not good usage. See for example:
> Documentation/devicetree/bindings/display/msm/dsi-controller-main.yaml

Thanks, you're right.

        items:
          enum: [1, 2]

validates correctly.

Thanks for the suggestion!

>
>
> Best regards,
> Krzysztof