Message-ID: <YyhFe+FW2C+R7nQg@pendragon.ideasonboard.com>
Date:   Mon, 19 Sep 2022 13:33:31 +0300
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     "Lad, Prabhakar" <prabhakar.csengg@...il.com>
Cc:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Maxime Ripard <maxime@...no.tech>,
        Steve Longerbeam <slongerbeam@...il.com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Hans Verkuil <hverkuil@...all.nl>, linux-media@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH] media: dt-bindings: i2c: ovti,ov5640: Drop ref to
 video-interface-devices.yaml

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@...renesas.com>
> > > > >>>
> > > > >>> 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@...renesas.com>
> > > > >>> ---
> > > > >>>  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@...il.com>
> > > > >>>
> > > > >>> -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.

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists