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 <[email protected]>
> > > > ---
> > > > .../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 =
> > > > <µn_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
> > > [email protected]
> > >
> >
>
--
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 |
