diff mbox series

[RFC,v3] dt-binding: media: document ON Semi AR0521 sensor bindings

Message ID m37dhkdrat.fsf@t19.piap.pl
State Superseded
Headers show
Series [RFC,v3] dt-binding: media: document ON Semi AR0521 sensor bindings | expand

Commit Message

Krzysztof Hałasa July 21, 2021, 8:06 a.m. UTC
This file documents DT bindings for the AR0521 camera sensor driver.

Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>
---
Changes from v2:
- changed "xclk" to "extclk"
- power regulator names etc.
- video output port properties
- cosmetics
- UTF-8 experiments :-)

Comments

Rob Herring (Arm) July 26, 2021, 10:45 p.m. UTC | #1
On Wed, Jul 21, 2021 at 10:06:34AM +0200, Krzysztof Hałasa wrote:
> This file documents DT bindings for the AR0521 camera sensor driver.
> 
> Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>
> ---
> Changes from v2:
> - changed "xclk" to "extclk"
> - power regulator names etc.
> - video output port properties
> - cosmetics
> - UTF-8 experiments :-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml
> new file mode 100644
> index 000000000000..785bae61bb5e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml
> @@ -0,0 +1,108 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/onnn,ar0521.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ON Semiconductor AR0521 MIPI CSI-2 sensor
> +
> +maintainers:
> +  - Krzysztof Hałasa <khalasa@piap.pl>
> +
> +description: |-
> +  The AR0521 is a raw CMOS image sensor with MIPI CSI-2 and
> +  I2C-compatible control interface.
> +
> +properties:
> +  compatible:
> +    const: onnn,ar0521
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: extclk
> +
> +  vaa-supply:
> +    description:
> +      Definition of the regulator used as analog (2.7 V) voltage supply.
> +
> +  vdd-supply:
> +    description:
> +      Definition of the regulator used as digital core (1.2 V) voltage supply.
> +
> +  vdd_io-supply:
> +    description:
> +      Definition of the regulator used as digital I/O (1.8 V) voltage supply.
> +
> +  reset-gpios:
> +    description: reset GPIO, usually active low
> +    maxItems: 1
> +
> +  port:
> +    $ref: /schemas/graph.yaml#/properties/port

$ref: /schemas/graph.yaml#/$defs/port-base
unevaluatedProperties: false


> +    description: |
> +      Video output port.
> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/media/video-interfaces.yaml#

           unevaluatedProperties: false

> +
> +        properties:
> +          data-lanes:
> +            anyOf:
> +              - items:
> +                  - const: 1
> +              - items:
> +                  - const: 1
> +                  - const: 2
> +              - items:
> +                  - const: 1
> +                  - const: 2
> +                  - const: 3
> +                  - const: 4
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - vaa-supply
> +  - vdd-supply
> +  - vdd_io-supply
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/clock/imx6qdl-clock.h>
> +
> +    i2c {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            ar0521: camera-sensor@36 {
> +                    compatible = "onnn,ar0521";
> +                    reg = <0x36>;
> +                    pinctrl-names = "default";
> +                    pinctrl-0 = <&pinctrl_mipi_camera>;
> +                    clocks = <&clks IMX6QDL_CLK_CKO>;
> +                    clock-names = "extclk";
> +                    reset-gpios = <&gpio1 7 GPIO_ACTIVE_LOW>;
> +                    vaa-supply = <&reg_2p7v>;
> +                    vdd-supply = <&reg_1p2v>;
> +                    vdd_io-supply = <&reg_1p8v>;
> +
> +                    port {
> +                           mipi_camera_to_mipi_csi2: endpoint {
> +                                    remote-endpoint = <&mipi_csi2_in>;
> +                                    data-lanes = <1 2 3 4>;
> +                            };
> +                    };
> +            };
> +    };
> 
> -- 
> Krzysztof "Chris" Hałasa
> 
> Sieć Badawcza Łukasiewicz
> Przemysłowy Instytut Automatyki i Pomiarów PIAP
> Al. Jerozolimskie 202, 02-486 Warszawa
>
Laurent Pinchart July 27, 2021, 1:08 a.m. UTC | #2
Hi Krzysztof,

(CC'ing Sakari Ailus)

Thank you for the patch.

On Wed, Jul 21, 2021 at 10:06:34AM +0200, Krzysztof Hałasa wrote:
> This file documents DT bindings for the AR0521 camera sensor driver.
> 
> Signed-off-by: Krzysztof Hałasa <khalasa@piap.pl>
> ---
> Changes from v2:
> - changed "xclk" to "extclk"
> - power regulator names etc.
> - video output port properties
> - cosmetics
> - UTF-8 experiments :-)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml
> new file mode 100644
> index 000000000000..785bae61bb5e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml
> @@ -0,0 +1,108 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/onnn,ar0521.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ON Semiconductor AR0521 MIPI CSI-2 sensor
> +
> +maintainers:
> +  - Krzysztof Hałasa <khalasa@piap.pl>
> +
> +description: |-
> +  The AR0521 is a raw CMOS image sensor with MIPI CSI-2 and
> +  I2C-compatible control interface.
> +
> +properties:
> +  compatible:
> +    const: onnn,ar0521
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: extclk
> +
> +  vaa-supply:
> +    description:
> +      Definition of the regulator used as analog (2.7 V) voltage supply.
> +
> +  vdd-supply:
> +    description:
> +      Definition of the regulator used as digital core (1.2 V) voltage supply.
> +
> +  vdd_io-supply:
> +    description:
> +      Definition of the regulator used as digital I/O (1.8 V) voltage supply.
> +
> +  reset-gpios:
> +    description: reset GPIO, usually active low
> +    maxItems: 1
> +
> +  port:
> +    $ref: /schemas/graph.yaml#/properties/port
> +    description: |
> +      Video output port.
> +
> +    properties:
> +      endpoint:
> +        $ref: /schemas/media/video-interfaces.yaml#
> +
> +        properties:
> +          data-lanes:
> +            anyOf:
> +              - items:
> +                  - const: 1
> +              - items:
> +                  - const: 1
> +                  - const: 2
> +              - items:
> +                  - const: 1
> +                  - const: 2
> +                  - const: 3
> +                  - const: 4

As the sensor also supports an HiSPi output, I would add the bus-type
property:

          data-lanes:
	    const: 4

Sakari, what do you think ? This way we won't have to rely on an
implicit default when (and if) the kernel gets support for HiSPi.

With or without this change, and with Rob's comments addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thank you for not giving up :-)

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - vaa-supply
> +  - vdd-supply
> +  - vdd_io-supply
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/clock/imx6qdl-clock.h>
> +
> +    i2c {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            ar0521: camera-sensor@36 {
> +                    compatible = "onnn,ar0521";
> +                    reg = <0x36>;
> +                    pinctrl-names = "default";
> +                    pinctrl-0 = <&pinctrl_mipi_camera>;
> +                    clocks = <&clks IMX6QDL_CLK_CKO>;
> +                    clock-names = "extclk";
> +                    reset-gpios = <&gpio1 7 GPIO_ACTIVE_LOW>;
> +                    vaa-supply = <&reg_2p7v>;
> +                    vdd-supply = <&reg_1p2v>;
> +                    vdd_io-supply = <&reg_1p8v>;
> +
> +                    port {
> +                           mipi_camera_to_mipi_csi2: endpoint {
> +                                    remote-endpoint = <&mipi_csi2_in>;
> +                                    data-lanes = <1 2 3 4>;
> +                            };
> +                    };
> +            };
> +    };
>
Krzysztof Hałasa July 27, 2021, 10:36 a.m. UTC | #3
Hi Laurent,

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

>> +        properties:

>> +          data-lanes:

>> +            anyOf:

>> +              - items:

>> +                  - const: 1

>> +              - items:

>> +                  - const: 1

>> +                  - const: 2

>> +              - items:

>> +                  - const: 1

>> +                  - const: 2

>> +                  - const: 3

>> +                  - const: 4

>

> As the sensor also supports an HiSPi output, I would add the bus-type

> property:

>

>           data-lanes:

> 	    const: 4


Is there any example of this? I'm not sure how should it it look like.
Something like the following?

        properties:
         data-lanes:
            anyOf:
              - items:
                  - const: 1
              - items:
                  - const: 1
                  - const: 2
              - items:
                  - const: 1
                  - const: 2
                  - const: 3
                  - const: 4
          bus-type:
            data-lanes:
              const: 4

And... HiSPi would need additional code in the driver. And preferably
some testing. I think I'd prefer to have DT and the driver staying in
some sort of sync. Also, I'm uncertain about the syntax and the meaning
of such, apparently redundant, construct. Nor about its relation to
HiSPi. An example would be welcome.
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
Sakari Ailus July 27, 2021, 10:58 a.m. UTC | #4
Hi Krzysztof,

On Tue, Jul 27, 2021 at 12:36:20PM +0200, Krzysztof Hałasa wrote:
> Hi Laurent,
> 
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> 
> >> +        properties:
> >> +          data-lanes:
> >> +            anyOf:
> >> +              - items:
> >> +                  - const: 1
> >> +              - items:
> >> +                  - const: 1
> >> +                  - const: 2
> >> +              - items:
> >> +                  - const: 1
> >> +                  - const: 2
> >> +                  - const: 3
> >> +                  - const: 4
> >
> > As the sensor also supports an HiSPi output, I would add the bus-type
> > property:
> >
> >           data-lanes:
> > 	    const: 4
> 
> Is there any example of this? I'm not sure how should it it look like.
> Something like the following?
> 
>         properties:
>          data-lanes:
>             anyOf:
>               - items:
>                   - const: 1
>               - items:
>                   - const: 1
>                   - const: 2
>               - items:
>                   - const: 1
>                   - const: 2
>                   - const: 3
>                   - const: 4
>           bus-type:
>             data-lanes:
>               const: 4

I think Laurent meant:

	    bus-type:
	      const: 4

This way the bindings can be later amended with HiSPi support without
relying on defaults. Albeit the other busses in practice almost never end
up being used even if supported, apart from the standard BT.601, BT.656 and
CSI-2.

Either way is fine IMO.

> 
> And... HiSPi would need additional code in the driver. And preferably
> some testing. I think I'd prefer to have DT and the driver staying in
> some sort of sync. Also, I'm uncertain about the syntax and the meaning
> of such, apparently redundant, construct. Nor about its relation to
> HiSPi. An example would be welcome.

No need to add support for the driver.
Krzysztof Hałasa July 27, 2021, 11:03 a.m. UTC | #5
Hi Sakari,

Sakari Ailus <sakari.ailus@iki.fi> writes:

> I think Laurent meant:
>
> 	    bus-type:
> 	      const: 4
>
> This way the bindings can be later amended with HiSPi support without
> relying on defaults. Albeit the other busses in practice almost never end
> up being used even if supported, apart from the standard BT.601, BT.656 and
> CSI-2.
>
> Either way is fine IMO.

Ok, so I'll leave it as is (apart from Rob's additions/changes).

> No need to add support for the driver.

Ok.

Thanks.
Krzysztof Hałasa July 30, 2021, 7:12 a.m. UTC | #6
Sakari, Laurent,

Sakari Ailus <sakari.ailus@iki.fi> writes:

> I think Laurent meant:

>

> 	    bus-type:

> 	      const: 4


Ok I found what it means :-)
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml b/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml
new file mode 100644
index 000000000000..785bae61bb5e
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/onnn,ar0521.yaml
@@ -0,0 +1,108 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/i2c/onnn,ar0521.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ON Semiconductor AR0521 MIPI CSI-2 sensor
+
+maintainers:
+  - Krzysztof Hałasa <khalasa@piap.pl>
+
+description: |-
+  The AR0521 is a raw CMOS image sensor with MIPI CSI-2 and
+  I2C-compatible control interface.
+
+properties:
+  compatible:
+    const: onnn,ar0521
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: extclk
+
+  vaa-supply:
+    description:
+      Definition of the regulator used as analog (2.7 V) voltage supply.
+
+  vdd-supply:
+    description:
+      Definition of the regulator used as digital core (1.2 V) voltage supply.
+
+  vdd_io-supply:
+    description:
+      Definition of the regulator used as digital I/O (1.8 V) voltage supply.
+
+  reset-gpios:
+    description: reset GPIO, usually active low
+    maxItems: 1
+
+  port:
+    $ref: /schemas/graph.yaml#/properties/port
+    description: |
+      Video output port.
+
+    properties:
+      endpoint:
+        $ref: /schemas/media/video-interfaces.yaml#
+
+        properties:
+          data-lanes:
+            anyOf:
+              - items:
+                  - const: 1
+              - items:
+                  - const: 1
+                  - const: 2
+              - items:
+                  - const: 1
+                  - const: 2
+                  - const: 3
+                  - const: 4
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - vaa-supply
+  - vdd-supply
+  - vdd_io-supply
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/clock/imx6qdl-clock.h>
+
+    i2c {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            ar0521: camera-sensor@36 {
+                    compatible = "onnn,ar0521";
+                    reg = <0x36>;
+                    pinctrl-names = "default";
+                    pinctrl-0 = <&pinctrl_mipi_camera>;
+                    clocks = <&clks IMX6QDL_CLK_CKO>;
+                    clock-names = "extclk";
+                    reset-gpios = <&gpio1 7 GPIO_ACTIVE_LOW>;
+                    vaa-supply = <&reg_2p7v>;
+                    vdd-supply = <&reg_1p2v>;
+                    vdd_io-supply = <&reg_1p8v>;
+
+                    port {
+                           mipi_camera_to_mipi_csi2: endpoint {
+                                    remote-endpoint = <&mipi_csi2_in>;
+                                    data-lanes = <1 2 3 4>;
+                            };
+                    };
+            };
+    };