Re: [PATCH] media: dt-bindings: i2c: ovti,ov5640: Drop ref to video-interface-devices.yaml

[Lad, Prabhakar]

Hi Laurent,

On Mon, Sep 19, 2022 at 11:33 AM Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Sep 19, 2022 at 10:41:00AM +0100, Lad, Prabhakar wrote:
> > On Mon, Sep 19, 2022 at 10:37 AM Laurent Pinchart wrote:
> > > On Mon, Sep 19, 2022 at 10:35:21AM +0100, Lad, Prabhakar wrote:
> > > > On Mon, Sep 19, 2022 at 9:19 AM Krzysztof Kozlowski wrote:
> > > > > On 19/09/2022 10:08, Lad, Prabhakar wrote:
> > > > > > On Sun, Sep 18, 2022 at 12:06 AM Laurent Pinchart wrote:
> > > > > >> On Fri, Sep 16, 2022 at 02:35:21PM +0100, Prabhakar wrote:
> > > > > >>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > > > > >>>
> > > > > >>> video-interface-devices.yaml isn't used so just drop it from the
> > > > > >>> DT binding doc.
> > > > > >>>
> > > > > >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > > > > >>> ---
> > > > > >>>  Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml | 3 ---
> > > > > >>>  1 file changed, 3 deletions(-)
> > > > > >>>
> > > > > >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml
> > > > > >>> index 540fd69ac39f..ce99aada75ad 100644
> > > > > >>> --- a/Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml
> > > > > >>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml
> > > > > >>> @@ -9,9 +9,6 @@ title: OmniVision OV5640 Image Sensor Device Tree Bindings
> > > > > >>>  maintainers:
> > > > > >>>    - Steve Longerbeam <slongerbeam@xxxxxxxxx>
> > > > > >>>
> > > > > >>> -allOf:
> > > > > >>> -  - $ref: /schemas/media/video-interface-devices.yaml#
> > > > > >>> -
> > > > > >>
> > > > > >> The rotation property listed in this binding uses the definition from
> > > > > >> video-interface-devices.yaml. I don't think just dropping this is the
> > > > > >> right solution. Changing additionaProperties to unevaluatedProperties
> > > > > >> seems a better option.
> > > > > >
> > > > > > Agreed, I missed rotation was used from video-interface-devices.yaml.
> > > > > > Agreed the changing additionaProperties to unevaluatedProperties seems
> > > > > > a better option.
> > > > >
> > > > > The meaning of unevaluatedProperties:false would be here - accept other
> > > > > properties (not mentioned here explicitly) from referenced schema. If
> > > > > this is your actual intention for this binding, it makes sense. But if
> > > > > the intention in this binding was to disallow these other properties,
> > > > > then it would be wrong to change to unevaluatedProperties.
> > > > >
> > > > Thank you for the clarification. The intention is to disallow the property.
> > >
> > > Why should they be disallowed ?
> >
> > my bad! "rotation" property is supposed to be allowed so the earlier
> > comment to change to unevaluatedProperties holds good.
>
> It's not just the rotation. The other properties are allowed too. For
> the rotation property you need to list it explicitly in ovti,ov5640.yaml
> if you want to restrict the values it can take, but other properties
> from video-interface-devices.yaml for which no additional constraints
> are needed don't need to be listed in ovti,ov5640.yaml.
>
> additionalProperties and unevaluatedProperties are often misunderstood.
> DT bindings are a set of rules, and validation will pass *only* if *all*
> rules are valid. Let's consider the following:
>
> allOf:
>   - $ref: /schemas/media/video-interface-devices.yaml#
>
> The allOf is valid if all of the elements in the list are valid. The
> $ref will essentially work as if the contents of
> video-interface-devices.yaml were copied in ovti,ov5640.yaml, under the
> corresponding allOf list entry (with a small but important difference,
> noted below). The file contains
>
>   rotation:
>     $ref: /schemas/types.yaml#/definitions/uint32
>     enum: [ 0, 90, 180, 270 ]
>
> so any "rotation" property in the device tree will be validated against
> this. ovti,ov5640.yaml also has
>
> properties:
>   rotation:
>     enum:
>       - 0
>       - 180
>
> which is a separate rule from the previous one. Both must be valid for
> validation to succeed, so this second rule essentially restricts the
> possible rotation values.
>
> The additionalProperties and unevaluatedProperties affect how properties
> that have no validation rule will be treated.
>
> With additionalProperties set to false, a property that has no
> validation rule in *this* schema will be considered invalid, even if it
> has a validation rule in another schema (either selected automatically
> through a "select" property in the other schema, or imported through an
> explicit $ref). So, in this particular example, even though
> video-interface-devices.yaml has, for instance, a rule for the
> lens-focus property, a DT that contains lens-focus will be considered as
> invalid as lens-focus is not validated by this schema. One way to allow
> the property would be to add
>
> properties:
>   lens-focus: true
>
> in this schema. The contents of lens-focus would be validated by the
> rule in video-interface-devices.yaml, and the rule in this schema would
> always be valid ("true" is always valid).
>
> Another way to allow the property would be to replace
> additionalProperties with unevaluatedProperties. When set to false,
> unevaluatedProperties makes validation fail if any property has not been
> evaluated by *any* rule in this schema or any other schema. As
> lens-focus would be evaluated by video-interface-devices.yaml, that
> would be enough to consider it valid. This also means that *all*
> properties listed in video-interface-devices.yaml would then be valid.
> If you wanted that behaviour, but also wanted to reject specific
> properties, you could add
>
> properties:
>   lens-focus: false
>
> in this schema. A lens-focus property in a DT would be valid when
> evaluated with the corresponding rule in video-interface-devices.yaml,
> but would be invalidated by the rule in this schema as "false" is always
> invalid.
>
> To conclude, setting additionalProperties to false creates a white
> listing mechanism that requires you to explicitly list in this schema
> all the properties you consider as valid with "foo: true", while setting
> unevaluatedProperties to false creates a black listing mechanism that
> requires you to explicitly list in this schema all the properties you
> consider as invalid with "foo: false". If multiple schemas that apply to
> the same device tree include rules for the same property, all those
> rules need to be valid for validation to pass, regardless of the value
> of additionalProperties and unevaluatedProperties.
>
Thank you for the detailed explanation! I'll make it a point to go
through this thread before doing a change in the binding file :)

Cheers,
Prabhakar