Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 27 Jul 2021 04:08:02 +0300
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Krzysztof =?utf-8?Q?Ha=C5=82asa?= <khalasa@piap.pl>
Cc:     devicetree@vger.kernel.org,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Rob Herring <robh+dt@kernel.org>, linux-media@vger.kernel.org,
        linux-kernel@vger.kernel.org, Sakari Ailus <sakari.ailus@iki.fi>
Subject: Re: [RFC v3] dt-binding: media: document ON Semi AR0521 sensor
 bindings
Message-ID: <YP9ccgd7WNpHuLgG@pendragon.ideasonboard.com>
References: <m37dhkdrat.fsf@t19.piap.pl>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <m37dhkdrat.fsf@t19.piap.pl>
Precedence: bulk

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>;
> +                            };
> +                    };
> +            };
> +    };
> 

-- 
Regards,

Laurent Pinchart