Re: [PATCH v3 4/4] dt-bindings: media: Document Synopsys Designware HDMI RX

[Sylwester Nawrocki <sylwester.nawrocki@gmail.com>]

Hi Jose,

On 06/16/2017 06:38 PM, Jose Abreu wrote:
> Document the bindings for the Synopsys Designware HDMI RX.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>

> new file mode 100644
> index 0000000..d30cc1e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.txt
> @@ -0,0 +1,45 @@
> +Synopsys DesignWare HDMI RX Decoder
> +===================================
> +
> +This document defines device tree properties for the Synopsys DesignWare HDMI
> +RX Decoder (DWC HDMI RX). It doesn't constitute a device tree binding
> +specification by itself but is meant to be referenced by platform-specific
> +device tree bindings.
> +
> +When referenced from platform device tree bindings the properties defined in
> +this document are defined as follows.
> +
> +- reg: Memory mapped base address and length of the DWC HDMI RX registers.
> +
> +- interrupts: Reference to the DWC HDMI RX interrupt and 5v sense interrupt.
> +
> +- snps,hdmi-phy-jtag-addr: JTAG address of the phy to be used.
> +
> +- snps,hdmi-phy-version: Version of the phy to be used.
> +
> +- snps,hdmi-phy-cfg-clk: Value of the cfg clk that is delivered to phy.
> +
> +- snps,hdmi-phy-driver: *Exact* name of the phy driver to be used.

I don't think we can put Linux driver name in DT like this, devicetree 
is supposed to be describing hardware in OS agnostic way.

> +- snps,hdmi-ctl-cfg-clk: Value of the cfg clk that is delivered to the
> +controller.

How about creating a separate node for the PHY? The binding could then 
be something like:


/* Fixed clock needed only if respective clock is not already defined    
   in the system */

refclk: clock {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <25000000>;
};

hdmi_phy: phy@f3 {
	/* PHY version can be derived from the compatible string */
	compatible = "snps,dw-hdmi-phy-e405"; 	
	/* JTAG address of the PHY */
	reg = <0xf3>

	clocks = <&refclk>
	clock-names = "cfg-clk";
}

hdmi-controller@0 {
	compatible = "snps,dw-hdmi-rx-controller-vX.X";
	reg = <0x0 0x10000>;
	interrupts = <1 3>; 	
	phys = <&hdmi_phy>;

	clocks = <&refclk>
	clock-names = "cfg-clk";
};

Or it might be better to make the PHY node child node of the RX controller 
node, since the RX controller could be considered a controller of the JTAG 
bus IIUC:

refclk: clock {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <25000000>;
};

hdmi-controller@0 {
	compatible = "snps,dw-hdmi-rx-controller-xxx";
	reg = <0x0 0x10000>;
	interrupts = <1 3>; 	
	clocks = <&refclk>
	clock-names = "cfg-clk";
	#address-cells = <1>;
	#size-cells = <0>;

	phy@f3 {
		/* PHY version can be derived from the compatible string */
		compatible = "snps,dw-hdmi-phy-e405";
 		/* address of the PHY on the JTAG bus */
		reg = <0xf3>

		clocks = <&refclk>
		clock-names = "cfg-clk";
	}
};

Then rather than creating platform device in the RX controller driver 
for the PHY just of_platform_populate() could be used.

> +- edid-phandle: phandle to the EDID driver. It can be, for example, the main
> +wrapper driver.
> +
> +A sample binding is now provided. The compatible string is for a wrapper driver
> +which then instantiates the Synopsys Designware HDMI RX decoder driver.

I would avoid talking about drivers in DT binding documentation, we should 
rather refer to hardware blocks.

> +Example:
> +
> +dw_hdmi_wrapper: dw-hdmi-wrapper@0 {
> +	compatible = "snps,dw-hdmi-rx-wrapper";
> +	reg = <0x0 0x10000>; /* controller regbank */
> +	interrupts = <1 3>; /* controller interrupt + 5v sense interrupt */
> +	snps,hdmi-phy-driver = "dw-hdmi-phy-e405";
> +	snps,hdmi-phy-version = <405>;
> +	snps,hdmi-phy-cfg-clk = <25>; /* MHz */
> +	snps,hdmi-ctl-cfg-clk = <25>; /* MHz */
> +	snps,hdmi-phy-jtag-addr = /bits/ 8 <0xfc>;
> +	edid-phandle = <&dw_hdmi_wrapper>;
> +};

--
Regards,
Sylwester