Re: [PATCH v2 1/2] dt-bindings: media: Add Cadence MIPI-CSI2 RX Device Tree bindings

[Maxime Ripard <maxime.ripard@free-electrons.com>]

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

Hi Laurent,

Thanks a lot for reviewing those patches.

On Mon, Aug 07, 2017 at 11:18:03PM +0300, Laurent Pinchart wrote:
> Hi Maxime,
> 
> Thank you for the patch.
> 
> On Thursday 20 Jul 2017 11:23:01 Maxime Ripard wrote:
> > The Cadence MIPI-CSI2 RX controller is a CSI2RX bridge that supports up to
> > 4 CSI-2 lanes, and can route the frames to up to 4 streams, depending on
> > the hardware implementation.
> > 
> > It can operate with an external D-PHY, an internal one or no D-PHY at all
> > in some configurations.
> 
> Without any PHY ? I'm curious, how does that work ?

We're currently working on an FPGA exactly with that configuration. So
I guess the answer would be "it doesn't on an ASIC" :)

> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  .../devicetree/bindings/media/cdns-csi2rx.txt      | 87 ++++++++++++++++++
> >  1 file changed, 87 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/cdns-csi2rx.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
> > b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt new file mode
> > 100644
> > index 000000000000..e08547abe885
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/cdns-csi2rx.txt
> > @@ -0,0 +1,87 @@
> > +Cadence MIPI-CSI2 RX controller
> > +===============================
> > +
> > +The Cadence MIPI-CSI2 RX controller is a CSI-2 bridge supporting up to 4
> > CSI
> > +lanes in input, and 4 different pixel streams in output.
> > +
> > +Required properties:
> > +  - compatible: must be set to "cdns,csi2rx" and an SoC-specific compatible
> > +  - reg: base address and size of the memory mapped region
> > +  - clocks: phandles to the clocks driving the controller
> > +  - clock-names: must contain:
> > +    * sys_clk: main clock
> > +    * p_clk: register bank clock
> > +    * p_free_clk: free running register bank clock
> > +    * pixel_ifX_clk: pixel stream output clock, one for each stream
> > +                     implemented in hardware, between 0 and 3
> 
> Nitpicking, I would write the name is pixel_if[0-3]_clk, it took me a few 
> seconds to see that the X was a placeholder.

Ok.

> > +    * dphy_rx_clk: D-PHY byte clock, if implemented in hardware
> > +  - phys: phandle to the external D-PHY
> > +  - phy-names: must contain dphy, if the implementation uses an
> > +               external D-PHY
> 
> I would move the last two properties in an optional category as they're 
> effectively optional. I think you should also explain a bit more clearly that 
> the phys property must not be present if the phy-names property is not 
> present.

It's not really optional. The IP has a configuration register that
allows you to see if it's been synthesized with or without a PHY. If
the right bit is set, that property will be mandatory, if not, it's
useless.

Maybe it's just semantics, but to me, optional means that it can
operate with or without it under any circumstances. It's not really
the case here.

> > +
> > +Required subnodes:
> > +  - ports: A ports node with endpoint definitions as defined in
> > +           Documentation/devicetree/bindings/media/video-interfaces.txt.
> > The
> > +           first port subnode should be the input endpoint, the second one
> > the
> > +           outputs
> > +
> > +  The output port should have as many endpoints as stream supported by
> > +  the hardware implementation, between 1 and 4, their ID being the
> > +  stream output number used in the implementation.
> 
> I don't think that's correct. The IP has four independent outputs, it should 
> have 4 output ports for a total for 5 ports. Multiple endpoints per port would 
> describe multiple connections from the same output to different sinks.

Ok.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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