Re: [PATCH 07/12] dt-bindings: tc358754: add DT bindings

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

Hi Andrzej,

On Wednesday, 30 May 2018 12:59:12 EEST Andrzej Hajda wrote:
> On 28.05.2018 12:18, Laurent Pinchart wrote:
> > On Monday, 28 May 2018 12:47:11 EEST Maciej Purski wrote:
> >> The patch adds bindings to Toshiba DSI/LVDS bridge TC358764.
> >> Bindings describe power supplies, reset gpio and video interfaces.
> >> 
> >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> >> Signed-off-by: Maciej Purski <m.purski@samsung.com>
> >> ---
> >> 
> >>  .../bindings/display/bridge/toshiba,tc358764.txt   | 42 ++++++++++++++++
> >>  1 file changed, 42 insertions(+)
> >>  create mode 100644
> >> 
> >> Documentation/devicetree/bindings/display/bridge/toshiba,tc358764.txt
> >> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358764.txt
> >> b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358764.txt
> >> new
> >> file mode 100644
> >> index 0000000..d09bdc2
> >> --- /dev/null
> >> +++
> >> b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358764.txt
> >> @@ -0,0 +1,42 @@
> >> +TC358764 MIPI-DSI to LVDS panel bridge
> >> +
> >> +Required properties:
> >> +  - compatible: "toshiba,tc358764"
> >> +  - reg: the virtual channel number of a DSI peripheral
> >> +  - vddc-supply: core voltage supply
> >> +  - vddio-supply: I/O voltage supply
> >> +  - vddmipi-supply: MIPI voltage supply
> >> +  - vddlvds133-supply: LVDS1 3.3V voltage supply
> >> +  - vddlvds112-supply: LVDS1 1.2V voltage supply
> > 
> > That's a lot of power supplies. Could some of them be merged together ?
> > See https://patchwork.freedesktop.org/patch/216058/ for an earlier
> > discussion on the same subject.
> 
> Specs says about 3 supply voltage values:
> - 1.2V - digital core, DSI-RX PHY
> - 1.8-3.3V - digital I/O
> - 3.3V - LVDS-TX PHY
> 
> So I guess it should be minimal number of supplies. Natural candidates:
> 
> - vddc-supply: core voltage supply, 1.2V
> - vddio-supply: I/O voltage supply, 1.8V or 3.3V
> - vddlvds-supply: LVDS1/2 voltage supply, 3.3V
> 
> I have changed name of the latest supply to be more consistent with
> other supplies, and changed 1.8-3.3 (which incorrectly suggest voltage
> range), to more precise voltage alternative.

This looks fine to me.

> >> +  - reset-gpios: a GPIO spec for the reset pin
> >> +
> >> +The device node can contain zero to two 'port' child nodes, each with
> >> one
> >> +child
> >> +'endpoint' node, according to the bindings defined in [1].
> >> +The following are properties specific to those nodes.
> >> +
> >> +port:
> >> +  - reg: (required) can be 0 for DSI port or 1 for LVDS port;
> > 
> > This seems pretty vague to me. It could be read as meaning that ports are
> > completely optional, and that the port number you list can be used, but
> > that something else could be used to.
> > 
> > Let's make the port nodes mandatory. I propose the following.
> > 
> > Required nodes:
> > 
> > The TC358764 has DSI and LVDS ports whose connections are described using
> > the OF graph bindings defined in
> > Documentation/devicetree/bindings/graph.txt. The device node must contain
> > one 'port' child node per DSI and LVDS port. The port nodes are numbered
> > as follows.
> > 
> >   Port                  Number
> > -------------------------------------------------------------------
> >   DSI Input             0
> >   LVDS Output           1
> > 
> > Each port node must contain endpoint nodes describing the hardware
> > connections.
> 
> Since the bridge is controlled via DSI bus, DSI input port is not necessary.

I don't agree with this. Regardless of how the bridge is controlled, I think 
we should always use ports to describe the data connections. Otherwise it 
would get more complicated for display controller drivers to use different 
types of bridges.

> >> +[1]: Documentation/devicetree/bindings/media/video-interfaces.txt
> >> +
> >> +Example:
> >> +
> >> +	bridge@0 {
> >> +		reg = <0>;
> >> +		compatible = "toshiba,tc358764";
> >> +		vddc-supply = <&vcc_1v2_reg>;
> >> +		vddio-supply = <&vcc_1v8_reg>;
> >> +		vddmipi-supply = <&vcc_1v2_reg>;
> >> +		vddlvds133-supply = <&vcc_3v3_reg>;
> >> +		vddlvds112-supply = <&vcc_1v2_reg>;
> >> +		reset-gpios = <&gpd1 6 GPIO_ACTIVE_LOW>;
> >> +		#address-cells = <1>;
> >> +		#size-cells = <0>;
> >> +		port@1 {
> >> +			reg = <1>;
> >> +			lvds_ep: endpoint {
> >> +				remote-endpoint = <&panel_ep>;
> >> +			};
> >> +		};
> >> +	};

-- 
Regards,

Laurent Pinchart