Re: [PATCH 1/3] media: dt-bindings: add bindings for Toshiba TC358746

[Jacopo Mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 4821 bytes --]

Hi Marco,
    thanks for the patch.

I have some comments, which I hope might get the ball rolling...

On Tue, Dec 18, 2018 at 03:12:38PM +0100, Marco Felsch wrote:
> Add corresponding dt-bindings for the Toshiba tc358746 device.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  .../bindings/media/i2c/toshiba,tc358746.txt   | 80 +++++++++++++++++++
>  1 file changed, 80 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/toshiba,tc358746.txt
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/toshiba,tc358746.txt b/Documentation/devicetree/bindings/media/i2c/toshiba,tc358746.txt
> new file mode 100644
> index 000000000000..499733df744a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/toshiba,tc358746.txt
> @@ -0,0 +1,80 @@
> +* Toshiba TC358746 Parallel to MIPI CSI2-TX or MIPI CSI2-RX to Parallel Bridge
> +
> +The Toshiba TC358746 is a bridge that converts a Parallel-in stream to MIPI CSI-2 TX

nit:
s/is a bridge that/is a bridge device that/
or drop is a bridge completely?

> +or a MIPI CSI-2 RX stream into a Parallel-out. It is programmable through I2C.

From the thin public available datasheet, it seems to support SPI as
programming interface, but only when doing Parallel->CSI-2. I would
mention that.

> +
> +Required Properties:
> +
> +- compatible: should be "toshiba,tc358746"
> +- reg: should be <0x0e>

nit: s/should/shall

> +- clocks: should contain a phandle link to the reference clock source

just "phandle to the reference clock source" ?

> +- clock-names: the clock input is named "refclk".

According to the clock bindings this is optional, and since you have
a single clock I would drop it.

> +
> +Optional Properties:
> +
> +- reset-gpios: gpio phandle GPIO connected to the reset pin

would you drop one of the two "gpio" here. Like ": phandle to the GPIO
connected to the reset input pin"

> +
> +Parallel Endpoint:

Here I got confused. The chip supports 2 inputs (parallel and CSI-2)
and two outputs (parallel and CSI-2 again). You mention endpoints
propery only here, but it seems from the example you want two ports,
with one endpoint child-node each.

Even if the driver does not support CSI-2->Parallel at the moment,
bindings should be future-proof, so I would reserve the first two
ports for the inputs, and the last two for the output, or, considering
that the two input-output combinations are mutually exclusive, provide
one "input" port with two optional endpoints, and one "output" port with
two optional endpoints.

In both cases only one input and one output at the time could be
described in DT. Up to you, maybe others have different ideas as
well...

> +
> +Required Properties:
> +
> +- reg: should be <0>
> +- bus-width: the data bus width e.g. <8> for eight bit bus, or <16>
> +	     for sixteen bit wide bus.

The chip seems to support up to 24 bits of data bus width

> +
> +MIPI CSI-2 Endpoint:
> +
> +Required Properties:
> +
> +- reg: should be <1>
> +- data-lanes: should be <1 2 3 4> for four-lane operation,
> +	      or <1 2> for two-lane operation
> +- clock-lanes: should be <0>

Can this be changed? If the chip does not allow lane re-ordering you
could drop this.

> +- link-frequencies: List of allowed link frequencies in Hz. Each frequency is
> +		    expressed as a 64-bit big-endian integer. The frequency
> +		    is half of the bps per lane due to DDR transmission.

Does the chip supports a limited set of bus frequencies, or are this
"hints" ? I admit this property actually puzzles me, so I might got it
wrong..

Thanks
   j

> +
> +Optional Properties:
> +
> +- clock-noncontinuous: Presence of this boolean property decides whether the
> +		       MIPI CSI-2 clock is continuous or non-continuous.
> +
> +For further information on the endpoint node properties, see
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example:
> +
> +&i2c {
> +	tc358746: tc358746@0e {
> +		reg = <0x0e>;
> +		compatible = "toshiba,tc358746";
> +		pinctrl-names = "default";
> +		clocks = <&clk_cam_ref>;
> +		clock-names = "refclk";
> +		reset-gpios = <&gpio3 2 GPIO_ACTIVE_LOW>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		port@0 {
> +			reg = <0>;
> +
> +			tc358746_parallel_in: endpoint {
> +				bus-width = <8>;
> +				remote-endpoint = <&micron_parallel_out>;
> +			};
> +		};
> +
> +		port@1 {
> +			reg = <1>;
> +
> +			tc358746_mipi2_out: endpoint {
> +				remote-endpoint = <&mipi_csi2_in>;
> +				data-lanes = <1 2>;
> +				clock-lanes = <0>;
> +				clock-noncontinuous;
> +				link-frequencies = /bits/ 64 <216000000>;
> +			};
> +		};
> +	};
> +};
> --
> 2.19.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]