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

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

 



Hi Jacopo, Krzysztof,

On Sat, Jan 28, 2023 at 10:58:31AM +0100, Jacopo Mondi wrote:
> 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).

It's been in my plan to split this into multiple files so you could refer
to fewer than all the properties. I have no schedule for this though.

> 
> > 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.

Me neither.

-- 
Kind regards,

Sakari Ailus



[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