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
>
