Re: [PATCH v6 1/9] media: dt-bindings: Add OV5670

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Krzysztof

On Fri, Jan 27, 2023 at 09:44:25PM +0100, Krzysztof Kozlowski wrote:
> On 27/01/2023 21:38, Sakari Ailus wrote:
> > Hi Krzysztof,
> >
> > On Fri, Jan 27, 2023 at 08:58:20PM +0100, Krzysztof Kozlowski wrote:
> >> On 27/01/2023 19:14, Jacopo Mondi wrote:
> >>> Hi Krzysztof
> >>>
> >>> On Fri, Jan 27, 2023 at 03:19:08PM +0100, Krzysztof Kozlowski wrote:
> >>>> On 26/01/2023 17:59, Jacopo Mondi wrote:
> >>>>> Add the bindings documentation for Omnivision OV5670 image sensor.
> >>>>>
> >>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> >>>>> ---
> >>>>
> >>>> (...)
> >>>>
> >>>>> +
> >>>>> +  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:
> >>>>> +            minItems: 1
> >>>>> +            maxItems: 2
> >>>>> +            items:
> >>>>> +              enum: [1, 2]
> >>>>> +
> >>>>> +          clock-noncontinuous: true
> >>>>
> >>>> You do not need this. Drop.
> >>>>
> >>>
> >>> Is this due to "unevaluatedProperties: false" ?
> >>>
> >>> I read you recent explanation to a similar question on the Visconti
> >>> bindings. Let me summarize my understanding:
> >>>
> >>> For a given schema a property could be
> >>> - required
> >>>         required:
> >>>           - foo
> >>>
> >>> - optional
> >>>         by default with "unevaluatedProperties: false"
> >>>         "foo: true" with "additionalProperties: false"
> >>>
> >>> - forbidden
> >>>         "foo: false" with "unevaluatedProperties: false"
> >>>         by default wiht "additionalProperties: false"
> >>>
> >>> clock-noncontinuous is defined in video-interfaces.yaml. as I specify
> >>> "unevaluatedProperties: false" does it mean
> >>> all the properties defined in video-interfaces.yaml are optionally
> >>> accepted ? If that's the case that's not what I want as
> >>> clock-noncontinuous is -the only- property from that file we want to
> >>> accept here (and data-lanes ofc).
> >>>
> >>> Should I change "unevaluatedProperties: false" to
> >>> "additionalProperties: false" and keep "clock-noncontinuous: true"  ?
> >>>
> >>
> >> Why would you disallow other properties? Just because driver does not
> >> use them? That's not correct, driver change but bindings should stay the
> >> same.
> >
> > The clock-noncontinuous property is relevant for the hardware. There are
> > some properties not listed here that might be relevant (for all camera
> > sensors) but most properties in video-interfaces.yaml are not applicable to
> > this device.
> >
> > I also think is be useful to say what is relevant in DT bindings, as the
> > other sources of information left are hardware datasheets (if you have
> > access to them) or the driver (which is supposed not to be relevant for the
> > bindings).
> >
>
> Then it might be meaningful to list all allowed properties - even if not
> currently supported by the driver - and use additionalProperties:false.

Have a look at what properties video-interfaces.yaml lists. Some of
them only apply to CSI-2 sensors (data lanes, link-frequencies etc),
some of them only to parallel sensors (lines polarities, bus-width
etc).

I see most of the bindings in media/i2c reporting

        $ref: /schemas/media/video-interfaces.yaml#
        unevaluatedProperties: false

I think that's actually wrong as there's no way all the properties in
video-interfaces.yaml can apply to a single device (with the exception
of a few sensors that support both bus types).

> This has drawback - whenever video-interfaces gets something new, the
> bindings here (and other such devices) will have to be explicitly enabled.

video-interfaces is rarely updated, and when it happes it's to add
properties required by a newly supported device, so this doesn't
concern me much personally.

>
> Best regards,
> Krzysztof
>



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux