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

[Marco Felsch <m.felsch@pengutronix.de>]

On 19-02-13 18:57, Jacopo Mondi wrote:
> Hi Marco,
>     thanks for the patch.
> 
> I have some comments, which I hope might get the ball rolling...

Hi Jacopo,

thanks for your review and sorry for the late response. My schedule was
a bit filled.

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

You're right, I will drop this statement.

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

You're right, the SPI interface is only supported in that mode.

Should I add something like:
It is programmable trough I2C and SPI. The SPI interface is only
supported in parallel-in -> csi-out mode.
 
> > +
> > +Required Properties:
> > +
> > +- compatible: should be "toshiba,tc358746"
> > +- reg: should be <0x0e>
> 
> nit: s/should/shall

Okay.

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

Okay.

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

Yes it's optional, but the device can also act as clock provider (not
now, patches in my personal queue for rework). So I won't drop it since
I never linked the generic clock-bindings.

> > +
> > +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"

Okay.

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

Nope, the device has one CSI and one Parallel interface. These
interfaces can be configured as receiver or as transmitter (according to
the selected mode). I got you but I remember also the discussion with
Mauro, Hans, Sakari about the TVP5150 ports. The result of that
discussion was "don't introduce 'virtual' ports". If I got you right
your Idea will introduce virtual ports too:

/* Parallel */
port@0{
	port@0 { ... }; /* input case */
	port@1 { ... }; /* output case */
};

/* CSI */
port@1{
	port@0 { ... }; /* input case */
	port@1 { ... }; /* output case */
};

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

I wouldn't map the combinations to the device tree since it is the
hw-abstraction and the signals still routed to the same pads. The only
difference in the CSI-2->Parallel case is the timing calculation which
is out of scope for the dt.

> 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

You're right, I will change that.

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

Nope it can't. Only the data-lanes can be disabled seperatly so I added
the data-lanes property to determine that number and for the sake of
completeness I added the clock-lanes property.

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

That's not that easy to answer. The user can add different link-freq.
the driver can choose. This is relevant for the Parallel-in --> CSI-out.
If the external pclk is to slow (due to dyn. fps change) and the link-freq.
is to fast the internally pixel buffer throws underrun interrupts. The
user notice that by green pixel artifacts. If the user adds more
possible link-freq. the driver will switch to that one wich full fill
the timings to avoid a fifo underrun.

> 
> Thanks
>    j

Regards,
Marco

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



-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |