Hi again Fabio
On Fri, Sep 29, 2023 at 08:30:19AM +0200, Jacopo Mondi wrote:
> Hi Rob, Fabio
>
> On Thu, Sep 28, 2023 at 10:54:46AM -0500, Rob Herring wrote:
> > 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@xxxxxxx>
> > > >
> > > > 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@xxxxxxx>
> > > > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> > > > Acked-by: Martin Kepplinger <martink@xxxxxxxxx>
> > > > ---
> > > > 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.
>
> ack! I think it's fine to allow all sensors to point to a lens or a
> flash device if they want to.
>
> I'll send a patch for:
>
> 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
>
> to s/additionalProperties/unevaluatedProperties/
>
> On this binding file instead. I noticed it again specifies
> unevaluatedProperties: false both in the and 'endpoint' nodes, which
> refers to /schemas/media/video-interfaces.yaml. This allows all
> properties from that schema to be specified.
>
> Should I send a patch to or is Fabio interested in doing so as
> part of a new version of this patch ?
>
Actually, if I'm going to s/additionalProperties/unevaluatedProperties/
in this bindings as well, am I wrong this patch is not needed anymore ?
