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

Commit Message

Marco Felsch Dec. 18, 2018, 2:12 p.m. UTC

Add corresponding dt-bindings for the Toshiba tc358746 device.

Signed-off-by: Marco Felsch <[e-mail]>
---
 .../bindings/media/i2c/toshiba,tc358746.txt   | 80 +++++++++++++++++++
 1 file changed, 80 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/toshiba,tc358746.txt

Comments

Rob Herring (Arm) Dec. 28, 2018, 11:10 p.m. UTC | #1

On Tue, 18 Dec 2018 15:12:38 +0100, Marco Felsch wrote:
> Add corresponding dt-bindings for the Toshiba tc358746 device.
> 
> Signed-off-by: Marco Felsch <[e-mail]>
> ---
>  .../bindings/media/i2c/toshiba,tc358746.txt   | 80 +++++++++++++++++++
>  1 file changed, 80 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/toshiba,tc358746.txt
> 

Reviewed-by: Rob Herring <[e-mail]>

Jacopo Mondi Feb. 13, 2019, 5:57 p.m. UTC | #2

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 <[e-mail]>
> ---
>  .../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
>

Sakari Ailus Feb. 18, 2019, 10:03 a.m. UTC | #3

Hi Marco,

My apologies for reviewing this so late. You've received good comments
already. I have a few more.

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 <[e-mail]>
> ---
>  .../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
> +or a MIPI CSI-2 RX stream into a Parallel-out. It is programmable through I2C.

This is interesting. The driver somehow needs to figure out the direction
of the data flow if it does not originate from DT. I guess it shouldn't as
it's not the property of an individual device, albeit in practice in all
hardware I've seen the direction of the pipeline is determinable and this
is visible in the kAPI as well. So I'm suggesting no changes due to this in
bindings, likely we'll need to address it somehow elsewhere going forward.

> +
> +Required Properties:
> +
> +- compatible: should be "toshiba,tc358746"
> +- reg: should be <0x0e>
> +- clocks: should contain a phandle link to the reference clock source
> +- clock-names: the clock input is named "refclk".
> +
> +Optional Properties:
> +
> +- reset-gpios: gpio phandle GPIO connected to the reset pin
> +
> +Parallel Endpoint:
> +
> +Required Properties:

It'd be nice if the relation between these sections would be somehow
apparent. E.g. using different underlining, such as in
Documentation/devicetree/bindings/media/ti,omap3isp.txt .

> +
> +- reg: should be <0>
> +- bus-width: the data bus width e.g. <8> for eight bit bus, or <16>
> +	     for sixteen bit wide bus.
> +
> +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>
> +- 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.
> +
> +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 {

The node name should be a generic name of the type of the device, not the
name of the specific device as such. A similar Cadence device uses
"csi-bridge".

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

Marco Felsch March 1, 2019, 10:26 a.m. UTC | #4

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 <[e-mail]>
> > ---
> >  .../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
> >

Marco Felsch March 1, 2019, 10:52 a.m. UTC | #5

Hi Sakari,

On 19-02-18 12:03, Sakari Ailus wrote:
> Hi Marco,
> 
> My apologies for reviewing this so late. You've received good comments
> already. I have a few more.

Thanks for your review for the other patches as well =) Sorry for my
delayed response.

> 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 <[e-mail]>
> > ---
> >  .../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
> > +or a MIPI CSI-2 RX stream into a Parallel-out. It is programmable through I2C.
> 
> This is interesting. The driver somehow needs to figure out the direction
> of the data flow if it does not originate from DT. I guess it shouldn't as
> it's not the property of an individual device, albeit in practice in all
> hardware I've seen the direction of the pipeline is determinable and this
> is visible in the kAPI as well. So I'm suggesting no changes due to this in
> bindings, likely we'll need to address it somehow elsewhere going forward.

What did you mean with "... and this is visible in the kAPI as well"?
I'm relative new in the linux-media world but I never saw a device which
supports two directions. Our customer which uses that chip use it
only in parallel-in/csi-out mode. To be flexible the switching should be
done by a subdev-ioctl but it is also reasonable to define a default value
within the DT.

> > +
> > +Required Properties:
> > +
> > +- compatible: should be "toshiba,tc358746"
> > +- reg: should be <0x0e>
> > +- clocks: should contain a phandle link to the reference clock source
> > +- clock-names: the clock input is named "refclk".
> > +
> > +Optional Properties:
> > +
> > +- reset-gpios: gpio phandle GPIO connected to the reset pin
> > +
> > +Parallel Endpoint:
> > +
> > +Required Properties:
> 
> It'd be nice if the relation between these sections would be somehow
> apparent. E.g. using different underlining, such as in
> Documentation/devicetree/bindings/media/ti,omap3isp.txt .

Thats a really good example thanks.

> 
> > +
> > +- reg: should be <0>
> > +- bus-width: the data bus width e.g. <8> for eight bit bus, or <16>
> > +	     for sixteen bit wide bus.
> > +
> > +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>
> > +- 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.
> > +
> > +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 {
> 
> The node name should be a generic name of the type of the device, not the
> name of the specific device as such. A similar Cadence device uses
> "csi-bridge".

Okay, I will change that.

> 
> > +		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>;
> > +			};
> > +		};
> > +	};
> > +};
> 
> -- 
> Kind regards,
> 
> Sakari Ailus
> [e-mail]
>

Ian Arkver March 1, 2019, 11:07 a.m. UTC | #6

Hi,

On 01/03/2019 10:52, Marco Felsch wrote:
> Hi Sakari,
> 
> On 19-02-18 12:03, Sakari Ailus wrote:
>> Hi Marco,
>>
>> My apologies for reviewing this so late. You've received good comments
>> already. I have a few more.
> 
> Thanks for your review for the other patches as well =) Sorry for my
> delayed response.
> 
>> 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 <[e-mail]>
>>> ---
>>>   .../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
>>> +or a MIPI CSI-2 RX stream into a Parallel-out. It is programmable through I2C.
>>
>> This is interesting. The driver somehow needs to figure out the direction
>> of the data flow if it does not originate from DT. I guess it shouldn't as
>> it's not the property of an individual device, albeit in practice in all
>> hardware I've seen the direction of the pipeline is determinable and this
>> is visible in the kAPI as well. So I'm suggesting no changes due to this in
>> bindings, likely we'll need to address it somehow elsewhere going forward.
> 
> What did you mean with "... and this is visible in the kAPI as well"?
> I'm relative new in the linux-media world but I never saw a device which
> supports two directions. Our customer which uses that chip use it
> only in parallel-in/csi-out mode. To be flexible the switching should be
> done by a subdev-ioctl but it is also reasonable to define a default value
> within the DT.

The mode is set by a pin strap at reset time (MSEL). It's not 
programmable by i2c. As far as I can see, looking at the registers, it's 
also not readable by i2c, so there's no easy way for a driver which 
supports both modes to see what the pinstrap is set to.

I'm not sure if the driver could tell from the direction of the 
endpoints it's linked to which mode to use, but if not it'll need to be 
told somehow and a DT property seems reasonable to me. Given that the 
same pins are used in each direction I think the direction is most 
likely to be hard wired and board specific.

Regards,
Ian.

>>> +
>>> +Required Properties:
>>> +
>>> +- compatible: should be "toshiba,tc358746"
>>> +- reg: should be <0x0e>
>>> +- clocks: should contain a phandle link to the reference clock source
>>> +- clock-names: the clock input is named "refclk".
>>> +
>>> +Optional Properties:
>>> +
>>> +- reset-gpios: gpio phandle GPIO connected to the reset pin
>>> +
>>> +Parallel Endpoint:
>>> +
>>> +Required Properties:
>>
>> It'd be nice if the relation between these sections would be somehow
>> apparent. E.g. using different underlining, such as in
>> Documentation/devicetree/bindings/media/ti,omap3isp.txt .
> 
> Thats a really good example thanks.
> 
>>
>>> +
>>> +- reg: should be <0>
>>> +- bus-width: the data bus width e.g. <8> for eight bit bus, or <16>
>>> +	     for sixteen bit wide bus.
>>> +
>>> +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>
>>> +- 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.
>>> +
>>> +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 {
>>
>> The node name should be a generic name of the type of the device, not the
>> name of the specific device as such. A similar Cadence device uses
>> "csi-bridge".
> 
> Okay, I will change that.
> 
>>
>>> +		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>;
>>> +			};
>>> +		};
>>> +	};
>>> +};
>>
>> -- 
>> Kind regards,
>>
>> Sakari Ailus
>> [e-mail]
>>
>

Marco Felsch March 1, 2019, 1:01 p.m. UTC | #7

Hi Ian,

On 19-03-01 11:07, Ian Arkver wrote:
> Hi,
> 
> On 01/03/2019 10:52, Marco Felsch wrote:
> > Hi Sakari,
> > 
> > On 19-02-18 12:03, Sakari Ailus wrote:
> > > Hi Marco,
> > > 
> > > My apologies for reviewing this so late. You've received good comments
> > > already. I have a few more.
> > 
> > Thanks for your review for the other patches as well =) Sorry for my
> > delayed response.
> > 
> > > 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 <[e-mail]>
> > > > ---
> > > >   .../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
> > > > +or a MIPI CSI-2 RX stream into a Parallel-out. It is programmable through I2C.
> > > 
> > > This is interesting. The driver somehow needs to figure out the direction
> > > of the data flow if it does not originate from DT. I guess it shouldn't as
> > > it's not the property of an individual device, albeit in practice in all
> > > hardware I've seen the direction of the pipeline is determinable and this
> > > is visible in the kAPI as well. So I'm suggesting no changes due to this in
> > > bindings, likely we'll need to address it somehow elsewhere going forward.
> > 
> > What did you mean with "... and this is visible in the kAPI as well"?
> > I'm relative new in the linux-media world but I never saw a device which
> > supports two directions. Our customer which uses that chip use it
> > only in parallel-in/csi-out mode. To be flexible the switching should be
> > done by a subdev-ioctl but it is also reasonable to define a default value
> > within the DT.
> 
> The mode is set by a pin strap at reset time (MSEL). It's not programmable
> by i2c. As far as I can see, looking at the registers, it's also not
> readable by i2c, so there's no easy way for a driver which supports both
> modes to see what the pinstrap is set to.
> 
> I'm not sure if the driver could tell from the direction of the endpoints
> it's linked to which mode to use, but if not it'll need to be told somehow
> and a DT property seems reasonable to me. Given that the same pins are used
> in each direction I think the direction is most likely to be hard wired and
> board specific.

You're absolutly right. Sorry didn't catched this, since it's a bit out of my
mind.. There 'can be' cases where the MSEL is connected to a GPIO but in
that case the device needs a hard reset to resample the pin. Also a
parallel-bus mux must be in front of the device. So I think that
'danymic switching' case is currently out of scope. I'm with you to
define the mode by a DT property is absolutly okay, the property should
something like:

(more device specific)
tc358746,default-mode = <CSI-Tx> /* Parallel-in -> CSI-out */
tc358746,default-mode = <CSI-Rx> /* CSI-in -> Parallel-out */

or

(more generic)
tc358746,default-dir = <PARALLEL_TO_CSI2>
tc358746,default-dir = <CSI2_TO_PARALLEL>

So we can add the 'maybe' dynamic switching later on.

Regards,
Marco

> 
> Regards,
> Ian.
> 
> > > > +
> > > > +Required Properties:
> > > > +
> > > > +- compatible: should be "toshiba,tc358746"
> > > > +- reg: should be <0x0e>
> > > > +- clocks: should contain a phandle link to the reference clock source
> > > > +- clock-names: the clock input is named "refclk".
> > > > +
> > > > +Optional Properties:
> > > > +
> > > > +- reset-gpios: gpio phandle GPIO connected to the reset pin
> > > > +
> > > > +Parallel Endpoint:
> > > > +
> > > > +Required Properties:
> > > 
> > > It'd be nice if the relation between these sections would be somehow
> > > apparent. E.g. using different underlining, such as in
> > > Documentation/devicetree/bindings/media/ti,omap3isp.txt .
> > 
> > Thats a really good example thanks.
> > 
> > > 
> > > > +
> > > > +- reg: should be <0>
> > > > +- bus-width: the data bus width e.g. <8> for eight bit bus, or <16>
> > > > +	     for sixteen bit wide bus.
> > > > +
> > > > +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>
> > > > +- 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.
> > > > +
> > > > +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 {
> > > 
> > > The node name should be a generic name of the type of the device, not the
> > > name of the specific device as such. A similar Cadence device uses
> > > "csi-bridge".
> > 
> > Okay, I will change that.
> > 
> > > 
> > > > +		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>;
> > > > +			};
> > > > +		};
> > > > +	};
> > > > +};
> > > 
> > > -- 
> > > Kind regards,
> > > 
> > > Sakari Ailus
> > > [e-mail]
> > > 
> > 
>

Jacopo Mondi March 4, 2019, 9:38 a.m. UTC | #8

Hi Marco,

On Fri, Mar 01, 2019 at 11:26:48AM +0100, Marco Felsch wrote:
> 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.
>

No worries at all

> >
> > 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 <[e-mail]>
> > > ---
> > >  .../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.
>

I would:
"It is programmable through I2C and SPI, with the SPI interface only
available in parallel to CSI-2 conversion mode"

matter of tastes, really up to you :)

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

As I read the clock bindings documentation, I don't think 'clock-names'
is related to clock provider functionalities, but it is only for
consumers.

Optional properties:
clock-names:	List of clock input name strings sorted in the same
		order as the clocks property.  Consumers drivers
		will use clock-names to match clock input names
		with clocks specifiers.

If you're going to support clock provider functionalities, you're
likely going to do that thought a clock-output-names property.

Maybe I don't get what you mean here, and that's anyway minor as it's not
wrong to have that property there, it's maybe just redundant.

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

Not really, that was more something like
        port@0{
                parallel-input-endpoint
                ....
        }
        port@1{
                mipi-input-endpoint
                ....
        }
        port@2{
                parallel-output-endpoint
                ....
        }
        port@3{
                mipi-output-endpoint
                ....
        }

As you explained below here, that's a bad idea.

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

I see, thanks for explaining. The hardware connections are certainly
the same, so yes, two ports for input and two ports for output is a
bad idea.

Though, you should better describe here you that you want two ports,
one input and one output one, with one optional endpoint describing a
parallel or CSI-2 connection

Do you think something like the following might apply?
Feel free to re-word and use what you think is appropriate:

"The device node must contain two ports children nodes, grouped in a 'ports'
node. The first port describes the input connection, the second one describes
the output one. Each port shall contain one endpoint subnode that connects
to a remote device and specifies the bus type of the input and output
ports. Only one endpoint per port shall be present.

Endpoint properties:
- Parallel endpoints:
  - Required properties:
    - bus-width:
  - Optional properties:
    -

- MIPI CSI-2 endpoints:
  - Required properties:
    - data-lanes:
  - Optional properties:
    -

 " ^ Here you might need to specify properties whose value depends if
 the endpoint is input or output, like link-frequencies above that
 afaict applies only to output CSI-2 endpoints, not input ones"

Example:

        ....
        tc358746: tc358746@0e {
                ....

                ports {
                        #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 {
                                        data-lanes = <1 2>;
                                        remote-endpoint = <&mipi_csi2_in>;
                                };
                        };
                };
         };

What I'm not sure about is if you would need to number the endpoints.
I don't think so as only one at the time could be there, but
video-interfaces.txt seems to suggest so:

If a port can be configured to work with more than one remote device on the same
bus, an 'endpoint' child node must be provided for each of them.  If more than
one port is present in a device node or there is more than one endpoint at a
port, or port node needs to be associated with a selected hardware interface,
a common scheme using '#address-cells', '#size-cells' and 'reg' properties is
used.


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

I see, still a required property with a fixed value is not that
necessary.

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

Ah, so the user is expected to specify a set of frequencies the
driver should pick from to handle slower pixel rates, I see. I cannot
tell how this should handle. If nobody else complains, I think it's
fine then :)

Thanks
  j

> >
> > 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: [phone]    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |

Jacopo Mondi March 4, 2019, 9:41 a.m. UTC | #9

Hi Marco, Ian,

On Fri, Mar 01, 2019 at 02:01:18PM +0100, Marco Felsch wrote:
> Hi Ian,
>
> On 19-03-01 11:07, Ian Arkver wrote:
> > Hi,
> >
> > On 01/03/2019 10:52, Marco Felsch wrote:
> > > Hi Sakari,
> > >
> > > On 19-02-18 12:03, Sakari Ailus wrote:
> > > > Hi Marco,
> > > >
> > > > My apologies for reviewing this so late. You've received good comments
> > > > already. I have a few more.
> > >
> > > Thanks for your review for the other patches as well =) Sorry for my
> > > delayed response.
> > >
> > > > 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 <[e-mail]>
> > > > > ---
> > > > >   .../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
> > > > > +or a MIPI CSI-2 RX stream into a Parallel-out. It is programmable through I2C.
> > > >
> > > > This is interesting. The driver somehow needs to figure out the direction
> > > > of the data flow if it does not originate from DT. I guess it shouldn't as
> > > > it's not the property of an individual device, albeit in practice in all
> > > > hardware I've seen the direction of the pipeline is determinable and this
> > > > is visible in the kAPI as well. So I'm suggesting no changes due to this in
> > > > bindings, likely we'll need to address it somehow elsewhere going forward.
> > >
> > > What did you mean with "... and this is visible in the kAPI as well"?
> > > I'm relative new in the linux-media world but I never saw a device which
> > > supports two directions. Our customer which uses that chip use it
> > > only in parallel-in/csi-out mode. To be flexible the switching should be
> > > done by a subdev-ioctl but it is also reasonable to define a default value
> > > within the DT.
> >
> > The mode is set by a pin strap at reset time (MSEL). It's not programmable
> > by i2c. As far as I can see, looking at the registers, it's also not
> > readable by i2c, so there's no easy way for a driver which supports both
> > modes to see what the pinstrap is set to.
> >
> > I'm not sure if the driver could tell from the direction of the endpoints
> > it's linked to which mode to use, but if not it'll need to be told somehow
> > and a DT property seems reasonable to me. Given that the same pins are used
> > in each direction I think the direction is most likely to be hard wired and
> > board specific.
>
> You're absolutly right. Sorry didn't catched this, since it's a bit out of my
> mind.. There 'can be' cases where the MSEL is connected to a GPIO but in
> that case the device needs a hard reset to resample the pin. Also a
> parallel-bus mux must be in front of the device. So I think that
> 'danymic switching' case is currently out of scope. I'm with you to
> define the mode by a DT property is absolutly okay, the property should
> something like:
>
> (more device specific)
> tc358746,default-mode = <CSI-Tx> /* Parallel-in -> CSI-out */
> tc358746,default-mode = <CSI-Rx> /* CSI-in -> Parallel-out */
>
> or
>
> (more generic)
> tc358746,default-dir = <PARALLEL_TO_CSI2>
> tc358746,default-dir = <CSI2_TO_PARALLEL>
>
> So we can add the 'maybe' dynamic switching later on.
>

I think if you model the bindings with one endpoint per input/output port,
you can just parse the endpoints, using the bus hints that are now
available, and deduct the bus types and thus the conversion directions
without introducing any custom property.

Thanks
   j

Sakari Ailus March 4, 2019, 12:10 p.m. UTC | #10

Hi Macro,

On Fri, Mar 01, 2019 at 11:52:35AM +0100, Marco Felsch wrote:
> Hi Sakari,
> 
> On 19-02-18 12:03, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > My apologies for reviewing this so late. You've received good comments
> > already. I have a few more.
> 
> Thanks for your review for the other patches as well =) Sorry for my
> delayed response.
> 
> > 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 <[e-mail]>
> > > ---
> > >  .../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
> > > +or a MIPI CSI-2 RX stream into a Parallel-out. It is programmable through I2C.
> > 
> > This is interesting. The driver somehow needs to figure out the direction
> > of the data flow if it does not originate from DT. I guess it shouldn't as
> > it's not the property of an individual device, albeit in practice in all
> > hardware I've seen the direction of the pipeline is determinable and this
> > is visible in the kAPI as well. So I'm suggesting no changes due to this in
> > bindings, likely we'll need to address it somehow elsewhere going forward.
> 
> What did you mean with "... and this is visible in the kAPI as well"?
> I'm relative new in the linux-media world but I never saw a device which
> supports two directions. Our customer which uses that chip use it
> only in parallel-in/csi-out mode. To be flexible the switching should be
> done by a subdev-ioctl but it is also reasonable to define a default value
> within the DT.

What I meant that the V4L2 sub-device API does not provide any information
on the direction. It is implicit --- MC does, but it does it based on the
links created by the driver.

I agree a DT property would be a good way to tell this, especially now that
there's a related hardware configuration (but which the software cannot
obtain directly).

Sakari Ailus March 4, 2019, 12:36 p.m. UTC | #11

Hi Marco,

On Fri, Mar 01, 2019 at 02:01:18PM +0100, Marco Felsch wrote:
> Hi Ian,
> 
> On 19-03-01 11:07, Ian Arkver wrote:
> > Hi,
> > 
> > On 01/03/2019 10:52, Marco Felsch wrote:
> > > Hi Sakari,
> > > 
> > > On 19-02-18 12:03, Sakari Ailus wrote:
> > > > Hi Marco,
> > > > 
> > > > My apologies for reviewing this so late. You've received good comments
> > > > already. I have a few more.
> > > 
> > > Thanks for your review for the other patches as well =) Sorry for my
> > > delayed response.
> > > 
> > > > 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 <[e-mail]>
> > > > > ---
> > > > >   .../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
> > > > > +or a MIPI CSI-2 RX stream into a Parallel-out. It is programmable through I2C.
> > > > 
> > > > This is interesting. The driver somehow needs to figure out the direction
> > > > of the data flow if it does not originate from DT. I guess it shouldn't as
> > > > it's not the property of an individual device, albeit in practice in all
> > > > hardware I've seen the direction of the pipeline is determinable and this
> > > > is visible in the kAPI as well. So I'm suggesting no changes due to this in
> > > > bindings, likely we'll need to address it somehow elsewhere going forward.
> > > 
> > > What did you mean with "... and this is visible in the kAPI as well"?
> > > I'm relative new in the linux-media world but I never saw a device which
> > > supports two directions. Our customer which uses that chip use it
> > > only in parallel-in/csi-out mode. To be flexible the switching should be
> > > done by a subdev-ioctl but it is also reasonable to define a default value
> > > within the DT.
> > 
> > The mode is set by a pin strap at reset time (MSEL). It's not programmable
> > by i2c. As far as I can see, looking at the registers, it's also not
> > readable by i2c, so there's no easy way for a driver which supports both
> > modes to see what the pinstrap is set to.
> > 
> > I'm not sure if the driver could tell from the direction of the endpoints
> > it's linked to which mode to use, but if not it'll need to be told somehow
> > and a DT property seems reasonable to me. Given that the same pins are used
> > in each direction I think the direction is most likely to be hard wired and
> > board specific.
> 
> You're absolutly right. Sorry didn't catched this, since it's a bit out of my
> mind.. There 'can be' cases where the MSEL is connected to a GPIO but in
> that case the device needs a hard reset to resample the pin. Also a
> parallel-bus mux must be in front of the device. So I think that
> 'danymic switching' case is currently out of scope. I'm with you to
> define the mode by a DT property is absolutly okay, the property should
> something like:
> 
> (more device specific)
> tc358746,default-mode = <CSI-Tx> /* Parallel-in -> CSI-out */
> tc358746,default-mode = <CSI-Rx> /* CSI-in -> Parallel-out */
> 
> or
> 
> (more generic)
> tc358746,default-dir = <PARALLEL_TO_CSI2>
> tc358746,default-dir = <CSI2_TO_PARALLEL>

The prefix for Toshiba is "toshiba". What would you think of
"toshiba,csi2-direction" with values of either "rx" or "tx"? Or
"toshiba,csi2-mode" with either "master" or "slave", which would be a
little bit more generic, but could be slightly more probable to get wrong
as well.

Marco Felsch March 4, 2019, 4:43 p.m. UTC | #12

Hi Jacopo,

On 19-03-04 10:38, Jacopo Mondi wrote:
> Hi Marco,
> 
> On Fri, Mar 01, 2019 at 11:26:48AM +0100, Marco Felsch wrote:
> > 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.
> >
> 
> No worries at all
> 
> > >
> > > 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 <[e-mail]>
> > > > ---
> > > >  .../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.
> >
> 
> I would:
> "It is programmable through I2C and SPI, with the SPI interface only
> available in parallel to CSI-2 conversion mode"
> 
> matter of tastes, really up to you :)
> 
> > > > +
> > > > +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.
> >
> 
> As I read the clock bindings documentation, I don't think 'clock-names'
> is related to clock provider functionalities, but it is only for
> consumers.
> 
> Optional properties:
> clock-names:	List of clock input name strings sorted in the same
> 		order as the clocks property.  Consumers drivers
> 		will use clock-names to match clock input names
> 		with clocks specifiers.
> 
> If you're going to support clock provider functionalities, you're
> likely going to do that thought a clock-output-names property.

You're absolutely right I mixed this, sry. Now after getting back into
the patch set I know why I added the clock-names property. It's because
I request the clk by a 'devm_clk_get(dev, "refclk")' call. I will change
this to drop the clock-names property.

> Maybe I don't get what you mean here, and that's anyway minor as it's not
> wrong to have that property there, it's maybe just redundant.
> 
> > > > +
> > > > +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 */
> > };
> >
> 
> Not really, that was more something like
>         port@0{
>                 parallel-input-endpoint
>                 ....
>         }
>         port@1{
>                 mipi-input-endpoint
>                 ....
>         }
>         port@2{
>                 parallel-output-endpoint
>                 ....
>         }
>         port@3{
>                 mipi-output-endpoint
>                 ....
>         }
> 
> As you explained below here, that's a bad idea.
> 
> > > 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.
> >
> 
> I see, thanks for explaining. The hardware connections are certainly
> the same, so yes, two ports for input and two ports for output is a
> bad idea.
> 
> Though, you should better describe here you that you want two ports,
> one input and one output one, with one optional endpoint describing a
> parallel or CSI-2 connection
> 
> Do you think something like the following might apply?
> Feel free to re-word and use what you think is appropriate:
> 
> "The device node must contain two ports children nodes, grouped in a 'ports'
> node. The first port describes the input connection, the second one describes
> the output one. Each port shall contain one endpoint subnode that connects
> to a remote device and specifies the bus type of the input and output
> ports. Only one endpoint per port shall be present.

That sounds good to me, I will take it as it is and add some more notes
from Sakari's feedback.

> 
> Endpoint properties:
> - Parallel endpoints:
>   - Required properties:
>     - bus-width:
>   - Optional properties:
>     -
> 
> - MIPI CSI-2 endpoints:
>   - Required properties:
>     - data-lanes:
>   - Optional properties:
>     -
> 
>  " ^ Here you might need to specify properties whose value depends if
>  the endpoint is input or output, like link-frequencies above that
>  afaict applies only to output CSI-2 endpoints, not input ones"

Sorry if I wasn't that clear in my explanation but I think the
link-frequencies property applies to both cases e.g. if the panel
refresh rate is to fast and the link-frequency to slow. But I can't
verify that since my customer board uses only the parallel-in -> csi-out
case.

> 
> Example:
> 
>         ....
>         tc358746: tc358746@0e {
>                 ....
> 
>                 ports {
>                         #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 {
>                                         data-lanes = <1 2>;
>                                         remote-endpoint = <&mipi_csi2_in>;
>                                 };
>                         };
>                 };
>          };
> 
> What I'm not sure about is if you would need to number the endpoints.
> I don't think so as only one at the time could be there, but
> video-interfaces.txt seems to suggest so:
> 
> If a port can be configured to work with more than one remote device on the same
> bus, an 'endpoint' child node must be provided for each of them.  If more than
> one port is present in a device node or there is more than one endpoint at a
> port, or port node needs to be associated with a selected hardware interface,
> a common scheme using '#address-cells', '#size-cells' and 'reg' properties is
> used.

No I don't think so too since only one endpoint at a time is supported.

> > > 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.
> 
> I see, still a required property with a fixed value is not that
> necessary.
> 
> >
> > >
> > > > +- 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.
> >
> 
> Ah, so the user is expected to specify a set of frequencies the
> driver should pick from to handle slower pixel rates, I see. I cannot
> tell how this should handle. If nobody else complains, I think it's
> fine then :)

Thanks for the feedback :)

Regards,
Marco

> Thanks
>   j
> 
> > >
> > > 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: [phone]    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |

Marco Felsch March 4, 2019, 4:55 p.m. UTC | #13

Hi Sakari,

On 19-03-04 14:36, Sakari Ailus wrote:
> Hi Marco,
> 
> On Fri, Mar 01, 2019 at 02:01:18PM +0100, Marco Felsch wrote:
> > Hi Ian,
> > 
> > On 19-03-01 11:07, Ian Arkver wrote:
> > > Hi,
> > > 
> > > On 01/03/2019 10:52, Marco Felsch wrote:
> > > > Hi Sakari,
> > > > 
> > > > On 19-02-18 12:03, Sakari Ailus wrote:
> > > > > Hi Marco,
> > > > > 
> > > > > My apologies for reviewing this so late. You've received good comments
> > > > > already. I have a few more.
> > > > 
> > > > Thanks for your review for the other patches as well =) Sorry for my
> > > > delayed response.
> > > > 
> > > > > 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 <[e-mail]>
> > > > > > ---
> > > > > >   .../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
> > > > > > +or a MIPI CSI-2 RX stream into a Parallel-out. It is programmable through I2C.
> > > > > 
> > > > > This is interesting. The driver somehow needs to figure out the direction
> > > > > of the data flow if it does not originate from DT. I guess it shouldn't as
> > > > > it's not the property of an individual device, albeit in practice in all
> > > > > hardware I've seen the direction of the pipeline is determinable and this
> > > > > is visible in the kAPI as well. So I'm suggesting no changes due to this in
> > > > > bindings, likely we'll need to address it somehow elsewhere going forward.
> > > > 
> > > > What did you mean with "... and this is visible in the kAPI as well"?
> > > > I'm relative new in the linux-media world but I never saw a device which
> > > > supports two directions. Our customer which uses that chip use it
> > > > only in parallel-in/csi-out mode. To be flexible the switching should be
> > > > done by a subdev-ioctl but it is also reasonable to define a default value
> > > > within the DT.
> > > 
> > > The mode is set by a pin strap at reset time (MSEL). It's not programmable
> > > by i2c. As far as I can see, looking at the registers, it's also not
> > > readable by i2c, so there's no easy way for a driver which supports both
> > > modes to see what the pinstrap is set to.
> > > 
> > > I'm not sure if the driver could tell from the direction of the endpoints
> > > it's linked to which mode to use, but if not it'll need to be told somehow
> > > and a DT property seems reasonable to me. Given that the same pins are used
> > > in each direction I think the direction is most likely to be hard wired and
> > > board specific.
> > 
> > You're absolutly right. Sorry didn't catched this, since it's a bit out of my
> > mind.. There 'can be' cases where the MSEL is connected to a GPIO but in
> > that case the device needs a hard reset to resample the pin. Also a
> > parallel-bus mux must be in front of the device. So I think that
> > 'danymic switching' case is currently out of scope. I'm with you to
> > define the mode by a DT property is absolutly okay, the property should
> > something like:
> > 
> > (more device specific)
> > tc358746,default-mode = <CSI-Tx> /* Parallel-in -> CSI-out */
> > tc358746,default-mode = <CSI-Rx> /* CSI-in -> Parallel-out */
> > 
> > or
> > 
> > (more generic)
> > tc358746,default-dir = <PARALLEL_TO_CSI2>
> > tc358746,default-dir = <CSI2_TO_PARALLEL>
> 
> The prefix for Toshiba is "toshiba". What would you think of
> "toshiba,csi2-direction" with values of either "rx" or "tx"? Or
> "toshiba,csi2-mode" with either "master" or "slave", which would be a
> little bit more generic, but could be slightly more probable to get wrong
> as well.

You're right mixed the prefix with the device.. If we need to introduce
a property I would prefer the "toshiba,csi2-direction" one. I said if
because as Jacopo mentioned we can avoid the property by define port@0
as input and port@1 as output. I tink that's the best solution, since we
can avoid device specific bindings and it's common to use the last port
as output (e.g. video-mux).

Regards,
Marco

> -- 
> Regards,
> 
> Sakari Ailus
> [e-mail]
>

Sakari Ailus March 4, 2019, 6:17 p.m. UTC | #14

Hi Marco,

On Mon, Mar 04, 2019 at 05:55:28PM +0100, Marco Felsch wrote:
> > > (more device specific)
> > > tc358746,default-mode = <CSI-Tx> /* Parallel-in -> CSI-out */
> > > tc358746,default-mode = <CSI-Rx> /* CSI-in -> Parallel-out */
> > > 
> > > or
> > > 
> > > (more generic)
> > > tc358746,default-dir = <PARALLEL_TO_CSI2>
> > > tc358746,default-dir = <CSI2_TO_PARALLEL>
> > 
> > The prefix for Toshiba is "toshiba". What would you think of
> > "toshiba,csi2-direction" with values of either "rx" or "tx"? Or
> > "toshiba,csi2-mode" with either "master" or "slave", which would be a
> > little bit more generic, but could be slightly more probable to get wrong
> > as well.
> 
> You're right mixed the prefix with the device.. If we need to introduce
> a property I would prefer the "toshiba,csi2-direction" one. I said if
> because as Jacopo mentioned we can avoid the property by define port@0
> as input and port@1 as output. I tink that's the best solution, since we
> can avoid device specific bindings and it's common to use the last port
> as output (e.g. video-mux).

The ports represent hardware and I think I would avoid reordering them. I
wonder what would the DT folks prefer.

The device specific property is to the point at least: it describes an
orthogonal part of the device configuration. That's why I'd pick that if I
were to choose. But I'll let Rob to comment on this.

Jacopo Mondi March 5, 2019, 8:49 a.m. UTC | #15

Hi Sakari, Marco,

On Mon, Mar 04, 2019 at 08:17:48PM +0200, Sakari Ailus wrote:
> Hi Marco,
>
> On Mon, Mar 04, 2019 at 05:55:28PM +0100, Marco Felsch wrote:
> > > > (more device specific)
> > > > tc358746,default-mode = <CSI-Tx> /* Parallel-in -> CSI-out */
> > > > tc358746,default-mode = <CSI-Rx> /* CSI-in -> Parallel-out */
> > > >
> > > > or
> > > >
> > > > (more generic)
> > > > tc358746,default-dir = <PARALLEL_TO_CSI2>
> > > > tc358746,default-dir = <CSI2_TO_PARALLEL>
> > >
> > > The prefix for Toshiba is "toshiba". What would you think of
> > > "toshiba,csi2-direction" with values of either "rx" or "tx"? Or
> > > "toshiba,csi2-mode" with either "master" or "slave", which would be a
> > > little bit more generic, but could be slightly more probable to get wrong
> > > as well.
> >
> > You're right mixed the prefix with the device.. If we need to introduce
> > a property I would prefer the "toshiba,csi2-direction" one. I said if
> > because as Jacopo mentioned we can avoid the property by define port@0
> > as input and port@1 as output. I tink that's the best solution, since we
> > can avoid device specific bindings and it's common to use the last port
> > as output (e.g. video-mux).
>
> The ports represent hardware and I think I would avoid reordering them. I
> wonder what would the DT folks prefer.
>

I might have missed why you mention re-ordering? :)

> The device specific property is to the point at least: it describes an
> orthogonal part of the device configuration. That's why I'd pick that if I
> were to choose. But I'll let Rob to comment on this.

That's true indeed. Let's wait for inputs from DT people, I'm fine
with both approaches.

Thanks
   j

>
> --
> Regards,
>
> Sakari Ailus
> [e-mail]

Marco Felsch March 5, 2019, 6:14 p.m. UTC | #16

Hi Rob,

I think you didn't followed the discussion in detail so I will ask you
personal. In short the tc358746 can act as parallel-in -> csi-out or as
csi->in -> parallel-out device. The phyiscal pins are always the same
only the internal timings are different. So we have two ports with two
endpoints.

Now the question is how we determine the mode. We have two approaches:
1)
  port@0 -> input port
  port@1 -> output port

  pro:
  + no extra vendor specific binding is needed to determine the mode

  con:
  - input/output endpoint can be parallel or mipi-csi2.

2)
  port@0 -> parallel port
  port@1 -> mipi-csi2 port

  pro:
  + input/output endpoint are fixed to parallel or mipi

  con:
  - vendor specific binding is needed to determine the mode

Thanks for your comments :)

Regards,
Marco

On 19-03-05 09:49, Jacopo Mondi wrote:
> Hi Sakari, Marco,
> 
> On Mon, Mar 04, 2019 at 08:17:48PM +0200, Sakari Ailus wrote:
> > Hi Marco,
> >
> > On Mon, Mar 04, 2019 at 05:55:28PM +0100, Marco Felsch wrote:
> > > > > (more device specific)
> > > > > tc358746,default-mode = <CSI-Tx> /* Parallel-in -> CSI-out */
> > > > > tc358746,default-mode = <CSI-Rx> /* CSI-in -> Parallel-out */
> > > > >
> > > > > or
> > > > >
> > > > > (more generic)
> > > > > tc358746,default-dir = <PARALLEL_TO_CSI2>
> > > > > tc358746,default-dir = <CSI2_TO_PARALLEL>
> > > >
> > > > The prefix for Toshiba is "toshiba". What would you think of
> > > > "toshiba,csi2-direction" with values of either "rx" or "tx"? Or
> > > > "toshiba,csi2-mode" with either "master" or "slave", which would be a
> > > > little bit more generic, but could be slightly more probable to get wrong
> > > > as well.
> > >
> > > You're right mixed the prefix with the device.. If we need to introduce
> > > a property I would prefer the "toshiba,csi2-direction" one. I said if
> > > because as Jacopo mentioned we can avoid the property by define port@0
> > > as input and port@1 as output. I tink that's the best solution, since we
> > > can avoid device specific bindings and it's common to use the last port
> > > as output (e.g. video-mux).
> >
> > The ports represent hardware and I think I would avoid reordering them. I
> > wonder what would the DT folks prefer.
> >
> 
> I might have missed why you mention re-ordering? :)
> 
> > The device specific property is to the point at least: it describes an
> > orthogonal part of the device configuration. That's why I'd pick that if I
> > were to choose. But I'll let Rob to comment on this.
> 
> That's true indeed. Let's wait for inputs from DT people, I'm fine
> with both approaches.
> 
> Thanks
>    j
> 
> >
> > --
> > Regards,
> >
> > Sakari Ailus
> > [e-mail]

Marco Felsch April 16, 2019, 10:45 a.m. UTC | #17

Hi Rob,

gentle ping.

Regards,
  Marco

On 19-03-05 19:14, Marco Felsch wrote:
> Hi Rob,
> 
> I think you didn't followed the discussion in detail so I will ask you
> personal. In short the tc358746 can act as parallel-in -> csi-out or as
> csi->in -> parallel-out device. The phyiscal pins are always the same
> only the internal timings are different. So we have two ports with two
> endpoints.
> 
> Now the question is how we determine the mode. We have two approaches:
> 1)
>   port@0 -> input port
>   port@1 -> output port
> 
>   pro:
>   + no extra vendor specific binding is needed to determine the mode
> 
>   con:
>   - input/output endpoint can be parallel or mipi-csi2.
> 
> 2)
>   port@0 -> parallel port
>   port@1 -> mipi-csi2 port
> 
>   pro:
>   + input/output endpoint are fixed to parallel or mipi
> 
>   con:
>   - vendor specific binding is needed to determine the mode
> 
> Thanks for your comments :)
> 
> Regards,
> Marco
> 
> On 19-03-05 09:49, Jacopo Mondi wrote:
> > Hi Sakari, Marco,
> > 
> > On Mon, Mar 04, 2019 at 08:17:48PM +0200, Sakari Ailus wrote:
> > > Hi Marco,
> > >
> > > On Mon, Mar 04, 2019 at 05:55:28PM +0100, Marco Felsch wrote:
> > > > > > (more device specific)
> > > > > > tc358746,default-mode = <CSI-Tx> /* Parallel-in -> CSI-out */
> > > > > > tc358746,default-mode = <CSI-Rx> /* CSI-in -> Parallel-out */
> > > > > >
> > > > > > or
> > > > > >
> > > > > > (more generic)
> > > > > > tc358746,default-dir = <PARALLEL_TO_CSI2>
> > > > > > tc358746,default-dir = <CSI2_TO_PARALLEL>
> > > > >
> > > > > The prefix for Toshiba is "toshiba". What would you think of
> > > > > "toshiba,csi2-direction" with values of either "rx" or "tx"? Or
> > > > > "toshiba,csi2-mode" with either "master" or "slave", which would be a
> > > > > little bit more generic, but could be slightly more probable to get wrong
> > > > > as well.
> > > >
> > > > You're right mixed the prefix with the device.. If we need to introduce
> > > > a property I would prefer the "toshiba,csi2-direction" one. I said if
> > > > because as Jacopo mentioned we can avoid the property by define port@0
> > > > as input and port@1 as output. I tink that's the best solution, since we
> > > > can avoid device specific bindings and it's common to use the last port
> > > > as output (e.g. video-mux).
> > >
> > > The ports represent hardware and I think I would avoid reordering them. I
> > > wonder what would the DT folks prefer.
> > >
> > 
> > I might have missed why you mention re-ordering? :)
> > 
> > > The device specific property is to the point at least: it describes an
> > > orthogonal part of the device configuration. That's why I'd pick that if I
> > > were to choose. But I'll let Rob to comment on this.
> > 
> > That's true indeed. Let's wait for inputs from DT people, I'm fine
> > with both approaches.
> > 
> > Thanks
> >    j
> > 
> > >
> > > --
> > > Regards,
> > >
> > > Sakari Ailus
> > > [e-mail]
> 
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: [phone]    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |
> 
>

Marco Felsch April 29, 2019, 4:44 p.m. UTC | #18

Hi Rob,

sorry instead of adding you to To you was on Cc.
Do you have any preferences about the below discussion?

Regards,
  Marco

On 19-04-16 12:45, Marco Felsch wrote:
> Hi Rob,
> 
> gentle ping.
> 
> Regards,
>   Marco
> 
> On 19-03-05 19:14, Marco Felsch wrote:
> > Hi Rob,
> > 
> > I think you didn't followed the discussion in detail so I will ask you
> > personal. In short the tc358746 can act as parallel-in -> csi-out or as
> > csi->in -> parallel-out device. The phyiscal pins are always the same
> > only the internal timings are different. So we have two ports with two
> > endpoints.
> > 
> > Now the question is how we determine the mode. We have two approaches:
> > 1)
> >   port@0 -> input port
> >   port@1 -> output port
> > 
> >   pro:
> >   + no extra vendor specific binding is needed to determine the mode
> > 
> >   con:
> >   - input/output endpoint can be parallel or mipi-csi2.
> > 
> > 2)
> >   port@0 -> parallel port
> >   port@1 -> mipi-csi2 port
> > 
> >   pro:
> >   + input/output endpoint are fixed to parallel or mipi
> > 
> >   con:
> >   - vendor specific binding is needed to determine the mode
> > 
> > Thanks for your comments :)
> > 
> > Regards,
> > Marco
> > 
> > On 19-03-05 09:49, Jacopo Mondi wrote:
> > > Hi Sakari, Marco,
> > > 
> > > On Mon, Mar 04, 2019 at 08:17:48PM +0200, Sakari Ailus wrote:
> > > > Hi Marco,
> > > >
> > > > On Mon, Mar 04, 2019 at 05:55:28PM +0100, Marco Felsch wrote:
> > > > > > > (more device specific)
> > > > > > > tc358746,default-mode = <CSI-Tx> /* Parallel-in -> CSI-out */
> > > > > > > tc358746,default-mode = <CSI-Rx> /* CSI-in -> Parallel-out */
> > > > > > >
> > > > > > > or
> > > > > > >
> > > > > > > (more generic)
> > > > > > > tc358746,default-dir = <PARALLEL_TO_CSI2>
> > > > > > > tc358746,default-dir = <CSI2_TO_PARALLEL>
> > > > > >
> > > > > > The prefix for Toshiba is "toshiba". What would you think of
> > > > > > "toshiba,csi2-direction" with values of either "rx" or "tx"? Or
> > > > > > "toshiba,csi2-mode" with either "master" or "slave", which would be a
> > > > > > little bit more generic, but could be slightly more probable to get wrong
> > > > > > as well.
> > > > >
> > > > > You're right mixed the prefix with the device.. If we need to introduce
> > > > > a property I would prefer the "toshiba,csi2-direction" one. I said if
> > > > > because as Jacopo mentioned we can avoid the property by define port@0
> > > > > as input and port@1 as output. I tink that's the best solution, since we
> > > > > can avoid device specific bindings and it's common to use the last port
> > > > > as output (e.g. video-mux).
> > > >
> > > > The ports represent hardware and I think I would avoid reordering them. I
> > > > wonder what would the DT folks prefer.
> > > >
> > > 
> > > I might have missed why you mention re-ordering? :)
> > > 
> > > > The device specific property is to the point at least: it describes an
> > > > orthogonal part of the device configuration. That's why I'd pick that if I
> > > > were to choose. But I'll let Rob to comment on this.
> > > 
> > > That's true indeed. Let's wait for inputs from DT people, I'm fine
> > > with both approaches.
> > > 
> > > Thanks
> > >    j
> > > 
> > > >
> > > > --
> > > > Regards,
> > > >
> > > > Sakari Ailus
> > > > [e-mail]
> > 
> > 
> > 
> > -- 
> > Pengutronix e.K.                           |                             |
> > Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: [phone]    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |
> > 
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: [phone]    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |
> 
>

Patch

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
+or a MIPI CSI-2 RX stream into a Parallel-out. It is programmable through I2C.
+
+Required Properties:
+
+- compatible: should be "toshiba,tc358746"
+- reg: should be <0x0e>
+- clocks: should contain a phandle link to the reference clock source
+- clock-names: the clock input is named "refclk".
+
+Optional Properties:
+
+- reset-gpios: gpio phandle GPIO connected to the reset pin
+
+Parallel Endpoint:
+
+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.
+
+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>
+- 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.
+
+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>;
+			};
+		};
+	};
+};