| Message ID | 20220916133521.73183-1-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | media: dt-bindings: i2c: ovti,ov5640: Drop ref to video-interface-devices.yaml | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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
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
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
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
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
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
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
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.
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
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
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.
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
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