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
