From: Marco Felsch <m.felsch@pengutronix.de>
To: Jacopo Mondi <jacopo@jmondi.org>
Cc: hans.verkuil@cisco.com, sakari.ailus@linux.intel.com,
mchehab@kernel.org, robh+dt@kernel.org, mark.rutland@arm.com,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
graphics@pengutronix.de
Subject: Re: [PATCH 1/3] media: dt-bindings: add bindings for Toshiba TC358746
Date: Mon, 4 Mar 2019 17:43:38 +0100 [thread overview]
Message-ID: <20190304164338.4vld6vndgnzcydxx@pengutronix.de> (raw)
In-Reply-To: <20190304093844.3puwnd3yk5rdpulw@uno.localdomain>
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 <m.felsch@pengutronix.de>
> > > > ---
> > > > .../bindings/media/i2c/toshiba,tc358746.txt | 80 +++++++++++++++++++
> > > > 1 file changed, 80 insertions(+)
> > > > create mode 100644 Documentation/devicetree/bindings/media/i2c/toshiba,tc358746.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/toshiba,tc358746.txt b/Documentation/devicetree/bindings/media/i2c/toshiba,tc358746.txt
> > > > new file mode 100644
> > > > index 000000000000..499733df744a
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/toshiba,tc358746.txt
> > > > @@ -0,0 +1,80 @@
> > > > +* Toshiba TC358746 Parallel to MIPI CSI2-TX or MIPI CSI2-RX to Parallel Bridge
> > > > +
> > > > +The Toshiba TC358746 is a bridge that converts a Parallel-in stream to MIPI CSI-2 TX
> > >
> > > nit:
> > > s/is a bridge that/is a bridge device that/
> > > or drop is a bridge completely?
> >
> > You're right, I will drop this statement.
> >
> > >
> > > > +or a MIPI CSI-2 RX stream into a Parallel-out. It is programmable through I2C.
> > >
> > > From the thin public available datasheet, it seems to support SPI as
> > > programming interface, but only when doing Parallel->CSI-2. I would
> > > mention that.
> >
> > You're right, the SPI interface is only supported in that mode.
> >
> > Should I add something like:
> > It is programmable trough I2C and SPI. The SPI interface is only
> > supported in parallel-in -> csi-out mode.
> >
>
> 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 = <µn_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 = <µ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>;
> > > > + };
> > > > + };
> > > > + };
> > > > +};
> > > > --
> > > > 2.19.1
> > > >
> >
> >
> >
> > --
> > Pengutronix e.K. | |
> > Industrial Linux Solutions | http://www.pengutronix.de/ |
> > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
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 |
next prev parent reply other threads:[~2019-03-04 16:43 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-18 14:12 [PATCH 0/3] media: add Toshiba TC358746 Bridge support Marco Felsch
2018-12-18 14:12 ` [PATCH 1/3] media: dt-bindings: add bindings for Toshiba TC358746 Marco Felsch
2018-12-28 23:10 ` Rob Herring
2019-02-13 17:57 ` Jacopo Mondi
2019-03-01 10:26 ` Marco Felsch
2019-03-04 9:38 ` Jacopo Mondi
2019-03-04 16:43 ` Marco Felsch [this message]
2019-02-18 10:03 ` Sakari Ailus
2019-03-01 10:52 ` Marco Felsch
2019-03-01 11:07 ` Ian Arkver
2019-03-01 13:01 ` Marco Felsch
2019-03-04 9:41 ` Jacopo Mondi
2019-03-04 12:36 ` Sakari Ailus
2019-03-04 16:55 ` Marco Felsch
2019-03-04 18:17 ` Sakari Ailus
2019-03-05 8:49 ` Jacopo Mondi
2019-03-05 18:14 ` Marco Felsch
2019-04-16 10:45 ` Marco Felsch
2019-04-29 16:44 ` Marco Felsch
2019-03-04 12:10 ` Sakari Ailus
2018-12-18 14:12 ` [PATCH 2/3] media: tc358746: add Toshiba TC358746 Parallel to CSI-2 bridge driver Marco Felsch
2018-12-19 1:24 ` kbuild test robot
2018-12-20 19:37 ` kbuild test robot
2019-02-18 11:25 ` Sakari Ailus
2018-12-18 14:12 ` [PATCH 3/3] media: tc358746: update MAINTAINERS file Marco Felsch
2019-02-18 11:46 ` Sakari Ailus
2019-03-04 17:31 ` Marco Felsch
2019-03-04 18:18 ` Sakari Ailus
2019-01-23 12:54 ` [PATCH 0/3] media: add Toshiba TC358746 Bridge support Marco Felsch
2019-02-12 16:10 ` Marco Felsch
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190304164338.4vld6vndgnzcydxx@pengutronix.de \
--to=m.felsch@pengutronix.de \
--cc=devicetree@vger.kernel.org \
--cc=graphics@pengutronix.de \
--cc=hans.verkuil@cisco.com \
--cc=jacopo@jmondi.org \
--cc=linux-media@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mchehab@kernel.org \
--cc=robh+dt@kernel.org \
--cc=sakari.ailus@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).