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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 ?

--- a/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml
@@ -55,7 +55,7 @@ properties:
     properties:
       endpoint:
         $ref: /schemas/media/video-interfaces.yaml#
-        unevaluatedProperties: false
+        additionalProperties: false

         properties:
           data-lanes:
@@ -70,6 +70,7 @@ properties:
                   - const: 2

           link-frequencies: true
+          remote-endpoint: true



>
> Rob




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux