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

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

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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@bp.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@gmail.com>
 
-allOf:
-  - $ref: /schemas/media/video-interface-devices.yaml#
-
 properties:
   compatible:
     const: ovti,ov5640
-- 
2.25.1

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

[Krzysztof Kozlowski]

On 16/09/2022 14:35, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> video-interface-devices.yaml isn't used so just drop it from the
> DT binding doc.

Same question - the schema is used - you can see it with your own eyes,
so please explain in commit msg what exactly is not used.



Best regards,
Krzysztof

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

[Lad, Prabhakar]

Hi Krzysztof,

Thank you for the review.

On Sat, Sep 17, 2022 at 5:47 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 16/09/2022 14:35, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > video-interface-devices.yaml isn't used so just drop it from the
> > DT binding doc.
>
> Same question - the schema is used - you can see it with your own eyes,
> so please explain in commit msg what exactly is not used.
>
As Laurent pointed out I need to additionaProperties to
unevaluatedProperties instead of dropping this reference. I'll update
the commit message with this change and send a v2 (and also similarly
for the ov02a10 binding)

Cheers,
Prabhakar

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

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Fri, Sep 16, 2022 at 02:35:21PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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@bp.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@gmail.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.

>  properties:
>    compatible:
>      const: ovti,ov5640

-- 
Regards,

Laurent Pinchart

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

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Sun, Sep 18, 2022 at 12:06 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Fri, Sep 16, 2022 at 02:35:21PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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@bp.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@gmail.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.

Cheers,
Prabhakar

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

[Krzysztof Kozlowski]

On 19/09/2022 10:08, Lad, Prabhakar wrote:
> Hi Laurent,
> 
> Thank you for the review.
> 
> On Sun, Sep 18, 2022 at 12:06 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> Hi Prabhakar,
>>
>> Thank you for the patch.
>>
>> On Fri, Sep 16, 2022 at 02:35:21PM +0100, Prabhakar wrote:
>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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@bp.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@gmail.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.

Therefore before sending patches and calling something better or not,
please instead focus on that aspect of referenced schema.


Best regards,
Krzysztof

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

[Lad, Prabhakar]

Hi Krzysztof,

On Mon, Sep 19, 2022 at 9:19 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 19/09/2022 10:08, Lad, Prabhakar wrote:
> > Hi Laurent,
> >
> > Thank you for the review.
> >
> > On Sun, Sep 18, 2022 at 12:06 AM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> >>
> >> Hi Prabhakar,
> >>
> >> Thank you for the patch.
> >>
> >> On Fri, Sep 16, 2022 at 02:35:21PM +0100, Prabhakar wrote:
> >>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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@bp.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@gmail.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.

> Therefore before sending patches and calling something better or not,
> please instead focus on that aspect of referenced schema.
>
Sure will do, sorry for the noise.

Cheers,
Prabhakar

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

[Laurent Pinchart]

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@bp.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@bp.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@gmail.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 ?

> > Therefore before sending patches and calling something better or not,
> > please instead focus on that aspect of referenced schema.
>
> Sure will do, sorry for the noise.

-- 
Regards,

Laurent Pinchart

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

[Lad, Prabhakar]

On Mon, Sep 19, 2022 at 10:37 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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@bp.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@bp.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@gmail.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.

Cheers,
Prabhakar

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

[Laurent Pinchart]

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@bp.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@bp.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@gmail.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

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@ideasonboard.com> 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@bp.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@bp.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@gmail.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.
>
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

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

[Krzysztof Kozlowski]

On 19/09/2022 11:35, Lad, Prabhakar wrote:
>>>> 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.

Which property? Can you be specific?


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On Fri, 16 Sep 2022 14:35:21 +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.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@bp.renesas.com>
> ---
>  Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml | 3 ---
>  1 file changed, 3 deletions(-)
> 

Running 'make dtbs_check' with the schema in this patch gives the
following warnings. Consider if they are expected or the schema is
incorrect. These may not be new warnings.

Note that it is not yet a requirement to have 0 warnings for dtbs_check.
This will change in the future.

Full log is available here: https://patchwork.ozlabs.org/patch/


camera@3c: 'AVDD-supply' is a required property
	arch/arm/boot/dts/imx6ul-14x14-evk.dtb
	arch/arm/boot/dts/imx6ull-14x14-evk.dtb
	arch/arm/boot/dts/imx6ulz-14x14-evk.dtb
	arch/arm/boot/dts/stm32mp157c-ev1.dtb
	arch/arm/boot/dts/stm32mp157c-ev1-scmi.dtb

camera@3c: 'DOVDD-supply' is a required property
	arch/arm64/boot/dts/freescale/imx8mm-beacon-kit.dtb
	arch/arm/boot/dts/imx6ul-14x14-evk.dtb
	arch/arm/boot/dts/imx6ull-14x14-evk.dtb
	arch/arm/boot/dts/imx6ulz-14x14-evk.dtb

camera@3c: 'DVDD-supply' is a required property
	arch/arm64/boot/dts/freescale/imx8mm-beacon-kit.dtb
	arch/arm/boot/dts/imx6ul-14x14-evk.dtb
	arch/arm/boot/dts/imx6ull-14x14-evk.dtb
	arch/arm/boot/dts/imx6ulz-14x14-evk.dtb
	arch/arm/boot/dts/stm32mp157c-ev1.dtb
	arch/arm/boot/dts/stm32mp157c-ev1-scmi.dtb

camera_rear@3b: 'clock-frequency', 'enable-gpios', 'vdda-supply', 'vddd-supply', 'vdddo-supply' do not match any of the regexes: 'pinctrl-[0-9]+'
	arch/arm64/boot/dts/qcom/apq8016-sbc.dtb
	arch/arm/boot/dts/qcom-apq8016-sbc.dtb

camera_rear@3b: port:endpoint:data-lanes:0:0: 0 is not one of [1, 2]
	arch/arm64/boot/dts/qcom/apq8016-sbc.dtb
	arch/arm/boot/dts/qcom-apq8016-sbc.dtb

ov5640@3c: 'AVDD-supply' is a required property
	arch/arm/boot/dts/dra71-evm.dtb
	arch/arm/boot/dts/dra72-evm.dtb
	arch/arm/boot/dts/dra72-evm-revc.dtb

ov5640@3c: 'DOVDD-supply' is a required property
	arch/arm/boot/dts/dra71-evm.dtb
	arch/arm/boot/dts/dra72-evm.dtb
	arch/arm/boot/dts/dra72-evm-revc.dtb

ov5640@3c: 'DVDD-supply' is a required property
	arch/arm/boot/dts/dra71-evm.dtb
	arch/arm/boot/dts/dra72-evm.dtb
	arch/arm/boot/dts/dra72-evm-revc.dtb