Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 5 Sep 2022 01:35:53 +0300
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Marco Felsch <m.felsch@pengutronix.de>
Cc:     mchehab@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, vkoul@kernel.org, kishon@ti.com,
        sakari.ailus@linux.intel.com, hverkuil@xs4all.nl,
        jacopo@jmondi.org, linux-kernel@vger.kernel.org,
        linux-phy@lists.infradead.org, devicetree@vger.kernel.org,
        linux-media@vger.kernel.org, kernel@pengutronix.de
Subject: Re: [PATCH 3/4] media: dt-bindings: add bindings for Toshiba TC358746
Message-ID: <YxUoSfbrPPPT5nz6@pendragon.ideasonboard.com>
References: <20220818143307.967150-1-m.felsch@pengutronix.de>
 <20220818143307.967150-4-m.felsch@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220818143307.967150-4-m.felsch@pengutronix.de>
Precedence: bulk

Hi Marco,

Thank you for the patch.

On Thu, Aug 18, 2022 at 04:33:06PM +0200, Marco Felsch wrote:
> Add the bindings for the Toshiba TC358746 Parallel <-> MIPI-CSI bridge
> driver.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  .../bindings/media/i2c/toshiba,tc358746.yaml  | 157 ++++++++++++++++++
>  1 file changed, 157 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/toshiba,tc358746.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/toshiba,tc358746.yaml b/Documentation/devicetree/bindings/media/i2c/toshiba,tc358746.yaml
> new file mode 100644
> index 000000000000..9783cca363c6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/toshiba,tc358746.yaml
> @@ -0,0 +1,157 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/toshiba,tc358746.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Toshiba TC358746 Parallel to MIPI CSI2 Bridge
> +
> +maintainers:
> +  - Marco Felsch <kernel@pengutronix.de>
> +
> +description: |-
> +  The Toshiba TC358746 converts a parallel video stream into a MIPI CSI-2
> +  stream. The direction can be either parallel-in -> csi-out or csi-in ->
> +  parallel-out The chip is programmable trough I2C and SPI but the SPI
> +  interface is only supported in parallel-in -> csi-out mode.
> +
> +  Note that the current device tree bindings only support the
> +  parallel-in -> csi-out path.
> +
> +properties:
> +  compatible:
> +    const: toshiba,tc358746
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    description:
> +      The phandle to the reference clock source. This corresponds to the
> +      hardware pin REFCLK.
> +    maxItems: 1
> +
> +  clock-names:
> +    const: refclk

As there's a single clock, should we omit the clock name ?

> +
> +# The bridge can act as clock provider for the sensor. To enable this support
> +# #clock-cells must be specified. Attention if this feature is used than the

s/than/then/

> +# mclk rate must be at least: (2 * link-frequency) / 8
> +#                             `------------------´   ^
> +#                             internal PLL rate   smallest possible mclk-div

Could this be captured in the description of the property instead of a
comment ?

> +  "#clock-cells":
> +    const: 0
> +
> +  clock-output-names:
> +    description:
> +      The clock name of the MCLK output, the default name is tc358746-mclk.
> +    maxItems: 1
> +
> +  vddc-supply:
> +    description: Digital core voltage supply, 1.2 volts
> +
> +  vddio-supply:
> +    description: Digital I/O voltage supply, 1.8 volts
> +
> +  vddmipi-supply:
> +    description: MIPI CSI phy voltage supply, 1.2 volts
> +
> +  reset-gpios:
> +    description:
> +      The phandle and specifier for the GPIO that controls the chip reset.
> +      This corresponds to the hardware pin RESX which is physically active low.
> +    maxItems: 1
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: Input port

Are the H/V sync signal polarities fixed, or should they be specified
here ? Does the chip support external sync only, or also BT.656 ? In the
latter case this needs a bus-type.

> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        description: Output port
> +
> +        properties:
> +          endpoint:
> +            $ref: /schemas/media/video-interfaces.yaml#
> +            unevaluatedProperties: false
> +
> +            properties:
> +              data-lanes:
> +                minItems: 1
> +                maxItems: 4
> +
> +              clock-noncontinuous: true
> +              link-frequencies: true
> +
> +            required:
> +              - data-lanes
> +              - link-frequencies
> +
> +    required:
> +      - port@0
> +      - port@1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - vddc-supply
> +  - vddio-supply
> +  - vddmipi-supply
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      csi-bridge@e {
> +        compatible = "toshiba,tc358746";
> +        reg = <0xe>;
> +
> +        clocks = <&refclk>;
> +        clock-names = "refclk";
> +
> +        reset-gpios = <&gpio 2 GPIO_ACTIVE_LOW>;
> +
> +        vddc-supply = <&v1_2d>;
> +        vddio-supply = <&v1_8d>;
> +        vddmipi-supply = <&v1_2d>;
> +
> +        /* sensor mclk provider */
> +        #clock-cells = <0>;
> +
> +        ports {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          /* Input */
> +          port@0 {
> +            reg = <0>;
> +            tc358746_in: endpoint {
> +              remote-endpoint = <&sensor_out>;
> +              };

Wrong indentation here.

> +          };
> +
> +          /* Output */
> +          port@1 {
> +            reg = <1>;
> +            tc358746_out: endpoint {
> +              remote-endpoint = <&mipi_csi2_in>;
> +              data-lanes = <1 2>;
> +              clock-noncontinuous;
> +              link-frequencies = /bits/ 64 <216000000>;
> +            };
> +          };
> +        };
> +      };
> +    };

-- 
Regards,

Laurent Pinchart