All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] media: dt-bindings: hynix,hi846: Document orientation and rotation
@ 2023-09-28 12:14 Fabio Estevam
  2023-09-28 14:57 ` Jacopo Mondi
  0 siblings, 1 reply; 8+ messages in thread
From: Fabio Estevam @ 2023-09-28 12:14 UTC (permalink / raw)
  To: mchehab
  Cc: sakari.ailus, robh+dt, krzysztof.kozlowski+dt, conor+dt, martink,
	linux-media, devicetree, Fabio Estevam, Krzysztof Kozlowski

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)

 .../devicetree/bindings/media/i2c/hynix,hi846.yaml         | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml b/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml
index 1e2df8cf2937..8b7a46a15458 100644
--- a/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml
@@ -14,6 +14,9 @@ description: |-
   interface and CCI (I2C compatible) control bus. The output format
   is raw Bayer.
 
+allOf:
+  - $ref: /schemas/media/video-interface-devices.yaml#
+
 properties:
   compatible:
     const: hynix,hi846
@@ -48,6 +51,10 @@ properties:
   vddd-supply:
     description: Definition of the regulator used for the VDDD power supply.
 
+  orientation: true
+
+  rotation: true
+
   port:
     $ref: /schemas/graph.yaml#/$defs/port-base
     unevaluatedProperties: false
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] media: dt-bindings: hynix,hi846: Document orientation and rotation
  2023-09-28 12:14 [PATCH v2] media: dt-bindings: hynix,hi846: Document orientation and rotation Fabio Estevam
@ 2023-09-28 14:57 ` Jacopo Mondi
  2023-09-28 15:14   ` Fabio Estevam
  2023-09-28 15:54   ` Rob Herring
  0 siblings, 2 replies; 8+ messages in thread
From: Jacopo Mondi @ 2023-09-28 14:57 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: mchehab, sakari.ailus, robh+dt, krzysztof.kozlowski+dt, conor+dt,
	martink, linux-media, devicetree, Fabio Estevam,
	Krzysztof Kozlowski

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 ?

Thanks
   j



>  .../devicetree/bindings/media/i2c/hynix,hi846.yaml         | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml b/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml
> index 1e2df8cf2937..8b7a46a15458 100644
> --- a/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml
> @@ -14,6 +14,9 @@ description: |-
>    interface and CCI (I2C compatible) control bus. The output format
>    is raw Bayer.
>
> +allOf:
> +  - $ref: /schemas/media/video-interface-devices.yaml#
> +
>  properties:
>    compatible:
>      const: hynix,hi846
> @@ -48,6 +51,10 @@ properties:
>    vddd-supply:
>      description: Definition of the regulator used for the VDDD power supply.
>
> +  orientation: true
> +
> +  rotation: true
> +
>    port:
>      $ref: /schemas/graph.yaml#/$defs/port-base
>      unevaluatedProperties: false
> --
> 2.34.1
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] media: dt-bindings: hynix,hi846: Document orientation and rotation
  2023-09-28 14:57 ` Jacopo Mondi
@ 2023-09-28 15:14   ` Fabio Estevam
  2023-09-28 15:54   ` Rob Herring
  1 sibling, 0 replies; 8+ messages in thread
From: Fabio Estevam @ 2023-09-28 15:14 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: Fabio Estevam, mchehab, sakari.ailus, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, martink, linux-media,
	devicetree, Krzysztof Kozlowski

Hi Jacopo,

On 28/09/2023 11:57, Jacopo Mondi wrote:

> 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#

Should we use unevaluatedProperties: false instead?

--- a/Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml
@@ -85,7 +85,7 @@ required:
    - DOVDD-supply
    - port

-additionalProperties: false
+unevaluatedProperties: false

  examples:
    - |
@@ -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 */

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] media: dt-bindings: hynix,hi846: Document orientation and rotation
  2023-09-28 14:57 ` Jacopo Mondi
  2023-09-28 15:14   ` Fabio Estevam
@ 2023-09-28 15:54   ` Rob Herring
  2023-09-29  6:30     ` Jacopo Mondi
  1 sibling, 1 reply; 8+ messages in thread
From: Rob Herring @ 2023-09-28 15:54 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: Fabio Estevam, mchehab, sakari.ailus, krzysztof.kozlowski+dt,
	conor+dt, martink, linux-media, devicetree, Fabio Estevam,
	Krzysztof Kozlowski

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] media: dt-bindings: hynix,hi846: Document orientation and rotation
  2023-09-28 15:54   ` Rob Herring
@ 2023-09-29  6:30     ` Jacopo Mondi
  2023-09-29 12:04       ` Fabio Estevam
  2023-09-29 14:36       ` Jacopo Mondi
  0 siblings, 2 replies; 8+ messages in thread
From: Jacopo Mondi @ 2023-09-29  6:30 UTC (permalink / raw)
  To: Rob Herring
  Cc: Jacopo Mondi, Fabio Estevam, mchehab, sakari.ailus,
	krzysztof.kozlowski+dt, conor+dt, martink, linux-media,
	devicetree, Fabio Estevam, Krzysztof Kozlowski

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@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.

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] media: dt-bindings: hynix,hi846: Document orientation and rotation
  2023-09-29  6:30     ` Jacopo Mondi
@ 2023-09-29 12:04       ` Fabio Estevam
  2023-09-29 14:36       ` Jacopo Mondi
  1 sibling, 0 replies; 8+ messages in thread
From: Fabio Estevam @ 2023-09-29 12:04 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: Rob Herring, mchehab, sakari.ailus, krzysztof.kozlowski+dt,
	conor+dt, martink, linux-media, devicetree, Fabio Estevam,
	Krzysztof Kozlowski

Hi Jacopo,

On Fri, Sep 29, 2023 at 3:30 AM Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:

> Should I send a patch to or is Fabio interested in doing so as
> part of a new version of this patch ?

Feel free to send it, thanks.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] media: dt-bindings: hynix,hi846: Document orientation and rotation
  2023-09-29  6:30     ` Jacopo Mondi
  2023-09-29 12:04       ` Fabio Estevam
@ 2023-09-29 14:36       ` Jacopo Mondi
  2023-09-29 14:47         ` Fabio Estevam
  1 sibling, 1 reply; 8+ messages in thread
From: Jacopo Mondi @ 2023-09-29 14:36 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: Rob Herring, Fabio Estevam, mchehab, sakari.ailus,
	krzysztof.kozlowski+dt, conor+dt, martink, linux-media,
	devicetree, Fabio Estevam, Krzysztof Kozlowski

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@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.
>
> 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 ?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] media: dt-bindings: hynix,hi846: Document orientation and rotation
  2023-09-29 14:36       ` Jacopo Mondi
@ 2023-09-29 14:47         ` Fabio Estevam
  0 siblings, 0 replies; 8+ messages in thread
From: Fabio Estevam @ 2023-09-29 14:47 UTC (permalink / raw)
  To: Jacopo Mondi
  Cc: Rob Herring, mchehab, sakari.ailus, krzysztof.kozlowski+dt,
	conor+dt, martink, linux-media, devicetree, Fabio Estevam,
	Krzysztof Kozlowski

Hi Jacopo,

On Fri, Sep 29, 2023 at 11:36 AM Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:

> Actually, if I'm going to s/additionalProperties/unevaluatedProperties/
> in this bindings as well, am I wrong this patch is not needed anymore ?

You can discard this patch.

Make sure that the schema warning below is gone:

imx8mq-librem5-r3.dtb: camera@20: 'orientation', 'rotation' do not
match any of the regexes: 'pinctrl-[0-9]+'
from schema $id: http://devicetree.org/schemas/media/i2c/hynix,hi846.yaml#

The diff below works for me:

--- a/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml
@@ -14,6 +14,9 @@ description: |-
   interface and CCI (I2C compatible) control bus. The output format
   is raw Bayer.

+allOf:
+  - $ref: /schemas/media/video-interface-devices.yaml#
+
 properties:
   compatible:
     const: hynix,hi846
@@ -86,7 +89,7 @@ required:
   - vddd-supply
   - port

-additionalProperties: false
+unevaluatedProperties: false

 examples:
   - |

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-09-29 14:48 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-28 12:14 [PATCH v2] media: dt-bindings: hynix,hi846: Document orientation and rotation Fabio Estevam
2023-09-28 14:57 ` Jacopo Mondi
2023-09-28 15:14   ` Fabio Estevam
2023-09-28 15:54   ` Rob Herring
2023-09-29  6:30     ` Jacopo Mondi
2023-09-29 12:04       ` Fabio Estevam
2023-09-29 14:36       ` Jacopo Mondi
2023-09-29 14:47         ` Fabio Estevam

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.