Since I got the attention of both of you, let me point out another
issue I'm facing.
We also have video-interface-devices.yaml which lists properties for
the device node and not for the endpoints.
video-interface-devices lists properties that should be all optionally
accepted, as they can potentially apply to all sensors (things like
rotation, orientation, lens-focus, flash-leds are valid for all
devices)
Being properties for the device node they should be specified in the
schema top-level and I see a few schema that do that by
allOf:
- $ref: /schemas/media/video-interface-devices.yaml#
However top level schemas usually specify
additionalProperties: false
Which means each sensor schema has to list the properties it accepts from
video-interface-devices.yaml. It's easy to verify this just by
adding "orientation" to the example in a schema that refers to
video-interface-devices.yaml and see that the bindings validation
fails (see below)
TL;DR is there a way to tell in a schema with a top-level
"additionalProperties: false" that all properties from a referenced
schema are accepted ?
I'll leave video-interface-devices.yaml this out from this series for
now and only resend this patch with the previous comments on the usage
of unevaluatedProperties fixed.
Thanks
j
-----------------------------------------------------------------------------
--- a/Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml
@@ -109,6 +109,7 @@ examples:
powerdown-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
rotation = <180>;
+ orientation = <0>;
port {
/* MIPI CSI-2 bus endpoint */
$ make ARCH=arm64 dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml
DTEX Documentation/devicetree/bindings/media/i2c/ovti,ov5640.example.dts
DTC_CHK Documentation/devicetree/bindings/media/i2c/ovti,ov5640.example.dtb
/home/jmondi/linux-worktree/mainline/Documentation/devicetree/bindings/media/i2c/ovti,ov5640.example.dtb: camera@3c: 'orientation' does not match any of the regexes: 'pinctrl-[0-9]+'
From schema: /home/jmondi/linux-worktree/mainline/Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml
On Sat, Jan 28, 2023 at 12:07:44PM +0200, Sakari Ailus wrote:
> 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
