Re: [PATCH v2] media: dt-bindings: hynix,hi846: Document orientation and rotation

[Rob Herring <robh@kernel.org>]

On Thu, Sep 28, 2023 at 04:57:23PM +0200, Jacopo Mondi wrote:
> Hi Fabio, Krzysztof
> 
> On Thu, Sep 28, 2023 at 09:14:24AM -0300, Fabio Estevam wrote:
> > From: Fabio Estevam <festevam@denx.de>
> >
> > Document the 'orientation' and 'rotation' properties, which
> > are valid for the SK Hynix Hi-846 sensor.
> >
> > Their definitions come from video-interface-devices.yaml, so add
> > a reference to it.
> >
> > Signed-off-by: Fabio Estevam <festevam@denx.de>
> > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Acked-by: Martin Kepplinger <martink@posteo.de>
> > ---
> > Changes since v1:
> > - Also pass a ref to video-interface-devices.yaml. (Martin)
> >
> 
> This patch is technically correct, so this message is not meant to
> delay its integration or anything like that, but I'll take the
> occasion to ask for guidance to the DT maintainers, as I think this
> approach doesn't scale.
> 
> I count the following sensor bindings that refer to
> video-interface-device.yaml
> 
> Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml
> Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml
> Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml
> Documentation/devicetree/bindings/media/i2c/ovti,ov5675.yaml
> Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml
> Documentation/devicetree/bindings/media/i2c/sony,imx214.yaml
> Documentation/devicetree/bindings/media/i2c/sony,imx415.yaml
> 
> These:
> 
> Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml:additionalProperties: false
> Documentation/devicetree/bindings/media/i2c/ovti,ov4689.yaml:additionalProperties: false
> Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml:additionalProperties: false
> Documentation/devicetree/bindings/media/i2c/sony,imx214.yaml:additionalProperties: false
> Documentation/devicetree/bindings/media/i2c/sony,imx415.yaml:additionalProperties: false
> 
> specify 'additionalProperties: false' at the top-level.
> This is correct imho, as it implies that any properties not
> specifically allowed by bindings is forbidden.
> 
> This unfortunately applies to  'rotation' and 'orientation' as well.
> This is not correct, as those two properties should apply to all
> sensors without the requiring the bindings to explicitly allow them.
> 
> Counterproof: It's very easy to break validation of, in example,
> ov5640
> 
> --- 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
>   DTC_CHK Documentation/devicetree/bindings/media/i2c/ovti,ov5640.example.dtb
>   'orientation' does not match any of the regexes: 'pinctrl-[0-9]+'
> 	from schema $id: http://devicetree.org/schemas/media/i2c/ovti,ov5640.yaml#
> 
> Is there a way to allow those two properties ('rotation' and
> 'orientation') to be accepted by all sensor drivers bindings ?

Use unevaluatedProperties instead of additionalProperties and add a $ref 
to video-interface-devices.yaml in the sensor schemas. However, that 
will allow all properties in video-interface-devices.yaml (which is just 
flash-leds and lens-focus which seem fine). If you don't want that, then 
you will have to split up video-interface-devices.yaml.

Rob