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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Maxime,

On Tuesday, 22 August 2017 11:53:20 EEST Maxime Ripard wrote:
> On Mon, Aug 07, 2017 at 11:18:03PM +0300, Laurent Pinchart wrote:
> > 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" :)

What's connected to the input of the CSI-2 receiver ?

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

Just to confirm, the PHY is a separate IP core, right ? Is the CSI-2 receiver 
input interface different when used with a PHY and when used without one ? 
Could a third-party PHY be used as well ? If so, would the PHY synthesis bit 
be set or not ?

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

It'sa semantic issue, but documenting a property as required when it can be 
ommitted under some circumstances seems even weirder to me :-) I understand 
the optional category as "can be ommitted in certain circumstances".

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

-- 
Regards,

Laurent Pinchart